api: support crls in client traffic policies #6955
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6955 +/- ##
=======================================
Coverage 71.09% 71.10%
=======================================
Files 227 227
Lines 40582 40582
=======================================
+ Hits 28851 28854 +3
+ Misses 10041 10035 -6
- Partials 1690 1693 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rudrakhp
force-pushed
the
api_ctp_crl
branch
2 times, most recently
from
f225537 to
3f29b52
Compare
| // Expects the content in a key named `crl.pem`. | ||
| // | ||
| // References to a resource in different namespace are invalid UNLESS there | ||
| // is a ReferenceGrant in the target namespace that allows the crl |
There was a problem hiding this comment.
thoughts on
crl:
refs:
- <>
onlyVerifyLeafCertificate:
There was a problem hiding this comment.
wdyt @envoyproxy/gateway-maintainers
There was a problem hiding this comment.
should this be
- Refs
- References
- CertificateReferences ?
There was a problem hiding this comment.
crl.refs[]should be explicit enough IMO.crl.references[]is more verbose and doesn't align with other APIs.- Not sure about
crl.certificateReferences[]as CRL itself is a list of certificates, so technically eachcertificateReferences[i]can be a list of certificates, might be technically inaccurate. Is this commonly used anywhere else?
api/v1alpha1/tls_types.go
Outdated
|
|
||
| // A reference to a Kubernetes ConfigMap or a Kubernetes Secret, | ||
| // containing the certificate revocation list in PEM format | ||
| // Expects the content in a key named `crl.pem`. |
There was a problem hiding this comment.
is crl.pem the most commonly used key in the ecosystem ?
There was a problem hiding this comment.
It's a mix between crl.pem and ca.crl. Both the Kubernetes project and nginx as well as HAProxy use ca.crl in their controllers, Contour and Ambassador use crl.pem, for example.
There was a problem hiding this comment.
Checked Istio as well, changing it to ca.crl since most recent projects seem to be using the same.
rudrakhp
force-pushed
the
api_ctp_crl
branch
from
3f29b52 to
ab077dd
Compare
Signed-off-by: Rudrakh Panigrahi <[email protected]>
| // Crl specifies the crl configuration that can be used to validate the client initiating the TLS connection | ||
| // +optional | ||
| // +notImplementedHide | ||
| Crl *CrlContext `json:"crl,omitempty"` |
There was a problem hiding this comment.
is CRL well known enough ? does most of the ecosystem use this acronym or should we use the long form here ?
There was a problem hiding this comment.
During the sync I mentioned a preference for a longer name but I think that's wrong. NGINX, HAProxy, and other load balancers all use the abbreviated crl so this should be fine.
- https://www.haproxy.com/documentation/haproxy-runtime-api/reference/set-ssl-crl-file/
- https://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_crl
- It's also abbreviated in many security research/government docs - https://csrc.nist.gov/glossary/term/certificate_revocation_list
|
/retest |
4 participants
What type of PR is this?
api: support configuring crls in client traffic policies
Which issue(s) this PR fixes:
Ref #3021
Release Notes: No