From 5d2c8fcda0f0cf31608bf80780b9f247bd1f398d Mon Sep 17 00:00:00 2001 From: Serge Bazanski Date: Sat, 30 Jan 2021 21:23:53 +0100 Subject: [PATCH] cluster/admitomatic: finish up ingress admission logic This gives us nearly everything required to run the admission controller. In addition to checking for allowed domains, we also do some nginx-inress-controller security checks. Change-Id: Ib187de6d2c06c58bd8c320503d4f850df2ec8abd --- cluster/admitomatic/BUILD.bazel | 8 ++ cluster/admitomatic/ingress.go | 102 +++++++++++++++++++++- cluster/admitomatic/ingress_test.go | 129 +++++++++++++++++++++++++++- 3 files changed, 236 insertions(+), 3 deletions(-) diff --git a/cluster/admitomatic/BUILD.bazel b/cluster/admitomatic/BUILD.bazel index 5cb23abc..4d5aebc9 100644 --- a/cluster/admitomatic/BUILD.bazel +++ b/cluster/admitomatic/BUILD.bazel @@ -12,6 +12,8 @@ go_library( "//go/mirko:go_default_library", "@com_github_golang_glog//:go_default_library", "@io_k8s_api//admission/v1beta1:go_default_library", + "@io_k8s_api//networking/v1beta1:go_default_library", + "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", ], ) @@ -25,4 +27,10 @@ go_test( name = "go_default_test", srcs = ["ingress_test.go"], embed = [":go_default_library"], + deps = [ + "@io_k8s_api//admission/v1beta1:go_default_library", + "@io_k8s_api//networking/v1beta1:go_default_library", + "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", + "@io_k8s_apimachinery//pkg/runtime:go_default_library", + ], ) diff --git a/cluster/admitomatic/ingress.go b/cluster/admitomatic/ingress.go index 42cab98b..298318fb 100644 --- a/cluster/admitomatic/ingress.go +++ b/cluster/admitomatic/ingress.go @@ -1,10 +1,15 @@ package main import ( + "encoding/json" "fmt" "strings" + "github.com/golang/glog" + admission "k8s.io/api/admission/v1beta1" + networking "k8s.io/api/networking/v1beta1" + meta "k8s.io/apimachinery/pkg/apis/meta/v1" ) // ingressFilter is a filter which allows or denies the creation of an ingress @@ -125,6 +130,99 @@ func (i *ingressFilter) admit(req *admission.AdmissionRequest) (*admission.Admis if req.Kind.Group != "networking.k8s.io" || req.Kind.Kind != "Ingress" { return nil, fmt.Errorf("not an ingress") } - // TODO(q3k); implement - return nil, fmt.Errorf("unimplemented") + + result := func(s string, args ...interface{}) (*admission.AdmissionResponse, error) { + res := &admission.AdmissionResponse{ + UID: req.UID, + } + if s == "" { + res.Allowed = true + } else { + res.Allowed = false + res.Result = &meta.Status{ + Code: 403, + Message: fmt.Sprintf("admitomatic: %s", fmt.Sprintf(s, args...)), + } + } + return res, nil + } + + // Permit any actions on critical system namespaes. See: + // https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/ + // “Avoiding operating on the kube-system namespace” + if req.Namespace == "kube-system" { + return result("") + } + + switch req.Operation { + case "CREATE": + case "UPDATE": + default: + // We only care about creations/updates, everything else is referred to plain RBAC. + return result("") + } + + ingress := networking.Ingress{} + err := json.Unmarshal(req.Object.Raw, &ingress) + if err != nil { + glog.Errorf("Unmarshaling Ingress failed: %v", err) + return result("invalid object") + } + + // Check TLS config for hosts. + for j, t := range ingress.Spec.TLS { + for k, h := range t.Hosts { + if strings.Contains(h, "*") { + // TODO(q3k): support wildcards + return result("wildcard host %q (%d in TLS entry %d) is not permitted", h, k, j) + } + if !i.domainAllowed(req.Namespace, h) { + return result("host %q (%d) in TLS entry %d is not allowed in namespace %q", h, k, j, req.Namespace) + } + } + } + + // Check rules for hosts. + for j, r := range ingress.Spec.Rules { + h := r.Host + // Per IngressRule spec: + // If the host is unspecified, the Ingress routes all traffic based + // on the specified IngressRuleValue. Host can be "precise" which is + // a domain name without the terminating dot of a network host (e.g. + // "foo.bar.com") or "wildcard", which is a domain name prefixed with + // a single wildcard label (e.g. "*.foo.com"). + // + // We reject everything other than precise hosts. + if h == "" { + return result("empty host %q (in rule %d) is not permitted", h, j) + } + if strings.Contains(h, "*") { + // TODO(q3k): support wildcards + return result("wildcard host %q (in rule %d) is not permitted", h, j) + } + if !i.domainAllowed(req.Namespace, h) { + return result("host %q (in rule %d) is not allowed in namespace %q", h, j, req.Namespace) + } + } + + // Only allow a trusted subset of n-i-c annotations. + // TODO(q3k): allow opt-out for some namespaces + allowed := map[string]bool{ + "proxy-body-size": true, + "ssl-redirect": true, + "backend-protocol": true, + } + prefix := "nginx.ingress.kubernetes.io/" + for k, _ := range ingress.Annotations { + if !strings.HasPrefix(k, prefix) { + continue + } + k = strings.TrimPrefix(k, prefix) + if !allowed[k] { + return result("forbidden annotation %q", k) + } + } + + // All clear, accept this Ingress. + return result("") } diff --git a/cluster/admitomatic/ingress_test.go b/cluster/admitomatic/ingress_test.go index 91cf2b9e..15a60492 100644 --- a/cluster/admitomatic/ingress_test.go +++ b/cluster/admitomatic/ingress_test.go @@ -1,6 +1,15 @@ package main -import "testing" +import ( + "encoding/json" + "strings" + "testing" + + admission "k8s.io/api/admission/v1beta1" + networking "k8s.io/api/networking/v1beta1" + meta "k8s.io/apimachinery/pkg/apis/meta/v1" + runtime "k8s.io/apimachinery/pkg/runtime" +) func TestPatterns(t *testing.T) { f := ingressFilter{} @@ -76,3 +85,121 @@ func TestMatch(t *testing.T) { } } } + +func TestIngressPermitted(t *testing.T) { + f := ingressFilter{} + // Errors discarded, tested in TestPatterns. + f.allow("matrix", "matrix.hackerspace.pl") + f.allow("ceph-waw3", "*.hackerspace.pl") + f.allow("personal-q3k", "*.k0.q3k.org") + f.allow("personal-vuko", "shells.vuko.pl") + f.allow("minecraft", "*.k0.q3k.org") + + mkReq := func(ns string, annotations map[string]string, is *networking.IngressSpec) *admission.AdmissionRequest { + i := &networking.Ingress{ + Spec: *is, + } + i.Annotations = annotations + raw, err := json.Marshal(i) + if err != nil { + t.Fatalf("marshaling test ingress: %v", err) + } + return &admission.AdmissionRequest{ + UID: "test", + Kind: meta.GroupVersionKind{ + Group: "networking.k8s.io", + Version: "v1beta1", + Kind: "Ingress", + }, + Namespace: ns, + Operation: "CREATE", + Object: runtime.RawExtension{ + Raw: raw, + }, + } + } + + for i, el := range []struct { + req *admission.AdmissionRequest + err string + }{ + // 0: unrelated domain, should be allowed + {mkReq("default", nil, &networking.IngressSpec{ + Rules: []networking.IngressRule{ + {Host: "example.com"}, + }, + TLS: []networking.IngressTLS{ + { + Hosts: []string{"example.com"}, + }, + }, + }), ""}, + // 1: permitted restricted domain, should be allowed + {mkReq("matrix", nil, &networking.IngressSpec{ + Rules: []networking.IngressRule{ + {Host: "matrix.hackerspace.pl"}, + }, + TLS: []networking.IngressTLS{ + { + Hosts: []string{"matrix.hackerspace.pl"}, + }, + }, + }), ""}, + // 2: forbidden restricted domain, should be rejected + {mkReq("personal-hacker", nil, &networking.IngressSpec{ + Rules: []networking.IngressRule{ + {Host: "matrix.hackerspace.pl"}, + }, + TLS: []networking.IngressTLS{ + { + Hosts: []string{"matrix.hackerspace.pl"}, + }, + }, + }), "not allowed in namespace"}, + // 3: weird ingress but okay + {mkReq("personal-hacker", nil, &networking.IngressSpec{}), ""}, + // 4: janky annotations, should be rejected + {mkReq("matrix", map[string]string{ + "nginx.ingress.kubernetes.io/configuration-snippet": "omghax", + }, &networking.IngressSpec{ + Rules: []networking.IngressRule{ + {Host: "matrix.hackerspace.pl"}, + }, + TLS: []networking.IngressTLS{ + { + Hosts: []string{"matrix.hackerspace.pl"}, + }, + }, + }), "forbidden annotation"}, + // 5: accepted annotations, should be allowed + {mkReq("matrix", map[string]string{ + "nginx.ingress.kubernetes.io/proxy-body-size": "2137", + "foo.q3k.org/bar": "baz", + }, &networking.IngressSpec{ + Rules: []networking.IngressRule{ + {Host: "matrix.hackerspace.pl"}, + }, + TLS: []networking.IngressTLS{ + { + Hosts: []string{"matrix.hackerspace.pl"}, + }, + }, + }), ""}, + } { + res, err := f.admit(el.req) + if err != nil { + t.Errorf("test %d: admit: %v", i, err) + } + if el.err == "" { + if !res.Allowed { + t.Errorf("test %d: wanted allow, got %q", i, res.Result.Message) + } + } else { + if res.Allowed { + t.Errorf("test %d: wanted %q, got allowed", i, el.err) + } else if !strings.Contains(res.Result.Message, el.err) { + t.Errorf("test %d: wanted %q, got %q", i, el.err, res.Result.Message) + } + } + } +}