-
Notifications
You must be signed in to change notification settings - Fork 327
Conversation
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.
Looks great! A few random questions and needing a little logging.
Off-Topic comment, not related to the commits themselves: thanks for this feature! Definitely looks awesome already! 🚀 |
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.
Looks pretty solid to me! Just a bunch of notes and suggestions.
|
||
// Initialize our callback server | ||
srv := &CallbackServer{ | ||
url: fmt.Sprintf("http://%s/oidc/callback", ln.Addr().String()), |
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.
It feels weird to have this coming over unencrypted http, but I suppose it's all happening locally so it's probably ok.
Could we throw a self-signed cert on this?
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.
We could, but the only way to sniff traffic is root access, and I'm not sure we want to include root access to the machine as part of our threat model (its not part of any of our software threat models, including Vault)
// the protocol is ignored anyways for loopback. | ||
allowedUris := make([]string, len(am.AllowedRedirectUris)) | ||
copy(allowedUris, am.AllowedRedirectUris) | ||
allowedUris = append(allowedUris, |
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.
It might be nice to make an API someday to exposes these redirect URIs, because you need to allowlist them in the OIDC provider. I can imagine some really nice admin UI that lists them, or maybe a CLI command like waypoint auth-method configure oidc
.
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.
Yeah, I didn't want to save them on the auth method itself in case we ever changed them. I think docs and UI help is the way to go here for now.
// We default this to 30 days for now but that is arbitrary. We can change | ||
// this default anytime or choose to make it configurable one day on the | ||
// server. | ||
oidcAuthExpiry = 30 * 24 * time.Hour |
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.
Could we make this way shorter? It seems like we only need the "login" to live long enough to complete the one login request. Maybe I'm not understanding exactly what this governs.
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.
This is the token expiration itself, not the OIDC session. So this is a Waypoint timeout.
@@ -0,0 +1,5 @@ | |||
```release-note:feature |
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.
Just a general concern: I feel like people might assume that deleting a user from their OIDC provider will revoke their access to waypoint, but I don't think that will happen in our implementation.
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.
Will note this for the docs, I don't think any of our OIDC implementations enforce this right now. That's what the token expiration is about (and why GitHub makes you re-OIDC every single damn day lol).
Co-authored-by: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com>
Co-authored-by: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com>
This builds on #1752 and #1644 and introduces the end goal: OIDC-based authentication.
This allows any Waypoint operator to configure an OIDC provider, and then any user to use
waypoint login
to authenticate and setup their CLI client.Currently, a new user logging in with OIDC creates a new user account unless (1) an existing account with a link exists to that exact OIDC iss/sub combo or (2) an existing account exists with that email address. There are still a lot of improvements we can and have to make one day around how to do account mapping here from OIDC, but this basic model works.
Future Work
Beyond this PR, there is still work to be done:
waypoint user
CLI to get access to your current user and so onDemo
(All secrets and so on have been revoked from this demo, no worries)
Turn audio on and unmute the video, voiced over what's going on.
CleanShot.2021-07-12.at.14.04.01.mp4