auth: login: explicit check for ipv6 port bindings#1251
auth: login: explicit check for ipv6 port bindings#1251svetlyopet wants to merge 1 commit intostackitcloud:mainfrom
Conversation
…stener for redirectUrl
|
This PR was marked as stale after 7 days of inactivity and will be closed after another 7 days of further inactivity. If this PR should be kept open, just add a comment, remove the stale label or push new commits to it. |
|
Adding a comment to keep the PR active. |
|
This PR was marked as stale after 7 days of inactivity and will be closed after another 7 days of further inactivity. If this PR should be kept open, just add a comment, remove the stale label or push new commits to it. |
marceljk
left a comment
There was a problem hiding this comment.
Thanks for you contribution! looks good to me 😃
| if ipv6ListenerErr != nil { | ||
| continue | ||
| } | ||
| _ = ipv6Listener.Close() |
There was a problem hiding this comment.
potential race condition:
If after this line another service binds to [::1]:{port} we have the same situation as before.
| @@ -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 { | |||
There was a problem hiding this comment.
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()
Description
Fixes 1250
This PR tries to address the issue of the net.Listen() not being able to see that the requested port is already taken on MacOS.
This happens specifically on MacOS, when there is already a service bound to a port on the IPv6 localhost interface, but the port is free on the IPv4 localhost interface. The net.Listen() method sees that the port is open on the 127.0.0.1 interface, and binds to that one. When the user gets redirected to localhost in the browser, it can redirect to either one of the services using the same port on the different interfaces(seems like localhost resolving to the IPv6 is preferred).
Testing
I tested by running the following python service locally, and executing
stackit auth loginafter that which replicates the issue and can be observed.Checklist
make fmtmake generate-docs(will be checked by CI)make test(will be checked by CI)make lint(will be checked by CI)