Add client certificate forwarding #13215
Conversation
gateway-bot
added
kind/feature
Signed-off-by: Ayush Dwivedi <[email protected]>
Ayushd785
force-pushed
the
add-client-certificate-forwarding
branch
from
26463a9 to
9131956
Compare
sheidkamp
left a comment
sheidkamp
left a comment
There was a problem hiding this comment.
What's there looks straightforward, though there are some gaps to address?
- It looks like this won't work without ForwardClientCertDetails being configured, which I don't see a way to currently do. Were you able to set this up and validate it e2e?
- Validation that ForwardClientCertDetails and mtls are set should be added
- Definitely need golden file translation tests: https://github.com/sheidkamp/kgateway/blob/main/pkg/kgateway/translator/gateway/gateway_translator_test.go
- Given the interconnected nature of this config with other bits of config, e2e tests would be ideal: https://github.com/sheidkamp/kgateway/blob/main/test/e2e/features/listener_policy/suite.go
| xffNumTrustedHops = ptr.To(uint32(*h.XffNumTrustedHops)) // nolint:gosec // G115: kubebuilder validation ensures safe for uint32 | ||
| } | ||
|
|
||
| clientCertForwarding := convertClientCertificateForwarding(h.ClientCertificateForwarding) |
There was a problem hiding this comment.
nit: could inline like convertHeaderMutations
|
|
||
| // ClientCertificateForwarding configures which parts of the client certificate | ||
| // should be forwarded to upstream backends via headers. | ||
| // This is only applicable when mTLS is configured on the listener. |
There was a problem hiding this comment.
The envoy docs say:
This field is valid only when [forward_client_cert_details](https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto#envoy-v3-api-field-extensions-filters-network-http-connection-manager-v3-httpconnectionmanager-forward-client-cert-details) is APPEND_FORWARD or SANITIZE_SET
The default value of forward_client_cert_details is "SANITIZE" and we do not set it anywhere in the code. It looks like we need to expose that field as well for this to work.
Also, we should validate that when ClientCertificateForwarding is configured, mTLS is enabled on the listener and forward_client_cert_details is properly set.
There was a problem hiding this comment.
agree re forward_client_cert_details - to simplify, do you think that if this field is not nil we can set it to SANITIZE_SET automatically? I'm trying to think how to reduce config knobs for users (while still allowing to support the future feature set in the future if needed)..
Re verify that mTLS is enabled - I agree that would be the ideal, but i'm not sure there's an easy way from this part in the code to know if the listener is doing mTLS or not...
There was a problem hiding this comment.
+1 to rename the field to express the desired state using a verb forward_client_cert_details
|
Hi @sheidkamp, thank you for taking the time to review my PR. I’ll go through your feedback carefully and update the changes accordingly. |
5 participants
Description
Motivation:
This change adds support for forwarding client certificate details to upstream backends via headers, addressing a feature gap for users migrating from ingress-nginx. In ingress-nginx, this functionality is available via the
nginx.ingress.kubernetes.io/auth-tls-pass-certificate-to-upstream: "true"annotation, and users need equivalent functionality in kgateway.What changed:
ClientCertificateForwardingfield toHTTPSettingsinListenerPolicyAPISetCurrentClientCertDetailsconfiguration to Envoy'sHttpConnectionManagerprotoSubject: Certificate subjectCert: Full certificate in URL encoded PEM format (appears in XFCC header)Chain: Certificate chain in URL encoded PEM format (appears in XFCC header)Dns: DNS type Subject Alternative NamesUri: URI type Subject Alternative NameRelated issues:
Fixes #13047
Change Type
Changelog