o365,sentinel_one_cloud_funnel,sysmon_linux,system,windows: tighten ipv4 extraction#11052
o365,sentinel_one_cloud_funnel,sysmon_linux,system,windows: tighten ipv4 extraction#11052efd6 merged 2 commits intoelastic:mainfrom
Conversation
🚀 Benchmarks reportTo see the full report comment with |
|
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
|
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
|
Pinging @elastic/sec-linux-platform (Team:Security-Linux Platform) |
|
Pinging @elastic/sec-windows-platform (Team:Security-Windows Platform) |
leehinman
left a comment
There was a problem hiding this comment.
I'm ok with code as proposed but I'm wondering about the value of an improvement.
Because the regex could match things like 999.999.999.999, maybe it would be a good idea to save the IP address to a temp field, and then use the convert processor to make sure it is a valid IP address. We could make the regex tighter, but that gets harder and harder for the human to read
|
There are multiple things that are wrong with this approach, but they all boil down to attempting to parse IPs by RE. The intention here is not to filter incorrect IP values (e.g. a 999.999.999.999), but rather to ensure that correctly formatted IPs that are not IP4-mapped IPv6 addresses are not treated as though they are. The risk of that is low even without this change as noted in the original PR, but I thought that this was a reasonably straightforward addition that would prevent it. |
|
/test |
|
I am unable to replicate this failure locally. |
|
/test |
chrisberkhout
left a comment
There was a problem hiding this comment.
Should it accept enclosing square brackets without a port?
If so I'd suggest this:
- pattern: '^\[?::ffff:([0-9]+\.[0-9]+\.[0-9]+\.[0-9]+)(?:\]:[0-9]+)?$'
+ pattern: '^\[?::ffff:([0-9]+\.[0-9]+\.[0-9]+\.[0-9]+)\]?(?::[0-9]+)?$'Would be handy to see a list of example values for it to parse.
|
From a defensive programming perspective, I think that's a good idea. Though it should be |
|
/test |
…pv4 extraction Make sure that the prefix of the IP address is ::ffff in all cases.
💚 Build Succeeded
History
cc @efd6 |
|
|
Package o365 - 2.6.0 containing this change is available at https://epr.elastic.co/search?package=o365 |
|
Package sentinel_one_cloud_funnel - 1.5.0 containing this change is available at https://epr.elastic.co/search?package=sentinel_one_cloud_funnel |
|
Package sysmon_linux - 1.7.0 containing this change is available at https://epr.elastic.co/search?package=sysmon_linux |
|
Package system - 1.61.0 containing this change is available at https://epr.elastic.co/search?package=system |
|
Package windows - 2.1.0 containing this change is available at https://epr.elastic.co/search?package=windows |
| pattern: '::ffff:([0-9]+\.[0-9]+\.[0-9]+\.[0-9]+)' | ||
| replacement: '$1' | ||
| pattern: '^\[?::ffff:([0-9]+\.[0-9]+\.[0-9]+\.[0-9]+)(?:\](:[0-9]+)?)?$' | ||
| replacement: '$1$2' |
There was a problem hiding this comment.
@efd6 - curious why you have $2 in this integration but only $1 in the rest. Care to elaborate for my understanding?
There was a problem hiding this comment.
This one actually uses the port value, the rest only check that it's there.
…pv4 extraction (elastic#11052) Make sure that the prefix of the IP address is ::ffff in all cases.
…pv4 extraction (elastic#11052) Make sure that the prefix of the IP address is ::ffff in all cases.
Proposed commit message
Make sure that the prefix of the IP address is ::ffff in all cases.
Checklist
changelog.ymlfile.Author's Checklist
How to test this PR locally
Related issues
Screenshots