Rework JWT detector to better block local IPs#4607
Conversation
shahzadhaider1
left a comment
shahzadhaider1
left a comment
There was a problem hiding this comment.
Approving to unblock. This looks good from a security standpoint, and I agree with using the detector HTTP client here.
One thing I hope we have considered: switching from common.SaneHttpClient means we lose the retry/instrumentation layer (metrics we currently export to Prometheus/Grafana).
rosecodym
left a comment
rosecodym
left a comment
There was a problem hiding this comment.
I like the code consolidation but I think it's a bummer that this costs us some instrumentation. Did you look into whether we can get that easily as well? (If we can't, don't worry about it.)
I believe this PR does introduce a data race, though.
| client := &http.Client{ | ||
| Transport: NewDetectorTransport(nil), | ||
| Timeout: DefaultResponseTimeout, | ||
| } | ||
|
|
||
| for _, opt := range opts { | ||
| opt(httpClient) | ||
| opt(client) | ||
| } | ||
| return httpClient | ||
|
|
||
| client.Transport = common.NewInstrumentedTransport(client.Transport) |
There was a problem hiding this comment.
@rosecodym I added instrumentation like we use in SaneHttpClient here in NewDetectorHttpClient. A few dozen detectors used these DetectorHttpClientWith* clients instead of SaneHttpClient, meaning they were totally missing the HTTP instrumentation before.
Does this look reasonable to you? Tests have passed locally for me.
…on (trufflesecurity#4607) * Rework JWT detector to better block local IPs * Add comment about concurrency / data races to `Detector` * Add instrumentation from `common.SaneHttpClient` to `detectors.NewDetectorHttpClient`
4 participants
This PR updates the JWT detector to more simply and more robustly prevent making requests to local IPs.
The previous implementation did its checking at the hostname level, but this approach doesn't work in general when redirects are involved. The new implementation has less code and does its checking at the HTTP transport level, after DNS resolution.
This PR also adds HTTP instrumentation to the
DetectorHttpClientWith*clients, as already exists inSaneHttpClient. This change should give us insight into some blind spots, as theDetectorHttpClientWith*clients are used in a few dozen existing detectors and previously had no HTTP instrumentation.