fix(login): use isolated HTTP mux, eagerly bind port, and return errors from oauth2login. Fixes #312#317
Open
rootp1 wants to merge 8 commits intomicrocks:masterfrom
Open
fix(login): use isolated HTTP mux, eagerly bind port, and return errors from oauth2login. Fixes #312#317rootp1 wants to merge 8 commits intomicrocks:masterfrom
rootp1 wants to merge 8 commits intomicrocks:masterfrom
Conversation
Signed-off-by: rootp1 <arnav.iitr@gmail.com>
Signed-off-by: rootp1 <arnav.iitr@gmail.com>
Signed-off-by: rootp1 <arnav.iitr@gmail.com>
Signed-off-by: rootp1 <arnav.iitr@gmail.com>
Signed-off-by: rootp1 <arnav.iitr@gmail.com>
Signed-off-by: rootp1 <arnav.iitr@gmail.com>
Signed-off-by: rootp1 <arnav.iitr@gmail.com>
Signed-off-by: rootp1 <arnav.iitr@gmail.com>
Author
Hey @Harsh4902, @lbroudoux @yada, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
http.HandleFunc(process-globalhttp.DefaultServeMux) with an isolatedhttp.NewServeMuxperoauth2logincall, eliminating handler pollution across repeated invocations and race conditions on the shared global muxoauth2loginto eagerly bind the callback port vianet.Listenbefore opening the browser, so port hijack attempts are detected immediately with a clear error message instead of crashing inside a goroutine after the OAuth flow has already startedssoAuthFlowopens the browser, guaranteeing the server is ready when the browser redirects backoauth2loginsignature from(string, string)to(string, string, error), replacinglog.Fatal/log.Fatalfcalls with returned errors so the caller controls error handling and the function is testablesrv.Shutdownon the error path before returning, ensuring the callback server port is released even when the OAuth flow failstime.Sleep(1 * time.Second)hack that previously tried to paper over the race between server startup and browser redirectReproduction results
Before fix - Port hijack:
A rogue process listening on port 58085 before
login --ssocauseslog.Fatalfinside a goroutine after the browser is already open. The user sees a confusing crash and the OAuth callback is silently routed to the attacker.After fix - Port hijack detected early:
oauth2loginreturnserror: "failed to bind callback port localhost:58085: listen tcp 127.0.0.1:58085: bind: address already in use (is another process using it?)"immediately - no browser opened, no OAuth flow started.Before fix - Global mux pollution:
http.HandleFunc("/auth/callback", ...)registers the route onhttp.DefaultServeMuxpermanently. A secondlogin --ssocall overwrites the first handler. In the same binary, this route interferes with any other HTTP serving.After fix - Isolated mux:
Each
oauth2logincall creates its ownhttp.NewServeMux./auth/callbackis never registered onhttp.DefaultServeMux. Concurrent and sequential calls are fully independent.Before fix - Server port leaked on error:
When the callback receives an error (e.g.
access_denied),log.Fatalis called without shutting down the server, leaving the port bound until the process exits.After fix - Clean shutdown on error:
srv.Shutdownis called before returning the error, releasing the port immediately.Test suite (5 tests, all passing):
TestPortHijackDetectionTestGlobalMuxNotPolluted/auth/callbacknot inDefaultServeMuxafter callTestEagerPortBindBeforeBrowserTestServerShutdownOnErrorPathTestConcurrentIsolatedMuxCallsRelated issue(s)
Fixes race condition, global mux pollution, and port hijack vulnerability in OAuth2 SSO callback server as described in issue
Fixes #312 .
BREAKING CHANGE
oauth2loginnow returns(string, string, error)instead of(string, string). Any callers of this function must handle the returned error.