-
Notifications
You must be signed in to change notification settings - Fork 6.5k
feat: Add generic JWT authentication (Alpha) #22901
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
base: master
Are you sure you want to change the base?
feat: Add generic JWT authentication (Alpha) #22901
Conversation
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
…e audiences Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Co-authored-by: Josh Soref <[email protected]> Signed-off-by: Wes <[email protected]>
Co-authored-by: Josh Soref <[email protected]> Signed-off-by: Wes <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
❌ Preview Environment undeployed from BunnyshellAvailable commands (reply to this comment):
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #22901 +/- ##
==========================================
+ Coverage 59.96% 60.00% +0.04%
==========================================
Files 343 343
Lines 57848 58053 +205
==========================================
+ Hits 34686 34837 +151
- Misses 20375 20428 +53
- Partials 2787 2788 +1 ☔ View full report in Codecov by Sentry. |
Signed-off-by: applyinnovations <[email protected]>
552b595 to
378e283
Compare
|
Thank you for getting this across the line! |
|
@crenshaw-dev I believe this would be a great addition to argo-cd. am very interested to see this PR merged, who would be the right person to review this, I am currently available to address any feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding my review, a lot of comment due to the size of the change, but looks good overall. I still havent reviewed provider.go and provider_test.go files. All other files are reviewed.
|
|
||
| ## External JWT Authentication Current Status: Alpha (Since v3.0.0) | ||
|
|
||
| Argo CD can be configured to verify JSON Web Tokens (JWTs) issued by an external authentication provider. This allows you to integrate Argo CD with existing authentication systems that issue JWTs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this mutually exclusive with having an oidc/dex configuration? If so, the doc should mention that both cannot be configured.
| jwkSetURL: "https://auth.example.com/.well-known/jwks.json" | ||
| # Optional: How long to cache the JWKS | ||
| cacheTTL: "1h" | ||
| # Optional: Expected audience for the JWT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To stay in sync with the OIDC configuration, could it be allowedAudiences instead?
| 4. Extract the username and email from the specified claims | ||
| 5. Use these values to identify the user within Argo CD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple features in argo will use the groups claims and user info endpoint. This is possible to be configured using oidc and dex.
Since userInfo is another endpoint, how would that work in this case?
| google.golang.org/genproto/googleapis/api v0.0.0-20250218202821-56aae31c358a | ||
| google.golang.org/grpc v1.72.1 | ||
| google.golang.org/protobuf v1.36.6 | ||
| gopkg.in/square/go-jose.v2 v2.6.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This package is deprecated.
| // If it's *MapClaims (less common but possible), dereference and return. | ||
| if mapClaimsPtr, ok := claims.(*jwtgo.MapClaims); ok { | ||
| return *mapClaimsPtr, nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is causing this new behaviour?
| // Attempt to get settings manager from the verifier if possible | ||
| // This assumes the TokenVerifier implementation might expose settings | ||
| var settingsMgr *settings.SettingsManager | ||
| if sm, ok := authn.(*SessionManager); ok { | ||
| settingsMgr = sm.settingsMgr | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create another method on the interface, or provide another interface param to the middleware to GetToken(r *http.Request) string
| prov, provErr := mgr.provider() | ||
| if provErr != nil { | ||
| // If OIDC/Dex is not configured, but we reached here, the token is invalid | ||
| return nil, "", fmt.Errorf("token issuer (%s) is not '%s' and OIDC provider is not available: %w", issuer, SessionManagerClaimsIssuer, provErr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return nil, "", fmt.Errorf("token issuer (%s) is not '%s' and OIDC provider is not available: %w", issuer, SessionManagerClaimsIssuer, provErr) | |
| return nil, "", fmt.Errorf("failed to get provider for OIDC Validation: %w", provErr) |
| if errors.As(oidcErr, &tokenExpiredError) { | ||
| // Return minimal claims indicating SSO source for expired tokens | ||
| // Use issuer variable from above, handle potential error if it wasn't retrieved | ||
| issForExpired := "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default to sso if not found?
| issForExpired := "" | |
| issForExpired := "sso" |
| return nil | ||
| } | ||
|
|
||
| // Check if JWT config specifies a groups claim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comments
| } | ||
|
|
||
| // Helper for pointer to bool | ||
| func boolPtr(b bool) *bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use ptr package
|
|
||
| Verify(tokenString string, argoSettings *settings.ArgoCDSettings) (*gooidc.IDToken, error) | ||
|
|
||
| VerifyJWT(tokenString string, argoSettings *settings.ArgoCDSettings) (*jwtgo.Token, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having a new VerifyJWT method on the existing provider interface, shouldn't you have a new implementation of the provider interface for JWT token? or have a new provider totally in something like /util/jwt/provider.go
Closes #14250
Based on: #20928
Feature implemented by @wrmedford
This allows for generic JWTs to be used for authentication that are minted outside of Argo. Argo currently mints its own JWTs for auth outside of Dex, and this extends its capabilities to utilize JWTs that originate from Identity Aware Proxies.
Checklist: