diff --git a/autocert/controller/main.go b/autocert/controller/main.go index 68cf3bfd..eeb8f393 100644 --- a/autocert/controller/main.go +++ b/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, diff --git a/autocert/controller/main_test.go b/autocert/controller/main_test.go new file mode 100644 index 00000000..1f0290eb --- /dev/null +++ b/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.") + } +} diff --git a/autocert/install/02-autocert.yaml b/autocert/install/02-autocert.yaml index f6453ca2..07f722bf 100644 --- a/autocert/install/02-autocert.yaml +++ b/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: