[OIDC 16] Add IFederatedCredentialValidator for additional token validation#10306
Merged
Conversation
erdembayar
previously approved these changes
Dec 20, 2024
The base branch was changed.
erdembayar
reviewed
Dec 23, 2024
erdembayar
approved these changes
Dec 23, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Progress on https://github.com/NuGet/Engineering/issues/5911.
Depends on #10305.
This adds a new abstraction called
IFederatedCredentialValidatorwhich allows us to inject custom token validation code (i.e. closed source, "shim" code) into the token validation pipeline.0 or more
IFederatedCredentialValidatorimplementations can be used by the policy evaluator to perform additional validations on bearer tokens. These additional implementations come from theadd-insdirectory via MEF, much like existing shims.The flow of validation BEFORE the change is like this:
EntraIdTokenValidatorThis PR adds a new step between 3 and 4 where the request headers (in particular the
Authorizationheader) is passed to eachIFederatedCredentialValidatorto get additional token validation results. If either the built-in token validation or any additionalIFederatedCredentialValidatorsays the token is bad, it will be rejected.We pass all request headers, the detected issuer type (e.g. Entra ID vs. GitHub Actions), and unvalidated claims to the
IFederatedCredentialValidator. This essentially provides all the context we have to the shim at the time so it can make the most informed decision.At no point will as "valid" result from an
IFederatedCredentialValidatoroverride a "bad" result from the built-in token validation. In other words, if there is an inconsistency between various validation flows, we fail close and reject the token. We will log a warning if any of the validators disagree on valid vs. invalid.A
IFederatedCredentialValidatorcan returnNotApplicableif the validator is only meant for a specific issuer. For example,IFederatedCredentialValidatormight only know how to validate GitHub Actions tokens, not Entra ID tokens. The GitHub Actions example is for the future of course. Right now, the only supported issuer is Entra ID.I chose to plumb the request headers in from the service layer (and eventually from the controller action) instead of using the current
HttpContextso that the flow of data was clearer. I would have preferred to only provide the bearer token toIFederatedCredentialValidatorinstead of all headers, but our internal token validation library expects all request headers, not just the bearer token.Our internal token validation shim uses a newer version of Microsoft.Extensions.Caching.Memory so I had to bump up the version to avoid runtime issues.