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.

[remi-gelinas]

+1 to this. Considering the spec allows for optional state, I'd hope any IDP that Kargo currently works with would be fine with Kargo always adding the parameter

[FrederikNJS]

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.

@dspereira004 I've been fighting with setting up Dex and Kargo with Okta for a while now, would you be able to share how you configured Dex and Kargo to work with Okta?

[dspereira004]

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.

@dspereira004 I've been fighting with setting up Dex and Kargo with Okta for a while now, would you be able to share how you configured Dex and Kargo to work with Okta?

Sure, this is the relevant config on my side:

api:
  adminAccount:
    enabled: false
  host: <host>
  oidc:
    enabled: true
    admins:
      claims:
        groups:
          - <SOME-OKTA-GROUP>
    dex:
      enabled: true
      env:
        - name: CLIENT_SECRET
          valueFrom:
            secretKeyRef:
              name: okta
              key: clientSecret
      connectors:
        - id: okta
          name: Okta
          type: oidc
          config:
            issuer: https://example.okta.com/oauth2/default
            clientID: <clientId>
            clientSecret: $CLIENT_SECRET
            redirectURI: https://<host>/dex/callback
            # Without the following option, we get a 'Missing "email_verified" in OIDC claim' response from Okta.
            # More info: https://github.com/dexidp/dex/issues/1405
            insecureSkipEmailVerified: true
            scopes:
              - openid
              - profile
              - email
            # Groups claims (like the rest of oidc claims through dex) only refresh when the id token is refreshed
            # meaning the regular refresh flow doesn't update the groups claim. As such by default the oidc connector
            # doesn't allow groups claims.
            # Since we really need to have the groups claims, we enable the following property.
            # For additional details, check: https://dexidp.io/docs/connectors/oidc/
            insecureEnableGroups: true

On the Okta side, don't forget to have both PKCE and DPoP options unchecked. I hope that helps.

[johnpekcan]

Thank you for the PR @02strich!
Any update from Kargo on this @krancour ? Is the recommendation to still use Dex, or is there a desire to match ArgoCD's ability to do the authloop including the state field?

[krancour]

Working on something else entirely today, I noticed that the CLI's login command has actually been sending state along with the code challenge all along. It is only the UI that was not. With this realization, any concerns I had about verifying this change does not break compatibility with IDPs with which Kargo was already known to work (without Dex as a middle man) have been assuaged.

I'll resolve the merge conflicts myself and then I think it's safe to move forward with this change.

[netlify]

✅ Deploy Preview for docs-kargo-io canceled.

Name	Link
🔨 Latest commit	ef0babc
🔍 Latest deploy log	https://app.netlify.com/projects/docs-kargo-io/deploys/6864339092521d0008dd16af

[krancour]

I resolved the merge conflicts.

Tested against Entra ID to verify continued compatibility with IDPs that we knew to natively support the authorization code flow with PKCE. This was merely for good measure, because as noted above, the fact that the CLI has sent state all along gave me confidence.

I tested against auth0 and ran into some trouble. After a lot of troubleshooting, it turned out that Kargo wasn't very forgiving about a trailing slash in an issuer URL (which is why I'd removed it), but this backfired because by simply removing that, the issuer URL known to the client no longer matched the issuer URL in the token's iss claim. I ended up sneaking a fix into here that makes Kargo tolerant of a trailing slash in an issuer URL. I also added some additional debugging in the request authentication interceptor to speed up the troubleshooting next time. (There's always a next time.)

Things worked at that point.

I wanted to compare before/after for good measure. As long as my slash fix was present, Kargo already worked fine with auth0. As it turns out, auth0, despite being owned/offered by Okta, is as forgiving as many other IDPs are when state is missing...

So I tested with a free trial of the full-blown/commercial Okta offering and can validate that missing state was a problem before and that the problem is indeed fixed by this PR.

LGTM, but since this has my fingerprints all over it now, I will defer final review to @hiddeco. 🙏

[codecov]

Codecov Report

Attention: Patch coverage is 65.90909% with 15 lines in your changes missing coverage. Please review.

Project coverage is 53.17%. Comparing base (112a555) to head (ef0babc).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/server/option/auth.go	65.90%	12 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2916      +/-   ##
==========================================
+ Coverage   53.15%   53.17%   +0.01%     
==========================================
  Files         366      366              
  Lines       31984    32027      +43     
==========================================
+ Hits        17002    17031      +29     
- Misses      14102    14114      +12     
- Partials      880      882       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.