Merge pull request #15550 from smarterclayton/strategy

Automatic merge from submit-queue

Changing from no-cert to edge encryption should not panic

A user who doesn't have permission to change from no cert to edge on an update should not be able to cause a panic. In addition, it should be possible for an unprivileged user to strip certificate info out of a previously valid route. Add better tests.

Fixes #15547

Also fixes the broken goversioninfo which prevented a 3.6.0 release build.

Author: openshift-merge-robot

Makefile

@@ -240,7 +240,7 @@ clean:
 #
 # Example:
 #   make release
-official-release: build-rpms build-cross
+official-release: build-cross build-rpms
 	hack/build-images.sh
 .PHONY: official-release
 

hack/lib/build/binaries.sh

@@ -240,7 +240,7 @@ os::build::internal::build_binaries() {
       fi
 
       if [[ "$platform" == "windows/amd64" ]]; then
-        rm ${OS_ROOT}/cmd/oc/oc.syso
+        rm -f ${OS_ROOT}/cmd/oc/oc.syso
       fi
 
       for test in "${tests[@]:+${tests[@]}}"; do
@@ -261,6 +261,10 @@ readonly -f os::build::build_binaries
 # Generates the .syso file used to add compile-time VERSIONINFO metadata to the
 # Windows binary.
 function os::build::generate_windows_versioninfo() {
+  if ! os::util::find::system_binary "goversioninfo" >/dev/null 2>&1; then
+    os::log::warning "No system binary 'goversioninfo' found, skipping version info for Windows builds"
+    return 0
+  fi
   os::build::version::get_vars
   local major="${OS_GIT_MAJOR}"
   local minor="${OS_GIT_MINOR%+}"

hack/release.sh

@@ -15,10 +15,12 @@ elif [[ "$( git rev-parse "${tag}" )" != "$( git rev-parse HEAD )" ]]; then
 fi
 commit="$( git rev-parse ${tag} )"
 
-# Ensure that the build is using the latest release image
-docker pull "${OS_BUILD_ENV_IMAGE}"
+# Ensure that the build is using the latest release image and base content
+if [[ -z "${OS_RELEASE_STALE}" ]]; then
+  docker pull "${OS_BUILD_ENV_IMAGE}"
+  hack/build-base-images.sh
+fi
 
-hack/build-base-images.sh
 hack/env OS_GIT_COMMIT="${commit}" make official-release
 OS_PUSH_ALWAYS=1 OS_PUSH_TAG="${tag}" OS_TAG="" OS_PUSH_LOCAL="1" hack/push-release.sh
 

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"))...)

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 {