-
Notifications
You must be signed in to change notification settings - Fork 34
auth: login: explicit check for ipv6 port bindings #1251
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -95,9 +95,15 @@ func AuthorizeUser(p *print.Printer, isReauthentication bool) error { | |
| var port int | ||
| for i := range configuredPortRange { | ||
| port = defaultPort + i | ||
| portString := fmt.Sprintf(":%s", strconv.Itoa(port)) | ||
| addr4 := fmt.Sprintf("127.0.0.1:%d", port) | ||
| addr6 := fmt.Sprintf("[::1]:%d", port) | ||
| p.Debug(print.DebugLevel, "trying to bind port %d for login redirect", port) | ||
| listener, listenerErr = net.Listen("tcp", portString) | ||
| ipv6Listener, ipv6ListenerErr := net.Listen("tcp6", addr6) | ||
| if ipv6ListenerErr != nil { | ||
| continue | ||
| } | ||
| _ = ipv6Listener.Close() | ||
| listener, listenerErr = net.Listen("tcp4", addr4) | ||
| if listenerErr == nil { | ||
| redirectURL = fmt.Sprintf("http://localhost:%d", port) | ||
| p.Debug(print.DebugLevel, "bound port %d for login redirect", port) | ||
|
|
@@ -106,7 +112,7 @@ func AuthorizeUser(p *print.Printer, isReauthentication bool) error { | |
| p.Debug(print.DebugLevel, "unable to bind port %d for login redirect: %s", port, listenerErr) | ||
| } | ||
| if listenerErr != nil { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should be extended with a check if ipv6ListenerErr is not nil. It's probably unlikely, but if someone uses already all the ports from 8000-8020, it will continue here without an error and try the authentication flow without an redirect url and the CLI will stop with a panic. I extended your python code, to simulate this case #!/usr/bin/env python3
import threading
from http.server import HTTPServer, SimpleHTTPRequestHandler
import socket
HOST = "::1"
PORT = 8000
class IPv6HTTPServer(HTTPServer):
address_family = socket.AF_INET6
for i in range(0, 21):
server = IPv6HTTPServer((HOST, PORT + i), SimpleHTTPRequestHandler)
print(f"Serving on http://[{HOST}]:{PORT+i}")
t = threading.Thread(target=server.serve_forever)
t.start() |
||
| return fmt.Errorf("unable to bind port for login redirect, tried from port %d to %d: %w", defaultPort, port, err) | ||
| return fmt.Errorf("unable to bind port for login redirect, tried from port %d to %d: %w", defaultPort, port, listenerErr) | ||
| } | ||
|
|
||
| conf := &oauth2.Config{ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
potential race condition:
If after this line another service binds to
[::1]:{port}we have the same situation as before.