-
Notifications
You must be signed in to change notification settings - Fork 327
Add OIDC Callback Server Port personalization #3646
Conversation
@@ -260,8 +261,12 @@ func (c *LoginCommand) loginOIDC(ctx context.Context) (string, int) { | |||
} | |||
refAM := &pb.Ref_AuthMethod{Name: c.flagAuthMethod} | |||
|
|||
if c.flagOidcCallbackServerPort == 0 { | |||
c.flagOidcCallbackServerPort = 8087 // Default port |
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.
Why was 8087 chosen? Is that a standard? This PR looks great, trying to understand why we shouldn't use an ephemeral port as the default.
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.
Why was 8087 chosen?
Because 8080 is where usually you might have other apps running, 8087 is less likely to be used
Is that a standard?
Nope
trying to understand why we shouldn't use an ephemeral port as the default.
Having a dynamic port doesn't work well with some OIDC Providers, you can't just say "allow everything that matches localhost" for some of them, and thus you have to specify a somewhat static URL.
On top of that, Waypoint itself doesn't seem to support them via -allowed-redirect-uri=
- but I might be wrong on this one.
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.
Gotcha gotcha. Let's still allow the user to pass in '0' then to mean "dynamic port" by moving the default code to the flag registration and having the Default by 8087. That sets a default but doesn't remove any flexibility a user might need.
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.
On the other hand, you have the issue where people must specify at each iteration what is the needed port (0
for dynamic, or a custom port).
I don't think it makes sense to have to ask the users to do something like waypoint login -oidc-server-callback=8888
every time.
Can this be maybe moved to the server instead?
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.
If you set 8087 as the default in the flag, they won't have to specify it, it will default to 8087.
The reason it's on login is it's so specific to the temporary server that login has to spin up to finish the OIDC process.
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.
Hello! I'm also in favor of having this default to a random port, namely because OIDC providers should allow any port to be specified for the callback when the loopback address (localhost) is being used, as per this RFC from the IETF. With that said though, I think that allowing a custom port to be specified is helpful when working with OIDC providers that don't allow random ports.
This is an example implementation of this requirement on Vault as the OIDC provider.
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.
The reason it's on login is it's so specific to the temporary server that login has to spin up to finish the OIDC process.
Yes, I know that - I'm just saying that since this is OIDC dependant, maybe you want to "force" your clients to use a static port to work around the providers which might not be complaint with RFC8252.
Is my case isolated or are there other OIDC providers in the wild that are not allowing custom ports to be specified?
It could also be all an user error here, or some integration issue in our company, since this just violates the specs Joe linked.
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.
maybe you want to "force" your clients to use a static port to work around the providers which might not be complaint with RFC8252.
At this time we see value in having the default port be dynamic. We don't want to force users to use a static port by default, but we do agree that having the option to configure the port to be static is something that we want 😄 Allowing people to do this will help those who are using non-compliant clients to still be able to use their OIDC provider.
I agree with @evanphx. This has to be fixed on the OIDC provider as it's a violation of the RFC. Let me close this issue, I'm anyways tracking the resolution of our issue on the OIDC provider side: |
Fixes #3645
This PR switches from using a dynamic TCP port for the HTTP server that listens for the OIDC Callback in the CLI to using a static port (
8087
) or a custom one provided by the user via the-oidc-callback-server-port
flag.Example: