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.

Sign up for GitHub

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

Jump to bottom

proposal: net/http: Server.ConnContext should be invoked *after* forking go c.serve(...) #64837

Open
ahmedtd opened this issue Dec 21, 2023 · 3 comments
Open

proposal: net/http: Server.ConnContext should be invoked *after* forking go c.serve(...) #64837

ahmedtd opened this issue Dec 21, 2023 · 3 comments
Labels
Proposal
Milestone
Proposal

Comments

@ahmedtd
Copy link

ahmedtd commented Dec 21, 2023

Proposal Details

Code link: https://cs.opensource.google/go/go/+/refs/tags/go1.21.5:src/net/http/server.go;l=3077

Today, when an http.Server has a ConnContext function set, ConnContext is called inline in the accept loop, before forking off a separate goroutine to run the HTTP serving logic.

If ConnContext takes time, this ends up driving tail latency for all incoming connections in the accept queue behind the slow invocation. We originally hit this while investigating high latency in GKE and ASM when using GKE Workload Identity; the flow involves a ConnContext function that looks up callers by IP to authenticate them).

If ConnContext were invoked as the first step after forking the goroutine, then long-running ConnContext calls would not block accept() for other incoming connections. This would technically be a behavior change, which is why I'm submitting a proposal, but I don't really see how it could cause a problem.
@gopherbot gopherbot added this to the Proposal milestone Dec 21, 2023
@Jorropo
Copy link
Member

Jorropo commented Dec 21, 2023

It seems to me that both features are useful, having it inline in the loop allows to apply back-pressure and avoid runaway memory usage.

Let's assume someone is DOSing you, and you are doing IP based rate limiting inside ConnContext.
If this is done after the fork you may have a transient where the attacker can spawn thousands of goroutines before the rate limiting code is being executed.

Someone wary of that could also implement this by wrapping net.Listener object and doing inline validation in the .Accept method.

@ahmedtd
Copy link
Author

ahmedtd commented Dec 22, 2023

Good point... Maybe there should be a separate PostForkConnContext function.

@bcmills
Copy link
Contributor

bcmills commented Jan 2, 2024

(attn @neild)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Proposal
Projects
Proposals
Status: Incoming
Milestone
Proposal
Development

No branches or pull requests
4 participants
@ahmedtd @bcmills @gopherbot @Jorropo