Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve http.Client usage #1891

Open
jentfoo opened this issue Jan 30, 2025 · 2 comments · May be fixed by #1926
Open

Improve http.Client usage #1891

jentfoo opened this issue Jan 30, 2025 · 2 comments · May be fixed by #1926
Assignees

Comments

@jentfoo
Copy link
Contributor

jentfoo commented Jan 30, 2025

We should improve our usage with http.Client. We don't currently reuse http clients, or define them in a consistent way in our Go code. This leads to several potential concerns:

  • A lack of reuse prevents the ability to gain potential performance gains through connection pooling. SSL handshakes are notably slow, we likely are able to achieve real performance gains by being able to reuse already established connections.
  • A lack of timeout configuration could be a point of DoS risk.
  • A lack of redirect limitations could result in a trusted source initiating a blind SSRF.

I plan to address this by creating a public function for returning a well configured http.Client from the sdk. Then utilize this client from within the SDK, platform, and backend broadly. If anyone has feedback on this design let me know in Slack or once I submit the PR.

@jentfoo jentfoo self-assigned this Jan 30, 2025
@dmihalcik
Copy link

Would callers to the sdk want to be able to configure this? Maybe it could be a configuration option to sdk.New

@jentfoo
Copy link
Contributor Author

jentfoo commented Feb 5, 2025

@dmihalcik What configuration were you thinking? For pooling I was just going to use DefaultTransport, which only pools 100 connections. That should be fine for even systems with relatively tight memory limits, while still providing a significant benefit over what we are doing today. Let me know your thoughts, thank you!

github-merge-queue bot pushed a commit that referenced this issue Feb 18, 2025
### Proposed Changes
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 PR 3/4 addresses #1891. After it's merged a small change will be
made in the `service` to fully utilize the API being introduced in the
SDK here.

### Checklist

- [ ] I have added or updated unit tests
- [ ] I have added or updated integration tests (if appropriate)
- [ ] I have added or updated documentation

### Testing Instructions
jentfoo added a commit that referenced this issue Feb 18, 2025
This PR follows #1910 by updating `service` to utilize the new `httputil` helper for constructing a client which wont follow redirects, and has sensible timeouts.

The prior auth API was marked as deprecated. There is no remaining use within this repo, so it may be able to be removed.

Merging this should fully resolve #1891.
@jentfoo jentfoo linked a pull request Feb 18, 2025 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants