Configurable JWT authentication to ateapi - #757
Conversation
|
I overall really like the idea of your PR and it honestly supersedes mine: #759 which is a much simpler version and focusses only on Human based authentication + K8 (only OIDC). Some comments:
|
Okie dokie, I'll post this a ready for review.
This can leak tokens into the shell so isn't a great idea, what about stdin or something like that?
👍
Absolutely. Do you think we should do that in a follow-up? |
|
Eitan Yarmush (@EItanya) yes, STDIN sounds good to me.
I would consider this a P2 since we aren't currently working towards having any other authentication mechanisms except the JWT for now. Let me know Taahir Ahmed (@ahmedtd) if you think otherwise. |
| ServiceAccountName string | ||
| ServiceAccountUID string | ||
| PodName string | ||
| PodUID string | ||
| SecretName string | ||
| SecretUID string | ||
| NodeName string | ||
| NodeUID string |
There was a problem hiding this comment.
[nit] Is it worthwhile to logically separate out the k8s-specific claims by placing them in a separate, inner struct, or just indicating their group with a comment?
| if key == nil && len(v.keys) > 0 { | ||
| v.lastUnknownKeyRefresh = now | ||
| } | ||
| keys, err := discoverKeysForIssuer(ctx, v.httpClient, v.issuer) |
There was a problem hiding this comment.
Once jwksRefreshInterval elapses, a token whose key may already be in the cache only verifies if this refresh succeeds. So a transient issuer outage 5+ minutes after the last refresh fails all JWT auth, even for keys we already hold.
I believe k8s' OIDC authenticator serves stale keys when refresh fails, should we do the same here?
| if key != nil && now.Sub(lastRefresh) < jwksRefreshInterval { | ||
| return key.PublicKey, nil | ||
| } | ||
| if key == nil && hasKeys && !lastUnknownKeyRefresh.IsZero() && now.Sub(lastUnknownKeyRefresh) < unknownKeyRefreshInterval { |
There was a problem hiding this comment.
Since we check hasKeys here, I believe this check does not guard against the case where the initial set of keys has not yet been fetched (possibly delayed due to an issuer outage), so each token validation would trigger a request to fetch keys unthrottled until the first set of keys is successfully fetched.
Should we implement a similar throttling mechanism to prevent unthrottled fetches before the initial set of keys is fetched?
There was a problem hiding this comment.
Failed initial fetches are now rate-limited as well.
| if provider.Issuer != issuer { | ||
| continue | ||
| } |
There was a problem hiding this comment.
[nit] Is it worth providing a log if no issuers match to make it easier to debug that case?
There was a problem hiding this comment.
Added a debug-level unmatched-issuer log to avoid unauthenticated warning spam.
| func Verify(ctx context.Context, httpClient *http.Client, jwt string, expectedIssuer, expectedAudience string, now time.Time) (*Claims, error) { | ||
| return NewVerifier(expectedIssuer, []string{expectedAudience}, httpClient).Verify(ctx, jwt, now) | ||
| } |
There was a problem hiding this comment.
IIUC this is dead code now, since we call verifier.Verify in cmd/ateapi/main.go. Should we remove?
There was a problem hiding this comment.
Removed the unused package-level helper and updated tests to use the cached verifier.
| return fmt.Errorf("at least one JWT provider is required") | ||
| } | ||
| for i, provider := range cfg.JWTProviders { | ||
| if provider.Name == "" || provider.Issuer == "" || provider.Verify == nil { |
There was a problem hiding this comment.
Should we also validate uniqueness of Issuer in the list of JWTProviders?
|
Hi Eitan Yarmush (@EItanya), are you working on this PR? |
Yup! I was focused on #708, but that’s in so I’ll go full steam on this now. The GitHub CI situation yesterday really messed me up |
…-auth # Conflicts: # cmd/ateapi/internal/actoridentity/actoridentity.go # cmd/ateapi/internal/actoridentity/actoridentity_test.go # cmd/ateapi/main.go
|
Addressed in the latest push: --token-file=- now reads one token from stdin. The unverified issuer only selects a configured verifier and grants no authority; signature, issuer, audience, subject, and time validation happen immediately afterward, since cryptographic validation requires first selecting the issuer keys. Agreed that authenticator chaining should remain a P2 follow-up when we add a non-JWT mechanism. |
Summary
Fixes #733 by adding configurable JWT authentication to ateapi.
Supports multiple OIDC/JWT providers through a YAML configuration file.
Validates issuer, audience, signature, and token lifetime.
Supports custom CAs and authenticated OIDC discovery for Kubernetes
ServiceAccount JWTs.
Restricts MintJWT to the configured actor-identity provider.
Replaces the existing --client-jwt-* flags.
RBAC remains follow-up work.
Testing