output/ipv6: Add per-output configuration option to shorten IPv6 IP addresses#14819
output/ipv6: Add per-output configuration option to shorten IPv6 IP addresses#14819
Conversation
Issue: 7399 Utility function to shorten IPv6 addresses per RFC-5952
Issue: 7399 Add option for EVE and alert output to shorten IPv6 addresses. The default is to not shorten; set ipv6-addr-shorten to yes to display shortened IPv6 addresses in output.
Issue: 7399 Add a IPv6 print utility function that accepts a parameter indicating whether the address should be in long or short form.
Issue: 7399 Determine the EVE IPv6 address display and use that when generating external display representation.
Issue: 7399 Add shorten ipv6 setting to file context.
When configured, display the short form of the IPv6 address. Issue: 7399
Issue: 7399 Document the IPv6 display behavior and how to display IPv6 addresses in their shortened form (per RFC-5952).
|
Information: QA ran without warnings. Pipeline = 29600 |
catenacyber
left a comment
There was a problem hiding this comment.
Thanks for the work,
CI : ✅
Code : will leave some remarks
Commits segmentation : 🟠 I would have less commits like the first one introducing SCIPv6Shorten should be merged with the first one using it (same for commit output/ipv6: Add output IPv6 shorten option which just adds line in suricata.yaml.in without reading them)
Commit messages : nice
Git ID set : looks fine for me
CLA : you already contributed
Doc update : cool
Redmine ticket : ok
Rustfmt : ok
Tests : README welcome but look ok
Dependencies added: none
| let mut ipv6_str = String::with_capacity(39); | ||
| let _ = std::fmt::Write::write_fmt(&mut ipv6_str, format_args!("{}", ipv6)); | ||
|
|
||
| // Sufficient room? |
There was a problem hiding this comment.
debug validation that this never happens ?
There was a problem hiding this comment.
The unittests exercise this logic ... suggestions for how to make the tests immune to a debug-validation check?
There was a problem hiding this comment.
but... "configure: error: debug_validation can't be enabled with enabled unittests!" isn't that so?
There was a problem hiding this comment.
Also, maybe the unit tests should not exercise this if Suricata itself never reaches this code
|
|
||
| // Copy string + NULL termination | ||
| std::ptr::copy_nonoverlapping(ipv6_str.as_ptr(), out_buf as *mut u8, ipv6_str.len()); | ||
| std::ptr::write((out_buf as *mut u8).add(ipv6_str.len()), 0u8); |
There was a problem hiding this comment.
Is there a simpler way to do this in rust @jasonish ?
There was a problem hiding this comment.
Yes.. Perhaps use crate::ffi::strings::copy_to_c_char, see SCSha256HashBufferToHex for an example on copying a Rust string to a C output pointer.
There was a problem hiding this comment.
Thanks; will adjust.
|
|
||
| struct ShortenResult { | ||
| string: String, | ||
| len: usize, |
There was a problem hiding this comment.
can we not use the string.len() ?
There was a problem hiding this comment.
This is unittest code -- the callers of this function verify the expected len -- is string.len() preferred over comparison with len?
There was a problem hiding this comment.
I would prefer string.len() over len to avoid adding a field, unless there is a reason to do otherwise
There was a problem hiding this comment.
Will make the change and remove len member.
|
|
||
| char srcip[46], dstip[46]; | ||
| PrintInet(AF_INET6, (const void *)GET_IPV6_SRC_ADDR(p), srcip, sizeof(srcip)); | ||
| PrintInet(AF_INET6, (const void *)GET_IPV6_DST_ADDR(p), dstip, sizeof(dstip)); |
There was a problem hiding this comment.
Is it exepcted that there are remaining cases with
git grep PrintInet | grep AF_INET6 ?
There was a problem hiding this comment.
Yes -- with a caveat. All usages that result in log output should be covered by this change.
Not covered with this PR are:
- log-httplog.c
- log-tcp-data.c
- log-tlsstore.c
Is coverage for those logs necessary?
There was a problem hiding this comment.
Adding the "Decision Required" label to highlight this.
There was a problem hiding this comment.
Is coverage for those logs necessary?
That was kind of my question seeing log-httplog.c
There was a problem hiding this comment.
Http-log is scheduled for removal in 9.
I will add shortening for the tcp-data log and an s-v test.
| filename: alert-debug.log #The name of the file in the default logging directory. | ||
| append: yes/no #If this option is set to yes, the last filled fast.log-file will not be | ||
| # overwritten while restarting Suricata. | ||
| # Shorten IPv6 addresses per RFC5952 as they are added to the fast log. The default is no. |
There was a problem hiding this comment.
nit: not fast.log ? :P
There was a problem hiding this comment.
good catch; will update.
| if (SCIPv6Shorten(src, dst, size)) { | ||
| return dst; | ||
| } | ||
|
|
There was a problem hiding this comment.
very nitty, but: unneeded extra line?
|
|
||
| char srcip[46], dstip[46]; | ||
| PrintInet(AF_INET6, (const void *)GET_IPV6_SRC_ADDR(p), srcip, sizeof(srcip)); | ||
| PrintInet(AF_INET6, (const void *)GET_IPV6_DST_ADDR(p), dstip, sizeof(dstip)); |
There was a problem hiding this comment.
Adding the "Decision Required" label to highlight this.
| let mut ipv6_str = String::with_capacity(39); | ||
| let _ = std::fmt::Write::write_fmt(&mut ipv6_str, format_args!("{}", ipv6)); | ||
|
|
||
| // Sufficient room? |
There was a problem hiding this comment.
but... "configure: error: debug_validation can't be enabled with enabled unittests!" isn't that so?
|
|
||
| pub mod base64; | ||
| pub mod datalink; | ||
| pub mod ip_addr; |
There was a problem hiding this comment.
This is a real picky style nit, but maybe rename to ipaddr?
There was a problem hiding this comment.
makes sense; will do.
|
Continued in #14867 |
Continuation of #14816
Display IPv6 addresses in long (default) or shortened form per RFC-5952, based on the per-output configuration setting. Each of these outputs will display shortened IPv6 addresses when the per-output config setting
ipv6-addr-shortenisyes.Here's an example of an IPv6 address with its shortened value::
fe80:0000:0000:0000:020c:29ff:faf2:ab42fe80::20c:29ff:faf2:ab42Link to ticket: https://redmine.openinfosecfoundation.org/issues/7399
Describe changes:
Updates:
Provide values to any of the below to override the defaults.
link to the pull request in the respective
_BRANCHvariable.SV_REPO=
SV_BRANCH=OISF/suricata-verify#2789
SU_REPO=
SU_BRANCH=