diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index 35f2409f6..2da4af083 100644 --- a/cmd/operator-controller/main.go +++ b/cmd/operator-controller/main.go @@ -61,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" @@ -409,16 +410,16 @@ func run() error { crdupgradesafety.NewPreflight(aeClient.CustomResourceDefinitions()), } - acm := applier.NewAuthClientMapper(clientRestConfigMapper, mgr.GetConfig()) + 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, - AuthClientMapper: acm, + 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 a43674087..7ce93d700 100644 --- a/internal/operator-controller/applier/helm.go +++ b/internal/operator-controller/applier/helm.go @@ -18,8 +18,6 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" apimachyaml "k8s.io/apimachinery/pkg/util/yaml" - authorizationv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" - "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" @@ -56,36 +54,10 @@ type Preflight interface { Upgrade(context.Context, *release.Release) error } -type RestConfigMapper func(context.Context, client.Object, *rest.Config) (*rest.Config, error) - -type NewForConfigFunc func(*rest.Config) (authorizationv1client.AuthorizationV1Interface, error) - -type AuthClientMapper struct { - rcm RestConfigMapper - baseCfg *rest.Config - NewForConfig NewForConfigFunc -} - -func (acm *AuthClientMapper) GetAuthenticationClient(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) -} - type Helm struct { - ActionClientGetter helmclient.ActionClientGetter - Preflights []Preflight - AuthClientMapper AuthClientMapper -} - -func NewAuthClientMapper(rcm RestConfigMapper, baseCfg *rest.Config) AuthClientMapper { - return AuthClientMapper{ - rcm: rcm, - baseCfg: baseCfg, - } + ActionClientGetter helmclient.ActionClientGetter + Preflights []Preflight + AuthorizationClientMapper authorization.AuthorizationClientMapper } // shouldSkipPreflight is a helper to determine if the preflight check is CRDUpgradeSafety AND @@ -110,14 +82,14 @@ 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) { - authclient, err := h.AuthClientMapper.GetAuthenticationClient(ctx, ext) + authclient, err := h.AuthorizationClientMapper.GetAuthorizationClient(ctx, ext) if err != nil { return nil, "", err } - err = h.checkContentPermissions(ctx, contentFS, authclient, ext) + err = h.AuthorizationClientMapper.CheckContentPermissions(ctx, contentFS, authclient, ext) if err != nil { - return nil, "", err + return nil, "", fmt.Errorf("failed checking content permissions: %w", err) } } @@ -195,23 +167,6 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte 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, authcl authorizationv1client.AuthorizationV1Interface, ext *ocv1.ClusterExtension) error { - reg, err := convert.ParseFS(ctx, contentFS) - if err != nil { - return err - } - - plain, err := convert.Convert(reg, ext.Spec.Namespace, []string{corev1.NamespaceAll}) - if err != nil { - return err - } - - err = authorization.CheckObjectPermissions(ctx, authcl, plain.Objects, ext) - - return err -} - 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()) diff --git a/internal/operator-controller/applier/helm_test.go b/internal/operator-controller/applier/helm_test.go index 5c3faedf5..384ec7e60 100644 --- a/internal/operator-controller/applier/helm_test.go +++ b/internal/operator-controller/applier/helm_test.go @@ -115,13 +115,13 @@ func newPassingSSRRAuthClient() authorizationv1client.AuthorizationV1Interface { } } -// Helper builds an AuthClientMapper with the passing SSRR -func newPassingAuthClientMapper() applier.AuthClientMapper { +// 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 := applier.NewAuthClientMapper(mockRCM, fakeRestConfig) + acm := applier.NewAuthorizationClientMapper(mockRCM, fakeRestConfig) acm.NewForConfig = func(*rest.Config) (authorizationv1client.AuthorizationV1Interface, error) { return newPassingSSRRAuthClient(), nil } @@ -131,9 +131,9 @@ func newPassingAuthClientMapper() applier.AuthClientMapper { // Helper builds a Helm applier with passing SSRR func buildHelmApplier(mockAcg *mockActionGetter, preflights []applier.Preflight) applier.Helm { return applier.Helm{ - ActionClientGetter: mockAcg, - AuthClientMapper: newPassingAuthClientMapper(), - Preflights: preflights, + ActionClientGetter: mockAcg, + AuthorizationClientMapper: newPassingAuthorizationClientMapper(), + Preflights: preflights, } } diff --git a/internal/operator-controller/authorization/authorization.go b/internal/operator-controller/authorization/authorization.go index 9e848817b..3b6fe6ea4 100644 --- a/internal/operator-controller/authorization/authorization.go +++ b/internal/operator-controller/authorization/authorization.go @@ -4,16 +4,20 @@ import ( "context" "errors" "fmt" + "io/fs" "slices" "strings" - authorizationv1 "k8s.io/api/authorization/v1" + ocv1 "github.com/operator-framework/operator-controller/api/v1" + "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/convert" + authv1 "k8s.io/api/authorization/v1" + corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/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" - - ocv1 "github.com/operator-framework/operator-controller/api/v1" ) const ( @@ -21,14 +25,58 @@ const ( SelfSubjectAccessReview string = "SelfSubjectAccessReview" ) -func CheckObjectPermissions(ctx context.Context, authcl authorizationv1client.AuthorizationV1Interface, objects []client.Object, ext *ocv1.ClusterExtension) error { - ssrr := &authorizationv1.SelfSubjectRulesReview{ - Spec: authorizationv1.SelfSubjectRulesReviewSpec{ +type RestConfigMapper func(context.Context, client.Object, *rest.Config) (*rest.Config, error) + +type NewForConfigFunc func(*rest.Config) (authorizationv1client.AuthorizationV1Interface, 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) +} + +// Check if RBAC allows the installer service account necessary permissions on the objects in the contentFS +func (acm *AuthorizationClientMapper) CheckContentPermissions(ctx context.Context, contentFS fs.FS, authcl authorizationv1client.AuthorizationV1Interface, ext *ocv1.ClusterExtension) error { + reg, err := convert.ParseFS(ctx, contentFS) + if err != nil { + return err + } + + plain, err := convert.Convert(reg, ext.Spec.Namespace, []string{corev1.NamespaceAll}) + if err != nil { + return err + } + + err = checkObjectPermissions(ctx, authcl, plain.Objects, ext) + + return err +} + +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, }, } - ssrr, err := authcl.SelfSubjectRulesReviews().Create(ctx, ssrr, metav1.CreateOptions{}) + opts := v1.CreateOptions{} + ssrr, err := authcl.SelfSubjectRulesReviews().Create(ctx, ssrr, opts) if err != nil { return err } @@ -50,14 +98,14 @@ func CheckObjectPermissions(ctx context.Context, authcl authorizationv1client.Au }) } - resAttrs := []authorizationv1.ResourceAttributes{} 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, authorizationv1.ResourceAttributes{ + resAttrs = append(resAttrs, authv1.ResourceAttributes{ Namespace: o.GetNamespace(), Verb: verb, Resource: sanitizeResourceName(o.GetObjectKind().GroupVersionKind().Kind), @@ -70,7 +118,7 @@ func CheckObjectPermissions(ctx context.Context, authcl authorizationv1client.Au for _, resAttr := range resAttrs { if !canI(resAttr, rules) { if resAttr.Namespace != "" { - namespacedErrs = append(namespacedErrs, fmt.Errorf("cannot %s %s %s in namespace %s", + namespacedErrs = append(namespacedErrs, fmt.Errorf("cannot %q %q %q in namespace %q", resAttr.Verb, strings.TrimSuffix(resAttr.Resource, "s"), resAttr.Name, @@ -92,7 +140,7 @@ func CheckObjectPermissions(ctx context.Context, authcl authorizationv1client.Au } // Checks if the rules allow the verb on the GroupVersionKind in resAttr -func canI(resAttr authorizationv1.ResourceAttributes, rules []rbacv1.PolicyRule) bool { +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, "*")) &&