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

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

Merged
merged 2 commits into from
Nov 7, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fcdff58
OCPBUGS-34373: route/custom-host permission update for externalCertif…
chiragkyal Jul 5, 2024
de58856
update comments
chiragkyal Jul 11, 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
37 changes: 0 additions & 37 deletions pkg/route/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,9 @@ package route

import (
"context"
"fmt"

authorizationv1 "k8s.io/api/authorization/v1"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/apiserver/pkg/endpoints/request"

routev1 "github.com/openshift/api/route/v1"
"github.com/openshift/library-go/pkg/authorization/authorizationutil"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

Expand All @@ -26,35 +21,3 @@ type RouteValidationOptions struct {
// feature gate is enabled.
AllowExternalCertificates bool
}

// CheckRouteCustomHostSAR checks if user has permission to create and update routes/custom-host
// sub-resource
func CheckRouteCustomHostSAR(ctx context.Context, fldPath *field.Path, sarc SubjectAccessReviewCreator) field.ErrorList {
user, ok := request.UserFrom(ctx)
if !ok {
return field.ErrorList{field.InternalError(fldPath, fmt.Errorf("unable to verify host field can be set"))}
}

var errs field.ErrorList
if err := authorizationutil.Authorize(sarc, user, &authorizationv1.ResourceAttributes{
Namespace: request.NamespaceValue(ctx),
Verb: "create",
Group: routev1.GroupName,
Resource: "routes",
Subresource: "custom-host",
}); err != nil {
errs = append(errs, field.Forbidden(fldPath, "user does not have create permission on custom-host"))
}

if err := authorizationutil.Authorize(sarc, user, &authorizationv1.ResourceAttributes{
Namespace: request.NamespaceValue(ctx),
Verb: "update",
Group: routev1.GroupName,
Resource: "routes",
Subresource: "custom-host",
}); err != nil {
errs = append(errs, field.Forbidden(fldPath, "user does not have update permission on custom-host"))
}

return errs
}
21 changes: 16 additions & 5 deletions pkg/route/hostassignment/assignment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ func hasCertificateInfo(tls *routev1.TLSConfig, opts route.RouteValidationOption
}

// 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
// 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.
func certificateChangeRequiresAuth(route, older *routev1.Route, opts route.RouteValidationOptions) bool {
Expand All @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

certChanged = true
}
}
Expand Down Expand Up @@ -166,8 +166,17 @@ func validateImmutableField(newVal, oldVal interface{}, fldPath *field.Path, err
// done to the route object. If the route's host/subdomain has been updated it checks if
// the user has "update" permission on custom-host subresource. If only the certificate
// has changed, it checks if the user has "create" permission on the custom-host subresource.
// Caveat here is that if the route uses externalCertificate, the certChanged condition will
// always be true since we cannot verify state of external secret object.
//
// Which means "update" permission is required to change host/subdomain and
// either "create" or "update" permission is required to change certificate.
// Removing certificate info is allowed without any permission.
// https://github.com/openshift/origin/pull/18177#issuecomment-360660024.
//
// Caveat here is that if the (newer/updated) route uses externalCertificate,
// the certChanged condition will always be true (even when the secret name remains unchanged),
// since we cannot verify state of external secret object.
// Due to this it proceeds with the assumption that the certificate has changed
// when the route has externalCertificate set.
func ValidateHostUpdate(ctx context.Context, route, older *routev1.Route, sarc route.SubjectAccessReviewCreator, opts route.RouteValidationOptions) field.ErrorList {
hostChanged := route.Spec.Host != older.Spec.Host
subdomainChanged := route.Spec.Subdomain != older.Spec.Subdomain
Expand Down Expand Up @@ -246,7 +255,9 @@ func ValidateHostUpdate(ctx context.Context, route, older *routev1.Route, sarc r
if route.Spec.TLS.ExternalCertificate == nil || older.Spec.TLS.ExternalCertificate == nil {
errs = append(errs, validateImmutableField(route.Spec.TLS.ExternalCertificate, older.Spec.TLS.ExternalCertificate, field.NewPath("spec", "tls", "externalCertificate"), routeTLSPermissionErrMsg)...)
} else {
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))
Copy link
Contributor

Choose a reason for hiding this comment

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

Which unit test covers this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

"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.

}
}
return errs
Expand Down
184 changes: 180 additions & 4 deletions pkg/route/hostassignment/assignment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,25 @@ func TestHostWithWildcardPolicies(t *testing.T) {
allow: false,
errs: 1,
},
{
name: "tls-permission-denied-external-certificate",
tls: &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, ExternalCertificate: &routev1.LocalObjectReference{Name: "b"}},
wildcardPolicy: routev1.WildcardPolicyNone,
allow: false,
errs: 1,

opts: route.RouteValidationOptions{AllowExternalCertificates: true},
},
{
name: "tls-permission-allowed-external-certificate",
expected: "mygeneratedhost.com", // auto generated by GenerateHostname()
tls: &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, ExternalCertificate: &routev1.LocalObjectReference{Name: "b"}},
wildcardPolicy: routev1.WildcardPolicyNone,
allow: true,
errs: 0,

opts: route.RouteValidationOptions{AllowExternalCertificates: true},
},
{
name: "no-host-but-allowed",
expected: "mygeneratedhost.com",
Expand Down Expand Up @@ -197,6 +216,29 @@ func TestHostWithWildcardPolicies(t *testing.T) {
allow: false,
errs: 1,
},
{
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,
},
Comment on lines +219 to +241
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

{
name: "certificate-unchanged",
host: "host",
Expand All @@ -219,14 +261,37 @@ func TestHostWithWildcardPolicies(t *testing.T) {
allow: false,
errs: 1,
},
{
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,
},
Comment on lines +264 to +286
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as #1754 (comment)

{
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,
errs: 1, // AllocateHost() -> routeHostPermissionErrMsg

opts: route.RouteValidationOptions{AllowExternalCertificates: true},
},
Expand All @@ -242,7 +307,7 @@ func TestHostWithWildcardPolicies(t *testing.T) {
opts: route.RouteValidationOptions{AllowExternalCertificates: true},
},
{
name: "no-certificate-changed-to-external-certificate-denied",
name: "external-certificate-added-from-no-certificate-denied",
host: "host",
expected: "host",
oldHost: "host",
Expand All @@ -255,7 +320,7 @@ func TestHostWithWildcardPolicies(t *testing.T) {
opts: route.RouteValidationOptions{AllowExternalCertificates: true},
},
{
name: "no-certificate-changed-to-external-certificate-allowed",
name: "external-certificate-added-from-no-certificate-allowed",
host: "host",
expected: "host",
oldHost: "host",
Expand All @@ -267,6 +332,88 @@ func TestHostWithWildcardPolicies(t *testing.T) {

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

opts: route.RouteValidationOptions{AllowExternalCertificates: true},
},
{
name: "external-certificate-added-from-nil-tls-allowed",
host: "host",
expected: "host",
oldHost: "host",
tls: &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, ExternalCertificate: &routev1.LocalObjectReference{Name: "b"}},
oldTLS: nil,
wildcardPolicy: routev1.WildcardPolicyNone,
allow: true,
errs: 0,

opts: route.RouteValidationOptions{AllowExternalCertificates: true},
},
{
name: "external-certificate-removed-and-set-no-certificate-denied",
host: "host",
expected: "host",
oldHost: "host",
tls: &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge},
oldTLS: &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, ExternalCertificate: &routev1.LocalObjectReference{Name: "a"}},
wildcardPolicy: routev1.WildcardPolicyNone,
allow: false,
// removing certificate info is allowed
errs: 0,

opts: route.RouteValidationOptions{AllowExternalCertificates: true},
},
{
name: "external-certificate-removed-and-set-no-certificate-allowed",
host: "host",
expected: "host",
oldHost: "host",
tls: &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge},
oldTLS: &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, ExternalCertificate: &routev1.LocalObjectReference{Name: "a"}},
wildcardPolicy: routev1.WildcardPolicyNone,
allow: true,
// removing certificate info is allowed
errs: 0,

opts: route.RouteValidationOptions{AllowExternalCertificates: true},
},
{
name: "external-certificate-removed-and-set-nil-tls-denied",
host: "host",
expected: "host",
oldHost: "host",
tls: nil,
oldTLS: &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, ExternalCertificate: &routev1.LocalObjectReference{Name: "a"}},
wildcardPolicy: routev1.WildcardPolicyNone,
allow: false,
// removing certificate info is allowed
errs: 0,

opts: route.RouteValidationOptions{AllowExternalCertificates: true},
},
{
name: "external-certificate-removed-and-set-nil-tls-allowed",
host: "host",
expected: "host",
oldHost: "host",
tls: nil,
oldTLS: &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, ExternalCertificate: &routev1.LocalObjectReference{Name: "a"}},
wildcardPolicy: routev1.WildcardPolicyNone,
allow: true,
// removing certificate info is allowed
errs: 0,

opts: route.RouteValidationOptions{AllowExternalCertificates: true},
},
{
name: "external-certificate-changed-to-certificate-denied",
host: "host",
Expand Down Expand Up @@ -319,6 +466,21 @@ func TestHostWithWildcardPolicies(t *testing.T) {

opts: route.RouteValidationOptions{AllowExternalCertificates: true},
},
{
name: "certificate-changed-to-external-certificate-denied-and-featuregate-is-not-set",
host: "host",
expected: "host",
oldHost: "host",
// if the featuregate was disabled, and ExternalCertificate wasn't previously set, apiserver will strip ExternalCertificate field.
// https://github.com/openshift/openshift-apiserver/blob/1fac5e7e3a6153efae875185af2dba48fbad41ab/pkg/route/apiserver/registry/route/strategy.go#L73-L93
tls: &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, ExternalCertificate: nil},
oldTLS: &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, Certificate: "a"},
wildcardPolicy: routev1.WildcardPolicyNone,
allow: false,
errs: 0,

opts: route.RouteValidationOptions{AllowExternalCertificates: false},
},
{
name: "certificate-changed-to-external-certificate-allowed-but-featuregate-is-not-set",
host: "host",
Expand Down Expand Up @@ -361,14 +523,28 @@ func TestHostWithWildcardPolicies(t *testing.T) {
opts: route.RouteValidationOptions{AllowExternalCertificates: true},
},
{
name: "external-certificate-secret-unchanged",
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},
},
{
name: "external-certificate-unchanged-allowed",
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: true,
errs: 0,

opts: route.RouteValidationOptions{AllowExternalCertificates: true},
Expand Down
35 changes: 0 additions & 35 deletions pkg/route/hostassignment/externalcertificate.go
View file
Open in desktop

This file was deleted.

Loading