Changing from no cert to edge encryption should not panic

The new route permission checks were too aggressive, erroring out if the user was attempting to clear route certificates, or panicking if setting a new TLS struct. Fix the conditions to be clearer, and add a test to guard the new edge cases.
openshift · Jul 30, 2017 · 0955cfa · 0955cfa
1 parent 3c30c9f
commit 0955cfa
Show file tree

Hide file tree

Showing 2 changed files with 68 additions and 5 deletions.
diff --git a/pkg/route/registry/route/strategy.go b/pkg/route/registry/route/strategy.go
@@ -146,24 +146,40 @@ func (s routeStrategy) ValidateUpdate(ctx apirequest.Context, obj, old runtime.O
 	return errs
 }
 
-func certificateChanged(route, older *routeapi.Route) bool {
+func hasCertificateInfo(tls *routeapi.TLSConfig) bool {
+	if tls == nil {
+		return false
+	}
+	return len(tls.Certificate) > 0 ||
+		len(tls.Key) > 0 ||
+		len(tls.CACertificate) > 0 ||
+		len(tls.DestinationCACertificate) > 0
+}
+
+func certificateChangeRequiresAuth(route, older *routeapi.Route) bool {
 	switch {
 	case route.Spec.TLS != nil && older.Spec.TLS != nil:
 		a, b := route.Spec.TLS, older.Spec.TLS
+		if !hasCertificateInfo(a) {
+			// removing certificate info is allowed
+			return false
+		}
 		return a.CACertificate != b.CACertificate ||
 			a.Certificate != b.Certificate ||
 			a.DestinationCACertificate != b.DestinationCACertificate ||
 			a.Key != b.Key
-	case route.Spec.TLS == nil && older.Spec.TLS == nil:
-		return false
+	case route.Spec.TLS != nil:
+		// using any default certificate is allowed
+		return hasCertificateInfo(route.Spec.TLS)
 	default:
-		return true
+		// all other cases we are not adding additional certificate info
+		return false
 	}
 }
 
 func (s routeStrategy) validateHostUpdate(ctx apirequest.Context, route, older *routeapi.Route) field.ErrorList {
 	hostChanged := route.Spec.Host != older.Spec.Host
-	certChanged := certificateChanged(route, older)
+	certChanged := certificateChangeRequiresAuth(route, older)
 	if !hostChanged && !certChanged {
 		return nil
 	}
@@ -191,6 +207,9 @@ func (s routeStrategy) validateHostUpdate(ctx apirequest.Context, route, older *
 		if hostChanged {
 			return kvalidation.ValidateImmutableField(route.Spec.Host, older.Spec.Host, field.NewPath("spec", "host"))
 		}
+		if route.Spec.TLS == nil || older.Spec.TLS == nil {
+			return kvalidation.ValidateImmutableField(route.Spec.TLS, older.Spec.TLS, field.NewPath("spec", "tls"))
+		}
 		errs := kvalidation.ValidateImmutableField(route.Spec.TLS.CACertificate, older.Spec.TLS.CACertificate, field.NewPath("spec", "tls", "caCertificate"))
 		errs = append(errs, kvalidation.ValidateImmutableField(route.Spec.TLS.Certificate, older.Spec.TLS.Certificate, field.NewPath("spec", "tls", "certificate"))...)
 		errs = append(errs, kvalidation.ValidateImmutableField(route.Spec.TLS.DestinationCACertificate, older.Spec.TLS.DestinationCACertificate, field.NewPath("spec", "tls", "destinationCACertificate"))...)

diff --git a/pkg/route/registry/route/strategy_test.go b/pkg/route/registry/route/strategy_test.go
@@ -328,6 +328,50 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			allow:          false,
 			errs:           1,
 		},
+		{
+			name:           "set-to-edge-changed",
+			host:           "host",
+			expected:       "host",
+			oldHost:        "host",
+			tls:            &routeapi.TLSConfig{Termination: routeapi.TLSTerminationEdge},
+			oldTLS:         nil,
+			wildcardPolicy: routeapi.WildcardPolicyNone,
+			allow:          false,
+			errs:           0,
+		},
+		{
+			name:           "cleared-edge",
+			host:           "host",
+			expected:       "host",
+			oldHost:        "host",
+			tls:            nil,
+			oldTLS:         &routeapi.TLSConfig{Termination: routeapi.TLSTerminationEdge},
+			wildcardPolicy: routeapi.WildcardPolicyNone,
+			allow:          false,
+			errs:           0,
+		},
+		{
+			name:           "removed-certificate",
+			host:           "host",
+			expected:       "host",
+			oldHost:        "host",
+			tls:            &routeapi.TLSConfig{Termination: routeapi.TLSTerminationReencrypt},
+			oldTLS:         &routeapi.TLSConfig{Termination: routeapi.TLSTerminationReencrypt, Certificate: "a"},
+			wildcardPolicy: routeapi.WildcardPolicyNone,
+			allow:          false,
+			errs:           0,
+		},
+		{
+			name:           "added-certificate-and-fails",
+			host:           "host",
+			expected:       "host",
+			oldHost:        "host",
+			tls:            &routeapi.TLSConfig{Termination: routeapi.TLSTerminationReencrypt, Certificate: "a"},
+			oldTLS:         nil,
+			wildcardPolicy: routeapi.WildcardPolicyNone,
+			allow:          false,
+			errs:           1,
+		},
 	}
 
 	for _, tc := range tests {