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.

Sign up for GitHub

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

Jump to bottom

feature: add ability to match any audience via '*' #6132

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feature: add ability to match any audience via '*' #6132

wants to merge 1 commit into from
+19 −3

Conversation

tvanhens
Copy link

@tvanhens tvanhens commented Jan 1, 2025 •
edited by flyte-bot
Loading

Why are the changes needed?

This change unblocks using Amazon Cognito as an external auth provider.

Amazon Cognito does not include the aud claim in access tokens as it optional in the spec. It is possible to add an audience for user-flow tokens via a pre-token-generation lambda., however, machine-to-machine tokens generated with the client_credentials flow do not trigger the pre-token-generation lambda. This change allows operators to opt into making the audience claim optional by specifying * for the allowedAudience configuration.

What changes were proposed in this pull request?

The ability to specify * for allowedAudience to allow any aud claim or missing aud claim.

How was this patch tested?

Unit tests in claims_verifier_test.go

Summary by Bito

Implementation of wildcard audience matching in authentication system, enabling '*' as a valid audience configuration. The changes enhance claims verification to support any audience claim or missing audience when '*' is configured. This update facilitates Amazon Cognito integration and includes configuration documentation updates.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 1

@welcome Welcome
Copy link

welcome bot commented Jan 1, 2025

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

@flyte-bot
Copy link
Collaborator

flyte-bot commented Jan 1, 2025 •
edited
Loading

Code Review Agent Run #a55e46

Actionable Suggestions - 0
Review Details
  • Files reviewed - 3 · Commit Range: 724246c..724246c
    • flyteadmin/auth/authzserver/claims_verifier.go
    • flyteadmin/auth/authzserver/claims_verifier_test.go
    • flyteadmin/auth/config/config.go
  • Files skipped - 0
  • Tools
    • Golangci-lint (Linter) - ✖︎ Failed
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • OWASP (Security Vulnerability) - ✔︎ Successful
    • GOVULNCHECK (Security Vulnerability) - ✖︎ Failed
    • SNYK (Security Vulnerability) - ✔︎ Successful

AI Code Review powered by Bito Logo

@tvanhens
feature: add ability to match any audience via '*'
21f5458 
Signed-off-by: Tyler van Hensbergen <tvanhens@gmail.com>
@flyte-bot
Copy link
Collaborator

flyte-bot commented Jan 1, 2025 •
edited
Loading

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Feature Improvement - Enhanced Authentication Flexibility with Wildcard Audience Support

- claims_verifier.go - Added support for wildcard audience matching and handling empty audience claims

- claims_verifier_test.go - Added test cases for wildcard audience matching functionality

- config.go - Updated configuration documentation to include wildcard audience option

@flyte-bot
Copy link
Collaborator

flyte-bot commented Jan 1, 2025 •
edited
Loading

Code Review Agent Run #34b05e

Actionable Suggestions - 1
  • flyteadmin/auth/authzserver/claims_verifier.go - 1
    • Consider handling empty audience lists · Line 28-28
Review Details
  • Files reviewed - 3 · Commit Range: 21f5458..21f5458
    • flyteadmin/auth/authzserver/claims_verifier.go
    • flyteadmin/auth/authzserver/claims_verifier_test.go
    • flyteadmin/auth/config/config.go
  • Files skipped - 0
  • Tools
    • Golangci-lint (Linter) - ✖︎ Failed
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • OWASP (Security Vulnerability) - ✔︎ Successful
    • GOVULNCHECK (Security Vulnerability) - ✖︎ Failed
    • SNYK (Security Vulnerability) - ✔︎ Successful

AI Code Review powered by Bito Logo

flyte-bot
flyte-bot reviewed Jan 1, 2025
View reviewed changes
flyteadmin/auth/authzserver/claims_verifier.go
break
}
}

if foundAudIndex < 0 {
if foundAudIndex < 0 && !expectedAudience.Has("*") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider handling empty audience lists

Consider checking for empty audience list before checking for wildcard match. If claims.Audience is empty and expectedAudience has *, we may want to allow that case.

Code suggestion
Check the AI-generated fix before applying
Suggested change
if foundAudIndex < 0 && !expectedAudience.Has("*") {
if foundAudIndex < 0 {
if len(claims.Audience) == 0 && expectedAudience.Has("*") {
return auth.NewIdentityContext("", claims.Subject, clientID, claims.IssuedAt, scopes, userInfo, claimsRaw)
} else if !expectedAudience.Has("*") {

Code Review Run #34b05e

Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

Copy link
Author

Choose a reason for hiding this comment

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

This case is covered and there is a test demonstrating this behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flyte-bot flyte-bot flyte-bot left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tvanhens @flyte-bot