ping_federate: strip brackets from IPv6 in audit CEF before decode_cef#17620
Conversation
f268aec to
b163a1b
Compare
|
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
efd6
left a comment
There was a problem hiding this comment.
Can you add a link to documentation that supports the rationale for this change? The CEF parser that we are using does not support IPv6 (the src field described in the v25 spec says that it is an IPv4, "Identifies the source that an event refers to in an IP network. The format is an IPv4 address. Example: “192.168.10.1”." ISTM that this is a workaround for a format that's not documented to support IPv6 (so it's not a bugfix, at least not in the processor; it is likely a bug in the data source if we are seeing IPv6 data coming through, though note paragraph below regarding spec versions).
If there is a later version of the spec that does support IPv6, we should probably make a change to the processor in beats. We can apply this workaround, but it should be noted as such.
|
Relates: elastic/beats#40269 (for CEF spec updates, mentions IPv6) |
The failure is due to brackets around the IP, which are not part of the CEF The change strips those brackets so the parser receives a valid IP string; it’s a format normalization, not an IPv6‑vs‑IPv4 feature. The CEF v25 “src is IPv4” point is a separate spec detail; if the processor later supports IPv6 per a newer spec, the same “no brackets” rule would still apply. |
|
Yes, it looks like Ping Federate had the same misconception as I did; they have misunderstood IPv6 syntax in a URI as being the general syntax. So what I said about IPv6 updates fixing it was wrong, but the comment about the nature of the change is now more correct; this is not a bugfix. The bug is in Ping Federate. We should probably relax the CEF processor, I'm sure others will have made the same mistake. |
🚀 Benchmarks reportTo see the full report comment with |
💚 Build Succeeded
History
|
| var msg = event.Get("event.original"); | ||
| if (msg != null) { | ||
| // Remove brackets from IPv6 addresses to comply with CEF specification | ||
| msg = msg.replace(/\[([0-9a-fA-F:]+)\]/gi, "$1"); |
There was a problem hiding this comment.
Just a note that the IPv6 pattern is more complex than this. An indication of this is from the grok patterns here
There was a problem hiding this comment.
Thanks for the note.
This step only strips the brackets; it doesn’t validate the contents. Real validation happens in decode_cef, which will still reject invalid IPs.
There was a problem hiding this comment.
Yeah, my concern (minor) was that if there was anything else in the message that is [hexdigitsandcolons] then it would be mutated as well even if it were not an IPv6. I think this is reasonably unlikely, but it is less unlikely than something that specifically looks like an IPv6 and isn't an IPv6.
There was a problem hiding this comment.
Thanks for clarifying.
Agreed - there's a small risk. We will keep this in mind for the future.
|
Package ping_federate - 1.2.0 containing this change is available at https://epr.elastic.co/package/ping_federate/1.2.0/ |
Proposed commit message
Checklist
changelog.ymlfile.Author's Checklist
How to test this PR locally
System Tests:
Related issues
Screenshots