Skip to content
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.

Sign up for GitHub

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

allow multiple SANS in upstream validation #5849

Merged
merged 21 commits into from
Jan 5, 2024
Merged

allow multiple SANS in upstream validation #5849

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
f238b48
internal: allow multiple SANs in upstream validation
KauzClay Oct 13, 2023
bb91438
docs: add changelog
KauzClay Oct 13, 2023
55d8ca0
api: fix deprecated comment
KauzClay Oct 13, 2023
ea42eb2
update generated code
KauzClay Oct 13, 2023
75efa44
add deprecation changelog
KauzClay Oct 13, 2023
6d855ec
internal: add happypath test for multiple sans on upstream tls context
KauzClay Oct 13, 2023
b716c6b
add limitations to all fields in upstream validation
KauzClay Nov 17, 2023
455aee2
update examples
KauzClay Nov 17, 2023
457ff9c
api: limit length of subject name to 100 in upstream validation
KauzClay Dec 15, 2023
a3ac00b
docs: update changelog to call out new limit
KauzClay Jan 2, 2024
fdf5c36
docs: update upstream tls docs
KauzClay Jan 4, 2024
9e83325
increase length limits
KauzClay Jan 4, 2024
4cb1fc2
increase subjectname max length
KauzClay Jan 4, 2024
dad53b3
decrease subjectname max length
KauzClay Jan 4, 2024
b998cc1
increase max length again, but not as high
KauzClay Jan 4, 2024
319c8c6
dag: consolidate into subjectNames
KauzClay Jan 4, 2024
4ec72ee
dag: fix test
KauzClay Jan 5, 2024
6b2bf22
dag: test lookupupstreamvalidation
KauzClay Jan 5, 2024
7fe2cf5
docs: remove todo
KauzClay Jan 5, 2024
4714e6f
docs: correct changelog
KauzClay Jan 5, 2024
b291479
test: add cel validation e2e test
KauzClay Jan 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions apis/projectcontour/v1/httpproxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1306,14 +1306,26 @@ type HeaderValue struct {
}

// UpstreamValidation defines how to verify the backend service's certificate
// +kubebuilder:validation:XValidation:message="subjectNames[0] must equal subjectName if set",rule="has(self.subjectNames) ? self.subjectNames[0] == self.subjectName : true"
type UpstreamValidation struct {
// Name or namespaced name of the Kubernetes secret used to validate the certificate presented by the backend.
// The secret must contain key named ca.crt.
// The name can be optionally prefixed with namespace "namespace/name".
// When cross-namespace reference is used, TLSCertificateDelegation resource must exist in the namespace to grant access to the secret.
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=256
skriss marked this conversation as resolved.
Show resolved Hide resolved
CACertificate string `json:"caSecret"`
// Key which is expected to be present in the 'subjectAltName' of the presented certificate.
// Deprecated: migrate to using the plural field subjectNames.
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=100
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know what the highest possible max we could use here is? The higher it is, the less likely it is to break a user, and given that this is effectively a breaking API change, I'd rather go as high as we possibly can to try to minimize the set of affected users.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, I will try to find the max possible value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, it seems to accept a max length of 250.

Aside, do you have any idea on how to better test this?

I've tried to apply the CRD to my own cluster, and I don't get an error, but then when I push and the PR tests run, it fails after installing giving me the warning about the CEL score.

Copy link
Member

@skriss skriss Jan 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, not sure off the top of my head, @sunjayBhatia any thoughts here since you were playing around with this earlier?

I do feel much better about 250 vs. 100 as a limit, seems much less likely to have any actual user impact.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what version of k8s is your cluster @KauzClay? You don't happen to have the CustomResourceValidationExpressions feature gate off?

Also TIL https://playcel.undistro.io/ exists which might be useful

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to test the CEL validation itself works as expected, we could add an e2e test here too:

contour/test/e2e/httpproxy/httpproxy_test.go

Lines 57 to 66 in 0b8ab80

// Contains specs that test that kubebuilder API validations
// work as expected, and do not require a Contour instance to
// be running.
var _ = Describe("HTTPProxy API validation", func() {
f.NamespacedTest("httpproxy-required-field-validation", testRequiredFieldValidation)
f.NamespacedTest("httpproxy-invalid-wildcard-fqdn", testWildcardFQDN)
f.NamespacedTest("invalid-cookie-rewrite-fields", testInvalidCookieRewriteFields)
})

e.g. try to create an HTTPProxy with upstreamvalidation set such that the resource should be rejected, and appropriate assertions etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was using a 1.27.7 from GKE, but I didn't know about CustomResourceValidationExpressions feature gate though, maybe that is turned off somehow?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/#feature-gates-for-graduated-or-deprecated-features that feature gate should be on by default in 1.27, but maybe GKE is doing something weird?

SubjectName string `json:"subjectName"`
// List of keys, of which at least one is expected to be present in the 'subjectAltName of the
// presented certificate.
// +optional
// +kubebuilder:validation:MinItems=1
// +kubebuilder:validation:MaxItems=8
SubjectNames []string `json:"subjectNames"`
skriss marked this conversation as resolved.
Show resolved Hide resolved
}

// DownstreamValidation defines how to verify the client certificate.
Expand Down
9 changes: 7 additions & 2 deletions apis/projectcontour/v1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion apis/projectcontour/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions changelogs/unreleased/5849-KauzClay-deprecation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
## Deprecate `subjectName` field on UpstreamValidation

The `subjectName` field is being deprecated in favor of `subjectNames`, which is
an list of subjectNames. `subjectName` will continue to behave as it has. If
using `subjectNames`, the first entry in `subjectNames` must match the value of
`subjectName`. this will be enforced by CEL validation.
5 changes: 5 additions & 0 deletions changelogs/unreleased/5849-KauzClay-minor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
## Allow Multiple SANs in Upstream Validation section of HTTPProxy

This change introduces a max length of 100 characters to the field `subjectName` in the UpstreamValidation block.
skriss marked this conversation as resolved.
Show resolved Hide resolved

Allow multiple SANs in Upstream Validation by adding a new field `subjectNames` to the UpstreamValidtion block. This will exist side by side with the previous `subjectName` field. Using CEL validation, we can enforce that when both are present, the first entry in `subjectNames` must match the value of `subjectName`.
89 changes: 81 additions & 8 deletions examples/contour/01-crds.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5034,15 +5034,32 @@ spec:
prefixed with namespace "namespace/name". When cross-namespace
reference is used, TLSCertificateDelegation resource must exist
in the namespace to grant access to the secret.
maxLength: 256
minLength: 1
type: string
subjectName:
description: Key which is expected to be present in the 'subjectAltName'
of the presented certificate.
description: 'Key which is expected to be present in the ''subjectAltName''
of the presented certificate. Deprecated: migrate to using the
plural field subjectNames.'
maxLength: 100
minLength: 1
type: string
subjectNames:
description: List of keys, of which at least one is expected to
be present in the 'subjectAltName of the presented certificate.
items:
type: string
maxItems: 8
minItems: 1
type: array
required:
- caSecret
- subjectName
type: object
x-kubernetes-validations:
- message: subjectNames[0] must equal subjectName if set
rule: 'has(self.subjectNames) ? self.subjectNames[0] == self.subjectName
: true'
required:
- services
type: object
Expand Down Expand Up @@ -6665,15 +6682,33 @@ spec:
reference is used, TLSCertificateDelegation resource
must exist in the namespace to grant access to the
secret.
maxLength: 256
minLength: 1
type: string
subjectName:
description: Key which is expected to be present in
the 'subjectAltName' of the presented certificate.
description: 'Key which is expected to be present
in the ''subjectAltName'' of the presented certificate.
Deprecated: migrate to using the plural field subjectNames.'
maxLength: 100
minLength: 1
type: string
subjectNames:
description: List of keys, of which at least one is
expected to be present in the 'subjectAltName of
the presented certificate.
items:
type: string
maxItems: 8
minItems: 1
type: array
required:
- caSecret
- subjectName
type: object
x-kubernetes-validations:
- message: subjectNames[0] must equal subjectName if set
rule: 'has(self.subjectNames) ? self.subjectNames[0]
== self.subjectName : true'
weight:
description: Weight defines percentage of traffic to balance
traffic
Expand Down Expand Up @@ -7060,15 +7095,33 @@ spec:
"namespace/name". When cross-namespace reference is
used, TLSCertificateDelegation resource must exist
in the namespace to grant access to the secret.
maxLength: 256
minLength: 1
type: string
subjectName:
description: Key which is expected to be present in
the 'subjectAltName' of the presented certificate.
description: 'Key which is expected to be present in
the ''subjectAltName'' of the presented certificate.
Deprecated: migrate to using the plural field subjectNames.'
maxLength: 100
minLength: 1
type: string
subjectNames:
description: List of keys, of which at least one is
expected to be present in the 'subjectAltName of the
presented certificate.
items:
type: string
maxItems: 8
minItems: 1
type: array
required:
- caSecret
- subjectName
type: object
x-kubernetes-validations:
- message: subjectNames[0] must equal subjectName if set
rule: 'has(self.subjectNames) ? self.subjectNames[0] ==
self.subjectName : true'
weight:
description: Weight defines percentage of traffic to balance
traffic
Expand Down Expand Up @@ -7382,15 +7435,35 @@ spec:
reference is used, TLSCertificateDelegation resource
must exist in the namespace to grant access to
the secret.
maxLength: 256
minLength: 1
type: string
subjectName:
description: Key which is expected to be present
in the 'subjectAltName' of the presented certificate.
description: 'Key which is expected to be present
in the ''subjectAltName'' of the presented certificate.
Deprecated: migrate to using the plural field
subjectNames.'
maxLength: 100
minLength: 1
type: string
subjectNames:
description: List of keys, of which at least one
is expected to be present in the 'subjectAltName
of the presented certificate.
items:
type: string
maxItems: 8
minItems: 1
type: array
required:
- caSecret
- subjectName
type: object
x-kubernetes-validations:
- message: subjectNames[0] must equal subjectName if
set
rule: 'has(self.subjectNames) ? self.subjectNames[0]
== self.subjectName : true'
required:
- uri
type: object
Expand Down
89 changes: 81 additions & 8 deletions examples/render/contour-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5253,15 +5253,32 @@ spec:
prefixed with namespace "namespace/name". When cross-namespace
reference is used, TLSCertificateDelegation resource must exist
in the namespace to grant access to the secret.
maxLength: 256
minLength: 1
type: string
subjectName:
description: Key which is expected to be present in the 'subjectAltName'
of the presented certificate.
description: 'Key which is expected to be present in the ''subjectAltName''
of the presented certificate. Deprecated: migrate to using the
plural field subjectNames.'
maxLength: 100
minLength: 1
type: string
subjectNames:
description: List of keys, of which at least one is expected to
be present in the 'subjectAltName of the presented certificate.
items:
type: string
maxItems: 8
minItems: 1
type: array
required:
- caSecret
- subjectName
type: object
x-kubernetes-validations:
- message: subjectNames[0] must equal subjectName if set
rule: 'has(self.subjectNames) ? self.subjectNames[0] == self.subjectName
: true'
required:
- services
type: object
Expand Down Expand Up @@ -6884,15 +6901,33 @@ spec:
reference is used, TLSCertificateDelegation resource
must exist in the namespace to grant access to the
secret.
maxLength: 256
minLength: 1
type: string
subjectName:
description: Key which is expected to be present in
the 'subjectAltName' of the presented certificate.
description: 'Key which is expected to be present
in the ''subjectAltName'' of the presented certificate.
Deprecated: migrate to using the plural field subjectNames.'
maxLength: 100
minLength: 1
type: string
subjectNames:
description: List of keys, of which at least one is
expected to be present in the 'subjectAltName of
the presented certificate.
items:
type: string
maxItems: 8
minItems: 1
type: array
required:
- caSecret
- subjectName
type: object
x-kubernetes-validations:
- message: subjectNames[0] must equal subjectName if set
rule: 'has(self.subjectNames) ? self.subjectNames[0]
== self.subjectName : true'
weight:
description: Weight defines percentage of traffic to balance
traffic
Expand Down Expand Up @@ -7279,15 +7314,33 @@ spec:
"namespace/name". When cross-namespace reference is
used, TLSCertificateDelegation resource must exist
in the namespace to grant access to the secret.
maxLength: 256
minLength: 1
type: string
subjectName:
description: Key which is expected to be present in
the 'subjectAltName' of the presented certificate.
description: 'Key which is expected to be present in
the ''subjectAltName'' of the presented certificate.
Deprecated: migrate to using the plural field subjectNames.'
maxLength: 100
minLength: 1
type: string
subjectNames:
description: List of keys, of which at least one is
expected to be present in the 'subjectAltName of the
presented certificate.
items:
type: string
maxItems: 8
minItems: 1
type: array
required:
- caSecret
- subjectName
type: object
x-kubernetes-validations:
- message: subjectNames[0] must equal subjectName if set
rule: 'has(self.subjectNames) ? self.subjectNames[0] ==
self.subjectName : true'
weight:
description: Weight defines percentage of traffic to balance
traffic
Expand Down Expand Up @@ -7601,15 +7654,35 @@ spec:
reference is used, TLSCertificateDelegation resource
must exist in the namespace to grant access to
the secret.
maxLength: 256
minLength: 1
type: string
subjectName:
description: Key which is expected to be present
in the 'subjectAltName' of the presented certificate.
description: 'Key which is expected to be present
in the ''subjectAltName'' of the presented certificate.
Deprecated: migrate to using the plural field
subjectNames.'
maxLength: 100
minLength: 1
type: string
subjectNames:
description: List of keys, of which at least one
is expected to be present in the 'subjectAltName
of the presented certificate.
items:
type: string
maxItems: 8
minItems: 1
type: array
required:
- caSecret
- subjectName
type: object
x-kubernetes-validations:
- message: subjectNames[0] must equal subjectName if
set
rule: 'has(self.subjectNames) ? self.subjectNames[0]
== self.subjectName : true'
required:
- uri
type: object
Expand Down
Loading