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.
| if client == nil { | ||
| // Use a client that disallows accessing local IPs. | ||
| // This avoids a potential blind SSRF. | ||
| client = detectors.NewDetectorHttpClient( |
There was a problem hiding this comment.
FromData can be called concurrently from multiple goroutines, so I believe this read/write needs some sort of synchronization. (My understanding is that this precise pattern generally works in the current version of Go but is technically disallowed by the spec, and I'd prefer to play it safe.)
There was a problem hiding this comment.
Yikes! Good to know; thanks for the pointer.
| type Scanner struct { | ||
| client *http.Client | ||
| } | ||
| type Scanner struct{} |
There was a problem hiding this comment.
IIRC the primary motivation for putting client inside the struct is test mocking. I don't know whether that still stands.
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.