CFE-882: Route external certificate validation by chiragkyal · Pull Request #1625 · openshift/library-go

chiragkyal · 2023-11-22T07:13:46Z

Description

Updates route validations based on openshift/enhancements#1307 which introduces a new field in the route API externalCertificate behind the TP feature gate.

Changes

ValidateRoute() has been updated to pass additional args (secrets lister and subjectaccessreviewer) to validate
externalCertificate and the rbac required. Now it also does additional subject access review authorization checks to verify if the router SA has the correct permissions to parse externalCertificate. It also adds a hard requirement that the secret referenced in externalCertificate be present prior to creating the route.
ValidateHostUpdate() has also been updated since updating route host/subdomain is also affected by updating
certificates on the route. Now certificateChangeRequiresAuth() will always return true if the route has a externalCertificate set (check EP for additional info).
ValidateHostExternalCertificate() has been introduced as part of the validations done during route updates, this function specifically checks if the user has the correct permissions on the custom-host sub-resource. Additional details can be found on the EP.

openshift-ci-robot · 2023-11-22T07:13:50Z

@chiragkyal: This pull request references CFE-882 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.15.0" version, but no target version was set.

Details

In response to this:

Description

Updates route validations based on openshift/enhancements#1307 which introduces a new field in the route API externalCertificate behind the TP feature gate.

Changes

ValidateRoute() has been updated to pass additional args (secrets lister and subjectaccessreviewer) to validate
externalCertificate and the rbac required. Now it also does additional subject access review authorization checks to verify if the router SA has the correct permissions to parse externalCertificate. It also adds a hard requirement that the secret referenced in externalCertificate be present prior to creating the route.

ValidateHostUpdate() has also been updated since updating route host/subdomain is also affected by updating
certificates on the route. Now certificateChangeRequiresAuth() will always return true if the route has a externalCertificate set (check EP for additional info).

ValidateHostExternalCertificate() has been introduced as part of the validations done during route updates, this function specifically checks if the user has the correct permissions on the custom-host sub-resource. Additional details can be found on the EP.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

chiragkyal · 2023-11-22T07:26:27Z

Continued from #1549

chiragkyal · 2023-11-30T11:40:09Z

/cc @alebedev87

chiragkyal · 2023-11-30T14:05:44Z

/assign @alebedev87

candita · 2023-11-30T23:34:14Z

/hold

candita · 2023-12-06T18:10:12Z

/assign @Miciah

candita · 2023-12-06T18:10:23Z

/unhold

alebedev87

Focus of this review was on the public functions and their signatures as they should be concise and meaningful. Also I took a glance at the test cases t understand the new logic but the test cases need some structural changes. Will have to do another re-iteration with the focus on the logic.

pkg/route/hostassignment/externalcertificate.go

pkg/route/common.go

pkg/route/hostassignment/assignment.go

pkg/route/validation/validation.go

pkg/route/validation/validation_test.go

pkg/route/hostassignment/assignment_test.go

pkg/route/hostassignment/externalcertificate_test.go

alebedev87

I see resolved comments which are said to be addressed but they are actually not. Maybe you forgot to push the latest changes? If yes, can you please do so? Thanks!

pkg/route/validation/validation_test.go

pkg/route/validation/validation.go

chiragkyal · 2024-01-26T11:42:08Z

@alebedev87 that's weird, I do have the latest changes pushed. Can you access bb12c49 commit and let me know please, if it's visible.

alebedev87 · 2024-01-26T13:12:31Z

@chiragkyal : I can see the changes now, seems like a GitHub glitch as I refreshed the PR many times. Sorry about that, I continue the review!

alebedev87

Another iteration, not fully done with the review yet.

pkg/route/common.go

pkg/route/validation/validation.go

pkg/route/hostassignment/externalcertificate.go

pkg/route/validation/validation_test.go

pkg/route/hostassignment/assignment_test.go

chiragkyal · 2024-02-01T09:32:21Z

/retest

alebedev87

Last iteration, some questions and remarks. Overall I think I'm close to the end :)

pkg/route/common.go

pkg/route/hostassignment/assignment.go

pkg/route/common.go

alebedev87 · 2024-02-08T10:03:41Z

pkg/route/hostassignment/assignment.go

+
+		if opts.AllowExternalCertificates && route.Spec.TLS != nil && older.Spec.TLS != nil {
+			if route.Spec.TLS.ExternalCertificate != nil || older.Spec.TLS.ExternalCertificate != nil {
+				certChanged = true


Why the cert is considered changed here? Shouldn't we check whether the name of the external certificate changed before setting certChanged=true?

Because we cannot definitively verify if the content of the secret that is referenced has been modified.

The reason this also mentioned in the EP:

If the old route or the new route uses .spec.tls.externalCertificate this validation will always have the precondition certificateChangeRequiresAuth() return true since we cannot definitively verify if the content of the secret that is referenced has been modified. Since the previous validation func (ValidateHostExternalCertificate()) would have already validation user permissions, we can safely make this assumption.

I was wondering about the secret name, not the secret contents.

Can the external certificate secret name change?

Yeah! The external certificate secret name can be changed, but the caveat here is even if the secret name remains the same, we need to assume that the secret content is changed, and hence, the certificate is changed.

but the caveat here is even if the secret name remains the same, we need to assume that the secret content is changed

True, we have no choice here but treat this as "always changed".

I do see you added a comment for this caveat to ValidateHostUpdate, but perhaps it deserves a code comment in certificateChangeRequiresAuth as well.

Okay, I've added a comment for certificateChangeRequiresAuth as well

pkg/route/hostassignment/assignment_test.go

pkg/route/validation/validation_test.go

pkg/route/validation/validation.go

pkg/route/validation/validation_test.go

pkg/route/hostassignment/assignment_test.go

alebedev87 · 2024-02-09T17:21:53Z

/lgtm

Miciah · 2024-04-01T07:30:51Z

pkg/route/hostassignment/assignment.go

+			if opts.AllowExternalCertificates && route.Spec.TLS.ExternalCertificate != nil && older.Spec.TLS.ExternalCertificate != nil {
+				errs = append(errs, apimachineryvalidation.ValidateImmutableField(route.Spec.TLS.ExternalCertificate.Name, older.Spec.TLS.ExternalCertificate.Name, field.NewPath("spec", "tls", "externalCertificate"))...)
+			}


Is this test case correct?

{ name: "no-certificate-changed-to-external-certificate-denied", host: "host", expected: "host", oldHost: "host", tls: &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, ExternalCertificate: &routev1.LocalObjectReference{Name: "b"}}, oldTLS: &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, ExternalCertificate: nil}, wildcardPolicy: routev1.WildcardPolicyNone, allow: false, errs: 1, opts: route.RouteValidationOptions{AllowExternalCertificates: true}, },

The above test case fails if I add it to TestHostWithWildcardPolicies:

go test ./pkg/route/hostassignment -run TestHostWithWildcardPolicies # github.com/openshift/library-go/pkg/route/hostassignment.test guile: warning: failed to install locale --- FAIL: TestHostWithWildcardPolicies (0.00s) --- FAIL: TestHostWithWildcardPolicies/no-certificate-changed-to-external-certificate-denied (0.00s) assignment_test.go:506: expected 1 errors, got 0: []

It seems to me that the logic in ValidateHostUpdate should be as follows:

Suggested change

if opts.AllowExternalCertificates && route.Spec.TLS.ExternalCertificate != nil && older.Spec.TLS.ExternalCertificate != nil {

errs = append(errs, apimachineryvalidation.ValidateImmutableField(route.Spec.TLS.ExternalCertificate.Name, older.Spec.TLS.ExternalCertificate.Name, field.NewPath("spec", "tls", "externalCertificate"))...)

}

if opts.AllowExternalCertificates {

if route.Spec.TLS.ExternalCertificate == nil || older.Spec.TLS.ExternalCertificate == nil {

errs = append(errs, apimachineryvalidation.ValidateImmutableField(route.Spec.TLS.ExternalCertificate, older.Spec.TLS.ExternalCertificate, field.NewPath("spec", "tls", "externalCertificate"))...)

} else {

errs = append(errs, apimachineryvalidation.ValidateImmutableField(route.Spec.TLS.ExternalCertificate.Name, older.Spec.TLS.ExternalCertificate.Name, field.NewPath("spec", "tls", "externalCertificate"))...)

}

}

You'll need to modify the "external-certificate-changed-to-certificate-denied" and "certificate-changed-to-external-certificate-denied" test cases to expect an additional error.

Thanks for the suggestion and finding this edge case. I've updated the logic in ValidateHostUpdate as suggested, and also added the above test case along with "no-certificate-changed-to-external-certificate-allowed" test into TestHostWithWildcardPolicies

Miciah · 2024-04-01T07:31:34Z

It looks like this suggestion from the original PR got dropped: #1549 (comment)

Miciah · 2024-04-01T07:58:29Z

pkg/route/common.go

+// RouteValidationOptions used to tweak how/what fields are validated. These
+// options are propagated by the apiserver.
+type RouteValidationOptions struct {
+
+	// AllowExternalCertificates option is set when RouteExternalCertificate
+	AllowExternalCertificates bool
+}


Is the AllowExternalCertificates flag (or perhaps this whole struct) destined to be removed when the feature graduates from tech preview to GA?

That's correct. The AllowExternalCertificates flag (along with RouteValidationOptions struct) will be removed while graduating from TP -> GA.

pkg/route/hostassignment/assignment.go

Miciah · 2024-04-01T08:10:28Z

pkg/route/hostassignment/externalcertificate.go

+// custom-host subresource of routes. This check is required to be done prior to ValidateHostUpdate()
+// since updating hosts while using externalCertificate is contingent on the user having both these
+// permissions. The ValidateHostUpdate() cannot differentiate if the certificate has changed since
+// now the certificates will be present as a secret object, due to this it proceeds with the assumption
+// that the certificate has changed when the route has externalCertificate set.


Ultimately, ValidateUpdate is going to call ValidateHostExternalCertificate and then call ValidateHostUpdate, right? This comment is helpful, but it still isn't quite clear why the logic in ValidateHostExternalCertificate cannot simply be added to ValidateHostUpdate.

Reminded me of my comment.

The EP did mention about invoking ValidateHostExternalCertificate prior to ValidateHostUpdate, but it's body lacks the actual reason why we decided to choose this approach.

As per our discussion over Slack and further investigation, I've added a TODO to consider merging ValidateHostExternalCertificate function into ValidateHostUpdate later.

pkg/route/hostassignment/externalcertificate.go

pkg/route/validation/validation.go

pkg/route/common.go

Update all route validation functions to parse TP featuregate and validate ExternalCertificate field on the route.

openshift-ci · 2024-04-02T18:50:04Z

@chiragkyal: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Miciah · 2024-04-02T19:39:42Z

pkg/route/hostassignment/assignment.go

+// certificateChangeRequiresAuth determines whether changes to the TLS certificate configuration require authentication.
+// Note: If either route uses externalCertificate, this function always returns true, as we cannot definitively verify if
+// the content of the referenced secret has been modified. Even if the secret name remains the same,
+// we must assume that the secret content is changed, necessitating authentication.


Suggested change

// we must assume that the secret content is changed, necessitating authentication.

// we must assume that the secret content is changed, necessitating authorization.

Covered in #1706

Miciah · 2024-04-02T20:05:19Z

pkg/route/hostassignment/assignment.go

+			len(route.Spec.TLS.Key) > 0)
+
+	if opts.AllowExternalCertificates && route.Spec.TLS != nil && route.Spec.TLS.ExternalCertificate != nil {
+		certSet = certSet || len(route.Spec.TLS.ExternalCertificate.Name) > 0


I just noticed that the tests don't cover this case (i.e. creating a new route that specifies externalCertificate). These test cases would give you coverage:

{ name: "create-with-external-certificate-denied", host: "host", expected: "host", tls: &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, ExternalCertificate: &routev1.LocalObjectReference{Name: "b"}}, wildcardPolicy: routev1.WildcardPolicyNone, allow: false, errs: 1, opts: route.RouteValidationOptions{AllowExternalCertificates: true}, }, { name: "create-with-external-certificate-allowed", host: "host", expected: "host", tls: &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, ExternalCertificate: &routev1.LocalObjectReference{Name: "b"}}, wildcardPolicy: routev1.WildcardPolicyNone, allow: true, errs: 0, opts: route.RouteValidationOptions{AllowExternalCertificates: true}, },

Covered in #1706

Miciah · 2024-04-02T20:37:45Z

Excellent!
/approve
/lgtm

I added two comments since your last update, but they are very minor things. @alebedev87 might want to take another quick look after your most recent changes, so I'll add a hold just in case.
/hold

openshift-ci · 2024-04-02T20:38:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alebedev87, chiragkyal, Miciah

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/route/OWNERS~~ [Miciah]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

alebedev87 · 2024-04-03T11:34:41Z

/unhold

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 22, 2023

openshift-ci bot requested review from candita and frobware November 22, 2023 07:14

openshift-ci bot requested a review from alebedev87 November 30, 2023 11:40

chiragkyal mentioned this pull request Nov 30, 2023

CFE-885:Feature route external certificate validation openshift/openshift-apiserver#407

Merged

openshift-ci bot assigned alebedev87 Nov 30, 2023

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 30, 2023

openshift-ci bot assigned Miciah Dec 6, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 6, 2023

alebedev87 reviewed Jan 4, 2024

View reviewed changes

chiragkyal mentioned this pull request Jan 17, 2024

CFE-882: Route external certificate validation #1549

Closed

alebedev87 reviewed Jan 26, 2024

View reviewed changes

pkg/route/validation/validation_test.go Outdated Show resolved Hide resolved

pkg/route/validation/validation_test.go Show resolved Hide resolved

pkg/route/validation/validation.go Outdated Show resolved Hide resolved

alebedev87 reviewed Jan 31, 2024

View reviewed changes

alebedev87 reviewed Feb 8, 2024

View reviewed changes

pkg/route/hostassignment/assignment_test.go Outdated Show resolved Hide resolved

alebedev87 reviewed Feb 9, 2024

View reviewed changes

pkg/route/hostassignment/assignment_test.go Outdated Show resolved Hide resolved

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 9, 2024

chiragkyal force-pushed the feature/route-external-certificate branch from 5f39723 to 6cfa61a Compare February 9, 2024 18:11

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 9, 2024

alebedev87 mentioned this pull request Mar 29, 2024

CFE-866: Add secret manager for routes to consume secret_monitor #1626

Merged

Miciah reviewed Apr 1, 2024

View reviewed changes

pkg/route/hostassignment/assignment.go Outdated Show resolved Hide resolved

Miciah reviewed Apr 1, 2024

View reviewed changes

pkg/route/hostassignment/externalcertificate.go Outdated Show resolved Hide resolved

Miciah reviewed Apr 1, 2024

View reviewed changes

pkg/route/validation/validation.go Outdated Show resolved Hide resolved

Miciah reviewed Apr 1, 2024

View reviewed changes

pkg/route/validation/validation.go Outdated Show resolved Hide resolved

Miciah reviewed Apr 1, 2024

View reviewed changes

pkg/route/common.go Outdated Show resolved Hide resolved

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 2, 2024

chiragkyal force-pushed the feature/route-external-certificate branch from aa1e4b9 to 8015e1e Compare April 2, 2024 15:06

clean up: remove unused pem encoding verification checks

b3a08bf

chiragkyal force-pushed the feature/route-external-certificate branch from 8015e1e to e02155c Compare April 2, 2024 15:29

Update route validations to honour ExternalCertificate

6b9c3c0

Update all route validation functions to parse TP featuregate and validate ExternalCertificate field on the route.

chiragkyal force-pushed the feature/route-external-certificate branch from 7bae080 to 6b9c3c0 Compare April 2, 2024 18:33

Miciah reviewed Apr 2, 2024

View reviewed changes

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Apr 2, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 2, 2024

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 3, 2024

openshift-merge-bot bot merged commit 5a3db7d into openshift:master Apr 3, 2024

chiragkyal deleted the feature/route-external-certificate branch April 3, 2024 11:41

chiragkyal mentioned this pull request Apr 3, 2024

CFE-882: add few test cases for external-certificate #1706

Merged

Miciah mentioned this pull request Apr 23, 2024

OCPBUGS-22332: route: Improve errors for missing custom-host permissions #1677

Merged

-			if opts.AllowExternalCertificates && route.Spec.TLS.ExternalCertificate != nil && older.Spec.TLS.ExternalCertificate != nil {
-				errs = append(errs, apimachineryvalidation.ValidateImmutableField(route.Spec.TLS.ExternalCertificate.Name, older.Spec.TLS.ExternalCertificate.Name, field.NewPath("spec", "tls", "externalCertificate"))...)
-			}
+			if opts.AllowExternalCertificates {
+				if route.Spec.TLS.ExternalCertificate == nil || older.Spec.TLS.ExternalCertificate == nil {
+					errs = append(errs, apimachineryvalidation.ValidateImmutableField(route.Spec.TLS.ExternalCertificate, older.Spec.TLS.ExternalCertificate, field.NewPath("spec", "tls", "externalCertificate"))...)
+				} else {
+					errs = append(errs, apimachineryvalidation.ValidateImmutableField(route.Spec.TLS.ExternalCertificate.Name, older.Spec.TLS.ExternalCertificate.Name, field.NewPath("spec", "tls", "externalCertificate"))...)
+				}
+			}

	// we must assume that the secret content is changed, necessitating authentication.
	// we must assume that the secret content is changed, necessitating authorization.

Conversation

chiragkyal commented Nov 22, 2023

Description

Changes

Uh oh!

openshift-ci-robot commented Nov 22, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Uh oh!

chiragkyal commented Nov 22, 2023

Uh oh!

chiragkyal commented Nov 30, 2023

Uh oh!

chiragkyal commented Nov 30, 2023

Uh oh!

candita commented Nov 30, 2023

Uh oh!

candita commented Dec 6, 2023

Uh oh!

candita commented Dec 6, 2023

Uh oh!

alebedev87 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alebedev87 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chiragkyal commented Jan 26, 2024

Uh oh!

alebedev87 commented Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alebedev87 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chiragkyal commented Feb 1, 2024

Uh oh!

alebedev87 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alebedev87 Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alebedev87 Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

openshift-ci-robot commented Nov 22, 2023 •

edited by openshift-ci bot

Loading

alebedev87 commented Jan 26, 2024 •

edited

Loading

alebedev87 Feb 8, 2024 •

edited

Loading

alebedev87 Feb 9, 2024 •

edited

Loading