diff --git a/pkg/route/common.go b/pkg/route/common.go new file mode 100644 index 0000000000..20f8e0c75f --- /dev/null +++ b/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 +} diff --git a/pkg/route/hostassignment/assignment.go b/pkg/route/hostassignment/assignment.go index bdf9e0e780..0341ea3ee3 100644 --- a/pkg/route/hostassignment/assignment.go +++ b/pkg/route/hostassignment/assignment.go @@ -12,16 +12,12 @@ 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) } @@ -29,9 +25,18 @@ type HostnameGenerator interface { // 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 } } diff --git a/pkg/route/hostassignment/assignment_test.go b/pkg/route/hostassignment/assignment_test.go index 98190569b0..3b1b4f2053 100644 --- a/pkg/route/hostassignment/assignment_test.go +++ b/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 { diff --git a/pkg/route/hostassignment/externalcertificate.go b/pkg/route/hostassignment/externalcertificate.go new file mode 100644 index 0000000000..f0a75e12c4 --- /dev/null +++ b/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 +} diff --git a/pkg/route/hostassignment/externalcertificate_test.go b/pkg/route/hostassignment/externalcertificate_test.go new file mode 100644 index 0000000000..1e4e9465e1 --- /dev/null +++ b/pkg/route/hostassignment/externalcertificate_test.go @@ -0,0 +1,209 @@ +package hostassignment + +import ( + "context" + "testing" + + "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/apiserver/pkg/authentication/user" + "k8s.io/apiserver/pkg/endpoints/request" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + + routev1 "github.com/openshift/api/route/v1" + routecommon "github.com/openshift/library-go/pkg/route" +) + +func TestValidateHostExternalCertificate(t *testing.T) { + + type args struct { + ctx context.Context + new *routev1.Route + older *routev1.Route + allow bool + opts routecommon.RouteValidationOptions + } + tests := []struct { + name string + args args + want field.ErrorList + }{ + { + name: "Updating new route and old route with nil TLS", + args: args{ + ctx: request.WithUser(context.Background(), &user.DefaultInfo{Name: "user1"}), + new: &routev1.Route{ + Spec: routev1.RouteSpec{}, + }, + older: &routev1.Route{ + Spec: routev1.RouteSpec{}, + }, + allow: false, + opts: routecommon.RouteValidationOptions{AllowExternalCertificates: true}, + }, + want: nil, + }, + { + name: "Updating new route with nil TLS", + args: args{ + ctx: request.WithUser(context.Background(), &user.DefaultInfo{Name: "user1"}), + new: &routev1.Route{ + Spec: routev1.RouteSpec{}, + }, + older: &routev1.Route{ + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + Certificate: "old-cert", + }, + }, + }, + allow: false, + opts: routecommon.RouteValidationOptions{AllowExternalCertificates: true}, + }, + want: nil, + }, + { + name: "Updating route from externalCertificate to certificate without permissions", + args: args{ + ctx: request.WithUser(context.Background(), &user.DefaultInfo{Name: "user1"}), + new: &routev1.Route{ + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + ExternalCertificate: &routev1.LocalObjectReference{ + Name: "dummy-cert", + }, + }, + }, + }, + older: &routev1.Route{ + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + Certificate: "old-cert", + }, + }, + }, + allow: false, + opts: routecommon.RouteValidationOptions{AllowExternalCertificates: true}, + }, + want: field.ErrorList{ + field.Forbidden(nil, "somedetail"), + field.Forbidden(nil, "somedetail"), + }, + }, + { + name: "Updating route from externalCertificate to certificate without permissions", + args: args{ + ctx: request.WithUser(context.Background(), &user.DefaultInfo{Name: "user1"}), + new: &routev1.Route{ + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + ExternalCertificate: &routev1.LocalObjectReference{ + Name: "dummy-cert", + }, + }, + }, + }, + older: &routev1.Route{ + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + Certificate: "old-cert", + }, + }, + }, + allow: false, + opts: routecommon.RouteValidationOptions{AllowExternalCertificates: true}, + }, + want: field.ErrorList{ + field.Forbidden(nil, "somedetail"), + field.Forbidden(nil, "somedetail"), + }, + }, + { + name: "Updating route from certificate to externalCertificate without permissions", + args: args{ + ctx: request.WithUser(context.Background(), &user.DefaultInfo{Name: "user1"}), + older: &routev1.Route{ + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + ExternalCertificate: &routev1.LocalObjectReference{ + Name: "dummy-cert", + }, + }, + }, + }, + new: &routev1.Route{ + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + Certificate: "old-cert", + }, + }, + }, + allow: false, + opts: routecommon.RouteValidationOptions{AllowExternalCertificates: true}, + }, + want: field.ErrorList{ + field.Forbidden(nil, "somedetail"), + field.Forbidden(nil, "somedetail"), + }, + }, + { + name: "Updating route from certificate to externalCertificate with permissions", + args: args{ + ctx: request.WithUser(context.Background(), &user.DefaultInfo{Name: "user1"}), + older: &routev1.Route{ + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + ExternalCertificate: &routev1.LocalObjectReference{ + Name: "dummy-cert", + }, + }, + }, + }, + new: &routev1.Route{ + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + Certificate: "old-cert", + }, + }, + }, + allow: true, + opts: routecommon.RouteValidationOptions{AllowExternalCertificates: true}, + }, + want: nil, + }, + { + name: "Updating route from certificate to externalCertificate when feature gate is off and no permission", + args: args{ + ctx: request.WithUser(context.Background(), &user.DefaultInfo{Name: "user1"}), + older: &routev1.Route{ + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + ExternalCertificate: &routev1.LocalObjectReference{ + Name: "dummy-cert", + }, + }, + }, + }, + new: &routev1.Route{ + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + Certificate: "old-cert", + }, + }, + }, + allow: false, + opts: routecommon.RouteValidationOptions{AllowExternalCertificates: false}, + }, + want: nil, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + opts := cmpopts.IgnoreFields(field.Error{}, "Field", "BadValue", "Detail") + if got := ValidateHostExternalCertificate(tt.args.ctx, tt.args.new, tt.args.older, &testSAR{allow: tt.args.allow}, tt.args.opts); !cmp.Equal(got, tt.want, opts, cmpopts.EquateEmpty()) { + t.Errorf("ValidateHostExternalCertificate() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/route/validation/validation.go b/pkg/route/validation/validation.go index 94f39d092a..15580c47e0 100644 --- a/pkg/route/validation/validation.go +++ b/pkg/route/validation/validation.go @@ -1,23 +1,26 @@ package validation import ( - "crypto/ecdsa" - "crypto/rsa" - "crypto/x509" - "encoding/pem" + "context" "fmt" "regexp" "strings" + authorizationv1 "k8s.io/api/authorization/v1" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/sets" kvalidation "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/apiserver/pkg/authentication/user" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" routev1 "github.com/openshift/api/route/v1" + "github.com/openshift/library-go/pkg/authorization/authorizationutil" + routecommon "github.com/openshift/library-go/pkg/route" ) const ( @@ -70,8 +73,8 @@ var ( permittedResponseHeaderValueRE = regexp.MustCompile(strings.Replace(permittedHeaderValueTemplate, "XYZ", "res", 1)) ) -func ValidateRoute(route *routev1.Route) field.ErrorList { - return validateRoute(route, true) +func ValidateRoute(ctx context.Context, route *routev1.Route, sarCreator routecommon.SubjectAccessReviewCreator, secretsGetter corev1client.SecretsGetter, opts routecommon.RouteValidationOptions) field.ErrorList { + return validateRoute(ctx, route, true, sarCreator, secretsGetter, opts) } // validLabels - used in the ValidateRouteUpdate function to check if "older" routes conform to DNS1123Labels or not @@ -95,7 +98,7 @@ func checkLabelSegments(host string) bool { } // validateRoute - private function to validate route -func validateRoute(route *routev1.Route, checkHostname bool) field.ErrorList { +func validateRoute(ctx context.Context, route *routev1.Route, checkHostname bool, sarc routecommon.SubjectAccessReviewCreator, secrets corev1client.SecretsGetter, opts routecommon.RouteValidationOptions) field.ErrorList { //ensure meta is set properly result := validateObjectMeta(&route.ObjectMeta, true, validateRouteName, field.NewPath("metadata")) @@ -202,18 +205,18 @@ func validateRoute(route *routev1.Route, checkHostname bool) field.ErrorList { } } - if errs := validateTLS(route, specPath.Child("tls")); len(errs) != 0 { + if errs := validateTLS(ctx, route, specPath.Child("tls"), sarc, secrets, opts); len(errs) != 0 { result = append(result, errs...) } return result } -func ValidateRouteUpdate(route *routev1.Route, older *routev1.Route) field.ErrorList { +func ValidateRouteUpdate(ctx context.Context, route *routev1.Route, older *routev1.Route, sarc routecommon.SubjectAccessReviewCreator, secrets corev1client.SecretsGetter, opts routecommon.RouteValidationOptions) field.ErrorList { allErrs := validateObjectMetaUpdate(&route.ObjectMeta, &older.ObjectMeta, field.NewPath("metadata")) allErrs = append(allErrs, apimachineryvalidation.ValidateImmutableField(route.Spec.WildcardPolicy, older.Spec.WildcardPolicy, field.NewPath("spec", "wildcardPolicy"))...) hostnameUpdated := route.Spec.Host != older.Spec.Host - allErrs = append(allErrs, validateRoute(route, hostnameUpdated && validLabels(older.Spec.Host))...) + allErrs = append(allErrs, validateRoute(ctx, route, hostnameUpdated && validLabels(older.Spec.Host), sarc, secrets, opts)...) return allErrs } @@ -228,85 +231,9 @@ func ValidateRouteStatusUpdate(route *routev1.Route, older *routev1.Route) field return allErrs } -type blockVerifierFunc func(block *pem.Block) (*pem.Block, error) - -func publicKeyBlockVerifier(block *pem.Block) (*pem.Block, error) { - key, err := x509.ParsePKIXPublicKey(block.Bytes) - if err != nil { - return nil, err - } - block = &pem.Block{ - Type: "PUBLIC KEY", - } - if block.Bytes, err = x509.MarshalPKIXPublicKey(key); err != nil { - return nil, err - } - return block, nil -} - -func certificateBlockVerifier(block *pem.Block) (*pem.Block, error) { - cert, err := x509.ParseCertificate(block.Bytes) - if err != nil { - return nil, err - } - block = &pem.Block{ - Type: "CERTIFICATE", - Bytes: cert.Raw, - } - return block, nil -} - -func privateKeyBlockVerifier(block *pem.Block) (*pem.Block, error) { - key, err := x509.ParsePKCS8PrivateKey(block.Bytes) - if err != nil { - key, err = x509.ParsePKCS1PrivateKey(block.Bytes) - if err != nil { - key, err = x509.ParseECPrivateKey(block.Bytes) - if err != nil { - return nil, fmt.Errorf("block %s is not valid", block.Type) - } - } - } - switch t := key.(type) { - case *rsa.PrivateKey: - block = &pem.Block{ - Type: "RSA PRIVATE KEY", - Bytes: x509.MarshalPKCS1PrivateKey(t), - } - case *ecdsa.PrivateKey: - block = &pem.Block{ - Type: "ECDSA PRIVATE KEY", - } - if block.Bytes, err = x509.MarshalECPrivateKey(t); err != nil { - return nil, err - } - default: - return nil, fmt.Errorf("block private key %T is not valid", key) - } - return block, nil -} - -func ignoreBlockVerifier(block *pem.Block) (*pem.Block, error) { - return nil, nil -} - -var knownBlockDecoders = map[string]blockVerifierFunc{ - "RSA PRIVATE KEY": privateKeyBlockVerifier, - "ECDSA PRIVATE KEY": privateKeyBlockVerifier, - "PRIVATE KEY": privateKeyBlockVerifier, - "PUBLIC KEY": publicKeyBlockVerifier, - // Potential "in the wild" PEM encoded blocks that can be normalized - "RSA PUBLIC KEY": publicKeyBlockVerifier, - "DSA PUBLIC KEY": publicKeyBlockVerifier, - "ECDSA PUBLIC KEY": publicKeyBlockVerifier, - "CERTIFICATE": certificateBlockVerifier, - // Blocks that should be dropped - "EC PARAMETERS": ignoreBlockVerifier, -} - // validateTLS tests fields for different types of TLS combinations are set. Called // by ValidateRoute. -func validateTLS(route *routev1.Route, fldPath *field.Path) field.ErrorList { +func validateTLS(ctx context.Context, route *routev1.Route, fldPath *field.Path, sarc routecommon.SubjectAccessReviewCreator, secrets corev1client.SecretsGetter, opts routecommon.RouteValidationOptions) field.ErrorList { result := field.ErrorList{} tls := route.Spec.TLS @@ -317,8 +244,13 @@ func validateTLS(route *routev1.Route, fldPath *field.Path) field.ErrorList { switch tls.Termination { // reencrypt may specify destination ca cert - // cert, key, cacert may not be specified because the route may be a wildcard + // externalCert, cert, key, cacert may not be specified because the route may be a wildcard case routev1.TLSTerminationReencrypt: + if opts.AllowExternalCertificates && tls.ExternalCertificate != nil { + if len(tls.Certificate) > 0 && len(tls.ExternalCertificate.Name) > 0 { + result = append(result, field.Invalid(fldPath.Child("externalCertificate"), tls.ExternalCertificate.Name, "cannot specify both tls.certificate and tls.externalCertificate")) + } + } //passthrough term should not specify any cert case routev1.TLSTerminationPassthrough: if len(tls.Certificate) > 0 { @@ -329,6 +261,12 @@ func validateTLS(route *routev1.Route, fldPath *field.Path) field.ErrorList { result = append(result, field.Invalid(fldPath.Child("key"), "redacted key data", "passthrough termination does not support certificates")) } + if opts.AllowExternalCertificates && tls.ExternalCertificate != nil { + if len(tls.ExternalCertificate.Name) > 0 { + result = append(result, field.Invalid(fldPath.Child("externalCertificate"), tls.ExternalCertificate.Name, "passthrough termination does not support certificates")) + } + } + if len(tls.CACertificate) > 0 { result = append(result, field.Invalid(fldPath.Child("caCertificate"), "redacted ca certificate data", "passthrough termination does not support certificates")) } @@ -342,6 +280,17 @@ func validateTLS(route *routev1.Route, fldPath *field.Path) field.ErrorList { if len(tls.DestinationCACertificate) > 0 { result = append(result, field.Invalid(fldPath.Child("destinationCACertificate"), "redacted destination ca certificate data", "edge termination does not support destination certificates")) } + + if opts.AllowExternalCertificates && tls.ExternalCertificate != nil { + if len(tls.Certificate) > 0 && len(tls.ExternalCertificate.Name) > 0 { + result = append(result, field.Invalid(fldPath.Child("externalCertificate"), tls.ExternalCertificate.Name, "cannot specify both tls.certificate and tls.externalCertificate")) + break + } else if len(tls.ExternalCertificate.Name) > 0 { + errs := validateTLSExternalCertificate(ctx, route, fldPath.Child("externalCertificate"), sarc, secrets) + result = append(result, errs...) + } + } + default: validValues := []string{string(routev1.TLSTerminationEdge), string(routev1.TLSTerminationPassthrough), string(routev1.TLSTerminationReencrypt)} result = append(result, field.NotSupported(fldPath.Child("termination"), tls.Termination, validValues)) @@ -354,6 +303,58 @@ func validateTLS(route *routev1.Route, fldPath *field.Path) field.ErrorList { return result } +// validateTLSExternalCertificate +func validateTLSExternalCertificate(ctx context.Context, route *routev1.Route, fldPath *field.Path, sarCreator routecommon.SubjectAccessReviewCreator, secretsGetter corev1client.SecretsGetter) field.ErrorList { + tls := route.Spec.TLS + var errs field.ErrorList + + errs = append(errs, routecommon.CheckRouteCustomHostSAR(ctx, fldPath, sarCreator)...) + + if err := authorizationutil.Authorize(sarCreator, &user.DefaultInfo{Name: "system:serviceaccount:openshift-ingress:router"}, + &authorizationv1.ResourceAttributes{ + Namespace: route.Namespace, + Verb: "get", + Resource: "secrets", + Name: tls.ExternalCertificate.Name, + }); err != nil { + errs = append(errs, field.Forbidden(fldPath, "router serviceaccount does not have permission to get this secret")) + } + + if err := authorizationutil.Authorize(sarCreator, &user.DefaultInfo{Name: "system:serviceaccount:openshift-ingress:router"}, + &authorizationv1.ResourceAttributes{ + Namespace: route.Namespace, + Verb: "watch", + Resource: "secrets", + Name: tls.ExternalCertificate.Name, + }); err != nil { + errs = append(errs, field.Forbidden(fldPath, "router serviceaccount does not have permission to watch this secret")) + } + + if err := authorizationutil.Authorize(sarCreator, &user.DefaultInfo{Name: "system:serviceaccount:openshift-ingress:router"}, + &authorizationv1.ResourceAttributes{ + Namespace: route.Namespace, + Verb: "list", + Resource: "secrets", + Name: tls.ExternalCertificate.Name, + }); err != nil { + errs = append(errs, field.Forbidden(fldPath, "router serviceaccount does not have permission to list this secret")) + } + + secret, err := secretsGetter.Secrets(route.Namespace).Get(ctx, tls.ExternalCertificate.Name, metav1.GetOptions{}) + if err != nil && apierrors.IsNotFound(err) { + errs = append(errs, field.NotFound(fldPath, err)) + return errs + } else if err != nil { + return append(errs, field.InternalError(fldPath, err)) + } + + if secret.Type != corev1.SecretTypeTLS { + errs = append(errs, field.Invalid(fldPath, tls.ExternalCertificate.Name, "secret of type 'kubernetes/tls' required")) + } + + return errs +} + // validateInsecureEdgeTerminationPolicy tests fields for different types of // insecure options. Called by validateTLS. func validateInsecureEdgeTerminationPolicy(tls *routev1.TLSConfig, fldPath *field.Path) *field.Error { diff --git a/pkg/route/validation/validation_test.go b/pkg/route/validation/validation_test.go index b8713245d1..558782f2a0 100644 --- a/pkg/route/validation/validation_test.go +++ b/pkg/route/validation/validation_test.go @@ -1,16 +1,26 @@ package validation import ( + "context" "fmt" "regexp" "strings" "testing" - routev1 "github.com/openshift/api/route/v1" - + authorizationv1 "k8s.io/api/authorization/v1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/apiserver/pkg/authentication/user" + "k8s.io/apiserver/pkg/endpoints/request" + testclient "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/kubernetes/scheme" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" + + routev1 "github.com/openshift/api/route/v1" + routecommon "github.com/openshift/library-go/pkg/route" ) const ( @@ -42,6 +52,63 @@ func createRouteSpecTo(name string, kind string) routev1.RouteTargetReference { return svc } +type testSARCreator struct { + allow bool + err error + sar *authorizationv1.SubjectAccessReview +} + +func (t *testSARCreator) Create(_ context.Context, subjectAccessReview *authorizationv1.SubjectAccessReview, _ metav1.CreateOptions) (*authorizationv1.SubjectAccessReview, error) { + t.sar = subjectAccessReview + return &authorizationv1.SubjectAccessReview{ + Status: authorizationv1.SubjectAccessReviewStatus{ + Allowed: t.allow, + }, + }, t.err +} + +type testSecretGetter struct { + getter corev1client.SecretsGetter + secrets []*corev1.Secret + namespace string +} + +func (t *testSecretGetter) Secrets(_ string) corev1client.SecretInterface { + existingObjects := []runtime.Object{} + for _, s := range t.secrets { + existingObjects = append(existingObjects, s) + } + return testclient.NewSimpleClientset(existingObjects...).CoreV1().Secrets(t.namespace) +} + +func (t *testSecretGetter) WithSecret(secret *corev1.Secret) *testSecretGetter { + t.secrets = append(t.secrets, secret) + return t +} + +func (t *testSecretGetter) WithNamespace(namespace string) *testSecretGetter { + t.namespace = namespace + return t +} + +func (t *testSecretGetter) WithSecretData(route *routev1.Route, data map[string]string) *testSecretGetter { + t.secrets = append(t.secrets, &corev1.Secret{ + StringData: data, + Type: corev1.SecretTypeTLS, + ObjectMeta: metav1.ObjectMeta{ + Name: route.Spec.TLS.ExternalCertificate.Name, + Namespace: route.Namespace, + }, + }) + return t +} + +func init() { + scheme := scheme.Scheme + corev1.AddToScheme(scheme) + testclient.AddToScheme(scheme) +} + // Context around testing hostname validation. // Currently the host name validation checks along these lines // @@ -971,7 +1038,7 @@ func TestValidateRoute(t *testing.T) { } for _, tc := range tests { - errs := ValidateRoute(tc.route) + errs := ValidateRoute(context.Background(), tc.route, &testSARCreator{allow: false}, &testSecretGetter{}, routecommon.RouteValidationOptions{AllowExternalCertificates: false}) if len(errs) != tc.expectedErrors { t.Errorf("Test case %s expected %d error(s), got %d. %v", tc.name, tc.expectedErrors, len(errs), errs) } @@ -1800,6 +1867,13 @@ func TestValidateTLS(t *testing.T) { name string route *routev1.Route expectedErrors int + + // fields for externalCertificate + allow bool + validType bool + validNS bool + opts routecommon.RouteValidationOptions + err error }{ { name: "No TLS Termination", @@ -1946,12 +2020,162 @@ func TestValidateTLS(t *testing.T) { }, }, }, + + expectedErrors: 1, + }, + { + name: "Invalid Reencrypt termination as externalCertificate and certificate set", + route: &routev1.Route{ + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + Termination: routev1.TLSTerminationReencrypt, + ExternalCertificate: &routev1.LocalObjectReference{ + Name: "tls-secret", + }, + Certificate: "dummy", + }, + }, + }, + opts: routecommon.RouteValidationOptions{AllowExternalCertificates: true}, + expectedErrors: 1, + }, + { + name: "Invalid Passthrough termination as externalCertificate", + route: &routev1.Route{ + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + Termination: routev1.TLSTerminationPassthrough, + ExternalCertificate: &routev1.LocalObjectReference{ + Name: "tls-secret", + }, + }, + }, + }, + opts: routecommon.RouteValidationOptions{AllowExternalCertificates: true}, + expectedErrors: 1, + }, + { + name: "Invalid Passthrough termination as externalCertificate and certificate set", + route: &routev1.Route{ + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + Termination: routev1.TLSTerminationPassthrough, + ExternalCertificate: &routev1.LocalObjectReference{ + Name: "tls-secret", + }, + Certificate: "dummy", + }, + }, + }, + opts: routecommon.RouteValidationOptions{AllowExternalCertificates: true}, + expectedErrors: 2, + }, + { + name: "Invalid Edge termination as externalCertificate and certificate", + route: &routev1.Route{ + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + Termination: routev1.TLSTerminationEdge, + ExternalCertificate: &routev1.LocalObjectReference{ + Name: "tls-secret", + }, + Certificate: "dummy", + }, + }, + }, + opts: routecommon.RouteValidationOptions{AllowExternalCertificates: true}, + expectedErrors: 1, + }, + { + name: "Invalid externalCertificate as not authorized", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "route-test", + Namespace: "sandbox", + }, + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + Termination: routev1.TLSTerminationEdge, + ExternalCertificate: &routev1.LocalObjectReference{ + Name: "tls-secret", + }, + }, + }, + }, + allow: false, + validType: true, + validNS: true, + opts: routecommon.RouteValidationOptions{AllowExternalCertificates: true}, + expectedErrors: 5, + }, + { + name: "Invalid externalCertificate as secret not found", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "route-test", + Namespace: "sandbox", + }, + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + Termination: routev1.TLSTerminationEdge, + ExternalCertificate: &routev1.LocalObjectReference{ + Name: "tls-secret", + }, + }, + }, + }, + allow: true, + validNS: false, + validType: true, + opts: routecommon.RouteValidationOptions{AllowExternalCertificates: true}, + expectedErrors: 1, + }, + { + name: "Invalid externalCertificate as secret of incorrect type", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "route-test", + Namespace: "sandbox", + }, + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + Termination: routev1.TLSTerminationEdge, + ExternalCertificate: &routev1.LocalObjectReference{ + Name: "tls-secret", + }, + }, + }, + }, + allow: true, + validNS: true, + validType: false, + opts: routecommon.RouteValidationOptions{AllowExternalCertificates: true}, expectedErrors: 1, }, } + ctx := request.WithUser(context.Background(), &user.DefaultInfo{}) for _, tc := range tests { - errs := validateTLS(tc.route, nil) + + secrets := &testSecretGetter{} + secrets.WithNamespace(tc.route.Namespace) + + var secret corev1.Secret + if tc.opts.AllowExternalCertificates { + secret.ObjectMeta.Name = tc.route.Spec.TLS.ExternalCertificate.Name + secret.ObjectMeta.Namespace = tc.route.Namespace + secret.Type = corev1.SecretTypeTLS + } + + if !tc.validType && tc.opts.AllowExternalCertificates { + secret.Type = corev1.SecretTypeBasicAuth + } else if !tc.validNS && tc.opts.AllowExternalCertificates { + secret.ObjectMeta.Namespace = "dummyns" + } + + secrets.WithSecret(&secret) + + errs := validateTLS(ctx, tc.route, nil, &testSARCreator{allow: tc.allow}, secrets, tc.opts) if len(errs) != tc.expectedErrors { t.Errorf("Test case %s expected %d error(s), got %d. %v", tc.name, tc.expectedErrors, len(errs), errs) @@ -1980,7 +2204,7 @@ func TestValidatePassthroughInsecureEdgeTerminationPolicy(t *testing.T) { }, } route.Spec.TLS.InsecureEdgeTerminationPolicy = key - errs := validateTLS(route, nil) + errs := validateTLS(context.Background(), route, nil, &testSARCreator{allow: false}, &testSecretGetter{}, routecommon.RouteValidationOptions{}) if !expected && len(errs) != 0 { t.Errorf("Test case for Passthrough termination with insecure=%s got %d errors where none where expected. %v", key, len(errs), errs) @@ -2145,7 +2369,7 @@ func TestValidateRouteUpdate(t *testing.T) { for i, tc := range tests { newRoute := tc.route.DeepCopy() tc.change(newRoute) - errs := ValidateRouteUpdate(newRoute, tc.route) + errs := ValidateRouteUpdate(context.Background(), newRoute, tc.route, &testSARCreator{allow: false}, &testSecretGetter{}, routecommon.RouteValidationOptions{}) if len(errs) != tc.expectedErrors { t.Errorf("%d: expected %d error(s), got %d. %v", i, tc.expectedErrors, len(errs), errs) } @@ -2321,7 +2545,7 @@ func TestValidateInsecureEdgeTerminationPolicy(t *testing.T) { }, }, } - errs := validateTLS(route, nil) + errs := validateTLS(context.Background(), route, nil, &testSARCreator{allow: false}, &testSecretGetter{}, routecommon.RouteValidationOptions{}) if len(errs) != tc.expectedErrors { t.Errorf("Test case %s expected %d error(s), got %d. %v", tc.name, tc.expectedErrors, len(errs), errs) @@ -2379,7 +2603,7 @@ func TestValidateEdgeReencryptInsecureEdgeTerminationPolicy(t *testing.T) { for _, tc := range tests { for key, expected := range insecureTypes { tc.route.Spec.TLS.InsecureEdgeTerminationPolicy = key - errs := validateTLS(tc.route, nil) + errs := validateTLS(context.Background(), tc.route, nil, &testSARCreator{allow: false}, &testSecretGetter{}, routecommon.RouteValidationOptions{}) if !expected && len(errs) != 0 { t.Errorf("Test case %s with insecure=%s got %d errors where none were expected. %v", tc.name, key, len(errs), errs)