[APPSEC-7946] Update the client IP extraction resolution base on the latest RFC. #2665
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
appsec
GustavoCaso
changed the title
Codecov Report
@@ Coverage Diff @@
## master #2665 +/- ##
==========================================
- Coverage 98.07% 98.06% -0.01%
==========================================
Files 1166 1168 +2
Lines 63832 63872 +40
Branches 2849 2856 +7
==========================================
+ Hits 62605 62638 +33
- Misses 1227 1234 +7
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
ivoanjo
reviewed
Mar 6, 2023
delner
reviewed
Mar 6, 2023
| def set_client_ip_tag(span, headers: nil, remote_ip: nil) | ||
| return unless configuration.enabled | ||
|
|
||
| set_client_ip_tag!(span, headers: headers, remote_ip: remote_ip) |
There was a problem hiding this comment.
We need to check on something internally for this.
ivoanjo
reviewed
Mar 6, 2023
Comment on lines
+5
to
+9
| def self.from_hash: (untyped hash) -> HashHeaderCollection | ||
| end | ||
|
|
||
| class HashHeaderCollection < HeaderCollection | ||
| def initialize: (untyped hash) -> void |
There was a problem hiding this comment.
Minor: I guess the hash could be ::Hash? Although any duck type of Hash that provides each_pair would work as well I guess :) (I don't know if rbs provides such duck types or not)
There was a problem hiding this comment.
any duck type of Hash that provides each_pair would work as well I guess :) (I don't know if rbs provides such duck types or not)
RBS provides interfaces! I have yet to make use of those.
ivoanjo
reviewed
Mar 7, 2023
ivoanjo
reviewed
Mar 7, 2023
ivoanjo
reviewed
Mar 7, 2023
GustavoCaso
force-pushed
the
appsec-client-ip-extraction
branch
3 times, most recently
from
f2be200
to
fc3877b
Compare
- Update the list of HTTP headers to look for IP - Add support to parsing HTTP IP Headers with multiple IP addresses - Update specs - Change Tracing::ClientIp module methods visibility
GustavoCaso
force-pushed
the
appsec-client-ip-extraction
branch
from
fc3877b
to
85ee26e
Compare
ivoanjo
added a commit
that referenced
this pull request
Mar 7, 2023
…:AddressFamilyError" **What does this PR do?**: This PR fixes a bug in the recently-introduced `IPAddr` module. During review of #2665 the `IPAddr` module was introduced, and while experimenting with the steep type checker, I noticed there was a collision between our `IPAddr` module and the `IPAddr` module where we were looking for execptions. I'm not sure if this would happen in production, since we always created an `IPAddr` instance with an IP, never a specific `family`, but let's fix this anyway. **Motivation**: Less bugs! **Additional Notes**: (N/A) **How to test the change?**: This module is a backport of `IPAddr` to older Rubies. Previously, we only tested it indirectly. I added a minimal test to validate this change.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Tracing::ClientIpmodule methods visibilityDatadog::Core::Utils::Networkmodule. Main functionality is parsing and extraction IP information for HTTP request headersrbsfilesI had to vendor the code from IPAddr default gem into the re; Because the ruby version below 2.5 does not have the methods:
private?,link_local?, andloopback?That code should be temporary until we deprecated those old ruby versions
Motivation
Fixing a bug found with the IP extraction resolution logic. Not supporting an HTTP header with multiple IPs MDN
Additional Notes
How to test the change?
CI