sso: add OIDC login RPC, HTTP, and CLI workflow #15764
Conversation
jrasell
force-pushed
the
jrasell/gh-13120-oidc-login
branch
from
ef798f6 to
522bb83
Compare
jrasell
force-pushed
the
jrasell/gh-13120-oidc-login
branch
from
522bb83 to
001efa6
Compare
There was a problem hiding this comment.
@jrasell I've taken an initial pass over this and it's looking good so far. I really appreciate the solid comments you've left on some of the gritty bits so that folks coming along later can orient themselves.
The double-forwarding in the RPC handler seems unfortunate but given that we need to query the state to know whether we need to forward to the authoritative region I don't really see any way around it unless AuthMethod was assumed immutable, which as I understand it isn't the case.
| // Create a context, so a server that is shutting down can correctly | ||
| // shut down the cache loop and OIDC provider background processes. | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
|
|
||
| result := &ProviderCache{ | ||
| providers: map[string]*oidc.Provider{}, | ||
| cancel: cancel, | ||
| } | ||
|
|
||
| // Start the cleanup timer | ||
| go result.runCleanupLoop(ctx) |
There was a problem hiding this comment.
IMO this is a bug in cap: the NewProvider method doesn't take a context, and uses the background context (ref provider.go#L70 at least in the config we pass in. Obviously we have to deal with that for now in this PR but it'd be worth pushing back on that upstream with whomever maintains that library (or submitting a PR and seeing what happens). It'd cut out a lot of complexity in this cache.
There was a problem hiding this comment.
Oh that is a great shout. I have a list of items to raise against cap and ultimately try and add. I'll add this item to the list, thanks.
| // TODO(jrasell) find a way to test the full login flow from the CLI | ||
| // perspective. |
There was a problem hiding this comment.
It's probably a huge chore but useful for E2E if we could spin up a dummy OIDC provider server. Does cap have something like that already?
There was a problem hiding this comment.
Cap does have a local test provider we can use. I think I have figured out the other blocker I had, so will look into adding that to this PR. Apologies for not doing it sooner, my initial aim with this PR was to allow us to manually run full e2e tests using multiple real providers.
There was a problem hiding this comment.
Oh, sorry, I didn't mean to suggest we had to block this PR on that. If we've manually tested this already it seems totally reasonable to add automated E2E in a separate PR!
| // duration. This ensures that we force refresh our provider info periodically | ||
| // in case anything changes. | ||
| func (c *ProviderCache) runCleanupLoop(ctx context.Context) { |
There was a problem hiding this comment.
We don't need to do it in this PR but we might want to consider whether the RPC handlers can/should force a cache flush of the provider if they hit errors that are caused by the upstream provider (ex. this is a very long lived HTTP client and I don't know how cap has its TCP connections configured to handle a DNS change at the provider; ref golang/go#23427)
There was a problem hiding this comment.
Great shout; I'll note this for future thought and raise as a followup issue.
jrasell
force-pushed
the
jrasell/gh-13120-oidc-login
branch
from
001efa6 to
f1df88b
Compare
This adds new OIDC endpoints on the RPC endpoint. These two RPCs handle generating the OIDC provider URL and then completing the login by exchanging the provider token with an internal Nomad token. The RPC endpoints both do double forwarding. The initial forward is to ensure we are talking to the regional leader; the second then takes into account whether the auth method generates local or global tokens. If it creates global tokens, we must then forward onto the federated regional leader.
The OIDC provider cache is used by the RPC handler as the OIDC implementation keeps long lived processes running. These process include connections to the remote OIDC provider. The Callback server is used by the CLI and starts when the login command is triggered. This callback server includes success HTML which is displayed when the user successfully logs into the remote OIDC provider.
jrasell
force-pushed
the
jrasell/gh-13120-oidc-login
branch
from
f1df88b to
847bf72
Compare
There was a problem hiding this comment.
This looks good @jrasell! I couldn't find anything but it is a pretty large PR and even though it has neat commits that separate by functionality (🙏), it's still a lot of code and at some point fatigue kicks in and I might've missed something. This being said, I did quite some testing of this code using vault as oidc provider and it seems to be working exactly as expected.
* Updated UI to handle OIDC method changes * Remove redundant store unload call
Ember Asset Size actionAs of d57b805 Files that got Bigger 🚨:
Files that stayed the same size 🤷:
|
Each commit targets a different section of the code and attempts to make reviewing easier.
Raising in draft initially so I can perform some additional local testing in parallel.
Related: #13120