From d6de0f3dc01971c93c555f384520f25c022489a4 Mon Sep 17 00:00:00 2001
From: Matt Moore <mattmoor@google.com>
Date: Thu, 9 May 2019 04:28:06 +0000
Subject: [PATCH] Be strict about env vars reserved by the runtime contract.

Add validation analogous to the "reserved paths" validation for
volume mounts, which rejects env vars that take a name that has
been specified as part of the runtime contract.

Fixes: https://github.com/knative/serving/issues/3328
---
 pkg/apis/serving/k8s_validation.go           | 25 ++++++++++++++++++--
 pkg/apis/serving/k8s_validation_test.go      | 22 +++++++++++++++++
 pkg/reconciler/revision/resources/env_var.go | 24 ++++++++-----------
 3 files changed, 55 insertions(+), 16 deletions(-)

diff --git a/pkg/apis/serving/k8s_validation.go b/pkg/apis/serving/k8s_validation.go
index c83d6a71b038..239c02f03111 100644
--- a/pkg/apis/serving/k8s_validation.go
+++ b/pkg/apis/serving/k8s_validation.go
@@ -46,6 +46,13 @@ var (
 		"/var/log",
 	)
 
+	reservedEnvVars = sets.NewString(
+		"PORT",
+		"K_SERVICE",
+		"K_CONFIGURATION",
+		"K_REVISION",
+	)
+
 	// The port is named "user-port" on the deployment, but a user cannot set an arbitrary name on the port
 	// in Configuration. The name field is reserved for content-negotiation. Currently 'h2c' and 'http1' are
 	// allowed.
@@ -96,11 +103,25 @@ func validateEnvValueFrom(source *corev1.EnvVarSource) *apis.FieldError {
 	return apis.CheckDisallowedFields(*source, *EnvVarSourceMask(source))
 }
 
+func validateEnvVar(env corev1.EnvVar) *apis.FieldError {
+	errs := apis.CheckDisallowedFields(env, *EnvVarMask(&env))
+
+	if env.Name == "" {
+		errs = errs.Also(apis.ErrMissingField("name"))
+	} else if reservedEnvVars.Has(env.Name) {
+		errs = errs.Also(&apis.FieldError{
+			Message: fmt.Sprintf("%q is a reserved environment variable", env.Name),
+			Paths:   []string{"name"},
+		})
+	}
+
+	return errs.Also(validateEnvValueFrom(env.ValueFrom).ViaField("valueFrom"))
+}
+
 func validateEnv(envVars []corev1.EnvVar) *apis.FieldError {
 	var errs *apis.FieldError
 	for i, env := range envVars {
-		errs = errs.Also(apis.CheckDisallowedFields(env, *EnvVarMask(&env)).ViaIndex(i)).Also(
-			validateEnvValueFrom(env.ValueFrom).ViaField("valueFrom").ViaIndex(i))
+		errs = errs.Also(validateEnvVar(env).ViaIndex(i))
 	}
 	return errs
 }
diff --git a/pkg/apis/serving/k8s_validation_test.go b/pkg/apis/serving/k8s_validation_test.go
index 23baf4e1bc94..89bfe4e53f57 100644
--- a/pkg/apis/serving/k8s_validation_test.go
+++ b/pkg/apis/serving/k8s_validation_test.go
@@ -567,6 +567,28 @@ func TestContainerValidation(t *testing.T) {
 			TerminationMessagePolicy: corev1.TerminationMessagePolicy("Not a Policy"),
 		},
 		want: apis.ErrInvalidValue(corev1.TerminationMessagePolicy("Not a Policy"), "terminationMessagePolicy"),
+	}, {
+		name: "empty env var name",
+		c: corev1.Container{
+			Image: "foo",
+			Env: []corev1.EnvVar{{
+				Value: "Foo",
+			}},
+		},
+		want: apis.ErrMissingField("env[0].name"),
+	}, {
+		name: "reserved env var name",
+		c: corev1.Container{
+			Image: "foo",
+			Env: []corev1.EnvVar{{
+				Name:  "PORT",
+				Value: "Foo",
+			}},
+		},
+		want: &apis.FieldError{
+			Message: `"PORT" is a reserved environment variable`,
+			Paths:   []string{"env[0].name"},
+		},
 	}, {
 		name: "disallowed envvarsource",
 		c: corev1.Container{
diff --git a/pkg/reconciler/revision/resources/env_var.go b/pkg/reconciler/revision/resources/env_var.go
index 3cc3d3cc396e..3d86219f64fb 100644
--- a/pkg/reconciler/revision/resources/env_var.go
+++ b/pkg/reconciler/revision/resources/env_var.go
@@ -29,18 +29,14 @@ const (
 )
 
 func getKnativeEnvVar(rev *v1alpha1.Revision) []corev1.EnvVar {
-	return []corev1.EnvVar{
-		{
-			Name:  knativeRevisionEnvVariableKey,
-			Value: rev.Name,
-		},
-		{
-			Name:  knativeConfigurationEnvVariableKey,
-			Value: rev.Labels[serving.ConfigurationLabelKey],
-		},
-		{
-			Name:  knativeServiceEnvVariableKey,
-			Value: rev.Labels[serving.ServiceLabelKey],
-		},
-	}
+	return []corev1.EnvVar{{
+		Name:  knativeRevisionEnvVariableKey,
+		Value: rev.Name,
+	}, {
+		Name:  knativeConfigurationEnvVariableKey,
+		Value: rev.Labels[serving.ConfigurationLabelKey],
+	}, {
+		Name:  knativeServiceEnvVariableKey,
+		Value: rev.Labels[serving.ServiceLabelKey],
+	}}
 }