search

akuity / kargo

Application lifecycle orchestration
https://kargo.akuity.io/
Apache License 2.0
1.77k stars 146 forks source link

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

Open 02strich opened 2 weeks ago

02strich commented 2 weeks ago

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[bot] commented 2 weeks ago

Deploy Preview for docs-kargo-io canceled.

Name Link
Latest commit f4151007263706e863509e5ac32a8e17db9b1f32
Latest deploy log https://app.netlify.com/sites/docs-kargo-io/deploys/6732dba00971d800084bffab
krancour commented 1 week ago

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 commented 1 week ago

@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!