CFE-882: Route external certificate validation

pkg/route/common.go

@@ -0,0 +1,60 @@
+package route
+
+import (
+	"context"
+	"fmt"
+
+	authorizationv1 "k8s.io/api/authorization/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/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"
+)
+
+// 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 RouteExternalCertificate
+	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 {
+
+	var errs field.ErrorList
+	user, ok := request.UserFrom(ctx)
+	if !ok {
+		return field.ErrorList{field.InternalError(fldPath, fmt.Errorf("unable to verify access"))}
+	}
+
+	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
+}

pkg/route/hostassignment/assignment.go

@@ -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"
 
-// Registry is an interface for performing subject access reviews
-type SubjectAccessReviewCreator interface {
-	Create(ctx context.Context, sar *authorizationv1.SubjectAccessReview, opts metav1.CreateOptions) (*authorizationv1.SubjectAccessReview, error)
-}
-
 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 SubjectAccessReviewCreator, routeAllocator HostnameGenerator) field.ErrorList {
+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)
+	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
+	}
+
 	if hostSet || certSet {
 		user, ok := request.UserFrom(ctx)
 		if !ok {
@@ -86,41 +91,61 @@ func AllocateHost(ctx context.Context, route *routev1.Route, sarc SubjectAccessR
 	return nil
 }
 
-func hasCertificateInfo(tls *routev1.TLSConfig) bool {
+func hasCertificateInfo(tls *routev1.TLSConfig, opts route.RouteValidationOptions) bool {
 	if tls == nil {
 		return false
 	}
-	return len(tls.Certificate) > 0 ||
+	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
 }
 
-func certificateChangeRequiresAuth(route, older *routev1.Route) bool {
+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) {
+		if !hasCertificateInfo(a, opts) {
 			// removing certificate info is allowed
 			return false
 		}
-		return a.CACertificate != b.CACertificate ||
+
+		certChanged := a.CACertificate != b.CACertificate ||
 			a.Certificate != b.Certificate ||
 			a.DestinationCACertificate != b.DestinationCACertificate ||
 			a.Key != b.Key
+
+		if opts.AllowExternalCertificates && route.Spec.TLS != nil && older.Spec.TLS != nil {
+			if route.Spec.TLS.ExternalCertificate != nil || older.Spec.TLS.ExternalCertificate != nil {
+				certChanged = true
+			}
+		}
+
+		return certChanged
 	case route.Spec.TLS != nil:
 		// using any default certificate is allowed
-		return hasCertificateInfo(route.Spec.TLS)
+		return hasCertificateInfo(route.Spec.TLS, opts)
 	default:
 		// all other cases we are not adding additional certificate info
 		return false
 	}
 }
 
-func ValidateHostUpdate(ctx context.Context, route, older *routev1.Route, sarc SubjectAccessReviewCreator) field.ErrorList {
+// 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)
+	certChanged := certificateChangeRequiresAuth(route, older, opts)
 	if !hostChanged && !certChanged && !subdomainChanged {
 		return nil
 	}
@@ -190,6 +215,10 @@ 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 && 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"))...)
+			}
 			return errs
 		}
 	}

pkg/route/hostassignment/assignment_test.go

@@ -12,6 +12,7 @@ import (
 	"k8s.io/apiserver/pkg/endpoints/request"
 
 	routev1 "github.com/openshift/api/route/v1"
+	"github.com/openshift/library-go/pkg/route"
 )
 
 type testAllocator struct {
@@ -52,6 +53,13 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 		expected          string
 		expectedSubdomain string
 
+		// fields for externalCertificate
+		validType  bool
+		secretData map[string]string
+		validNS    bool
+		opts       route.RouteValidationOptions
+		err        error
+
 		errs  int
 		allow bool
 	}{
@@ -215,6 +223,62 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			allow:          false,
 			errs:           1,
 		},
+		{
+			name:           "external-certificate-changed-from-certificate",
+			host:           "host",
+			expected:       "host",
+			oldHost:        "host",
+			tls:            &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, Certificate: "a"},
+			oldTLS:         &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, ExternalCertificate: &routev1.LocalObjectReference{Name: "b"}},
+			wildcardPolicy: routev1.WildcardPolicyNone,
+			allow:          false,
+			validNS:        true,
+			errs:           1,
+
+			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
+		},
+		{
+			name:           "certificate-changed-from-external-certificate",
+			host:           "host",
+			expected:       "host",
+			oldHost:        "host",
+			tls:            &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, ExternalCertificate: &routev1.LocalObjectReference{Name: "b"}},
+			oldTLS:         &routev1.TLSConfig{Termination: routev1.TLSTerminationEdge, Certificate: "a"},
+			wildcardPolicy: routev1.WildcardPolicyNone,
+			allow:          false,
+			validNS:        true,
+			errs:           1,
+
+			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
+		},
+		{
+			name:           "external-certificate-changed",
+			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: "old-b"}},
+			wildcardPolicy: routev1.WildcardPolicyNone,
+			allow:          false,
+			validNS:        true,
+			errs:           1,
+
+			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
+		},
+		{
+			name:           "external-certificate-secret-changed",
+			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,
+			validNS:        true,
+			errs:           0,
+
+			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
+		},
 		{
 			name:           "ca-certificate-unchanged",
 			host:           "host",
@@ -368,9 +432,9 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 						},
 					},
 				}
-				errs = ValidateHostUpdate(ctx, route, oldRoute, &testSAR{allow: tc.allow})
+				errs = ValidateHostUpdate(ctx, route, oldRoute, &testSAR{allow: tc.allow}, tc.opts)
 			} else {
-				errs = AllocateHost(ctx, route, &testSAR{allow: tc.allow}, testAllocator{})
+				errs = AllocateHost(ctx, route, &testSAR{allow: tc.allow}, testAllocator{}, tc.opts)
 			}
 
 			if route.Spec.Host != tc.expected {

pkg/route/hostassignment/externalcertificate.go

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