-
Notifications
You must be signed in to change notification settings - Fork 630
Support for configurable tls in remote jwks store #13014
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
Support for configurable tls in remote jwks store #13014
Conversation
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
…ayPolicy Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[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 introduces support for configurable TLS options when connecting to remote JWKS sources in the JWKS store. It allows users to specify custom TLS configurations via BackendTLSPolicy by setting a backendRef on an AgentgatewayPolicy.
Key changes include:
- Refactored JWKS store architecture to use event-driven updates via channels instead of queue-based polling
- Added support for custom TLS configurations per JWKS source through
BackendTLSPolicyintegration - Separated concerns into dedicated controllers: policy controller for JWKS source changes and ConfigMap controller for persistence
- Improved security by removing default
InsecureSkipVerify: truefrom HTTP clients
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
internal/kgateway/jwks/jwks_store.go |
Refactored to use channel-based updates; added mapping between ConfigMap names and JWKS URIs |
internal/kgateway/jwks/jwks_fetcher.go |
Added TLS configuration support per keyset; simplified keyset management API; removed default insecure TLS |
internal/kgateway/jwks/jwks_cache.go |
Added thread-safe methods with mutex locks; removed comparison logic from add operation |
internal/kgateway/jwks/config_map_syncer.go |
Simplified by removing write logic and KRT collection; now only handles loading ConfigMaps |
internal/kgateway/agentjwksstore/policy_controller.go |
New controller that watches policies and creates JWKS sources with TLS configs from BackendTLSPolicy |
internal/kgateway/agentjwksstore/cm_controller.go |
New controller that persists JWKS to ConfigMaps using event queue pattern |
internal/kgateway/controller/start.go |
Updated to initialize new controllers in correct order |
internal/kgateway/jwks/jwks_fetcher_test.go |
Updated tests to use new defaultJwksClient field name |
api/v1alpha1/agentgateway_policy_types.go |
Made BackendRef a pointer and JwksUri required; removed mutual exclusivity validation |
install/helm/kgateway-crds/templates/gateway.kgateway.dev_agentgatewaypolicies.yaml |
Updated CRD to reflect API changes |
api/v1alpha1/zz_generated.deepcopy.go |
Generated code for pointer-based BackendRef deep copy |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
…-controller Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
…-controller Signed-off-by: Dmitri Dolguikh <[email protected]>
…-controller Signed-off-by: Dmitri Dolguikh <[email protected]>
…-controller Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
| // +kubebuilder:validation:Pattern=`^(https|http):\/\/[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)*(:\d+)?\/.*$` | ||
| // +optional | ||
| JwksUri string `json:"uri,omitempty"` | ||
| // Path to IdP jwks endpoint. Default tls settings are used to connect to this url. |
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: would be nice to have a trivial example here on the format, e.g. /path/here.
we could also throw validation to e.g. enforce : is not included, etc.
| // Supported types: Service and Backend. | ||
| // +optional | ||
| BackendRef gwv1.BackendObjectReference `json:"backendRef,omitempty"` | ||
| // +required |
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: we can add some color here explaining that the backendRef determines where and how to establish a connection and allows policy to be attached etc.
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.
will do
| backends krt.Collection[*agentgateway.AgentgatewayBackend], | ||
| agentgatewayPolicies krt.Collection[*agentgateway.AgentgatewayPolicy]) JwksUrlBuilder { | ||
|
|
||
| policiesByTargetRefIndex := krtpkg.UnnamedIndex(agentgatewayPolicies, func(in *agentgateway.AgentgatewayPolicy) []TargetRefIndexKey { |
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.
just curious, do we not have an index like this elsewhere in the agw codebase?
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.
Not that I could find.
| return "", nil, fmt.Errorf("only static backends are supported; backend: %s, policy: %s", backendRef, types.NamespacedName{Namespace: defaultNS, Name: policyName}) | ||
| } | ||
|
|
||
| // TODO (dmitri-d) only inline tls config is supported atm, do we want to support attching AgentgatewayPolicy too? |
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 TODO is a bit confusing, is this saying we may want to support AgwPolicy being attached to the Backend being referenced?
If so I think we would want to do that in the future to have a consistent approach.
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'll remove the TODO, that's been implemented.
| Kind: string(*ref.Kind), | ||
| Group: string(ptr.OrEmpty(ref.Group)), | ||
| Namespace: refNamespace, | ||
| // no port, as policy targetRef may not have it |
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.
// no port, as policy targetRef may not have it
is there an implication here?
i.e. can AgwPolicy not sectionName specific ports?
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.
an issue here is that sectionName is optional and we don't know apriori if it's present in the policy's targetRef; so we either ignore it completely (an issue if there are multiple policies targeting the same service but different ports), or do multiple searches (with the port set first, then without the port).
| backendRef, types.NamespacedName{Namespace: refNamespace, Name: policyName}, err) | ||
| } | ||
| tlsConfig = tlsc | ||
| } else { // check if a |
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.
// check if a
is this just leftover cruft?
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.
indeed it is.
|
Feedback on required Following up on the discussion about keeping Use case: JWT authentication with public IdP JWKS endpoints (GitHub Actions, GitLab CI) Before: jwks:
remote:
uri: https://token.actions.githubusercontent.com/.well-known/jwks
cacheDuration: 30mAfter: # Need to create a Backend for a public HTTPS endpoint
backends:
github-jwks:
static:
host: token.actions.githubusercontent.com
port: 443
policies:
tls:
sni: token.actions.githubusercontent.com
# Then reference it
jwks:
remote:
jwksPath: .well-known/jwks
backendRef:
name: github-jwks
group: agentgateway.dev
kind: AgentgatewayBackend
cacheDuration: 30mFor public HTTPS endpoints with standard TLS (common for IdPs like GitHub, GitLab, Auth0, Okta, etc.), the Backend object adds a bit of extra configuration when custom TLS settings aren't needed. Thought: Would it be possible to make Thanks for considering! |
Description
This PR depends on changes in #13011. The PR introduces support for setting of tls options for connections to remote jwks sources in jwks store. This is accomplished by setting a
backendRefon aAgentgatewayPolicy, finding a matchingBackendTLSPolicy, and using it to configure tls options.If no
BackendTLSPolicyis present, an http client with default tls options is used (including settingInsecureSkipVerifyto false).Implemenation Notes
InsecureSkipVerify. I don't think we support this anywhere else, could be confusing.Change Type
/kind feature
Changelog
Additional Notes
Example of a policy using remote jwks, configured via static backend:
Example of a policy using remote jwks, configured via k8s service: