Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kargo OIDC Azure Entra Id is missing code_challenge_methods_supported #2338

Closed
4 tasks done
BasJ93 opened this issue Jul 24, 2024 · 10 comments
Closed
4 tasks done

Kargo OIDC Azure Entra Id is missing code_challenge_methods_supported #2338

BasJ93 opened this issue Jul 24, 2024 · 10 comments

Comments

@BasJ93
Copy link
Contributor

BasJ93 commented Jul 24, 2024

Checklist

  • I've searched the issue queue to verify this is not a duplicate bug report.
  • I've included steps to reproduce the bug.
  • I've pasted the output of kargo version.
  • I've pasted logs, if applicable.

Description

When configuring Kargo to use Azure Entra Id as OIDC provider, an error is returned stating that there was an error getting the OIDC configuration. This error is due to the code_challenge_methods_supported field missing in the metadata. This is a known shortcoming in the Azure OIDC implementation.

The ArgoCD project had discussion #16649 regarding handling this case, and chose to drop the check in the frontend.

Steps to Reproduce

Configure Azure as the OIDC provider.

Version

v0.7.1

Logs

N/A even with debug level enabled.

Additional question

Is it an option to drop the check in the frontend like it was done in ArgoCD? Or is it an option to add a config flag that disables the check like proposed in kubelogin #858.

If this can not be fixed, what is the recommended way to integrate with Azure as the OIDC provider?

@BasJ93 BasJ93 changed the title Kargo OIDC Azure Entra Id Issuer does not match Kargo OIDC Azure Entra Id is missing code_challenge_methods_supported Jul 24, 2024
@krancour
Copy link
Member

I will take a closer look at this.

If this can not be fixed, what is the recommended way to integrate with Azure as the OIDC provider?

In the meantime, you might look into whether using Dex as a middle-man improves the situation at all.

@BasJ93
Copy link
Contributor Author

BasJ93 commented Jul 24, 2024

I actually pulled the code, and removed the lines in the front-end that run the check. This allows me to authenticate with Azure Entra Id.

For now I'll run our custom build, but I am interested in seeing the solution.

@krancour
Copy link
Member

@BasJ93 if you are at all able to investigate whether using Dex as a middle-man helps, it would speed up a more strategic resolution.

I haven't read the Argo CD issue yet, nor have I read up about this known short-coming of Entra yet. (I will as soon as I am caffeinated better.) But my very first instinct is to say that I don't want to litter front-end code with IDP-specific workarounds when the whole point of having Dex (optionally) built-in to Kargo is to smooth over incompatibilities with IDPs that have non-compliant (or non-existent) OIDC implementations.

@krancour
Copy link
Member

krancour commented Jul 24, 2024

I hadn't initially connected the dots that this was PKCE-related.

Argo CD's support for PKCE was actually modeled after Kargo's and implemented by my colleague @Marvin9.

IDPs not supporting (properly?) PKCE, even when they otherwise do support OIDC was one of the main drivers for us offering Dex as an optional middle-man.

I am double-checking right now with my own Entra ID tenant that Dex solves the problem and if that is the case, I would rather suggest that people using Entra, also use Dex over making Entra-specific concessions in the UI. I'm somewhat perplexed that Argo CD, which also supports Dex, didn't opt for that.

I'll circle back with what I find and hopefully will post working connector config for you.

@krancour
Copy link
Member

krancour commented Jul 24, 2024

With these values overrides on helm install, OIDC w/ Entra works flawlessly without any need for code modifications:

api:
  host: <wherever kargo is running; no leading protocol; include port if not 80 or 443; e.g. localhost:8000>
  oidc:
    enabled: true
    dex:
      enabled: true
      connectors:
      - type: microsoft
        id: microsoft
        name: Microsoft
        config:
          clientID: <redacted>
          clientSecret: $ENTRA_CLIENT_SECRET
          redirectURI: http(s)://<wherever kargo is running>/dex/callback
          tenant: <redacted>
      env:
      - name: ENTRA_CLIENT_SECRET
        valueFrom:
          secretKeyRef:
            name: entra
            key: clientSecret

@BasJ93
Copy link
Contributor Author

BasJ93 commented Jul 25, 2024

I will give that a shot, thank you!

Would it be an idea to add this example to the docs?

@krancour
Copy link
Member

krancour commented Jul 25, 2024

Would it be an idea to add this example to the docs?

If we did, it probably needs to go in a whole new page about OIDC setup, which itself probably belongs under a whole new "operator reference" section. (As docs grow, I'm feeling fairly certain that they need to be re-organized into "developer reference" for people who use Kargo and "operator reference" for people who install/administer Kargo.)

In the meantime, it could be one of the commented example connectors in the chart's values.yaml file.

@BasJ93
Copy link
Contributor Author

BasJ93 commented Jul 25, 2024

I can confirm the provided sample configuration works, so thanks for that!

And I agree, it feels like the docs should be split like the ArgoCD project does. Though, in my opinion the designation "developer reference" does not convey that it is meant for the users. It feels more like it targets contributors to the project. (That's how I felt reading through the ArgoCD docs as a developer who was trying to set up ArgoCD.)

Adding this to the values.yaml as a sample definitely works in the interim.

@krancour
Copy link
Member

Though, in my opinion the designation "developer reference" does not convey that it is meant for the users.

You're right. "User reference" will be more clear and I will keep that in mind.

Adding this to the values.yaml as a sample definitely works in the interim.

Any interest in adding that yourself? A maintainer would get to it eventually, but you may be faster.

@BasJ93
Copy link
Contributor Author

BasJ93 commented Jul 26, 2024

No problem! I created a PR for the addition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants