feat: Close security gaps OIDC implementation to make it work with Okta

[02strich]

The OIDC implementation in Kargo is a good starting point with using PKCE and looking to leverage latest security practices. Unfortunately, it still has some issues that make it incompatible with some providers, e.g. Okta. This PR adds usage of the state variable to add a further data point around call validation, and removes the offline_access forced scope as both are considered security requirements by Okta. Neither should create issues for existing customers as state usage is standards-compliant (and quite common) and the removed scope can be re-added in configuration.

Does that seem reasonable?

[netlify]

✅ Deploy Preview for docs-kargo-io canceled.

Name	Link
🔨 Latest commit	f415100
🔍 Latest deploy log	https://app.netlify.com/sites/docs-kargo-io/deploys/6732dba00971d800084bffab

[krancour]

We can take a look at this, but it may be a while.

My initial instinct is to avoid this sort of change. Part of the reason we ship with the option to integrate Dex is that it easily corrects for incompatibilities of this nature. For instance, some IDPs don't (yet) support PKCE. Others, like GitHub, don't support OIDC at all. With Dex, resolving these things is a matter of a little configuration instead of code changes.

[02strich]

@krancour happy to wait! In general

I would agree with you, though both changes I am making in this PR are security things that Okta just happens to enforce, while the standard just strongly recommends them. I believe they close issues with the implementation as is (I would expect even using it with Dex). Let me know what you think once you have time to look at it!

[krancour]

offline_access wasn't meant to be forced. The UI is doing it accidentally. The CLI does not. There's a separate issue open regarding that: #3005

The desired behavior is for it to be requested conditionally if the provider supports it. I believe @Marvin is already working on that.

I think that leaves just this to grapple with:

This PR adds usage of the state variable to add a further data point around call validation

I need to research this a bit more. As PKCE extends the authorization code flow, that would seem to imply all IDPs that support PKCE should be able to support PKCE with the optional use of state, but I'm hesitant to take that for granted. Let me poke around a few IDPs that I know Kargo already works well with without Dex as a middle-man and verify that they can indeed handle this.

[Marvin9]

Something (or I think exact similar) related to this had been done in ArgoCD - argoproj/argo-cd#17235 @krancour

[krancour]

Thanks @Marvin9!

[Tchoupinax]

Hello @02strich,
I guess you were on the right way. i tested integration with Authelia and it does not succeed because a state is missing? I guess we should try with a state in the URL. (The fact that ArgoCD added it is a good validation).

[dspereira004]

Dex, resolving these thing

I stumbled upon this PR while trying to set up Kargo with Okta. As people are saying here, the state is missing and therefore, we cannot use Kargo's oidc implementation.

Then, I followed your suggestion and tried to use Dex. I configured it in more different ways than I can count to make it work, but without success. The problem I get is always this one.

This is just to let you know that It looks like there is no PKCE support in Dex's OIDC connector as well, according to this closed PR and this new one which was not merged.

Therefore, if I'm not wrong, there's no way to have SSO in Kargo with Okta (and other IdPs that always require PKCE).

[krancour]

It's still on my todo list to research this to establish that PKCE with state doesn't break integration with IDPs Kargo's already known to work with directly.

But regarding this, in particular, in case it unblocks some folks:

This is just to let you know that It looks like there is no PKCE support in Dex's OIDC connector as well

The communication between Dex and the back end IDP need not use PKCE.

Unlike the UI and CLI, Dex isn't distributed to end users, so (provided your Secret-management game is on point) Dex can be trusted to keep the client secret a secret, which means the communication between Dex and the IDP should utilize the traditional authorization code flow.

[dspereira004]

It's still on my todo list to research this to establish that PKCE with state doesn't break integration with IDPs Kargo's already known to work with directly.

But regarding this, in particular, in case it unblocks some folks:

This is just to let you know that It looks like there is no PKCE support in Dex's OIDC connector as well

The communication between Dex and the back end IDP need not use PKCE.

Unlike the UI and CLI, Dex isn't distributed to end users, so (provided your Secret-management game is on point) Dex can be trusted to keep the client secret a secret, which means the communication between Dex and the IDP should utilize the traditional authorization code flow.

Well, it actually unblocked me and I have now SSO working :) I was being naive, and your comment was on point. Thanks for your help and (very) fast reply.

[krancour]

it actually unblocked me and I have now SSO working

Thanks for your help

My pleasure. And I'm happy to hear it's working for you now.