From 79f5a0efa9545f3f504be9a9b6af741e6eb9efdd Mon Sep 17 00:00:00 2001 From: Jacob Aronoff Date: Thu, 4 Jan 2024 12:11:34 -0500 Subject: [PATCH] Add new operator capability to check if it has access to do an operation (#2467) * rbac pr testing * makefile convenience * Add test * add chlog * Add a comment * update to allow for policy rule checking * change package * lint fail * better formatting * don't use leading slash for empty group * add more detail for comment --- .chloggen/mandate-rbac.yaml | 16 ++ Makefile | 5 + apis/v1alpha1/collector_webhook.go | 86 ++++++- apis/v1alpha1/collector_webhook_test.go | 160 +++++++++++- controllers/suite_test.go | 9 +- go.mod | 1 + internal/rbac/access.go | 132 ++++++++++ internal/rbac/access_test.go | 237 ++++++++++++++++++ .../podmutation/webhookhandler_suite_test.go | 10 +- main.go | 11 +- pkg/collector/upgrade/suite_test.go | 11 +- .../targetallocator-features/00-install.yaml | 3 - 12 files changed, 655 insertions(+), 26 deletions(-) create mode 100755 .chloggen/mandate-rbac.yaml create mode 100644 internal/rbac/access.go create mode 100644 internal/rbac/access_test.go diff --git a/.chloggen/mandate-rbac.yaml b/.chloggen/mandate-rbac.yaml new file mode 100755 index 0000000000..ad1f6b33cd --- /dev/null +++ b/.chloggen/mandate-rbac.yaml @@ -0,0 +1,16 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action) +component: operator + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: enables the operator to create subject access reviews for different required permissions. + +# One or more tracking issues related to the change +issues: [2426] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: diff --git a/Makefile b/Makefile index d14311a893..0f3320de11 100644 --- a/Makefile +++ b/Makefile @@ -97,6 +97,11 @@ all: manager .PHONY: ci ci: test +# helper to get your gomod up-to-date +.PHONY: gomod +gomod: + go mod tidy && go mod vendor && (cd cmd/otel-allocator && go mod tidy) && (cd cmd/operator-opamp-bridge && go mod tidy) + # Run tests # setup-envtest uses KUBEBUILDER_ASSETS which points to a directory with binaries (api-server, etcd and kubectl) .PHONY: test diff --git a/apis/v1alpha1/collector_webhook.go b/apis/v1alpha1/collector_webhook.go index 0f83047a04..48aeb34f6e 100644 --- a/apis/v1alpha1/collector_webhook.go +++ b/apis/v1alpha1/collector_webhook.go @@ -17,9 +17,12 @@ package v1alpha1 import ( "context" "fmt" + "strings" "github.com/go-logr/logr" + v1 "k8s.io/api/authorization/v1" autoscalingv2 "k8s.io/api/autoscaling/v2" + rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/validation" @@ -28,12 +31,41 @@ import ( "github.com/open-telemetry/opentelemetry-operator/internal/config" ta "github.com/open-telemetry/opentelemetry-operator/internal/manifests/targetallocator/adapters" + "github.com/open-telemetry/opentelemetry-operator/internal/rbac" "github.com/open-telemetry/opentelemetry-operator/pkg/featuregate" ) var ( _ admission.CustomValidator = &CollectorWebhook{} _ admission.CustomDefaulter = &CollectorWebhook{} + + // targetAllocatorCRPolicyRules are the policy rules required for the CR functionality. + targetAllocatorCRPolicyRules = []*rbacv1.PolicyRule{ + { + APIGroups: []string{"monitoring.coreos.com"}, + Resources: []string{"servicemonitors", "podmonitors"}, + Verbs: []string{"*"}, + }, { + APIGroups: []string{""}, + Resources: []string{"nodes", "nodes/metrics", "services", "endpoints", "pods"}, + Verbs: []string{"get", "list", "watch"}, + }, { + APIGroups: []string{""}, + Resources: []string{"configmaps"}, + Verbs: []string{"get"}, + }, { + APIGroups: []string{"discovery.k8s.io"}, + Resources: []string{"endpointslices"}, + Verbs: []string{"get", "list", "watch"}, + }, { + APIGroups: []string{"networking.k8s.io"}, + Resources: []string{"ingresses"}, + Verbs: []string{"get", "list", "watch"}, + }, { + NonResourceURLs: []string{"/metrics"}, + Verbs: []string{"get"}, + }, + } ) // +kubebuilder:webhook:path=/mutate-opentelemetry-io-v1alpha1-opentelemetrycollector,mutating=true,failurePolicy=fail,groups=opentelemetry.io,resources=opentelemetrycollectors,verbs=create;update,versions=v1alpha1,name=mopentelemetrycollector.kb.io,sideEffects=none,admissionReviewVersions=v1 @@ -42,9 +74,10 @@ var ( // +kubebuilder:object:generate=false type CollectorWebhook struct { - logger logr.Logger - cfg config.Config - scheme *runtime.Scheme + logger logr.Logger + cfg config.Config + scheme *runtime.Scheme + reviewer *rbac.Reviewer } func (c CollectorWebhook) Default(ctx context.Context, obj runtime.Object) error { @@ -60,7 +93,7 @@ func (c CollectorWebhook) ValidateCreate(ctx context.Context, obj runtime.Object if !ok { return nil, fmt.Errorf("expected an OpenTelemetryCollector, received %T", obj) } - return c.validate(otelcol) + return c.validate(ctx, otelcol) } func (c CollectorWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { @@ -68,7 +101,7 @@ func (c CollectorWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj run if !ok { return nil, fmt.Errorf("expected an OpenTelemetryCollector, received %T", newObj) } - return c.validate(otelcol) + return c.validate(ctx, otelcol) } func (c CollectorWebhook) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { @@ -76,7 +109,7 @@ func (c CollectorWebhook) ValidateDelete(ctx context.Context, obj runtime.Object if !ok || otelcol == nil { return nil, fmt.Errorf("expected an OpenTelemetryCollector, received %T", obj) } - return c.validate(otelcol) + return c.validate(ctx, otelcol) } func (c CollectorWebhook) defaulter(r *OpenTelemetryCollector) error { @@ -169,7 +202,7 @@ func (c CollectorWebhook) defaulter(r *OpenTelemetryCollector) error { return nil } -func (c CollectorWebhook) validate(r *OpenTelemetryCollector) (admission.Warnings, error) { +func (c CollectorWebhook) validate(ctx context.Context, r *OpenTelemetryCollector) (admission.Warnings, error) { warnings := admission.Warnings{} // validate volumeClaimTemplates if r.Spec.Mode != ModeStatefulSet && len(r.Spec.VolumeClaimTemplates) > 0 { @@ -214,6 +247,14 @@ func (c CollectorWebhook) validate(r *OpenTelemetryCollector) (admission.Warning if err != nil { return warnings, fmt.Errorf("the OpenTelemetry Spec Prometheus configuration is incorrect, %w", err) } + // if the prometheusCR is enabled, it needs a suite of permissions to function + if r.Spec.TargetAllocator.PrometheusCR.Enabled { + if subjectAccessReviews, err := c.reviewer.CheckPolicyRules(ctx, r.GetNamespace(), r.Spec.TargetAllocator.ServiceAccount, targetAllocatorCRPolicyRules...); err != nil { + return warnings, fmt.Errorf("unable to check rbac rules %w", err) + } else if allowed, deniedReviews := rbac.AllSubjectAccessReviewsAllowed(subjectAccessReviews); !allowed { + warnings = append(warnings, warningsGroupedByResource(deniedReviews)...) + } + } } // validator port config @@ -360,11 +401,34 @@ func checkAutoscalerSpec(autoscaler *AutoscalerSpec) error { return nil } -func SetupCollectorWebhook(mgr ctrl.Manager, cfg config.Config) error { +// warningsGroupedByResource is a helper to take the missing permissions and format them as warnings. +func warningsGroupedByResource(reviews []*v1.SubjectAccessReview) []string { + fullResourceToVerbs := make(map[string][]string) + for _, review := range reviews { + if review.Spec.ResourceAttributes != nil { + key := fmt.Sprintf("%s/%s", review.Spec.ResourceAttributes.Group, review.Spec.ResourceAttributes.Resource) + if len(review.Spec.ResourceAttributes.Group) == 0 { + key = review.Spec.ResourceAttributes.Resource + } + fullResourceToVerbs[key] = append(fullResourceToVerbs[key], review.Spec.ResourceAttributes.Verb) + } else if review.Spec.NonResourceAttributes != nil { + key := fmt.Sprintf("nonResourceURL: %s", review.Spec.NonResourceAttributes.Path) + fullResourceToVerbs[key] = append(fullResourceToVerbs[key], review.Spec.NonResourceAttributes.Verb) + } + } + var warnings []string + for fullResource, verbs := range fullResourceToVerbs { + warnings = append(warnings, fmt.Sprintf("missing the following rules for %s: [%s]", fullResource, strings.Join(verbs, ","))) + } + return warnings +} + +func SetupCollectorWebhook(mgr ctrl.Manager, cfg config.Config, reviewer *rbac.Reviewer) error { cvw := &CollectorWebhook{ - logger: mgr.GetLogger().WithValues("handler", "CollectorWebhook"), - scheme: mgr.GetScheme(), - cfg: cfg, + reviewer: reviewer, + logger: mgr.GetLogger().WithValues("handler", "CollectorWebhook"), + scheme: mgr.GetScheme(), + cfg: cfg, } return ctrl.NewWebhookManagedBy(mgr). For(&OpenTelemetryCollector{}). diff --git a/apis/v1alpha1/collector_webhook_test.go b/apis/v1alpha1/collector_webhook_test.go index e947a8c6e5..ea67ac5886 100644 --- a/apis/v1alpha1/collector_webhook_test.go +++ b/apis/v1alpha1/collector_webhook_test.go @@ -23,15 +23,19 @@ import ( "github.com/go-logr/logr" "github.com/stretchr/testify/assert" appsv1 "k8s.io/api/apps/v1" + authv1 "k8s.io/api/authorization/v1" autoscalingv2 "k8s.io/api/autoscaling/v2" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/kubernetes/scheme" + kubeTesting "k8s.io/client-go/testing" "github.com/open-telemetry/opentelemetry-operator/internal/config" + "github.com/open-telemetry/opentelemetry-operator/internal/rbac" ) var ( @@ -438,6 +442,7 @@ func TestOTELColValidatingWebhook(t *testing.T) { otelcol OpenTelemetryCollector expectedErr string expectedWarnings []string + shouldFailSar bool }{ { name: "valid empty spec", @@ -469,6 +474,131 @@ func TestOTELColValidatingWebhook(t *testing.T) { protocols: thrift_http: endpoint: 0.0.0.0:15268 +`, + Ports: []v1.ServicePort{ + { + Name: "port1", + Port: 5555, + }, + { + Name: "port2", + Port: 5554, + Protocol: v1.ProtocolUDP, + }, + }, + Autoscaler: &AutoscalerSpec{ + Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ + ScaleDown: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &three, + }, + ScaleUp: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &five, + }, + }, + TargetCPUUtilization: &five, + }, + }, + }, + expectedWarnings: []string{ + "MaxReplicas is deprecated", + "MinReplicas is deprecated", + }, + }, + { + name: "prom CR admissions warning", + shouldFailSar: true, // force failure + otelcol: OpenTelemetryCollector{ + Spec: OpenTelemetryCollectorSpec{ + Mode: ModeStatefulSet, + MinReplicas: &one, + Replicas: &three, + MaxReplicas: &five, + UpgradeStrategy: "adhoc", + TargetAllocator: OpenTelemetryTargetAllocator{ + Enabled: true, + PrometheusCR: OpenTelemetryTargetAllocatorPrometheusCR{Enabled: true}, + }, + Config: `receivers: + examplereceiver: + endpoint: "0.0.0.0:12345" + examplereceiver/settings: + endpoint: "0.0.0.0:12346" + prometheus: + config: + scrape_configs: + - job_name: otel-collector + scrape_interval: 10s + jaeger/custom: + protocols: + thrift_http: + endpoint: 0.0.0.0:15268 +`, + Ports: []v1.ServicePort{ + { + Name: "port1", + Port: 5555, + }, + { + Name: "port2", + Port: 5554, + Protocol: v1.ProtocolUDP, + }, + }, + Autoscaler: &AutoscalerSpec{ + Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ + ScaleDown: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &three, + }, + ScaleUp: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &five, + }, + }, + TargetCPUUtilization: &five, + }, + }, + }, + expectedWarnings: []string{ + "missing the following rules for monitoring.coreos.com/servicemonitors: [*]", + "missing the following rules for monitoring.coreos.com/podmonitors: [*]", + "missing the following rules for nodes/metrics: [get,list,watch]", + "missing the following rules for services: [get,list,watch]", + "missing the following rules for endpoints: [get,list,watch]", + "missing the following rules for networking.k8s.io/ingresses: [get,list,watch]", + "missing the following rules for nodes: [get,list,watch]", + "missing the following rules for pods: [get,list,watch]", + "missing the following rules for configmaps: [get]", + "missing the following rules for discovery.k8s.io/endpointslices: [get,list,watch]", + "missing the following rules for nonResourceURL: /metrics: [get]", + "MaxReplicas is deprecated", + "MinReplicas is deprecated", + }, + }, + { + name: "prom CR no admissions warning", + shouldFailSar: false, // force SAR okay + otelcol: OpenTelemetryCollector{ + Spec: OpenTelemetryCollectorSpec{ + Mode: ModeStatefulSet, + Replicas: &three, + UpgradeStrategy: "adhoc", + TargetAllocator: OpenTelemetryTargetAllocator{ + Enabled: true, + PrometheusCR: OpenTelemetryTargetAllocatorPrometheusCR{Enabled: true}, + }, + Config: `receivers: + examplereceiver: + endpoint: "0.0.0.0:12345" + examplereceiver/settings: + endpoint: "0.0.0.0:12346" + prometheus: + config: + scrape_configs: + - job_name: otel-collector + scrape_interval: 10s + jaeger/custom: + protocols: + thrift_http: + endpoint: 0.0.0.0:15268 `, Ports: []v1.ServicePort{ { @@ -923,19 +1053,37 @@ func TestOTELColValidatingWebhook(t *testing.T) { config.WithCollectorImage("collector:v0.0.0"), config.WithTargetAllocatorImage("ta:v0.0.0"), ), + reviewer: getReviewer(test.shouldFailSar), } ctx := context.Background() warnings, err := cvw.ValidateCreate(ctx, &test.otelcol) if test.expectedErr == "" { assert.NoError(t, err) - return - } - if len(test.expectedWarnings) == 0 { - assert.Empty(t, warnings, test.expectedWarnings) } else { - assert.ElementsMatch(t, warnings, test.expectedWarnings) + assert.ErrorContains(t, err, test.expectedErr) } - assert.ErrorContains(t, err, test.expectedErr) + assert.Equal(t, len(test.expectedWarnings), len(warnings)) + assert.ElementsMatch(t, warnings, test.expectedWarnings) }) } } + +func getReviewer(shouldFailSAR bool) *rbac.Reviewer { + c := fake.NewSimpleClientset() + c.PrependReactor("create", "subjectaccessreviews", func(action kubeTesting.Action) (handled bool, ret runtime.Object, err error) { + // check our expectation here + if !action.Matches("create", "subjectaccessreviews") { + return false, nil, fmt.Errorf("must be a create for a SAR") + } + sar, ok := action.(kubeTesting.CreateAction).GetObject().DeepCopyObject().(*authv1.SubjectAccessReview) + if !ok || sar == nil { + return false, nil, fmt.Errorf("bad object") + } + sar.Status = authv1.SubjectAccessReviewStatus{ + Allowed: !shouldFailSAR, + Denied: shouldFailSAR, + } + return true, sar, nil + }) + return rbac.NewReviewer(c) +} diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 51fe86d5a1..86e5cc02e5 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -35,6 +35,7 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/uuid" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" "k8s.io/client-go/tools/record" @@ -52,6 +53,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/internal/config" "github.com/open-telemetry/opentelemetry-operator/internal/manifests" "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/testdata" + "github.com/open-telemetry/opentelemetry-operator/internal/rbac" // +kubebuilder:scaffold:imports ) @@ -147,8 +149,13 @@ func TestMain(m *testing.M) { fmt.Printf("failed to start webhook server: %v", mgrErr) os.Exit(1) } + clientset, clientErr := kubernetes.NewForConfig(cfg) + if err != nil { + fmt.Printf("failed to setup kubernetes clientset %v", clientErr) + } + reviewer := rbac.NewReviewer(clientset) - if err = v1alpha1.SetupCollectorWebhook(mgr, config.New()); err != nil { + if err = v1alpha1.SetupCollectorWebhook(mgr, config.New(), reviewer); err != nil { fmt.Printf("failed to SetupWebhookWithManager: %v", err) os.Exit(1) } diff --git a/go.mod b/go.mod index aee779ec86..67900b8ffa 100644 --- a/go.mod +++ b/go.mod @@ -53,6 +53,7 @@ require ( github.com/emicklei/go-restful/v3 v3.11.0 // indirect github.com/envoyproxy/go-control-plane v0.11.1 // indirect github.com/envoyproxy/protoc-gen-validate v1.0.2 // indirect + github.com/evanphx/json-patch v5.6.0+incompatible // indirect github.com/evanphx/json-patch/v5 v5.6.0 // indirect github.com/fatih/color v1.15.0 // indirect github.com/fsnotify/fsnotify v1.7.0 // indirect diff --git a/internal/rbac/access.go b/internal/rbac/access.go new file mode 100644 index 0000000000..5bdc9b27cf --- /dev/null +++ b/internal/rbac/access.go @@ -0,0 +1,132 @@ +// Copyright The OpenTelemetry Authors +// +// 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 rbac + +import ( + "context" + "errors" + "fmt" + + v1 "k8s.io/api/authorization/v1" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" +) + +const ( + serviceAccountFmtStr = "system:serviceaccount:%s:%s" +) + +type Reviewer struct { + client kubernetes.Interface +} + +func NewReviewer(c kubernetes.Interface) *Reviewer { + return &Reviewer{ + client: c, + } +} + +// AllSubjectAccessReviewsAllowed checks if all of subjectAccessReviews are explicitly allowed. If false, the method +// returns the reviews that were denied. +func AllSubjectAccessReviewsAllowed(subjectAccessReviews []*v1.SubjectAccessReview) (bool, []*v1.SubjectAccessReview) { + allowed := true + var deniedReviews []*v1.SubjectAccessReview + for _, review := range subjectAccessReviews { + if review.Status.Denied { + allowed = false + deniedReviews = append(deniedReviews, review) + } else if !review.Status.Allowed { + allowed = false + deniedReviews = append(deniedReviews, review) + } + } + return allowed, deniedReviews +} + +// CheckPolicyRules is a convenience function that lets the caller check access for a set of PolicyRules. +func (r *Reviewer) CheckPolicyRules(ctx context.Context, serviceAccount, serviceAccountNamespace string, rules ...*rbacv1.PolicyRule) ([]*v1.SubjectAccessReview, error) { + var subjectAccessReviews []*v1.SubjectAccessReview + var errs []error + for _, rule := range rules { + if rule == nil { + continue + } + resourceAttributes := policyRuleToResourceAttributes(rule) + nonResourceAttributes := policyRuleToNonResourceAttributes(rule) + for _, res := range resourceAttributes { + sar, err := r.CanAccess(ctx, serviceAccount, serviceAccountNamespace, res, nil) + subjectAccessReviews = append(subjectAccessReviews, sar) + errs = append(errs, err) + } + for _, nonResourceAttribute := range nonResourceAttributes { + sar, err := r.CanAccess(ctx, serviceAccount, serviceAccountNamespace, nil, nonResourceAttribute) + subjectAccessReviews = append(subjectAccessReviews, sar) + errs = append(errs, err) + } + } + return subjectAccessReviews, errors.Join(errs...) +} + +// CanAccess checks if the given serviceAccount is able to access a single requested resource attribute. +// The operator uses this functionality to ensure that users have the right RBAC configured for collector +// related service accounts. +func (r *Reviewer) CanAccess(ctx context.Context, serviceAccount, serviceAccountNamespace string, res *v1.ResourceAttributes, nonResourceAttributes *v1.NonResourceAttributes) (*v1.SubjectAccessReview, error) { + sar := &v1.SubjectAccessReview{ + Spec: v1.SubjectAccessReviewSpec{ + ResourceAttributes: res, + NonResourceAttributes: nonResourceAttributes, + User: fmt.Sprintf(serviceAccountFmtStr, serviceAccountNamespace, serviceAccount), + }, + } + return r.client.AuthorizationV1().SubjectAccessReviews().Create(ctx, sar, metav1.CreateOptions{}) +} + +// policyRuleToResourceAttributes converts a single policy rule in to a list of resource attribute requests. +// policyRules have lists of resources, verbs, groups, etc. whereas resource attributes do not work on lists. This +// requires us to iterate over each list and flatten. +func policyRuleToResourceAttributes(rule *rbacv1.PolicyRule) []*v1.ResourceAttributes { + var resourceAttributes []*v1.ResourceAttributes + for _, verb := range rule.Verbs { + for _, group := range rule.APIGroups { + for _, resource := range rule.Resources { + res := &v1.ResourceAttributes{ + Verb: verb, + Group: group, + Resource: resource, + } + resourceAttributes = append(resourceAttributes, res) + } + } + } + return resourceAttributes +} + +// policyRuleToResourceAttributes converts a single policy rule in to a list of resource attribute requests. +// policyRules have lists of resources, verbs, groups, etc. whereas resource attributes do not work on lists. This +// requires us to iterate over each list and flatten. +func policyRuleToNonResourceAttributes(rule *rbacv1.PolicyRule) []*v1.NonResourceAttributes { + var nonResourceAttributes []*v1.NonResourceAttributes + for _, verb := range rule.Verbs { + for _, nonResourceUrl := range rule.NonResourceURLs { + nonResourceAttribute := &v1.NonResourceAttributes{ + Verb: verb, + Path: nonResourceUrl, + } + nonResourceAttributes = append(nonResourceAttributes, nonResourceAttribute) + } + } + return nonResourceAttributes +} diff --git a/internal/rbac/access_test.go b/internal/rbac/access_test.go new file mode 100644 index 0000000000..acc0c4bbea --- /dev/null +++ b/internal/rbac/access_test.go @@ -0,0 +1,237 @@ +// Copyright The OpenTelemetry Authors +// +// 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 rbac + +import ( + "context" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + v1 "k8s.io/api/authorization/v1" + rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/kubernetes/fake" + kubeTesting "k8s.io/client-go/testing" +) + +const ( + createVerb = "create" + sarResource = "subjectaccessreviews" +) + +type fakeClientGenerator func() kubernetes.Interface + +func reactorFactory(status v1.SubjectAccessReviewStatus, mockErr error) fakeClientGenerator { + return func() kubernetes.Interface { + c := fake.NewSimpleClientset() + c.PrependReactor(createVerb, sarResource, func(action kubeTesting.Action) (handled bool, ret runtime.Object, err error) { + // check our expectation here + if !action.Matches(createVerb, sarResource) { + return false, nil, fmt.Errorf("must be a create for a SAR") + } + sar, ok := action.(kubeTesting.CreateAction).GetObject().DeepCopyObject().(*v1.SubjectAccessReview) + if !ok || sar == nil { + return false, nil, fmt.Errorf("bad object") + } + sar.Status = status + return true, sar, mockErr + }) + return c + } +} + +func TestReviewer_CanAccess(t *testing.T) { + type args struct { + serviceAccount string + serviceAccountNamespace string + res *v1.ResourceAttributes + } + tests := []struct { + name string + clientGenerator fakeClientGenerator + args args + want bool + wantErr bool + }{ + { + name: "cannot access", + clientGenerator: reactorFactory(v1.SubjectAccessReviewStatus{ + Denied: true, + }, nil), + args: args{ + serviceAccount: "test", + serviceAccountNamespace: "default", + res: &v1.ResourceAttributes{ + Namespace: "", + Verb: "list", + Resource: "namespaces", + }, + }, + want: false, + wantErr: false, + }, + { + name: "can access", + clientGenerator: reactorFactory(v1.SubjectAccessReviewStatus{ + Allowed: true, + }, nil), + args: args{ + serviceAccount: "test", + serviceAccountNamespace: "default", + res: &v1.ResourceAttributes{ + Namespace: "", + Verb: "list", + Resource: "namespaces", + }, + }, + want: true, + wantErr: false, + }, + { + name: "handles error", + clientGenerator: reactorFactory(v1.SubjectAccessReviewStatus{}, fmt.Errorf("failed to create SAR")), + args: args{ + serviceAccount: "test", + serviceAccountNamespace: "default", + res: &v1.ResourceAttributes{ + Namespace: "", + Verb: "list", + Resource: "namespaces", + }, + }, + want: false, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := NewReviewer(tt.clientGenerator()) + got, err := r.CanAccess(context.Background(), tt.args.serviceAccount, tt.args.serviceAccountNamespace, tt.args.res, nil) + if (err != nil) != tt.wantErr { + t.Errorf("CanAccess() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got.Status.Denied && got.Status.Denied != !tt.want { + assert.Equal(t, tt.want, got.Status.Denied) + } else if got.Status.Allowed != tt.want { + assert.Equal(t, tt.want, got.Status.Allowed) + } + }) + } +} + +func TestReviewer_CheckPolicyRules(t *testing.T) { + type args struct { + serviceAccount string + serviceAccountNamespace string + policyRules []*rbacv1.PolicyRule + } + tests := []struct { + name string + clientGenerator fakeClientGenerator + args args + want bool + wantErr bool + numFailedReviews int + }{ + { + name: "cannot access", + clientGenerator: reactorFactory(v1.SubjectAccessReviewStatus{ + Denied: true, + }, nil), + args: args{ + serviceAccount: "test", + serviceAccountNamespace: "default", + policyRules: []*rbacv1.PolicyRule{ + { + Verbs: []string{"get", "list", "watch"}, + APIGroups: []string{""}, + Resources: []string{"nodes", "nodes/metrics", "services", "endpoints", "pods"}, + }, + { + Verbs: []string{"get"}, + NonResourceURLs: []string{"/metrics"}, + }, + }, + }, + want: false, + wantErr: false, + numFailedReviews: 16, + }, + { + name: "can access", + clientGenerator: reactorFactory(v1.SubjectAccessReviewStatus{ + Allowed: true, + }, nil), + args: args{ + serviceAccount: "test", + serviceAccountNamespace: "default", + policyRules: []*rbacv1.PolicyRule{ + { + Verbs: []string{"get", "list", "watch"}, + APIGroups: []string{""}, + Resources: []string{"nodes", "nodes/metrics", "services", "endpoints", "pods"}, + }, + nil, // check that we handle nil policy rules + { + Verbs: []string{"get"}, + NonResourceURLs: []string{"/metrics"}, + }, + }, + }, + want: true, + wantErr: false, + }, + { + name: "handles error", + clientGenerator: reactorFactory(v1.SubjectAccessReviewStatus{}, fmt.Errorf("failed to create SAR")), + args: args{ + serviceAccount: "test", + serviceAccountNamespace: "default", + policyRules: []*rbacv1.PolicyRule{ + { + Verbs: []string{"get", "list", "watch"}, + APIGroups: []string{""}, + Resources: []string{"nodes", "nodes/metrics", "services", "endpoints", "pods"}, + }, + { + Verbs: []string{"get"}, + NonResourceURLs: []string{"/metrics"}, + }, + }, + }, + want: false, + wantErr: true, + numFailedReviews: 16, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := NewReviewer(tt.clientGenerator()) + got, err := r.CheckPolicyRules(context.Background(), tt.args.serviceAccount, tt.args.serviceAccountNamespace, tt.args.policyRules...) + if (err != nil) != tt.wantErr { + t.Errorf("CheckPolicyRules() error = %v, wantErr %v", err, tt.wantErr) + return + } + ok, deniedReviews := AllSubjectAccessReviewsAllowed(got) + assert.Equal(t, tt.want, ok) + if !ok { + assert.Equal(t, tt.numFailedReviews, len(deniedReviews)) + } + }) + } +} diff --git a/internal/webhook/podmutation/webhookhandler_suite_test.go b/internal/webhook/podmutation/webhookhandler_suite_test.go index 05f6c062be..c591c92ecb 100644 --- a/internal/webhook/podmutation/webhookhandler_suite_test.go +++ b/internal/webhook/podmutation/webhookhandler_suite_test.go @@ -27,6 +27,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" "k8s.io/client-go/util/retry" @@ -38,6 +39,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" "github.com/open-telemetry/opentelemetry-operator/internal/config" + "github.com/open-telemetry/opentelemetry-operator/internal/rbac" // +kubebuilder:scaffold:imports ) @@ -98,7 +100,13 @@ func TestMain(m *testing.M) { os.Exit(1) } - if err = v1alpha1.SetupCollectorWebhook(mgr, config.New()); err != nil { + clientset, clientErr := kubernetes.NewForConfig(cfg) + if err != nil { + fmt.Printf("failed to setup kubernetes clientset %v", clientErr) + } + reviewer := rbac.NewReviewer(clientset) + + if err = v1alpha1.SetupCollectorWebhook(mgr, config.New(), reviewer); err != nil { fmt.Printf("failed to SetupWebhookWithManager: %v", err) os.Exit(1) } diff --git a/main.go b/main.go index 11dd91cb13..d70ae3e0c2 100644 --- a/main.go +++ b/main.go @@ -30,6 +30,7 @@ import ( colfeaturegate "go.opentelemetry.io/collector/featuregate" k8sruntime "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" + "k8s.io/client-go/kubernetes" clientgoscheme "k8s.io/client-go/kubernetes/scheme" _ "k8s.io/client-go/plugin/pkg/client/auth/gcp" "k8s.io/client-go/tools/record" @@ -47,6 +48,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/controllers" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" "github.com/open-telemetry/opentelemetry-operator/internal/config" + "github.com/open-telemetry/opentelemetry-operator/internal/rbac" "github.com/open-telemetry/opentelemetry-operator/internal/version" "github.com/open-telemetry/opentelemetry-operator/internal/webhook/podmutation" collectorupgrade "github.com/open-telemetry/opentelemetry-operator/pkg/collector/upgrade" @@ -235,7 +237,7 @@ func main() { }, } - mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), mgrOptions) + mgr, err := ctrl.NewManager(restConfig, mgrOptions) if err != nil { setupLog.Error(err, "unable to start manager") os.Exit(1) @@ -247,6 +249,11 @@ func main() { setupLog.Error(err, "failed to add/run bootstrap dependencies to the controller manager") os.Exit(1) } + clientset, clientErr := kubernetes.NewForConfig(mgr.GetConfig()) + if err != nil { + setupLog.Error(clientErr, "failed to create kubernetes clientset") + } + reviewer := rbac.NewReviewer(clientset) if err = controllers.NewReconciler(controllers.Params{ Client: mgr.GetClient(), @@ -271,7 +278,7 @@ func main() { } if os.Getenv("ENABLE_WEBHOOKS") != "false" { - if err = otelv1alpha1.SetupCollectorWebhook(mgr, cfg); err != nil { + if err = otelv1alpha1.SetupCollectorWebhook(mgr, cfg, reviewer); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "OpenTelemetryCollector") os.Exit(1) } diff --git a/pkg/collector/upgrade/suite_test.go b/pkg/collector/upgrade/suite_test.go index 9496b9f47d..024423d599 100644 --- a/pkg/collector/upgrade/suite_test.go +++ b/pkg/collector/upgrade/suite_test.go @@ -27,6 +27,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" "k8s.io/client-go/util/retry" @@ -38,6 +39,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" "github.com/open-telemetry/opentelemetry-operator/internal/config" + "github.com/open-telemetry/opentelemetry-operator/internal/rbac" // +kubebuilder:scaffold:imports ) @@ -49,7 +51,6 @@ var ( cancel context.CancelFunc err error cfg *rest.Config - conf = config.New() ) func TestMain(m *testing.M) { @@ -100,7 +101,13 @@ func TestMain(m *testing.M) { os.Exit(1) } - if err = v1alpha1.SetupCollectorWebhook(mgr, conf); err != nil { + clientset, clientErr := kubernetes.NewForConfig(cfg) + if err != nil { + fmt.Printf("failed to setup kubernetes clientset %v", clientErr) + } + reviewer := rbac.NewReviewer(clientset) + + if err = v1alpha1.SetupCollectorWebhook(mgr, config.New(), reviewer); err != nil { fmt.Printf("failed to SetupWebhookWithManager: %v", err) os.Exit(1) } diff --git a/tests/e2e/targetallocator-features/00-install.yaml b/tests/e2e/targetallocator-features/00-install.yaml index d134dac262..e2c69abe12 100644 --- a/tests/e2e/targetallocator-features/00-install.yaml +++ b/tests/e2e/targetallocator-features/00-install.yaml @@ -53,9 +53,6 @@ spec: runAsUser: 1000 runAsGroup: 3000 fsGroup: 3000 - allowPrivilegeEscalation: false - capabilities: - drop: ["ALL"] affinity: nodeAffinity: requiredDuringSchedulingIgnoredDuringExecution: