From e6e5cc92dacbd31b4cf0e098b483520c0e644b3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20Serv=C3=A9n=20Mar=C3=ADn?= Date: Fri, 1 Apr 2022 21:21:27 +0200 Subject: [PATCH 1/3] config: allow multiple configurations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit fixes issue #154 by enabling the user to repeat the `--config-file` flag multiple times to specify multiple configurations. Doing so, allows the user to declare that the krp should, e.g., enforce multiple resource attributes. Signed-off-by: Lucas Servén Marín --- README.md | 2 +- main.go | 37 +++-- pkg/proxy/proxy.go | 145 ++++++++++-------- pkg/proxy/proxy_test.go | 89 ++++++++--- test/e2e/main_test.go | 1 + .../multiple-configs/clusterRole-client.yaml | 8 + test/e2e/multiple-configs/clusterRole.yaml | 14 ++ .../clusterRoleBinding-client.yaml | 12 ++ .../multiple-configs/clusterRoleBinding.yaml | 13 ++ .../multiple-configs/configmap-resource.yaml | 26 ++++ test/e2e/multiple-configs/deployment.yaml | 44 ++++++ test/e2e/multiple-configs/service.yaml | 14 ++ test/e2e/multiple-configs/serviceAccount.yaml | 5 + test/e2e/multiple_configs.go | 142 +++++++++++++++++ 14 files changed, 449 insertions(+), 103 deletions(-) create mode 100644 test/e2e/multiple-configs/clusterRole-client.yaml create mode 100644 test/e2e/multiple-configs/clusterRole.yaml create mode 100644 test/e2e/multiple-configs/clusterRoleBinding-client.yaml create mode 100644 test/e2e/multiple-configs/clusterRoleBinding.yaml create mode 100644 test/e2e/multiple-configs/configmap-resource.yaml create mode 100644 test/e2e/multiple-configs/deployment.yaml create mode 100644 test/e2e/multiple-configs/service.yaml create mode 100644 test/e2e/multiple-configs/serviceAccount.yaml create mode 100644 test/e2e/multiple_configs.go diff --git a/README.md b/README.md index 0a7b81233..c94b8e281 100644 --- a/README.md +++ b/README.md @@ -37,7 +37,7 @@ Usage of _output/kube-rbac-proxy: --auth-header-user-field-name string The name of the field inside a http(2) request header to tell the upstream server about the user's name (default "x-remote-user") --auth-token-audiences strings Comma-separated list of token audiences to accept. By default a token does not have to have any specific audience. It is recommended to set a specific audience. --client-ca-file string If set, any request presenting a client certificate signed by one of the authorities in the client-ca-file is authenticated with an identity corresponding to the CommonName of the client certificate. - --config-file string Configuration file to configure kube-rbac-proxy. + --config-file strings Configuration file to configure kube-rbac-proxy. Can be specified multiple times to configure multiple resource authorizations. --ignore-paths strings Comma-separated list of paths against which kube-rbac-proxy will proxy without performing an authentication or authorization check. Cannot be used with --allow-paths. --insecure-listen-address string The address the kube-rbac-proxy HTTP server should listen on. --kubeconfig string Path to a kubeconfig file, specifying how to connect to the API server. If unset, in-cluster configuration will be used diff --git a/main.go b/main.go index b7435c3f3..41e0001da 100644 --- a/main.go +++ b/main.go @@ -86,10 +86,10 @@ func main() { OIDC: &authn.OIDCConfig{}, Token: &authn.TokenConfig{}, }, - Authorization: &authz.Config{}, + Authorizations: []*authz.Config{}, }, } - configFileName := "" + var configFileNames []string // Add klog flags klogFlags := flag.NewFlagSet(os.Args[0], flag.ExitOnError) @@ -104,7 +104,7 @@ func main() { flagset.StringVar(&cfg.upstream, "upstream", "", "The upstream URL to proxy to once requests have successfully been authenticated and authorized.") flagset.BoolVar(&cfg.upstreamForceH2C, "upstream-force-h2c", false, "Force h2c to communiate with the upstream. This is required when the upstream speaks h2c(http/2 cleartext - insecure variant of http/2) only. For example, go-grpc server in the insecure mode, such as helm's tiller w/o TLS, speaks h2c only") flagset.StringVar(&cfg.upstreamCAFile, "upstream-ca-file", "", "The CA the upstream uses for TLS connection. This is required when the upstream uses TLS and its own CA certificate") - flagset.StringVar(&configFileName, "config-file", "", "Configuration file to configure kube-rbac-proxy.") + flagset.StringSliceVar(&configFileNames, "config-file", []string{}, "Configuration file to configure kube-rbac-proxy. Can be specified multiple times to configure multiple resource authorizations.") flagset.StringSliceVar(&cfg.allowPaths, "allow-paths", nil, "Comma-separated list of paths against which kube-rbac-proxy matches the incoming request. If the request doesn't match, kube-rbac-proxy responds with a 404 status code. If omitted, the incoming request path isn't checked. Cannot be used with --ignore-paths.") flagset.StringSliceVar(&cfg.ignorePaths, "ignore-paths", nil, "Comma-separated list of paths against which kube-rbac-proxy will proxy without performing an authentication or authorization check. Cannot be used with --allow-paths.") @@ -146,21 +146,22 @@ func main() { klog.Fatalf("Failed to parse upstream URL: %v", err) } - if configFileName != "" { - klog.Infof("Reading config file: %s", configFileName) - b, err := ioutil.ReadFile(configFileName) - if err != nil { - klog.Fatalf("Failed to read resource-attribute file: %v", err) - } + if len(configFileNames) != 0 { + for _, cfn := range configFileNames { + klog.Infof("Reading config file: %s", cfn) + b, err := ioutil.ReadFile(cfn) + if err != nil { + klog.Fatalf("Failed to read resource-attribute file: %v", err) + } - configfile := configfile{} + configfile := configfile{} - err = yaml.Unmarshal(b, &configfile) - if err != nil { - klog.Fatalf("Failed to parse config file content: %v", err) - } + if err := yaml.Unmarshal(b, &configfile); err != nil { + klog.Fatalf("Failed to parse config file content: %v", err) + } - cfg.auth.Authorization = configfile.AuthorizationConfig + cfg.auth.Authorizations = append(cfg.auth.Authorizations, configfile.AuthorizationConfig) + } } kubeClient, err := kubernetes.NewForConfig(kcfg) @@ -202,7 +203,11 @@ func main() { klog.Fatalf("Failed to create sar authorizer: %v", err) } - staticAuthorizer, err := authz.NewStaticAuthorizer(cfg.auth.Authorization.Static) + var sacs []authz.StaticAuthorizationConfig + for _, ac := range cfg.auth.Authorizations { + sacs = append(sacs, ac.Static...) + } + staticAuthorizer, err := authz.NewStaticAuthorizer(sacs) if err != nil { klog.Fatalf("Failed to create static authorizer: %v", err) } diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index aaac103cc..29d654887 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -35,7 +35,7 @@ import ( // Config holds proxy authorization and authentication settings type Config struct { Authentication *authn.AuthnConfig - Authorization *authz.Config + Authorizations []*authz.Config } type kubeRBACProxy struct { @@ -50,7 +50,7 @@ type kubeRBACProxy struct { } func new(authenticator authenticator.Request, authorizer authorizer.Authorizer, config Config) *kubeRBACProxy { - return &kubeRBACProxy{authenticator, authorizer, newKubeRBACProxyAuthorizerAttributesGetter(config.Authorization), config} + return &kubeRBACProxy{authenticator, authorizer, newKubeRBACProxyAuthorizerAttributesGetter(config.Authorizations), config} } // New creates an authenticator, an authorizer, and a matching authorizer attributes getter compatible with the kube-rbac-proxy @@ -116,12 +116,12 @@ func (h *kubeRBACProxy) Handle(w http.ResponseWriter, req *http.Request) bool { return true } -func newKubeRBACProxyAuthorizerAttributesGetter(authzConfig *authz.Config) *krpAuthorizerAttributesGetter { - return &krpAuthorizerAttributesGetter{authzConfig} +func newKubeRBACProxyAuthorizerAttributesGetter(authzConfigs []*authz.Config) *krpAuthorizerAttributesGetter { + return &krpAuthorizerAttributesGetter{authzConfigs} } type krpAuthorizerAttributesGetter struct { - authzConfig *authz.Config + authzConfigs []*authz.Config } // GetRequestAttributes populates authorizer attributes for the requests to kube-rbac-proxy. @@ -148,67 +148,76 @@ func (n krpAuthorizerAttributesGetter) GetRequestAttributes(u user.Info, r *http } }() - if n.authzConfig.ResourceAttributes == nil { - // Default attributes mirror the API attributes that would allow this access to kube-rbac-proxy - allAttrs := append(allAttrs, authorizer.AttributesRecord{ - User: u, - Verb: apiVerb, - Namespace: "", - APIGroup: "", - APIVersion: "", - Resource: "", - Subresource: "", - Name: "", - ResourceRequest: false, - Path: r.URL.Path, - }) - return allAttrs + // Default attributes mirror the API attributes that would allow this access to kube-rbac-proxy + defaultAttributesRecord := authorizer.AttributesRecord{ + User: u, + Verb: apiVerb, + Namespace: "", + APIGroup: "", + APIVersion: "", + Resource: "", + Subresource: "", + Name: "", + ResourceRequest: false, + Path: r.URL.Path, } - if n.authzConfig.Rewrites == nil { - allAttrs := append(allAttrs, authorizer.AttributesRecord{ - User: u, - Verb: apiVerb, - Namespace: n.authzConfig.ResourceAttributes.Namespace, - APIGroup: n.authzConfig.ResourceAttributes.APIGroup, - APIVersion: n.authzConfig.ResourceAttributes.APIVersion, - Resource: n.authzConfig.ResourceAttributes.Resource, - Subresource: n.authzConfig.ResourceAttributes.Subresource, - Name: n.authzConfig.ResourceAttributes.Name, - ResourceRequest: true, - }) + if len(n.authzConfigs) == 0 { + allAttrs = append(allAttrs, defaultAttributesRecord) return allAttrs } - params := []string{} - if n.authzConfig.Rewrites.ByQueryParameter != nil && n.authzConfig.Rewrites.ByQueryParameter.Name != "" { - if ps, ok := r.URL.Query()[n.authzConfig.Rewrites.ByQueryParameter.Name]; ok { - params = append(params, ps...) + for _, ac := range n.authzConfigs { + if ac.ResourceAttributes == nil { + allAttrs = append(allAttrs, defaultAttributesRecord) + continue } - } - if n.authzConfig.Rewrites.ByHTTPHeader != nil && n.authzConfig.Rewrites.ByHTTPHeader.Name != "" { - if p := r.Header.Get(n.authzConfig.Rewrites.ByHTTPHeader.Name); p != "" { - params = append(params, p) + + if ac.Rewrites == nil { + allAttrs = append(allAttrs, authorizer.AttributesRecord{ + User: u, + Verb: apiVerb, + Namespace: ac.ResourceAttributes.Namespace, + APIGroup: ac.ResourceAttributes.APIGroup, + APIVersion: ac.ResourceAttributes.APIVersion, + Resource: ac.ResourceAttributes.Resource, + Subresource: ac.ResourceAttributes.Subresource, + Name: ac.ResourceAttributes.Name, + ResourceRequest: true, + }) + continue } - } - if len(params) == 0 { - return allAttrs - } + params := []string{} + if ac.Rewrites.ByQueryParameter != nil && ac.Rewrites.ByQueryParameter.Name != "" { + if ps, ok := r.URL.Query()[ac.Rewrites.ByQueryParameter.Name]; ok { + params = append(params, ps...) + } + } + if ac.Rewrites.ByHTTPHeader != nil && ac.Rewrites.ByHTTPHeader.Name != "" { + if p := r.Header.Get(ac.Rewrites.ByHTTPHeader.Name); p != "" { + params = append(params, p) + } + } - for _, param := range params { - attrs := authorizer.AttributesRecord{ - User: u, - Verb: apiVerb, - Namespace: templateWithValue(n.authzConfig.ResourceAttributes.Namespace, param), - APIGroup: templateWithValue(n.authzConfig.ResourceAttributes.APIGroup, param), - APIVersion: templateWithValue(n.authzConfig.ResourceAttributes.APIVersion, param), - Resource: templateWithValue(n.authzConfig.ResourceAttributes.Resource, param), - Subresource: templateWithValue(n.authzConfig.ResourceAttributes.Subresource, param), - Name: templateWithValue(n.authzConfig.ResourceAttributes.Name, param), - ResourceRequest: true, + if len(params) == 0 { + continue + } + + for _, param := range params { + attrs := authorizer.AttributesRecord{ + User: u, + Verb: apiVerb, + Namespace: templateWithValue(ac.ResourceAttributes.Namespace, param), + APIGroup: templateWithValue(ac.ResourceAttributes.APIGroup, param), + APIVersion: templateWithValue(ac.ResourceAttributes.APIVersion, param), + Resource: templateWithValue(ac.ResourceAttributes.Resource, param), + Subresource: templateWithValue(ac.ResourceAttributes.Subresource, param), + Name: templateWithValue(ac.ResourceAttributes.Name, param), + ResourceRequest: true, + } + allAttrs = append(allAttrs, attrs) } - allAttrs = append(allAttrs, attrs) } return allAttrs } @@ -238,17 +247,19 @@ func (c *Config) DeepCopy() *Config { } } - if c.Authorization != nil { - if c.Authorization.ResourceAttributes != nil { - res.Authorization = &authz.Config{ - ResourceAttributes: &authz.ResourceAttributes{ - Namespace: c.Authorization.ResourceAttributes.Namespace, - APIGroup: c.Authorization.ResourceAttributes.APIGroup, - APIVersion: c.Authorization.ResourceAttributes.APIVersion, - Resource: c.Authorization.ResourceAttributes.Resource, - Subresource: c.Authorization.ResourceAttributes.Subresource, - Name: c.Authorization.ResourceAttributes.Name, - }, + if len(c.Authorizations) != 0 { + for _, a := range c.Authorizations { + if a != nil && a.ResourceAttributes != nil { + res.Authorizations = append(res.Authorizations, &authz.Config{ + ResourceAttributes: &authz.ResourceAttributes{ + Namespace: a.ResourceAttributes.Namespace, + APIGroup: a.ResourceAttributes.APIGroup, + APIVersion: a.ResourceAttributes.APIVersion, + Resource: a.ResourceAttributes.Resource, + Subresource: a.ResourceAttributes.Subresource, + Name: a.ResourceAttributes.Name, + }, + }) } } } diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index d5afc3622..d1c7e130f 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -45,7 +45,7 @@ func TestProxyWithOIDCSupport(t *testing.T) { }, Token: &authn.TokenConfig{}, }, - Authorization: &authz.Config{}, + Authorizations: []*authz.Config{&authz.Config{}}, } fakeUser := user.DefaultInfo{Name: "Foo Bar", Groups: []string{"foo-bars"}} @@ -86,14 +86,33 @@ func TestProxyWithOIDCSupport(t *testing.T) { func TestGeneratingAuthorizerAttributes(t *testing.T) { cases := []struct { - desc string - authzCfg *authz.Config - req *http.Request - expected []authorizer.Attributes + desc string + authzCfgs []*authz.Config + req *http.Request + expected []authorizer.Attributes }{ + { + "without any configs", + []*authz.Config{}, + createRequest(nil, nil), + []authorizer.Attributes{ + authorizer.AttributesRecord{ + User: nil, + Verb: "get", + Namespace: "", + APIGroup: "", + APIVersion: "", + Resource: "", + Subresource: "", + Name: "", + ResourceRequest: false, + Path: "/accounts", + }, + }, + }, { "without resource attributes and rewrites", - &authz.Config{}, + []*authz.Config{{}}, createRequest(nil, nil), []authorizer.Attributes{ authorizer.AttributesRecord{ @@ -111,8 +130,8 @@ func TestGeneratingAuthorizerAttributes(t *testing.T) { }, }, { - "without rewrites config", - &authz.Config{ResourceAttributes: &authz.ResourceAttributes{Namespace: "tenant1", APIVersion: "v1", Resource: "namespace", Subresource: "metrics"}}, + "with resource attributes", + []*authz.Config{{ResourceAttributes: &authz.ResourceAttributes{Namespace: "tenant1", APIVersion: "v1", Resource: "namespace", Subresource: "metrics"}}}, createRequest(nil, nil), []authorizer.Attributes{ authorizer.AttributesRecord{ @@ -128,12 +147,44 @@ func TestGeneratingAuthorizerAttributes(t *testing.T) { }, }, }, + { + "with multiple resource attributes", + []*authz.Config{ + {ResourceAttributes: &authz.ResourceAttributes{Namespace: "tenant1", APIVersion: "v1", Resource: "namespace", Subresource: "metrics"}}, + {ResourceAttributes: &authz.ResourceAttributes{Namespace: "tenant1", APIVersion: "v1", Resource: "pod"}}, + }, + createRequest(nil, nil), + []authorizer.Attributes{ + authorizer.AttributesRecord{ + User: nil, + Verb: "get", + Namespace: "tenant1", + APIGroup: "", + APIVersion: "v1", + Resource: "namespace", + Subresource: "metrics", + Name: "", + ResourceRequest: true, + }, + authorizer.AttributesRecord{ + User: nil, + Verb: "get", + Namespace: "tenant1", + APIGroup: "", + APIVersion: "v1", + Resource: "pod", + Subresource: "", + Name: "", + ResourceRequest: true, + }, + }, + }, { "with query param rewrites config", - &authz.Config{ + []*authz.Config{{ Rewrites: &authz.SubjectAccessReviewRewrites{ByQueryParameter: &authz.QueryParameterRewriteConfig{Name: "namespace"}}, ResourceAttributes: &authz.ResourceAttributes{Namespace: "{{ .Value }}", APIVersion: "v1", Resource: "namespace", Subresource: "metrics"}, - }, + }}, createRequest(map[string]string{"namespace": "tenant1"}, nil), []authorizer.Attributes{ authorizer.AttributesRecord{ @@ -151,19 +202,19 @@ func TestGeneratingAuthorizerAttributes(t *testing.T) { }, { "with query param rewrites config but missing URL query", - &authz.Config{ + []*authz.Config{{ Rewrites: &authz.SubjectAccessReviewRewrites{ByQueryParameter: &authz.QueryParameterRewriteConfig{Name: "namespace"}}, ResourceAttributes: &authz.ResourceAttributes{Namespace: "{{ .Value }}", APIVersion: "v1", Resource: "namespace", Subresource: "metrics"}, - }, + }}, createRequest(nil, nil), nil, }, { "with http header rewrites config", - &authz.Config{ + []*authz.Config{{ Rewrites: &authz.SubjectAccessReviewRewrites{ByHTTPHeader: &authz.HTTPHeaderRewriteConfig{Name: "namespace"}}, ResourceAttributes: &authz.ResourceAttributes{Namespace: "{{ .Value }}", APIVersion: "v1", Resource: "namespace", Subresource: "metrics"}, - }, + }}, createRequest(nil, map[string]string{"namespace": "tenant1"}), []authorizer.Attributes{ authorizer.AttributesRecord{ @@ -181,22 +232,22 @@ func TestGeneratingAuthorizerAttributes(t *testing.T) { }, { "with http header rewrites config but missing header", - &authz.Config{ + []*authz.Config{{ Rewrites: &authz.SubjectAccessReviewRewrites{ByQueryParameter: &authz.QueryParameterRewriteConfig{Name: "namespace"}}, ResourceAttributes: &authz.ResourceAttributes{Namespace: "{{ .Value }}", APIVersion: "v1", Resource: "namespace", Subresource: "metrics"}, - }, + }}, createRequest(nil, nil), nil, }, { "with http header and query param rewrites config", - &authz.Config{ + []*authz.Config{{ Rewrites: &authz.SubjectAccessReviewRewrites{ ByHTTPHeader: &authz.HTTPHeaderRewriteConfig{Name: "namespace"}, ByQueryParameter: &authz.QueryParameterRewriteConfig{Name: "namespace"}, }, ResourceAttributes: &authz.ResourceAttributes{Namespace: "{{ .Value }}", APIVersion: "v1", Resource: "namespace", Subresource: "metrics"}, - }, + }}, createRequest(map[string]string{"namespace": "tenant1"}, map[string]string{"namespace": "tenant2"}), []authorizer.Attributes{ authorizer.AttributesRecord{ @@ -228,7 +279,7 @@ func TestGeneratingAuthorizerAttributes(t *testing.T) { for _, c := range cases { t.Run(c.desc, func(t *testing.T) { t.Log(c.req.URL.Query()) - n := krpAuthorizerAttributesGetter{authzConfig: c.authzCfg} + n := krpAuthorizerAttributesGetter{authzConfigs: c.authzCfgs} res := n.GetRequestAttributes(nil, c.req) if !cmp.Equal(res, c.expected) { t.Errorf("Generated authorizer attributes are not correct. Expected %v, recieved %v", c.expected, res) diff --git a/test/e2e/main_test.go b/test/e2e/main_test.go index 3b22e5c88..6e403a006 100644 --- a/test/e2e/main_test.go +++ b/test/e2e/main_test.go @@ -57,6 +57,7 @@ func Test(t *testing.T) { "IgnorePath": testIgnorePaths(suite), "TLS": testTLS(suite), "StaticAuthorizer": testStaticAuthorizer(suite), + "MultipleConfigs": testMultipleConfigs(suite), } for name, tc := range tests { diff --git a/test/e2e/multiple-configs/clusterRole-client.yaml b/test/e2e/multiple-configs/clusterRole-client.yaml new file mode 100644 index 000000000..e64b9bae2 --- /dev/null +++ b/test/e2e/multiple-configs/clusterRole-client.yaml @@ -0,0 +1,8 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: metrics +rules: +- apiGroups: [""] + resources: ["pods"] + verbs: ["get"] diff --git a/test/e2e/multiple-configs/clusterRole.yaml b/test/e2e/multiple-configs/clusterRole.yaml new file mode 100644 index 000000000..e9bc500b7 --- /dev/null +++ b/test/e2e/multiple-configs/clusterRole.yaml @@ -0,0 +1,14 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: kube-rbac-proxy + namespace: default +rules: + - apiGroups: ["authentication.k8s.io"] + resources: + - tokenreviews + verbs: ["create"] + - apiGroups: ["authorization.k8s.io"] + resources: + - subjectaccessreviews + verbs: ["create"] diff --git a/test/e2e/multiple-configs/clusterRoleBinding-client.yaml b/test/e2e/multiple-configs/clusterRoleBinding-client.yaml new file mode 100644 index 000000000..d34563cfd --- /dev/null +++ b/test/e2e/multiple-configs/clusterRoleBinding-client.yaml @@ -0,0 +1,12 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: metrics +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: metrics +subjects: +- kind: ServiceAccount + name: default + namespace: default diff --git a/test/e2e/multiple-configs/clusterRoleBinding.yaml b/test/e2e/multiple-configs/clusterRoleBinding.yaml new file mode 100644 index 000000000..f7be8fa4e --- /dev/null +++ b/test/e2e/multiple-configs/clusterRoleBinding.yaml @@ -0,0 +1,13 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: kube-rbac-proxy + namespace: default +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: kube-rbac-proxy +subjects: + - kind: ServiceAccount + name: kube-rbac-proxy + namespace: default diff --git a/test/e2e/multiple-configs/configmap-resource.yaml b/test/e2e/multiple-configs/configmap-resource.yaml new file mode 100644 index 000000000..7fa880a71 --- /dev/null +++ b/test/e2e/multiple-configs/configmap-resource.yaml @@ -0,0 +1,26 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: kube-rbac-proxy +data: + config-file1.yaml: |+ + authorization: + rewrites: + byQueryParameter: + name: "namespace" + resourceAttributes: + resource: namespaces + subresource: metrics + namespace: "{{ .Value }}" + static: + - user: + name: system:serviceaccount:default:default + resourceRequest: true + resource: namespaces + subresource: metrics + namespace: default + verbs: get + config-file2.yaml: |+ + authorization: + resourceAttributes: + resource: pods diff --git a/test/e2e/multiple-configs/deployment.yaml b/test/e2e/multiple-configs/deployment.yaml new file mode 100644 index 000000000..2aed7e36c --- /dev/null +++ b/test/e2e/multiple-configs/deployment.yaml @@ -0,0 +1,44 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: kube-rbac-proxy + namespace: default +spec: + replicas: 1 + selector: + matchLabels: + app: kube-rbac-proxy + template: + metadata: + labels: + app: kube-rbac-proxy + spec: + securityContext: + runAsUser: 65532 + serviceAccountName: kube-rbac-proxy + containers: + - name: kube-rbac-proxy + image: quay.io/brancz/kube-rbac-proxy:local + args: + - "--secure-listen-address=0.0.0.0:8443" + - "--upstream=http://127.0.0.1:8081/" + - "--config-file=/etc/kube-rbac-proxy/config-file1.yaml" + - "--config-file=/etc/kube-rbac-proxy/config-file2.yaml" + - "--logtostderr=true" + - "--v=10" + ports: + - containerPort: 8443 + name: https + volumeMounts: + - name: config + mountPath: /etc/kube-rbac-proxy + securityContext: + allowPrivilegeEscalation: false + - name: prometheus-example-app + image: quay.io/brancz/prometheus-example-app:v0.1.0 + args: + - "--bind=127.0.0.1:8081" + volumes: + - name: config + configMap: + name: kube-rbac-proxy diff --git a/test/e2e/multiple-configs/service.yaml b/test/e2e/multiple-configs/service.yaml new file mode 100644 index 000000000..b1ae11686 --- /dev/null +++ b/test/e2e/multiple-configs/service.yaml @@ -0,0 +1,14 @@ +apiVersion: v1 +kind: Service +metadata: + labels: + app: kube-rbac-proxy + name: kube-rbac-proxy + namespace: default +spec: + ports: + - name: https + port: 8443 + targetPort: https + selector: + app: kube-rbac-proxy diff --git a/test/e2e/multiple-configs/serviceAccount.yaml b/test/e2e/multiple-configs/serviceAccount.yaml new file mode 100644 index 000000000..45feecc9c --- /dev/null +++ b/test/e2e/multiple-configs/serviceAccount.yaml @@ -0,0 +1,5 @@ +apiVersion: v1 +kind: ServiceAccount +metadata: + name: kube-rbac-proxy + namespace: default diff --git a/test/e2e/multiple_configs.go b/test/e2e/multiple_configs.go new file mode 100644 index 000000000..9d14d8cbf --- /dev/null +++ b/test/e2e/multiple_configs.go @@ -0,0 +1,142 @@ +/* +Copyright 2017 Frederic Branczyk All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package e2e + +import ( + "fmt" + "testing" + + "github.com/brancz/kube-rbac-proxy/test/kubetest" +) + +func testMultipleConfigs(s *kubetest.Suite) kubetest.TestSuite { + return func(t *testing.T) { + command := `curl --connect-timeout 5 -v -s -k --fail -H "Authorization: Bearer $(cat /var/run/secrets/kubernetes.io/serviceaccount/token)" https://kube-rbac-proxy.default.svc.cluster.local:8443%v` + + for _, tc := range []struct { + name string + given []kubetest.Setup + check []kubetest.Check + }{ + { + name: "RBAC + correct namespace", + given: []kubetest.Setup{ + kubetest.CreatedManifests( + s.KubeClient, + "multiple-configs/configmap-resource.yaml", + "multiple-configs/clusterRole.yaml", + "multiple-configs/clusterRoleBinding.yaml", + "multiple-configs/clusterRole-client.yaml", + "multiple-configs/clusterRoleBinding-client.yaml", + "multiple-configs/deployment.yaml", + "multiple-configs/service.yaml", + "multiple-configs/serviceAccount.yaml", + ), + }, + check: []kubetest.Check{ + ClientSucceeds( + s.KubeClient, + fmt.Sprintf(command, "/metrics?namespace=default"), + nil, + ), + }, + }, + { + name: "no RBAC + correct namespace", + given: []kubetest.Setup{ + kubetest.CreatedManifests( + s.KubeClient, + "multiple-configs/configmap-resource.yaml", + "multiple-configs/clusterRole.yaml", + "multiple-configs/clusterRoleBinding.yaml", + "multiple-configs/deployment.yaml", + "multiple-configs/service.yaml", + "multiple-configs/serviceAccount.yaml", + ), + }, + check: []kubetest.Check{ + ClientFails( + s.KubeClient, + fmt.Sprintf(command, "/metrics?namespace=default"), + nil, + ), + }, + }, + { + name: "RBAC + wrong namespace", + given: []kubetest.Setup{ + kubetest.CreatedManifests( + s.KubeClient, + "multiple-configs/configmap-resource.yaml", + "multiple-configs/clusterRole.yaml", + "multiple-configs/clusterRoleBinding.yaml", + "multiple-configs/clusterRole-client.yaml", + "multiple-configs/clusterRoleBinding-client.yaml", + "multiple-configs/deployment.yaml", + "multiple-configs/service.yaml", + "multiple-configs/serviceAccount.yaml", + ), + }, + check: []kubetest.Check{ + ClientFails( + s.KubeClient, + fmt.Sprintf(command, "/metrics?namespace=forbidden"), + nil, + ), + }, + }, + { + name: "no RBAC + wrong namespace", + given: []kubetest.Setup{ + kubetest.CreatedManifests( + s.KubeClient, + "multiple-configs/configmap-resource.yaml", + "multiple-configs/clusterRole.yaml", + "multiple-configs/clusterRoleBinding.yaml", + "multiple-configs/deployment.yaml", + "multiple-configs/service.yaml", + "multiple-configs/serviceAccount.yaml", + ), + }, + check: []kubetest.Check{ + ClientFails( + s.KubeClient, + fmt.Sprintf(command, "/metrics?namespace=forbidden"), + nil, + ), + }, + }, + } { + kubetest.Scenario{ + Name: tc.name, + Given: kubetest.Setups(tc.given...), + When: kubetest.Conditions( + kubetest.PodsAreReady( + s.KubeClient, + 1, + "app=kube-rbac-proxy", + ), + kubetest.ServiceIsReady( + s.KubeClient, + "kube-rbac-proxy", + ), + ), + Then: kubetest.Checks(tc.check...), + }.Run(t) + } + } +} From f57d21634f7c9c7c6c6d8e56a1521a1de2278e6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20Serv=C3=A9n=20Mar=C3=ADn?= Date: Fri, 1 Apr 2022 23:25:15 +0200 Subject: [PATCH 2/3] pkg/proxy: make authorization concurrent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that KRP can take multiple config files, it is possible that it will need to make multiple outbout SubjectAccessReview requests to the Kubernetes API. In order to keep authorization fast, this commit modifies the proxy so that all authorizations are concurrent. As soon as one authorization fails, the context is cancelled and all other in-flight calls are stopped. Signed-off-by: Lucas Servén Marín --- pkg/proxy/proxy.go | 45 +++++++++++++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 29d654887..99c5e635e 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -18,9 +18,11 @@ package proxy import ( "bytes" + "context" "fmt" "net/http" "strings" + "sync" "text/template" "github.com/brancz/kube-rbac-proxy/pkg/authn" @@ -88,21 +90,36 @@ func (h *kubeRBACProxy) Handle(w http.ResponseWriter, req *http.Request) bool { return false } + var wg sync.WaitGroup + ctx, cancel := context.WithCancel(ctx) + defer cancel() for _, attrs := range allAttrs { - // Authorize - authorized, reason, err := h.Authorize(ctx, attrs) - if err != nil { - msg := fmt.Sprintf("Authorization error (user=%s, verb=%s, resource=%s, subresource=%s)", u.User.GetName(), attrs.GetVerb(), attrs.GetResource(), attrs.GetSubresource()) - klog.Errorf("%s: %s", msg, err) - http.Error(w, msg, http.StatusInternalServerError) - return false - } - if authorized != authorizer.DecisionAllow { - msg := fmt.Sprintf("Forbidden (user=%s, verb=%s, resource=%s, subresource=%s)", u.User.GetName(), attrs.GetVerb(), attrs.GetResource(), attrs.GetSubresource()) - klog.V(2).Infof("%s. Reason: %q.", msg, reason) - http.Error(w, msg, http.StatusForbidden) - return false - } + // Authorize concurrently, as there can be many outbound requests to the kube API. + wg.Add(1) + go func(attrs authorizer.Attributes) { + defer wg.Done() + authorized, reason, err := h.Authorize(ctx, attrs) + if err != nil { + msg := fmt.Sprintf("Authorization error (user=%s, verb=%s, resource=%s, subresource=%s)", u.User.GetName(), attrs.GetVerb(), attrs.GetResource(), attrs.GetSubresource()) + klog.Errorf("%s: %s", msg, err) + http.Error(w, msg, http.StatusInternalServerError) + cancel() + return + } + if authorized != authorizer.DecisionAllow { + msg := fmt.Sprintf("Forbidden (user=%s, verb=%s, resource=%s, subresource=%s)", u.User.GetName(), attrs.GetVerb(), attrs.GetResource(), attrs.GetSubresource()) + klog.V(2).Infof("%s. Reason: %q.", msg, reason) + http.Error(w, msg, http.StatusForbidden) + cancel() + } + }(attrs) + } + wg.Wait() + select { + case <-ctx.Done(): + // If the context was cancelled by any goroutine, then there was an authz failure. + return false + default: } if h.Config.Authentication.Header.Enabled { From 98af9c6e5158f7dc2bfc90d29cf5887a1e0331af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20Serv=C3=A9n=20Mar=C3=ADn?= Date: Wed, 13 Apr 2022 16:26:13 +0200 Subject: [PATCH 3/3] pkg/proxy: ensure we only send HTTP response once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Lucas Servén Marín --- pkg/proxy/proxy.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 99c5e635e..9f6923fac 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -23,6 +23,7 @@ import ( "net/http" "strings" "sync" + "sync/atomic" "text/template" "github.com/brancz/kube-rbac-proxy/pkg/authn" @@ -91,6 +92,7 @@ func (h *kubeRBACProxy) Handle(w http.ResponseWriter, req *http.Request) bool { } var wg sync.WaitGroup + var authzDone uint32 ctx, cancel := context.WithCancel(ctx) defer cancel() for _, attrs := range allAttrs { @@ -102,24 +104,25 @@ func (h *kubeRBACProxy) Handle(w http.ResponseWriter, req *http.Request) bool { if err != nil { msg := fmt.Sprintf("Authorization error (user=%s, verb=%s, resource=%s, subresource=%s)", u.User.GetName(), attrs.GetVerb(), attrs.GetResource(), attrs.GetSubresource()) klog.Errorf("%s: %s", msg, err) - http.Error(w, msg, http.StatusInternalServerError) + if atomic.CompareAndSwapUint32(&authzDone, 0, 1) { + http.Error(w, msg, http.StatusInternalServerError) + } cancel() return } if authorized != authorizer.DecisionAllow { msg := fmt.Sprintf("Forbidden (user=%s, verb=%s, resource=%s, subresource=%s)", u.User.GetName(), attrs.GetVerb(), attrs.GetResource(), attrs.GetSubresource()) klog.V(2).Infof("%s. Reason: %q.", msg, reason) - http.Error(w, msg, http.StatusForbidden) + if atomic.CompareAndSwapUint32(&authzDone, 0, 1) { + http.Error(w, msg, http.StatusForbidden) + } cancel() } }(attrs) } wg.Wait() - select { - case <-ctx.Done(): - // If the context was cancelled by any goroutine, then there was an authz failure. + if authzDone == 1 { return false - default: } if h.Config.Authentication.Header.Enabled {