From 0955cfa48000352b46a73a427abe14a2c60e1ac8 Mon Sep 17 00:00:00 2001
From: Clayton Coleman <ccoleman@redhat.com>
Date: Sun, 30 Jul 2017 14:48:08 -0400
Subject: [PATCH] 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.
---
 pkg/route/registry/route/strategy.go      | 29 ++++++++++++---
 pkg/route/registry/route/strategy_test.go | 44 +++++++++++++++++++++++
 2 files changed, 68 insertions(+), 5 deletions(-)

diff --git a/pkg/route/registry/route/strategy.go b/pkg/route/registry/route/strategy.go
index 2a8503517bcf..54282b3ede71 100644
--- 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
index b09862661cf5..d9b15d25269f 100644
--- 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 {