Do not allow pods in one namespace to create certificates for hostnames from another namespace. (#54)

* Do not allow pods in one namespace to create certificates for hostnames from another namespace.

* Make cluster domain configurable, clean up shouldMutate() logic, and make namespace restrictions configurable with restrictCertificatesToNamespace.

* Return certificate hostname validation errors in the admission webhook response.

* Appease the gometalinter.

autocert/controller/main.go

@@ -45,12 +45,24 @@ const (
 
 // Config options for the autocert admission controller.
 type Config struct {
-	LogFormat    string           `yaml:"logFormat"`
-	CaURL        string           `yaml:"caUrl"`
-	CertLifetime string           `yaml:"certLifetime"`
-	Bootstrapper corev1.Container `yaml:"bootstrapper"`
-	Renewer      corev1.Container `yaml:"renewer"`
-	CertsVolume  corev1.Volume    `yaml:"certsVolume"`
+	LogFormat                       string           `yaml:"logFormat"`
+	CaURL                           string           `yaml:"caUrl"`
+	CertLifetime                    string           `yaml:"certLifetime"`
+	Bootstrapper                    corev1.Container `yaml:"bootstrapper"`
+	Renewer                         corev1.Container `yaml:"renewer"`
+	CertsVolume                     corev1.Volume    `yaml:"certsVolume"`
+	RestrictCertificatesToNamespace bool             `yaml:"restrictCertificatesToNamespace"`
+	ClusterDomain                   string           `yaml:"clusterDomain"`
+}
+
+// GetClusterDomain returns the Kubernetes cluster domain, defaults to
+// "cluster.local" if not specified in the configuration.
+func (c Config) GetClusterDomain() string {
+	if c.ClusterDomain != "" {
+		return c.ClusterDomain
+	}
+
+	return "cluster.local"
 }
 
 // PatchOperation represents a RFC6902 JSONPatch Operation
@@ -216,6 +228,7 @@ func mkBootstrapper(config *Config, commonName string, namespace string, provisi
 		Name:  "COMMON_NAME",
 		Value: commonName,
 	})
+
 	b.Env = append(b.Env, corev1.EnvVar{
 		Name: "STEP_TOKEN",
 		ValueFrom: &corev1.EnvVarSource{
@@ -357,7 +370,8 @@ func addAnnotations(existing, new map[string]string) (ops []PatchOperation) {
 func patch(pod *corev1.Pod, namespace string, config *Config, provisioner Provisioner) ([]byte, error) {
 	var ops []PatchOperation
 
-	commonName := pod.ObjectMeta.GetAnnotations()[admissionWebhookAnnotationKey]
+	annotations := pod.ObjectMeta.GetAnnotations()
+	commonName := annotations[admissionWebhookAnnotationKey]
 	renewer := mkRenewer(config)
 	bootstrapper, err := mkBootstrapper(config, commonName, namespace, provisioner)
 	if err != nil {
@@ -376,7 +390,10 @@ func patch(pod *corev1.Pod, namespace string, config *Config, provisioner Provis
 // shouldMutate checks whether a pod is subject to mutation by this admission controller. A pod
 // is subject to mutation if it's annotated with the `admissionWebhookAnnotationKey` and if it
 // has not already been processed (indicated by `admissionWebhookStatusKey` set to `injected`).
-func shouldMutate(metadata *metav1.ObjectMeta) bool {
+// If the pod requests a certificate with a subject matching a namespace other than its own
+// and restrictToNamespace is true, then shouldMutate will return a validation error
+// that should be returned to the client.
+func shouldMutate(metadata *metav1.ObjectMeta, namespace string, clusterDomain string, restrictToNamespace bool) (bool, error) {
 	annotations := metadata.GetAnnotations()
 	if annotations == nil {
 		annotations = map[string]string{}
@@ -385,10 +402,26 @@ func shouldMutate(metadata *metav1.ObjectMeta) bool {
 	// Only mutate if the object is annotated appropriately (annotation key set) and we haven't
 	// mutated already (status key isn't set).
 	if annotations[admissionWebhookAnnotationKey] == "" || annotations[admissionWebhookStatusKey] == "injected" {
-		return false
+		return false, nil
+	}
+
+	if !restrictToNamespace {
+		return true, nil
+	}
+
+	subject := strings.Trim(annotations[admissionWebhookAnnotationKey], ".")
+
+	err := fmt.Errorf("subject \"%s\" matches a namespace other than \"%s\" and is not permitted. This check can be disabled by setting restrictCertificatesToNamespace to false in the autocert-config ConfigMap", subject, namespace)
+
+	if strings.HasSuffix(subject, ".svc") && !strings.HasSuffix(subject, fmt.Sprintf(".%s.svc", namespace)) {
+		return false, err
 	}
 
-	return true
+	if strings.HasSuffix(subject, fmt.Sprintf(".svc.%s", clusterDomain)) && !strings.HasSuffix(subject, fmt.Sprintf(".%s.svc.%s", namespace, clusterDomain)) {
+		return false, err
+	}
+
+	return true, nil
 }
 
 // mutate takes an `AdmissionReview`, determines whether it is subject to mutation, and returns
@@ -418,7 +451,20 @@ func mutate(review *v1beta1.AdmissionReview, config *Config, provisioner Provisi
 		"user":         request.UserInfo,
 	})
 
-	if !shouldMutate(&pod.ObjectMeta) {
+	mutationAllowed, validationErr := shouldMutate(&pod.ObjectMeta, request.Namespace, config.GetClusterDomain(), config.RestrictCertificatesToNamespace)
+
+	if validationErr != nil {
+		ctxLog.WithField("error", validationErr).Info("Validation error")
+		return &v1beta1.AdmissionResponse{
+			Allowed: false,
+			UID:     request.UID,
+			Result: &metav1.Status{
+				Message: validationErr.Error(),
+			},
+		}
+	}
+
+	if !mutationAllowed {
 		ctxLog.WithField("annotations", pod.Annotations).Info("Skipping mutation")
 		return &v1beta1.AdmissionResponse{
 			Allowed: true,

autocert/controller/main_test.go

@@ -0,0 +1,75 @@
+package main
+
+import (
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"testing"
+)
+
+func TestGetClusterDomain(t *testing.T) {
+	c := Config{}
+	if c.GetClusterDomain() != "cluster.local" {
+		t.Errorf("cluster domain should default to cluster.local, not: %s", c.GetClusterDomain())
+	}
+
+	c.ClusterDomain = "mydomain.com"
+	if c.GetClusterDomain() != "mydomain.com" {
+		t.Errorf("cluster domain should default to cluster.local, not: %s", c.GetClusterDomain())
+	}
+}
+
+func TestShouldMutate(t *testing.T) {
+	testCases := []struct {
+		description string
+		subject     string
+		namespace   string
+		expected    bool
+	}{
+		{"full cluster domain", "test.default.svc.cluster.local", "default", true},
+		{"full cluster domain wrong ns", "test.default.svc.cluster.local", "kube-system", false},
+		{"left dots get stripped", ".test.default.svc.cluster.local", "default", true},
+		{"left dots get stripped wrong ns", ".test.default.svc.cluster.local", "kube-system", false},
+		{"right dots get stripped", "test.default.svc.cluster.local.", "default", true},
+		{"right dots get stripped wrong ns", "test.default.svc.cluster.local.", "kube-system", false},
+		{"dots get stripped", ".test.default.svc.cluster.local.", "default", true},
+		{"dots get stripped wrong ns", ".test.default.svc.cluster.local.", "kube-system", false},
+		{"partial cluster domain", "test.default.svc.cluster", "default", true},
+		{"partial cluster domain wrong ns is still allowed because not valid hostname", "test.default.svc.cluster", "kube-system", true},
+		{"service domain", "test.default.svc", "default", true},
+		{"service domain wrong ns", "test.default.svc", "kube-system", false},
+		{"two part domain", "test.default", "default", true},
+		{"two part domain different ns", "test.default", "kube-system", true},
+		{"one hostname", "test", "default", true},
+		{"no subject specified", "", "default", false},
+		{"three part not cluster", "test.default.com", "kube-system", true},
+		{"four part not cluster", "test.default.svc.com", "kube-system", true},
+		{"five part not cluster", "test.default.svc.cluster.com", "kube-system", true},
+		{"six part not cluster", "test.default.svc.cluster.local.com", "kube-system", true},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.description, func(t *testing.T) {
+			mutationAllowed, validationErr := shouldMutate(&metav1.ObjectMeta{
+				Annotations: map[string]string{
+					admissionWebhookAnnotationKey: testCase.subject,
+				},
+			}, testCase.namespace, "cluster.local", true)
+			if mutationAllowed != testCase.expected {
+				t.Errorf("shouldMutate did not return %t for %s", testCase.expected, testCase.description)
+			}
+			if testCase.subject != "" && mutationAllowed == false && validationErr == nil {
+				t.Errorf("shouldMutate should return validation error for invalid hostname")
+			}
+		})
+	}
+}
+
+func TestShouldMutateNotRestrictToNamespace(t *testing.T) {
+	mutationAllowed, _ := shouldMutate(&metav1.ObjectMeta{
+		Annotations: map[string]string{
+			admissionWebhookAnnotationKey: "test.default.svc.cluster.local",
+		},
+	}, "kube-system", "cluster.local", false)
+	if mutationAllowed == false {
+		t.Errorf("shouldMutate should return true even with a wrong namespace if restrictToNamespace is false.")
+	}
+}

autocert/install/02-autocert.yaml

@@ -21,6 +21,8 @@ metadata:
 data:
   config.yaml: |
     logFormat: json # or text
+    restrictCertificatesToNamespace: true
+    clusterDomain: cluster.local
     caUrl: https://ca.step.svc.cluster.local
     certLifetime: 24h
     renewer: