fix: Improve http.Client usage for security and performance #1910
Conversation
| ) | ||
|
|
||
| var preventRedirectCheck = func(_ *http.Request, _ []*http.Request) error { | ||
| return http.ErrUseLastResponse // Prevent following redirects |
There was a problem hiding this comment.
Completely preventing redirects may not be realistic. However I am hopeful, because determining what IDOR's are allowed can be challenging.
I am proposing this initially, but it need more testing. If we are uncomfortable with this, maybe we can add some logging or metrics that we could use to help validate the safety here?
There was a problem hiding this comment.
Is this something where an end-user might want to support following redirects? Or maybe an allowlist of redirect endpoints? The latter seems more complicated than adding sdk.WithUnsafeFollowRedirects()
There was a problem hiding this comment.
Is this something where an end-user might want to support
Not unless we add an allow list / allowed IDOR configuration. Without that configuration this feature is insecure and could be used to make internal requests with the help of a redirect.
Since configuration adds complexity, I think we should only add it if we find it's necessary. There should not be a large need for this, so it would be good to understand that need first.
jentfoo
force-pushed
the
jent/http.Client_handling
branch
from
f8ee52c to
da0f385
Compare
| return SafeHTTPClientWithTransport(&http.Transport{ | ||
| TLSClientConfig: cfg, | ||
| // config below matches DefaultTransport | ||
| Proxy: http.ProxyFromEnvironment, | ||
| ForceAttemptHTTP2: true, | ||
| MaxIdleConns: maxIdleConns, | ||
| IdleConnTimeout: idleConnTimeout, | ||
| TLSHandshakeTimeout: tLSHandshakeTimeout, | ||
| ExpectContinueTimeout: expectContinueTimeout, | ||
| }) |
There was a problem hiding this comment.
We might want to expose something in the sdk which will allow developers to change these defaults (since they are defaults). This way we can support a variety of network environments, especially over remote areas with high-latency.
There was a problem hiding this comment.
I don't think most users will need to concern themselves with this. But if they have a long running process with spare memory it's conceivable that someone may want to change it. However I don't believe it should be necessary, and even without precise tuning this will be significantly better than what we have today.
I guess generally I prefer to just keep configuration simple and defaults sane, and only bring in the config complexity when a clear use case is understood. It's easy to expand, but hard to retract.
That said, no strong preference here since tuning these defaults wont increase risk to our users. If you would like them to be configurable should I just add those values to the config struct? Let me know your preference.
service/pkg/server/start.go
Outdated
| @@ -231,7 +232,7 @@ func Start(f ...StartOptions) error { | |||
| return fmt.Errorf("error creating ERS tokensource: %w", err) | |||
| } | |||
|
|
|||
| interceptor := sdkauth.NewTokenAddingInterceptor(ts, tlsConfig) | |||
| interceptor := sdkauth.NewTokenAddingInterceptor(ts, httputil.SafeHTTPClientWithTLSConfig(tlsConfig)) | |||
There was a problem hiding this comment.
I think this is one area it can get challenging. We probably want to ensure we can specify this from the config since we are unfamiliar with the network connection between the deployed platform and the IdP.
There was a problem hiding this comment.
I could use a little clarification. What specifically should be tunable from the config here? Or what challenge did I overlook?
I will be pulling this out of this PR and following up on the service changes in a second PR, but it would be good for me to understand for that PR. Thank you!
This change switches us from maintaining a tls config which we then on-demand initialize an `http.Client` with to instead maintain and reuse an `http.Client` instance. This enables us to utilize the connection pooling which occurs within the `http.Transport` to reduce ssl handshakes and thus reduce latency. In addition this change provides us a central place to configure out `http.Client` (`httputil`). Allowing us to easily set configuration options to reduce the security risks of using an unconfigured `http.Client`. Notably timeouts to reduce DoS risks, and control around following redirects to prevent blind SSRF's. This change will provide significant benefit without making API changes. A future PR will update the service and also cleanup some of the API.
jentfoo
force-pushed
the
jent/http.Client_handling
branch
from
06093c7 to
c7026b8
Compare
Proposed Changes
This change switches us from maintaining a tls config which we then on-demand initialize an
http.Clientwith to instead maintain and reuse anhttp.Clientinstance. This enables us to utilize the connection pooling which occurs within thehttp.Transportto reduce ssl handshakes and thus reduce latency.In addition this change provides us a central place to configure out
http.Client(httputil). Allowing us to easily set configuration options to reduce the security risks of using an unconfiguredhttp.Client. Notably timeouts to reduce DoS risks, and control around following redirects to prevent blind SSRF's.This PR 3/4 addresses #1891. After it's merged a small change will be made in the
serviceto fully utilize the API being introduced in the SDK here.Checklist
Testing Instructions