OCPBUGS-34373: routes/custom-host permission update for externalCertificate #1754

chiragkyal · 2024-07-05T13:55:19Z

“create” is required to set spec.tls.externalCertificate on route creation
either “create” or “update” is required to update spec.tls.externalCertificate
Note: since the state of the external secret cannot be verified, even keeping the secret name unchanged, requires permission check.
NO PERMISSION is required to remove spec.tls.externalCertificate
EP changes OCPBUGS-34373: routes/custom-host permission update for externalCertificate enhancements#1652

The decision was taken based on openshift/origin#18177 (comment) discussion on custom-host permission and keep it identical with the existing spec.tls.Certificate field.

…icate * “create” is required to set spec.tls.externalCertificate on route creation * either “create” or “update” is required to update spec.tls.externalCertificate * Note: since the state of the external secret cannot be verified, even keeping the secret name unchanged, requires permission check. * NO PERMISSION is required to remove spec.tls.externalCertificate Signed-off-by: chiragkyal <ckyal@redhat.com>

openshift-ci · 2024-07-05T13:55:23Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2024-07-05T13:55:24Z

@chiragkyal: This pull request references Jira Issue OCPBUGS-34373, which is invalid:

expected the bug to target the "4.17.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

“create” is required to set spec.tls.externalCertificate on route creation

either “create” or “update” is required to update spec.tls.externalCertificate

Note: since the state of the external secret cannot be verified, even keeping the secret name unchanged, requires permission check.

NO PERMISSION is required to remove spec.tls.externalCertificate

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 openshift-eng/jira-lifecycle-plugin repository.

chiragkyal · 2024-07-08T13:04:11Z

/assign @alebedev87 @Miciah

Signed-off-by: chiragkyal <ckyal@redhat.com>

lihongan · 2024-07-23T01:06:47Z

/jira refresh

openshift-ci-robot · 2024-07-23T01:06:52Z

@lihongan: This pull request references Jira Issue OCPBUGS-34373, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.17.0) matches configured target version for branch (4.17.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @lihongan

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

alebedev87 · 2024-08-16T22:09:49Z

pkg/route/hostassignment/assignment_test.go

+		{
+			name:           "key-added",
+			host:           "host",
+			expected:       "host",
+			oldHost:        "host",
+			tls:            &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, Key: "a"},
+			oldTLS:         &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge},
+			wildcardPolicy: routev1.WildcardPolicyNone,
+			allow:          false,
+			errs:           1,
+		},
+		{
+			name:           "key-removed",
+			host:           "host",
+			expected:       "host",
+			oldHost:        "host",
+			tls:            &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge},
+			oldTLS:         &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, Key: "b"},
+			wildcardPolicy: routev1.WildcardPolicyNone,
+			allow:          false,
+			// removing key info is allowed
+			errs: 0,
+		},


Do we need these tests for the change you did or you just wanted to enhance the unit tests for other cases?

These tests are unrelated to the changes I made related to externalCertificate; I just wanted to enhance different test scenarios.

alebedev87 · 2024-08-16T22:10:30Z

pkg/route/hostassignment/assignment_test.go

+		{
+			name:           "certificate-added",
+			host:           "host",
+			expected:       "host",
+			oldHost:        "host",
+			tls:            &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, Certificate: "a"},
+			oldTLS:         &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge},
+			wildcardPolicy: routev1.WildcardPolicyNone,
+			allow:          false,
+			errs:           1,
+		},
+		{
+			name:           "certificate-removed",
+			host:           "host",
+			expected:       "host",
+			oldHost:        "host",
+			tls:            &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge},
+			oldTLS:         &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, Certificate: "b"},
+			wildcardPolicy: routev1.WildcardPolicyNone,
+			allow:          false,
+			// removing certificate info is allowed
+			errs: 0,
+		},


Same question.

Same as #1754 (comment)

alebedev87 · 2024-08-16T22:54:30Z

pkg/route/hostassignment/assignment.go

-					errs = append(errs, validateImmutableField(route.Spec.TLS.ExternalCertificate.Name, older.Spec.TLS.ExternalCertificate.Name, field.NewPath("spec", "tls", "externalCertificate"), routeTLSPermissionErrMsg)...)
+					// since the state of the external secret cannot be verified, return error (even when secret name remains unchanged)
+					// without performing immutability checks, if externalCertificate is set.
+					errs = append(errs, field.Invalid(field.NewPath("spec", "tls", "externalCertificate"), route.Spec.TLS.ExternalCertificate, routeTLSPermissionErrMsg))


Which unit test covers this change?

"external-certificate-unchanged-denied"

{ name: "external-certificate-unchanged-denied", host: "host", expected: "host", oldHost: "host", tls: &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, ExternalCertificate: &routev1.LocalObjectReference{Name: "a"}}, oldTLS: &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, ExternalCertificate: &routev1.LocalObjectReference{Name: "a"}}, wildcardPolicy: routev1.WildcardPolicyNone, allow: false, // permission is required even if referenced secret name remains unchanged errs: 1, opts: route.RouteValidationOptions{AllowExternalCertificates: true}, },

We need to remove the immutability check to catch the externalCertificate unchanged scenario, without having necessary permissions.

alebedev87 · 2024-08-16T22:58:10Z

pkg/route/hostassignment/assignment.go

@@ -137,7 +137,7 @@ func certificateChangeRequiresAuth(route, older *routev1.Route, opts route.Route
 			a.Key != b.Key

 		if opts.AllowExternalCertificates {
-			if route.Spec.TLS.ExternalCertificate != nil || older.Spec.TLS.ExternalCertificate != nil {
+			if route.Spec.TLS.ExternalCertificate != nil {


I suppose this change is responsible for the note from the PR description:

Note: since the state of the external secret cannot be verified, even keeping the secret name unchanged, requires permission check.

I think this code deserves a comment with similar description.

The function godoc has a similar description. Let me know if we need to include anything else.

// Note: If (newer/updated) 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 authorization.

alebedev87 · 2024-08-29T12:32:33Z

/approve

Can I (doubt)?

alebedev87 · 2024-08-29T12:33:27Z

/lgtm

openshift-ci · 2024-08-29T12:33:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alebedev87, chiragkyal
Once this PR has been reviewed and has the lgtm label, please ask for approval from miciah. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

pkg/route/OWNERS

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

chiragkyal · 2024-08-29T13:20:51Z

/cc @Miciah
for approval

chiragkyal · 2024-08-29T13:33:46Z

/jira refresh

openshift-ci-robot · 2024-08-29T13:33:50Z

@chiragkyal: This pull request references Jira Issue OCPBUGS-34373, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.18.0) matches configured target version for branch (4.18.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @lihongan

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

Miciah · 2024-09-23T04:16:48Z

pkg/route/hostassignment/externalcertificate.go

-func ValidateHostExternalCertificate(ctx context.Context, new, older *routev1.Route, sarc routecommon.SubjectAccessReviewCreator, opts routecommon.RouteValidationOptions) field.ErrorList {
-
-	if !opts.AllowExternalCertificates {
-		// Return nil since the feature gate is off.
-		// ValidateHostUpdate() is sufficient to validate
-		// permissions.
-		return nil
-	}
-
-	newTLS := new.Spec.TLS
-	oldTLS := older.Spec.TLS
-	if (newTLS != nil && newTLS.ExternalCertificate != nil) || (oldTLS != nil && oldTLS.ExternalCertificate != nil) {
-		return routecommon.CheckRouteCustomHostSAR(ctx, field.NewPath("spec", "tls", "externalCertificate"), sarc)
-	}
-
-	return nil
-}


Since ValidateHostExternalCertificate is used in openshift-apiserver, removing its definition now could cause difficulties if someone else needs a change from library-go in openshift-apiserver and tries to bump it before you update openshift-apiserver not to use ValidateHostExternalCertificate. That is:

You remove ValidateHostExternalCertificate from library-go.

You don't have time to post the openshift-apiserver PR right away, or reviews get delayed.

Meanwhile, someone else adds function xyz to library-go and bumps openshift-apiserver to get xyz.

Building openshift-apiserver fails because ValidateHostExternalCertificate is not defined.

It would be safer to do the following:

Update logic in library-go, but keep ValidateHostExternalCertificate (maybe add a // Deprecated comment) so library-go can be bumped in openshift-apiserver without breaking anything.

Update code in openshift-apiserver not to use ValidateHostExternalCertificate.

Remove ValidateHostExternalCertificate from library-go.

Thanks for the suggestion, and I agree that we should be doing the way you suggested above.

However, even if we keep ValidateHostExternalCertificate definition, the unit tests present in the openshift-apiserver will complain due to the changes in validateTLSExternalCertificate function because we are dropping custom-host permission check from that function as well. There are fair amount of unit tests overlap between openshift-apiserver and library-go, so updating logic in library-go requires immediate intervention (bump) in openshift-apiserver as well.

If these weren't any unit tests in openshift-apiserver, then it could have been much easier.

Miciah · 2024-09-23T04:17:57Z

Need to resolve openshift/enhancements#1652 (comment).

openshift-ci · 2024-10-14T14:15:18Z

@chiragkyal: all tests passed!

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 5, 2024

openshift-ci-robot added jira/severity-important Referenced Jira bug's severity is important for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. labels Jul 5, 2024

openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Jul 5, 2024

chiragkyal changed the title ~~OCPBUGS-34373: route/custom-host permission update for externalCertificate~~ OCPBUGS-34373: routes/custom-host permission update for externalCertificate Jul 5, 2024

chiragkyal marked this pull request as ready for review July 8, 2024 13:02

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 8, 2024

openshift-ci bot requested review from frobware and Miciah July 8, 2024 13:02

openshift-ci bot assigned alebedev87 and Miciah Jul 8, 2024

update comments

de58856

Signed-off-by: chiragkyal <ckyal@redhat.com>

openshift-ci-robot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Jul 23, 2024

openshift-ci-robot removed the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Jul 23, 2024

openshift-ci bot requested a review from lihongan July 23, 2024 01:06

alebedev87 reviewed Aug 16, 2024

View reviewed changes

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

Miciah reviewed Sep 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-34373: routes/custom-host permission update for externalCertificate #1754

OCPBUGS-34373: routes/custom-host permission update for externalCertificate #1754

chiragkyal commented Jul 5, 2024 •

edited

Loading

openshift-ci bot commented Jul 5, 2024

openshift-ci-robot commented Jul 5, 2024

chiragkyal commented Jul 8, 2024

lihongan commented Jul 23, 2024

openshift-ci-robot commented Jul 23, 2024

alebedev87 Aug 16, 2024

chiragkyal Aug 22, 2024

alebedev87 Aug 16, 2024

chiragkyal Aug 22, 2024

alebedev87 Aug 16, 2024

chiragkyal Aug 22, 2024

alebedev87 Aug 16, 2024

chiragkyal Aug 22, 2024

alebedev87 commented Aug 29, 2024 •

edited

Loading

alebedev87 commented Aug 29, 2024

openshift-ci bot commented Aug 29, 2024

chiragkyal commented Aug 29, 2024

chiragkyal commented Aug 29, 2024

openshift-ci-robot commented Aug 29, 2024

Miciah Sep 23, 2024

chiragkyal Oct 10, 2024

Miciah commented Sep 23, 2024

openshift-ci bot commented Oct 14, 2024

OCPBUGS-34373: routes/custom-host permission update for externalCertificate #1754

Are you sure you want to change the base?

OCPBUGS-34373: routes/custom-host permission update for externalCertificate #1754

Conversation

chiragkyal commented Jul 5, 2024 • edited Loading

openshift-ci bot commented Jul 5, 2024

openshift-ci-robot commented Jul 5, 2024

chiragkyal commented Jul 8, 2024

lihongan commented Jul 23, 2024

openshift-ci-robot commented Jul 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alebedev87 commented Aug 29, 2024 • edited Loading

alebedev87 commented Aug 29, 2024

openshift-ci bot commented Aug 29, 2024

chiragkyal commented Aug 29, 2024

chiragkyal commented Aug 29, 2024

openshift-ci-robot commented Aug 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miciah commented Sep 23, 2024

openshift-ci bot commented Oct 14, 2024

chiragkyal commented Jul 5, 2024 •

edited

Loading

alebedev87 commented Aug 29, 2024 •

edited

Loading