diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index 2a46afc6d..2da4af083 100644 --- a/cmd/operator-controller/main.go +++ b/cmd/operator-controller/main.go @@ -37,8 +37,10 @@ import ( k8slabels "k8s.io/apimachinery/pkg/labels" k8stypes "k8s.io/apimachinery/pkg/types" apimachineryrand "k8s.io/apimachinery/pkg/util/rand" + authorizationv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" _ "k8s.io/client-go/plugin/pkg/client/auth" + "k8s.io/client-go/rest" "k8s.io/klog/v2" "k8s.io/klog/v2/textlogger" "k8s.io/utils/ptr" @@ -59,6 +61,7 @@ import ( "github.com/operator-framework/operator-controller/internal/operator-controller/action" "github.com/operator-framework/operator-controller/internal/operator-controller/applier" "github.com/operator-framework/operator-controller/internal/operator-controller/authentication" + "github.com/operator-framework/operator-controller/internal/operator-controller/authorization" "github.com/operator-framework/operator-controller/internal/operator-controller/catalogmetadata/cache" catalogclient "github.com/operator-framework/operator-controller/internal/operator-controller/catalogmetadata/client" "github.com/operator-framework/operator-controller/internal/operator-controller/contentmanager" @@ -407,9 +410,16 @@ func run() error { crdupgradesafety.NewPreflight(aeClient.CustomResourceDefinitions()), } + acm := authorization.NewAuthorizationClientMapper(clientRestConfigMapper, mgr.GetConfig()) + acm.NewForConfig = func(cfg *rest.Config) (authorizationv1client.AuthorizationV1Interface, error) { + // *AuthorizationV1Client implements AuthorizationV1Interface + return authorizationv1client.NewForConfig(cfg) + } + helmApplier := &applier.Helm{ - ActionClientGetter: acg, - Preflights: preflights, + ActionClientGetter: acg, + Preflights: preflights, + AuthorizationClientMapper: acm, } cm := contentmanager.NewManager(clientRestConfigMapper, mgr.GetConfig(), mgr.GetRESTMapper()) diff --git a/internal/operator-controller/applier/helm.go b/internal/operator-controller/applier/helm.go index 76df085cb..c63c7ab03 100644 --- a/internal/operator-controller/applier/helm.go +++ b/internal/operator-controller/applier/helm.go @@ -24,6 +24,7 @@ import ( helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client" ocv1 "github.com/operator-framework/operator-controller/api/v1" + "github.com/operator-framework/operator-controller/internal/operator-controller/authorization" "github.com/operator-framework/operator-controller/internal/operator-controller/features" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/convert" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/preflights/crdupgradesafety" @@ -54,8 +55,9 @@ type Preflight interface { } type Helm struct { - ActionClientGetter helmclient.ActionClientGetter - Preflights []Preflight + ActionClientGetter helmclient.ActionClientGetter + Preflights []Preflight + AuthorizationClientMapper authorization.AuthorizationClientMapper } // shouldSkipPreflight is a helper to determine if the preflight check is CRDUpgradeSafety AND @@ -79,7 +81,20 @@ func shouldSkipPreflight(ctx context.Context, preflight Preflight, ext *ocv1.Clu } func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels map[string]string, storageLabels map[string]string) ([]client.Object, string, error) { + if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) { + rawAuthClient, err := h.AuthorizationClientMapper.GetAuthorizationClient(ctx, ext) + if err != nil { + return nil, "", fmt.Errorf("failed to get authorization client: %w", err) + } + + authClient := authorization.NewClient(rawAuthClient) + if err := h.checkContentPermissions(ctx, contentFS, authClient, ext); err != nil { + return nil, "", fmt.Errorf("failed checking content permissions: %w", err) + } + } + chrt, err := convert.RegistryV1ToHelmChart(ctx, contentFS, ext.Spec.Namespace, []string{corev1.NamespaceAll}) + if err != nil { return nil, "", err } @@ -87,7 +102,7 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte ac, err := h.ActionClientGetter.ActionClientFor(ctx, ext) if err != nil { - return nil, "", err + return nil, "", fmt.Errorf("failed to get action client: %w", err) } post := &postrenderer{ @@ -105,14 +120,12 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte } switch state { case StateNeedsInstall: - err := preflight.Install(ctx, desiredRel) - if err != nil { - return nil, state, err + if err := preflight.Install(ctx, desiredRel); err != nil { + return nil, state, fmt.Errorf("preflight install check failed: %w", err) } case StateNeedsUpgrade: - err := preflight.Upgrade(ctx, desiredRel) - if err != nil { - return nil, state, err + if err := preflight.Upgrade(ctx, desiredRel); err != nil { + return nil, state, fmt.Errorf("preflight upgrade check failed: %w", err) } } } @@ -125,7 +138,7 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte return nil }, helmclient.AppendInstallPostRenderer(post)) if err != nil { - return nil, state, err + return nil, state, fmt.Errorf("failed to install release: %w", err) } case StateNeedsUpgrade: rel, err = ac.Upgrade(ext.GetName(), ext.Spec.Namespace, chrt, values, func(upgrade *action.Upgrade) error { @@ -134,11 +147,11 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte return nil }, helmclient.AppendUpgradePostRenderer(post)) if err != nil { - return nil, state, err + return nil, state, fmt.Errorf("failed to upgrade release: %w", err) } case StateUnchanged: if err := ac.Reconcile(rel); err != nil { - return nil, state, err + return nil, state, fmt.Errorf("failed to reconcile release: %w", err) } default: return nil, state, fmt.Errorf("unexpected release state %q", state) @@ -146,14 +159,30 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte relObjects, err := util.ManifestObjects(strings.NewReader(rel.Manifest), fmt.Sprintf("%s-release-manifest", rel.Name)) if err != nil { - return nil, state, err + return nil, state, fmt.Errorf("failed to convert manifest to objects: %w", err) } return relObjects, state, nil } +// Check if RBAC allows the installer service account necessary permissions on the objects in the contentFS +func (h *Helm) checkContentPermissions(ctx context.Context, contentFS fs.FS, authClient authorization.AuthorizationClient, ext *ocv1.ClusterExtension) error { + reg, err := convert.ParseFS(ctx, contentFS) + if err != nil { + return fmt.Errorf("failed to parse content FS: %w", err) + } + + plain, err := convert.Convert(reg, ext.Spec.Namespace, []string{corev1.NamespaceAll}) + if err != nil { + return fmt.Errorf("failed to convert registry: %w", err) + } + + return authClient.CheckContentPermissions(ctx, plain.Objects, ext) +} + func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterExtension, chrt *chart.Chart, values chartutil.Values, post postrender.PostRenderer) (*release.Release, *release.Release, string, error) { currentRelease, err := cl.Get(ext.GetName()) + if errors.Is(err, driver.ErrReleaseNotFound) { desiredRelease, err := cl.Install(ext.GetName(), ext.Spec.Namespace, chrt, values, func(i *action.Install) error { i.DryRun = true @@ -161,16 +190,12 @@ func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterE return nil }, helmclient.AppendInstallPostRenderer(post)) if err != nil { - if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) { - _ = struct{}{} // minimal no-op to satisfy linter - // probably need to break out this error as it's the one for helm dry-run as opposed to any returned later - } - return nil, nil, StateError, err + return nil, nil, StateError, fmt.Errorf("failed dry-run install: %w", err) } return nil, desiredRelease, StateNeedsInstall, nil } if err != nil { - return nil, nil, StateError, err + return nil, nil, StateError, fmt.Errorf("failed to get current release: %w", err) } desiredRelease, err := cl.Upgrade(ext.GetName(), ext.Spec.Namespace, chrt, values, func(upgrade *action.Upgrade) error { @@ -180,7 +205,7 @@ func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterE return nil }, helmclient.AppendUpgradePostRenderer(post)) if err != nil { - return currentRelease, nil, StateError, err + return currentRelease, nil, StateError, fmt.Errorf("failed dry-run upgrade: %w", err) } relState := StateUnchanged if desiredRelease.Manifest != currentRelease.Manifest || diff --git a/internal/operator-controller/applier/helm_test.go b/internal/operator-controller/applier/helm_test.go index 78d6629a5..73a15ff62 100644 --- a/internal/operator-controller/applier/helm_test.go +++ b/internal/operator-controller/applier/helm_test.go @@ -13,6 +13,10 @@ import ( "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/release" "helm.sh/helm/v3/pkg/storage/driver" + authorizationv1 "k8s.io/api/authorization/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + authorizationv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" + "k8s.io/client-go/rest" featuregatetesting "k8s.io/component-base/featuregate/testing" "sigs.k8s.io/controller-runtime/pkg/client" @@ -20,6 +24,7 @@ import ( v1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/operator-controller/applier" + "github.com/operator-framework/operator-controller/internal/operator-controller/authorization" "github.com/operator-framework/operator-controller/internal/operator-controller/features" ) @@ -94,6 +99,83 @@ func (mag *mockActionGetter) Reconcile(rel *release.Release) error { return mag.reconcileErr } +// Helper returns an AuthorizationV1Interface where SSRR always passes. +func newPassingSSRRAuthClient() authorizationv1client.AuthorizationV1Interface { + return &mockAuthorizationV1Client{ + ssrrInterface: &mockSelfSubjectRulesReviewInterface{ + retSSRR: &authorizationv1.SelfSubjectRulesReview{ + Status: authorizationv1.SubjectRulesReviewStatus{ + ResourceRules: []authorizationv1.ResourceRule{{ + Verbs: []string{"*"}, + APIGroups: []string{"*"}, + Resources: []string{"*"}, + }}, + }, + }, + }, + } +} + +// Helper builds an AuthorizationClientMapper with the passing SSRR +func newPassingAuthorizationClientMapper() applier.AuthorizationClientMapper { + fakeRestConfig := &rest.Config{Host: "fake-server"} + mockRCM := func(ctx context.Context, obj client.Object, cfg *rest.Config) (*rest.Config, error) { + return cfg, nil + } + acm := authorization.NewAuthorizationClientMapper(mockRCM, fakeRestConfig) + acm.NewForConfig = func(*rest.Config) (authorizationv1client.AuthorizationV1Interface, error) { + return newPassingSSRRAuthClient(), nil + } + return acm +} + +// Helper builds a Helm applier with passing SSRR +func buildHelmApplier(mockAcg *mockActionGetter, preflights []applier.Preflight) applier.Helm { + return applier.Helm{ + ActionClientGetter: mockAcg, + AuthorizationClientMapper: newPassingAuthorizationClientMapper(), + Preflights: preflights, + } +} + +type mockAuthorizationV1Client struct { + ssrrInterface authorizationv1client.SelfSubjectRulesReviewInterface +} + +func (m *mockAuthorizationV1Client) SelfSubjectRulesReviews() authorizationv1client.SelfSubjectRulesReviewInterface { + return m.ssrrInterface +} +func (m *mockAuthorizationV1Client) RESTClient() rest.Interface { + return nil +} + +// Mock for SelfSubjectRulesReviewInterface +type mockSelfSubjectRulesReviewInterface struct { + retSSRR *authorizationv1.SelfSubjectRulesReview + retErr error +} + +func (m *mockSelfSubjectRulesReviewInterface) Create( + ctx context.Context, + ssrr *authorizationv1.SelfSubjectRulesReview, + opts metav1.CreateOptions, +) (*authorizationv1.SelfSubjectRulesReview, error) { + // Return either a success or an error, depending on what you want in the test. + return m.retSSRR, m.retErr +} + +func (m *mockAuthorizationV1Client) LocalSubjectAccessReviews(namespace string) authorizationv1client.LocalSubjectAccessReviewInterface { + return nil +} + +func (m *mockAuthorizationV1Client) SelfSubjectAccessReviews() authorizationv1client.SelfSubjectAccessReviewInterface { + return nil +} + +func (m *mockAuthorizationV1Client) SubjectAccessReviews() authorizationv1client.SubjectAccessReviewInterface { + return nil +} + var ( // required for unmockable call to convert.RegistryV1ToHelmChart validFS = fstest.MapFS{ @@ -229,6 +311,7 @@ func TestApply_Installation(t *testing.T) { } func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { + // Set feature gate ONCE at parent level featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.PreflightPermissions, true) t.Run("fails during dry-run installation", func(t *testing.T) { @@ -236,9 +319,15 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { getClientErr: driver.ErrReleaseNotFound, dryRunInstallErr: errors.New("failed attempting to dry-run install chart"), } - helmApplier := applier.Helm{ActionClientGetter: mockAcg} - - objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + helmApplier := buildHelmApplier(mockAcg, nil) + + objs, state, err := helmApplier.Apply( + context.TODO(), + validFS, + testCE, + testObjectLabels, + testStorageLabels, + ) require.Error(t, err) require.ErrorContains(t, err, "attempting to dry-run install chart") require.Nil(t, objs) @@ -251,7 +340,8 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { installErr: errors.New("failed installing chart"), } mockPf := &mockPreflight{installErr: errors.New("failed during install pre-flight check")} - helmApplier := applier.Helm{ActionClientGetter: mockAcg, Preflights: []applier.Preflight{mockPf}} + + helmApplier := buildHelmApplier(mockAcg, []applier.Preflight{mockPf}) objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) require.Error(t, err) @@ -265,9 +355,15 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { getClientErr: driver.ErrReleaseNotFound, installErr: errors.New("failed installing chart"), } - helmApplier := applier.Helm{ActionClientGetter: mockAcg} - - objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + helmApplier := buildHelmApplier(mockAcg, nil) + + objs, state, err := helmApplier.Apply( + context.TODO(), + validFS, + testCE, + testObjectLabels, + testStorageLabels, + ) require.Error(t, err) require.ErrorContains(t, err, "installing chart") require.Equal(t, applier.StateNeedsInstall, state) @@ -282,9 +378,15 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { Manifest: validManifest, }, } - helmApplier := applier.Helm{ActionClientGetter: mockAcg} - - objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + helmApplier := buildHelmApplier(mockAcg, nil) + + objs, state, err := helmApplier.Apply( + context.TODO(), + validFS, + testCE, + testObjectLabels, + testStorageLabels, + ) require.NoError(t, err) require.Equal(t, applier.StateNeedsInstall, state) require.NotNil(t, objs) diff --git a/internal/operator-controller/authorization/authorization.go b/internal/operator-controller/authorization/authorization.go new file mode 100644 index 000000000..7b637ef10 --- /dev/null +++ b/internal/operator-controller/authorization/authorization.go @@ -0,0 +1,134 @@ +package authorization + +import ( + "context" + "errors" + "fmt" + "slices" + "strings" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" + authv1 "k8s.io/api/authorization/v1" + rbacv1 "k8s.io/api/rbac/v1" + + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + authorizationv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +type RestConfigMapper func(context.Context, client.Object, *rest.Config) (*rest.Config, error) + +type AuthorizationClientMapper struct { + rcm RestConfigMapper + baseCfg *rest.Config + NewForConfig NewForConfigFunc +} + +func NewAuthorizationClientMapper(rcm RestConfigMapper, baseCfg *rest.Config) AuthorizationClientMapper { + return AuthorizationClientMapper{ + rcm: rcm, + baseCfg: baseCfg, + } +} + +func (acm *AuthorizationClientMapper) GetAuthorizationClient(ctx context.Context, ext *ocv1.ClusterExtension) (authorizationv1client.AuthorizationV1Interface, error) { + authcfg, err := acm.rcm(ctx, ext, acm.baseCfg) + if err != nil { + return nil, err + } + return acm.NewForConfig(authcfg) +} + +// CheckObjectPermissions verifies that the given objects have the required permissions. +func CheckObjectPermissions(ctx context.Context, authcl authorizationv1client.AuthorizationV1Interface, objects []client.Object, ext *ocv1.ClusterExtension) error { + ssrr := &authv1.SelfSubjectRulesReview{ + Spec: authv1.SelfSubjectRulesReviewSpec{ + Namespace: ext.Spec.Namespace, + }, + } + + opts := v1.CreateOptions{} + ssrr, err := authcl.SelfSubjectRulesReviews().Create(ctx, ssrr, opts) + if err != nil { + return fmt.Errorf("failed to create SelfSubjectRulesReview: %w", err) + } + + rules := []rbacv1.PolicyRule{} + for _, rule := range ssrr.Status.ResourceRules { + rules = append(rules, rbacv1.PolicyRule{ + Verbs: rule.Verbs, + APIGroups: rule.APIGroups, + Resources: rule.Resources, + ResourceNames: rule.ResourceNames, + }) + } + + for _, rule := range ssrr.Status.NonResourceRules { + rules = append(rules, rbacv1.PolicyRule{ + Verbs: rule.Verbs, + NonResourceURLs: rule.NonResourceURLs, + }) + } + + namespacedErrs := []error{} + clusterScopedErrs := []error{} + requiredVerbs := []string{"get", "create", "update", "list", "watch", "delete", "patch"} + resAttrs := make([]authv1.ResourceAttributes, 0, len(requiredVerbs)*len(objects)) + + for _, o := range objects { + for _, verb := range requiredVerbs { + resAttrs = append(resAttrs, authv1.ResourceAttributes{ + Namespace: o.GetNamespace(), + Verb: verb, + Resource: sanitizeResourceName(o.GetObjectKind().GroupVersionKind().Kind), + Group: o.GetObjectKind().GroupVersionKind().Group, + Name: o.GetName(), + }) + } + } + + for _, resAttr := range resAttrs { + if !canI(resAttr, rules) { + if resAttr.Namespace != "" { + namespacedErrs = append(namespacedErrs, fmt.Errorf("cannot %s %q %q in namespace %q", + resAttr.Verb, + strings.TrimSuffix(resAttr.Resource, "s"), + resAttr.Name, + resAttr.Namespace)) + continue + } + clusterScopedErrs = append(clusterScopedErrs, fmt.Errorf("cannot %s %q %q", + resAttr.Verb, + strings.TrimSuffix(resAttr.Resource, "s"), + resAttr.Name)) + } + } + + allErrs := append(namespacedErrs, clusterScopedErrs...) + if len(allErrs) > 0 { + return fmt.Errorf("installer service account %q is missing required permissions: %w", ext.Spec.ServiceAccount.Name, errors.Join(allErrs...)) + } + + return nil +} + +// Checks if the rules allow the verb on the GroupVersionKind in resAttr +func canI(resAttr authv1.ResourceAttributes, rules []rbacv1.PolicyRule) bool { + var canI bool + for _, rule := range rules { + if (slices.Contains(rule.APIGroups, resAttr.Group) || slices.Contains(rule.APIGroups, "*")) && + (slices.Contains(rule.Resources, resAttr.Resource) || slices.Contains(rule.Resources, "*")) && + (slices.Contains(rule.Verbs, resAttr.Verb) || slices.Contains(rule.Verbs, "*")) && + (slices.Contains(rule.ResourceNames, resAttr.Name) || len(rule.ResourceNames) == 0) { + canI = true + break + } + } + return canI +} + +// SelfSubjectRulesReview formats the resource names as lowercase and plural +func sanitizeResourceName(resourceName string) string { + return strings.ToLower(resourceName) + "s" +} diff --git a/internal/operator-controller/authorization/client.go b/internal/operator-controller/authorization/client.go new file mode 100644 index 000000000..8a855b513 --- /dev/null +++ b/internal/operator-controller/authorization/client.go @@ -0,0 +1,33 @@ +package authorization + +import ( + "context" + + authorizationv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" +) + +type NewForConfigFunc func(*rest.Config) (authorizationv1client.AuthorizationV1Interface, error) + +// AuthorizationClient is an interface exposing only the needed functionality +type AuthorizationClient interface { + CheckContentPermissions(ctx context.Context, objects []client.Object, ext *ocv1.ClusterExtension) error +} + +// clientImpl wraps the underlying authorization client +type clientImpl struct { + authClient authorizationv1client.AuthorizationV1Interface +} + +// NewClient wraps an authorizationv1client.AuthorizationV1Interface +func NewClient(authClient authorizationv1client.AuthorizationV1Interface) AuthorizationClient { + return &clientImpl{authClient: authClient} +} + +// CheckContentPermissions is the public method that internally calls the generic check +func (c *clientImpl) CheckContentPermissions(ctx context.Context, objects []client.Object, ext *ocv1.ClusterExtension) error { + return CheckObjectPermissions(ctx, c.authClient, objects, ext) +}