CFE-882: Route external certificate validation #1625
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| 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" | ||
| ) | ||
|
|
||
| // SubjectAccessReviewCreator is an interface for performing subject access reviews | ||
| type SubjectAccessReviewCreator interface { | ||
| Create(ctx context.Context, sar *authorizationv1.SubjectAccessReview, opts metav1.CreateOptions) (*authorizationv1.SubjectAccessReview, error) | ||
| } | ||
|
|
||
| // RouteValidationOptions used to tweak how/what fields are validated. These | ||
| // options are propagated by the apiserver. | ||
| type RouteValidationOptions struct { | ||
|
|
||
| // AllowExternalCertificates option is set when the RouteExternalCertificate | ||
| // 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 { | ||
chiragkyal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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 | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -12,26 +12,31 @@ import ( | |||||
|
|
||||||
| routev1 "github.com/openshift/api/route/v1" | ||||||
| "github.com/openshift/library-go/pkg/authorization/authorizationutil" | ||||||
| "github.com/openshift/library-go/pkg/route" | ||||||
| ) | ||||||
|
|
||||||
| // HostGeneratedAnnotationKey is the key for an annotation set to "true" if the route's host was generated | ||||||
| const HostGeneratedAnnotationKey = "openshift.io/host.generated" | ||||||
|
|
||||||
| type HostnameGenerator interface { | ||||||
| GenerateHostname(*routev1.Route) (string, error) | ||||||
| } | ||||||
|
|
||||||
| // AllocateHost allocates a host name ONLY if the route doesn't specify a subdomain wildcard policy and | ||||||
| // the host name on the route is empty and an allocator is configured. | ||||||
| // It must first allocate the shard and may return an error if shard allocation fails. | ||||||
| func AllocateHost(ctx context.Context, route *routev1.Route, sarc route.SubjectAccessReviewCreator, routeAllocator HostnameGenerator, opts route.RouteValidationOptions) field.ErrorList { | ||||||
| hostSet := len(route.Spec.Host) > 0 | ||||||
| certSet := route.Spec.TLS != nil && | ||||||
| (len(route.Spec.TLS.CACertificate) > 0 || | ||||||
| len(route.Spec.TLS.Certificate) > 0 || | ||||||
| len(route.Spec.TLS.DestinationCACertificate) > 0 || | ||||||
| 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 | ||||||
|
Contributor
There was a problem hiding this comment. I just noticed that the tests don't cover this case (i.e. creating a new route that specifies {
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},
}, |
||||||
| } | ||||||
|
|
||||||
| if hostSet || certSet { | ||||||
| user, ok := request.UserFrom(ctx) | ||||||
| if !ok { | ||||||
|
|
@@ -86,41 +91,65 @@ func AllocateHost(ctx context.Context, route *routev1.Route, sarc SubjectAccessR | |||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| func hasCertificateInfo(tls *routev1.TLSConfig, opts route.RouteValidationOptions) bool { | ||||||
| if tls == nil { | ||||||
| return false | ||||||
| } | ||||||
| hasInfo := len(tls.Certificate) > 0 || | ||||||
| len(tls.Key) > 0 || | ||||||
| len(tls.CACertificate) > 0 || | ||||||
| len(tls.DestinationCACertificate) > 0 | ||||||
|
|
||||||
| if opts.AllowExternalCertificates && tls.ExternalCertificate != nil { | ||||||
| hasInfo = hasInfo || len(tls.ExternalCertificate.Name) > 0 | ||||||
| } | ||||||
| return hasInfo | ||||||
| } | ||||||
|
|
||||||
| // 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. | ||||||
|
Contributor
There was a problem hiding this comment.
Suggested change
|
||||||
| func certificateChangeRequiresAuth(route, older *routev1.Route, opts route.RouteValidationOptions) bool { | ||||||
| switch { | ||||||
| case route.Spec.TLS != nil && older.Spec.TLS != nil: | ||||||
| a, b := route.Spec.TLS, older.Spec.TLS | ||||||
| if !hasCertificateInfo(a, opts) { | ||||||
| // removing certificate info is allowed | ||||||
| return false | ||||||
| } | ||||||
|
|
||||||
| certChanged := a.CACertificate != b.CACertificate || | ||||||
| a.Certificate != b.Certificate || | ||||||
| a.DestinationCACertificate != b.DestinationCACertificate || | ||||||
| a.Key != b.Key | ||||||
|
|
||||||
| if opts.AllowExternalCertificates { | ||||||
| if route.Spec.TLS.ExternalCertificate != nil || older.Spec.TLS.ExternalCertificate != nil { | ||||||
| certChanged = true | ||||||
|
Contributor
There was a problem hiding this comment. Why the cert is considered changed here? Shouldn't we check whether the name of the external certificate changed before setting
Member
Author
There was a problem hiding this comment. 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
Contributor
There was a problem hiding this comment. I was wondering about the secret name, not the secret contents.
Contributor
There was a problem hiding this comment. Can the external certificate secret name change?
Member
Author
There was a problem hiding this comment. 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.
Contributor
There was a problem hiding this comment.
True, we have no choice here but treat this as "always changed".
Contributor
There was a problem hiding this comment. I do see you added a comment for this caveat to
Member
Author
There was a problem hiding this comment. Okay, I've added a comment for |
||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return certChanged | ||||||
| case route.Spec.TLS != nil: | ||||||
| // using any default certificate is allowed | ||||||
| return hasCertificateInfo(route.Spec.TLS, opts) | ||||||
| default: | ||||||
| // all other cases we are not adding additional certificate info | ||||||
| return false | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // ValidateHostUpdate checks if the user has the correct permissions based on the updates | ||||||
| // 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. | ||||||
| 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 | ||||||
| certChanged := certificateChangeRequiresAuth(route, older, opts) | ||||||
| if !hostChanged && !certChanged && !subdomainChanged { | ||||||
| return nil | ||||||
| } | ||||||
|
|
@@ -190,6 +219,14 @@ func ValidateHostUpdate(ctx context.Context, route, older *routev1.Route, sarc S | |||||
| errs = append(errs, apimachineryvalidation.ValidateImmutableField(route.Spec.TLS.Certificate, older.Spec.TLS.Certificate, field.NewPath("spec", "tls", "certificate"))...) | ||||||
| errs = append(errs, apimachineryvalidation.ValidateImmutableField(route.Spec.TLS.DestinationCACertificate, older.Spec.TLS.DestinationCACertificate, field.NewPath("spec", "tls", "destinationCACertificate"))...) | ||||||
| errs = append(errs, apimachineryvalidation.ValidateImmutableField(route.Spec.TLS.Key, older.Spec.TLS.Key, field.NewPath("spec", "tls", "key"))...) | ||||||
|
|
||||||
| 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"))...) | ||||||
| } | ||||||
| } | ||||||
| return errs | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| package hostassignment | ||
|
|
||
| import ( | ||
| "context" | ||
|
|
||
| "k8s.io/apimachinery/pkg/util/validation/field" | ||
|
|
||
| routev1 "github.com/openshift/api/route/v1" | ||
| routecommon "github.com/openshift/library-go/pkg/route" | ||
| ) | ||
|
|
||
| // ValidateHostExternalCertificate checks if the user has permissions to create and update | ||
| // 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. | ||
|
Comment on lines
+13
to
+17
Contributor
There was a problem hiding this comment. Ultimately,
Member
Author
There was a problem hiding this comment. The EP did mention about invoking As per our discussion over Slack and further investigation, I've added a |
||
| // TODO: Consider merging this function into ValidateHostUpdate. | ||
| func ValidateHostExternalCertificate(ctx context.Context, new, older *routev1.Route, sarc routecommon.SubjectAccessReviewCreator, opts routecommon.RouteValidationOptions) field.ErrorList { | ||
|
|
||
| if !opts.AllowExternalCertificates { | ||
chiragkyal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // 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 | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.