feat: add ClientCertPrincipal to SecurityPolicy authorization - #9936
feat: add ClientCertPrincipal to SecurityPolicy authorization#9936joshuaecook wants to merge 6 commits into
Conversation
Signed-off-by: Joshua E Cook <jec@apple.com>
Signed-off-by: Joshua E Cook <jec@apple.com>
…zation Signed-off-by: Joshua E Cook <jec@apple.com>
…ed principal Signed-off-by: Joshua E Cook <jec@apple.com>
Signed-off-by: Joshua E Cook <jec@apple.com>
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 875d46157f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return nil, fmt.Errorf("unable to translate authorization rule: %w", err) | ||
| } | ||
|
|
||
| irPrincipal.ClientCert = rule.Principal.ClientCert |
There was a problem hiding this comment.
A CEL-only authorization rule is valid because AuthorizationRule requires principal or cel, but this unconditional dereference panics whenever such a rule omits principal. This regresses every valid CEL-only SecurityPolicy during gateway translation; copy ClientCert inside the existing rule.Principal != nil block.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I’d prefer to reject the combination of an unverified certificate and ClientCertPrincipal authorization, since allowing it could introduce a security issue.
| if len(san.URIs) == 0 && len(san.DNSNames) == 0 { | ||
| return predicates, nil |
There was a problem hiding this comment.
When subjectAltNames: {} is supplied, the new CEL validation accepts it because the object is present, but this branch silently contributes no certificate predicate. In a principal that also contains a CIDR, header, or JWT condition, the intended certificate requirement is therefore erased and the rule can authorize without matching any certificate identity; an empty SAN object should be rejected rather than treated as absent.
Useful? React with 👍 / 👎.
| case egv1a1.StringMatchRegularExpression: | ||
| return &matcherv3.StringMatcher{ | ||
| MatchPattern: &matcherv3.StringMatcher_SafeRegex{ | ||
| SafeRegex: &matcherv3.RegexMatcher{ | ||
| Regex: sm.Value, |
There was a problem hiding this comment.
For a client-certificate StringMatch with type: RegularExpression, admission only validates the enum and nonempty value, and this conversion embeds the value without compiling it. A malformed expression such as [ consequently reaches Envoy as an invalid RegexMatcher, causing the generated route configuration to be rejected instead of surfacing a SecurityPolicy validation error; validate it with the same regex helper used for authorization path expressions before producing xDS.
Useful? React with 👍 / 👎.
| | `jwt` | _[JWTPrincipal](#jwtprincipal)_ | false | | JWT authorize the request based on the JWT claims and scopes.<br />Note: in order to use JWT claims for authorization, you must configure the<br />JWT authentication in the same `SecurityPolicy`. | | ||
| | `headers` | _[AuthorizationHeaderMatch](#authorizationheadermatch) array_ | false | | Headers authorize the request based on user identity extracted from custom headers.<br />If multiple headers are specified, all headers must match for the rule to match. | | ||
| | `clientIPGeoLocations` | _[ClientIPGeoLocation](#clientipgeolocation) array_ | false | | ClientIPGeoLocations authorizes the request based on geolocation metadata derived from the client IP.<br />This field is supported for HTTPRoute and GRPCRoute authorization.<br />It is not supported for TCPRoute targets.<br />If multiple entries are specified, one of the ClientIPGeoLocation entries must match for the rule to match.<br />The client IP is inferred from the X-Forwarded-For header, a custom header, or the<br />direct downstream connection source address (the TCP peer of the connection terminated by Envoy).<br />You can use the `ClientIPDetection` field in the `ClientTrafficPolicy` to configure the client IP detection. | | ||
| | `clientCert` | _[ClientCertPrincipal](#clientcertprincipal)_ | false | | ClientCert authorizes the request based on the client certificate<br />presented during the mutual TLS handshake.<br />This principal requires that mTLS is configured for the gateway listener<br />receiving the request, via a `ClientTrafficPolicy` whose<br />`spec.tls.clientValidation.caCertificateRefs` validates the presented<br />client certificate. Without mTLS configured, no client certificate is<br />available and this principal will never match.<br />At least one of `subject` or `subjectAltNames` must be specified. When<br />both are set, both must match for the principal to match. Within<br />`subjectAltNames`, any matching SAN entry across DNS, email, IP, URI,<br />or otherNames satisfies the principal.<br />This principal is supported for HTTPRoute and GRPCRoute authorization<br />targets. It is not applicable to TCPRoute targets. | |
There was a problem hiding this comment.
The generated API reference tells users that email, IP, and OtherName SAN entries satisfy clientCert, while the API validations and translator added in this commit explicitly reject all three and support only URI and DNS SANs. Users following this reference will create policies rejected at admission, so regenerate or correct this description to list only the supported SAN types.
Useful? React with 👍 / 👎.
- Guard the ClientCert copy in buildAuthorization with the existing
rule.Principal != nil check. AuthorizationRule allows a CEL-only rule
(no principal), and the unconditional dereference panicked gateway-wide
translation for any SecurityPolicy containing one.
- Reject an empty subjectAltNames{} object via a new CEL rule instead of
silently treating it as "no SAN constraint" during translation, which
could over-authorize a rule that ANDs clientCert with another condition.
- Validate RegularExpression StringMatch values (subject, uris, dnsNames)
at admission using the existing regex helper, so a malformed pattern
fails SecurityPolicy validation instead of reaching xDS as an invalid
RegexMatcher and rejecting the whole route configuration.
- Regenerate the CRD bundles and API reference docs to match, correcting
stale doc text that claimed emailAddresses/ipAddresses/otherNames SANs
satisfy the principal, and fold in the AND/OR semantics wording raised
in the earlier internal review (subject AND subjectAltNames group; uris
and dnsNames OR-combine into one group).
Also fixes two pre-existing, unrelated defects surfaced while getting the
test/cel-validation module to actually build and pass (it never had, since
`go build` skips _test.go files and this was the first time `go test` ran
against it in this fork):
- api/v1alpha1.Principal is a pointer field; several existing test cases
constructed it as a value, which failed to compile.
- The CEL message for Principal's own emptiness check gained "clientCert"
when this feature was added; one pre-existing test's expected string
was never updated to match.
Signed-off-by: Joshua E Cook <jec@apple.com>
|
@codex review again please, I have addressed your findings in the latest commit. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eeba0c4390
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // +kubebuilder:validation:XValidation:rule="!has(self.subjectAltNames) || !has(self.subjectAltNames.emailAddresses)",message="emailAddresses is not supported in clientCert; use uris or dnsNames" | ||
| // +kubebuilder:validation:XValidation:rule="!has(self.subjectAltNames) || !has(self.subjectAltNames.ipAddresses)",message="ipAddresses is not supported in clientCert; use uris or dnsNames" | ||
| // +kubebuilder:validation:XValidation:rule="!has(self.subjectAltNames) || !has(self.subjectAltNames.otherNames)",message="otherNames is not supported in clientCert; use uris or dnsNames" | ||
| // +kubebuilder:validation:XValidation:rule="!has(self.subjectAltNames) || has(self.subjectAltNames.uris) || has(self.subjectAltNames.dnsNames)",message="subjectAltNames must specify at least one of uris or dnsNames" |
There was a problem hiding this comment.
The new validation rejects subjectAltNames: {}, but explicitly setting uris: [] or dnsNames: [] still makes the corresponding has(...) expression true because the field is present. Fresh evidence beyond the earlier finding is that these empty arrays have no MinItems constraint, while buildClientCertPredicate treats both zero-length lists as no SAN predicate; when this principal also contains a header, CIDR, or subject condition, authorization can therefore succeed without any SAN match. Check size(...) > 0 rather than field presence.
Useful? React with 👍 / 👎.
| if clientCert.Subject != nil { | ||
| subjectInputAny, err := proto.ToAnyWithValidation(&sslinput.SubjectInput{}) |
There was a problem hiding this comment.
When the listener's ClientTrafficPolicy uses mode: Request or mode: RequireAny, the existing TLS translation sets ACCEPT_UNTRUSTED, yet this predicate directly authorizes against the presented certificate's subject/SAN without checking that its chain was verified. An attacker can consequently present a self-signed certificate containing an allowed identity and satisfy a clientCert rule, despite the new API describing this as matching a validated peer certificate. Reject this combination or add a verified-certificate requirement before trusting these fields.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9936 +/- ##
==========================================
+ Coverage 81.31% 81.33% +0.01%
==========================================
Files 263 263
Lines 40870 40998 +128
==========================================
+ Hits 33233 33345 +112
- Misses 7637 7653 +16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // supported and will be rejected with a validation error. | ||
| // | ||
| // This principal is supported for HTTPRoute and GRPCRoute authorization | ||
| // targets. It is not applicable to TCPRoute targets. |
There was a problem hiding this comment.
I think this could work at L4 too, as a follow-up.
| TypedConfig: uriSanInputAny, | ||
| }, | ||
| Matcher: &matcherv3.Matcher_MatcherList_Predicate_SinglePredicate_ValueMatch{ | ||
| ValueMatch: stringMatcher, |
There was a problem hiding this comment.
On the SAN matchers: envoy.matching.inputs.uri_san and dns_san collapse all SANs of that type into one comma-joined string, and the list on the EG API side is the set of accepted values.
For a cert with URI:spiffe://ns/foo/sa/a and URI:spiffe://ns/foo/sa/b, the input value is spiffe://ns/foo/sa/a,spiffe://ns/foo/sa/b, so this doesn't match:
uris:
- value: "spiffe://ns/foo/sa/a" # Exact
Prefix only matches the first SAN, Suffix only the last.
Could the translator absorb this, lowering Exact/Prefix/Suffix on uris/dnsNames to
regexes anchored on the comma boundaries? internal/utils/regex does something
similar already for :path. RegularExpression probably can't be wrapped safely and
would just need a doc note.
Worth a 2-SAN cert in the testdata either way — the current e2e cert has a single
SAN, so this wouldn't show up there.
There was a problem hiding this comment.
@zhaohuabing thanks so much for your review and feedback. I think you are correct that this may not handle certs with multiple SANs.
As I understand the practical usage of multiple SANs is to provide multiple a/k/a identifiers for the caller, so that the same the same cert could be presented to different services, and each service may know the peer under a different name. I don't know if there's a use case where a single service would require the peer to present a 2-SAN cert.
Your suggestion to lower the string matching operators into anchored regexes makes sense to me for the common case, where Exact X is translated to a regex (^|,)X(,|$) and similarly for prefix & suffix.
As for the RegularExpression operator, I think passing it through with a clear doc note that explains how it matches against the comma-spliced value is a fair compromise. This at least gives an escape hatch for the uncommon case of requiring a 2-SAN presentation.
If you agree with this approach, I'll update this PR.
3 participants
What type of PR is this?
feat
What this PR does / why we need it:
Adds
clientCertas a principal type underSecurityPolicy.authorization.rules[].principal, authorizing requests against the client certificate presented during mTLS. This gives per-route access control keyed on the workload or service identity carried in the peer certificate.Match types map to Envoy's native SSL matching inputs:
subjectSubjectInputRegularExpressionsubjectAltNames.urisUriSanInputsubjectAltNames.dnsNamesDnsSanInputemailAddresses,ipAddresses, andotherNameshave no native Envoy input. A CEL rule rejects them at admission rather than letting translation silently drop the matcher.The translator lowers a principal to
AND(subject, OR(uris…, dnsNames…)). The Subject DN, when set, must match, and the certificate must carry at least one of the listed URI or DNS identities. URI and DNS SANs share a single OR group rather than AND-combining across types, since a workload certificate carries a URI SAN and a service certificate a DNS SAN, rarely both.mTLS is a prerequisite.
clientCertmatches the certificate from the TLS handshake, which is present only when aClientTrafficPolicysetsspec.tls.clientValidation.caCertificateRefson the listener. Without mTLS, no certificate is presented and the principal never matches.clientCertapplies to HTTPRoute and GRPCRoute targets. Attaching it to a TCPRoute setsAccepted=False.Which issue(s) this PR fixes:
Fixes #5392
Release Notes: Yes