Add multiple port support #108
Conversation
Okta does not implement the BCP212 correctly, and does not allow wildcard ports in loopback addresses. This is our way of circumventing that in the event that one of the ports is taken.
Some browsers would send two requests, with one landing reliably between when the OAuth2Listener was closed (and thus, its channel was closed) and when the http server would be closed. This change solves this problem by closing the channel after the request is received and only ever processing a single request. Other requests will receive responses, but will be silently ignored
Instead of returning a struct with mutable struct, we can make it harder to cause a race condition by wrapping all of that state instead of a function closure which cannot be modified from the outside, and by using sync.Once() to ensure that only one request is ever handled. We only need to have the sync.Once() for close()ing the channel to permit a user to permit a user to free the resources manually. It is still possible to cause a panic by closing this channel before receiving a request.
cli/consts.go
Outdated
| @@ -8,6 +8,7 @@ var ( | |||
| Version = "TBD" | |||
| BuildTimestamp = "BuildTimestamp is not set" | |||
| DownloadURL = "URL not set yet" | |||
| CallbackPorts = []string{"57468", "58888", "59999", "60000"} | |||
There was a problem hiding this comment.
Do you know how many ports Okta allows? I am wondering if we can add more if that makes sense.
There was a problem hiding this comment.
I'm not sure but I want to try to increase this to 16 ports that are non-contiguous from the 40 to 50k range
| getCmd.Flags().BoolP(FlagURLOnly, "u", false, "Print only the URL to visit rather than a user-friendly message") | ||
| getCmd.Flags().BoolP(FlagNoBrowser, "b", false, "Do not open a browser window, printing the URL instead") |
There was a problem hiding this comment.
What would be the use cases for these options? Is it supposed to be used by users or dev for debugging purposes? Can you give a few examples?
There was a problem hiding this comment.
These options are mirroring the options in keyconjurer login and are meant to be used with keyconjurer get --login account-name.
The main use-case for these is that if you specify the --login flag. The --login flag in keyconjurer get prompts keyconjurer to log you in if you're not logged in already, rather than erroring out. With these flags, you can control the behavior of keyconjurer, where keyconjurer either:
- opens a browser (default)
- displays a url only (not the most user intuitive)
- displays a human-friendly message on opening a browser with a given url (default if
-bspecified)
This feature has caused us a lot of problems over the years: * It would brick itself on Windows if you failed to download the file for any reason * Sometimes the DownloadURL would not be set * Relying on the releases always being at a specific URL meant that it was difficult to change it for any reason (For example, to support a stable and development release channel) We are removing this command and will instead move to supporting package managers like Homebrew for OSX and Chocolatey for Windows. It is recommended that folks who depend on this project do the same.
Context
As part of the OAuth2 authorization code flow keyconjurer conducts to retrieve an access token for the user, keyconjurer currently listens on the address
0.0.0.0:57468for inbound HTTP requests.As per RFC 8252, IdP MUST allow for ephemeral port support:
Unfortunately, Okta does not support this; It is not possible to specify any port using a wildcard; all ports must be explicitly specified in the OAuth2 client configuration.
The problem
At Riot, we have some users who are using Windows in particular that are finding that Windows is assigning the port
57468to other processes. It is possible that Windows could assign that port number as we are not making any reservations or modifications to the Windows registry - Doing so would likely involve using an installer or a great deal of Windows-specific code which there isn't the appetite to author right now; if we were to take that step it would be easier to simply make a custom URL scheme and bypass ports entirely (similar tospotify://).This PR attempts to resolve this by allowing KeyConjurer to try a number of different ports (which each must be explicitly passlisted by the IdP) before giving up and reporting an error to the user.