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

OpenIDConnect Policy RFC #114

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

OpenIDConnect Policy RFC #114

wants to merge 11 commits into from

Conversation

alexsnaps
Copy link
Member

No description provided.

Signed-off-by: Alex Snaps <[email protected]>
Copy link

gitguardian bot commented Feb 10, 2025

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
15561461 Triggered GitLab OAuth Application Token 7baefe8 rfcs/0000-oidc-policy.md View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Signed-off-by: Alex Snaps <[email protected]>
@alexsnaps
Copy link
Member Author

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Just to be clear: those are fake 🤦

Co-authored-by: Thomas Maas <[email protected]>
Copy link
Member

@jasonmadigan jasonmadigan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice RFC. I get how it works as a layman, so that's probably a good thing. one comment/q re: cookies


It is important to note here that the access token is to be negotiated stored by the client. While this may change in
the future, currently the token is to either be provided by the appropriate HTTP header (`Authorization: Bearer`, which
is the default source), or by a Cookie.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

impl detail, but thoughts on cookie name? oidc connect novice, but unaware of a convention (as opposed to the authz: bearer header)

Copy link
Member

@jasonmadigan jasonmadigan Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thoughts on what has precedence if both present?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tokenSource: would be overridden to e.g. cookie. So either we make it aware of the name to look for (e.g. cookie["auth"]; or make it another config bit (e.g. tokenSourceKey?); and/or with a sensible default (e.g. Bearer?); I'm wondering if this wouldn't best be expressed as CEL ... "somehow".

wrt to precedence, it don't matter, as the user specify what "source" to use.

@eguzki
Copy link
Contributor

eguzki commented Feb 14, 2025

Nice API

I feel I cannot spot missing required fields, or unneeded fields until we do some hands on PoC. For instance, not sure the clientID is required field as the client is present in the token (the aud field). The documented AuthConfig example only requires the issuer.

PS: I would pick keycloak as OIDC provider, but gitlab.com is also good, why not!

@alexsnaps alexsnaps added the RFC Request For Comments label Feb 17, 2025
@alexsnaps
Copy link
Member Author

not sure the clientID is required field as the client is present in the token (the aud field). The documented AuthConfig example only requires the issuer.

Little confused here: what token? The client_id is required to obtain a token, as done here in that same doc

PS: I would pick keycloak as OIDC provider, but gitlab.com is also good, why not!

The plan is to have this working with keycloak too, as well as hopefully most (all?) providers out there. The reason I didn't use keycloak is two fold:

  • We already use it in many other cases; and
  • It's another dependency for users to install/get running; I wanted a thing that's out there today. I had hoped for github (as that's probably something our users already have an account on), but that's no OIDC provider...

@eguzki
Copy link
Contributor

eguzki commented Feb 19, 2025

Little confused here: what token? The client_id is required to obtain a token, as done here in that same doc

I am referring to the JWT token. I cannot tell for sure, as I would need to test it. AFAICT, the downstream user first gets a valid token with provided client_id (following oauth2 workflow for an specific flow type). That (jwt) token is then added to the request. The gateway can download (public) keys (belonging to one SSO realm and not client_id) to verify the JWT and once validated, it can trust on the provided data from the token, one field of which is the client_id. Thus, the gateway does not need any configuration having a client_id to verify JWT tokens. Only the SSO provider and realm.

I could be wrong :)

@alexsnaps
Copy link
Member Author

I am referring to the JWT token. I cannot tell for sure, as I would need to test it. AFAICT, the downstream user first gets a valid token with provided client_id

I get even more confused now! As you say you need a client_id to get to a token. In order to get the authorization code, the user needs to be redirected to the issuer's endpoint and start the flow. That very request, including in the link you provided yourself and then highlighted again above, requires that client_id... So how is it that:

not sure the clientID is required field as the client is present in the token

I must be completely missing the point you are trying to make... 😕

@alexsnaps
Copy link
Member Author

alexsnaps commented Feb 20, 2025

not sure the clientID is required field as the client is present in the token

The point was that it isn't required per se to "merely" do token verify the token, which is indeed correct. But token verification isn't enough and this proposal concerns itself with the entire flow. So the client_id is, if only for that, required to craft the URI to redirect users to in order to obtain said token (and probably check that verified incoming tokens to be the ones expected).

I think this talking point can be considered "resolved"

Signed-off-by: Alex Snaps <[email protected]>
@alexsnaps
Copy link
Member Author

I started this draft RFC describing how I plan on implementing the infrastructure that would support implementing such OpenIDConnectPolicy, as explained in this RFC.

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

Successfully merging this pull request may close these issues.

5 participants