-
Notifications
You must be signed in to change notification settings - Fork 596
Envoy jwt #12811
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: main
Are you sure you want to change the base?
Envoy jwt #12811
Conversation
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
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.
Pull Request Overview
This PR adds comprehensive JWT authentication functionality to the kgateway project, enabling JWT validation for HTTP routes and gateways.
- Adds JWT provider configuration through GatewayExtension CRD
- Implements JWT validation policies attachable to gateways and routes
- Supports JWKS from inline keys, files, or Kubernetes secrets
Reviewed Changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| api/v1alpha1/jwt.go | New API types for JWT validation configuration including providers, token sources, and JWKS |
| api/v1alpha1/gateway_extensions_types.go | Adds JWTProviders extension type to GatewayExtension spec |
| api/v1alpha1/traffic_policy_types.go | Adds JWT field to TrafficPolicySpec |
| internal/kgateway/extensions2/plugins/trafficpolicy/jwt.go | Core JWT translation logic converting CRD specs to Envoy JWT auth filters |
| internal/kgateway/extensions2/plugins/trafficpolicy/jtw_test.go | Unit tests for JWT translation functions |
| internal/kgateway/extensions2/plugins/trafficpolicy/gateway_extension.go | Resolves JWT providers into Envoy configuration |
| internal/kgateway/extensions2/plugins/trafficpolicy/traffic_policy_plugin.go | Integrates JWT handling into traffic policy plugin |
| test/e2e/features/jwt/suite.go | E2E test suite for JWT authentication |
| install/helm/kgateway-crds/templates/*.yaml | CRD definitions for JWT configuration |
| go.mod / go.sum | Adds go-jose/v3 dependency for JWT/JWKS handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/e2e/features/jwt/suite.go
Outdated
| err = s.testInstallation.Actions.Kubectl().ApplyFile(s.ctx, testdefaults.HttpbinManifest) | ||
| s.Require().NoError(err) | ||
|
|
||
| // Apply curl pod for testing | ||
| err = s.testInstallation.Actions.Kubectl().ApplyFile(s.ctx, testdefaults.CurlPodManifest) |
Copilot
AI
Nov 4, 2025
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.
Comment on line 119 says 'Apply curl pod for testing' but it's actually applying the httpbin manifest. Comment on line 122 is correct. Update line 119 comment to 'Apply httpbin for testing' or similar.
Signed-off-by: omar <[email protected]>
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.
None of my comments are blocking.
| // +kubebuilder:validation:MinItems=1 | ||
| // +kubebuilder:validation:MaxItems=100 | ||
| // +optional | ||
| HeaderSource []HeaderSource `json:"headers,omitempty"` |
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.
do we need a list for this and query params? seems like you only want to fetch from one place generally?
| // Name is the JWT claim name, for example, "sub". | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=2048 | ||
| Name string `json:"name"` |
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.
claims can be nested (they are JSON). do we support that?
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.
I think it technically would work using dot-separated paths
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=2048 | ||
| // +optional | ||
| InlineKey *string `json:"key,omitempty"` |
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.
nit: key is probably an inaccurate name. Its a keyset, not a key
api/v1alpha1/jwt_types.go
Outdated
|
|
||
| // SecretRef configures storing the JWK in a Kubernetes secret in the same namespace as the JWTValidationPolicy. | ||
| // +optional | ||
| SecretRef *corev1.LocalObjectReference `json:"secretRef,omitempty"` |
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.
Secret may be a bit odd given its not really secret.. configMap may be appropriate?
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.
technically a jwks can contain a private key, but I can't think where we'd need it? ConfigMap should be ok, but maybe with a note in the doc re: private keys (should someone copy-paste jwks from an IdP)
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.
changed to ConfigMap
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
| @@ -0,0 +1,77 @@ | |||
| --- | |||
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.
Let's add an e2e test to check jwt is working with rbac like this example: https://github.com/npolshakova/kgateway/blob/d7da5dfb9b2e626c6ce2ff764d85d768b1f042c2/test/kubernetes/e2e/features/jwt/testdata/jwt-rbac.yaml#L50
from the additional notes section: #11923
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.
added
| // +required | ||
| ExtensionRef *NamespacedObjectReference `json:"extensionRef"` | ||
|
|
||
| // TODO: add support for ValidationMode here (REQUIRE_VALID,ALLOW_MISSING,ALLOW_MISSING_OR_FAILED) |
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.
I think we should add the allow_missing.required/allow_missing_or_failed option here as well.
|
|
||
| // TODO: add support for ValidationMode here (REQUIRE_VALID,ALLOW_MISSING,ALLOW_MISSING_OR_FAILED) | ||
|
|
||
| // TODO(npolshak): Add option to disable all jwt filters. |
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.
We probably want this as well now that there is a pattern for it: https://github.com/kgateway-dev/kgateway/blob/main/api/v1alpha1/ext_auth_types.go#L27
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 isn't straightforward for jwt. would require composite filter similar to extproc.
the alternative is to disable using RBAC
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=2048 | ||
| // +optional | ||
| InlineKey *string `json:"key,omitempty"` |
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.
do we support PEM format here? or only JWKS format?
(asking as gloo edge supported PEM)
| // JWTProviders configures a map of unique JWTProvider name to JWTProviders | ||
| // +optional | ||
| // +kubebuilder:validation:MaxProperties=32 | ||
| JWTProviders map[string]JWTProvider `json:"jwtProviders,omitempty"` |
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 the name used for here?
can we use a list of map type instead of map? i think it renders more consistently?
is there a way to configure if this is pre or post ext auth?
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.
I think the provider name needs to be unique, but that can be enforced at the plugin level. We might want to add a way to configure pre/post ext auth using the kgateway.dev/policy-weight?
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.
you can validate uniqueness of a list in CEL as well FWIW
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
Description
Add envoy JWT policy using GatewayExtension. Based on Nina's work in #11445.
Co-authored by: npolshakova [email protected]
Change Type
/kind feature
Changelog
Additional Notes