From db9faf5835ed7df593bad0a978d95985dd48fee9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Charles-Edouard=20Br=C3=A9t=C3=A9ch=C3=A9?= Date: Thu, 1 Dec 2022 09:02:21 +0100 Subject: [PATCH] fix: cleanup policy validation (#5514) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: cleanup policy validation Signed-off-by: Charles-Edouard Brétéché * fix Signed-off-by: Charles-Edouard Brétéché * fix Signed-off-by: Charles-Edouard Brétéché Signed-off-by: Charles-Edouard Brétéché Signed-off-by: Charles-Edouard Brétéché Co-authored-by: Vyankatesh Kudtarkar --- cmd/cleanup-controller/handlers.go | 8 +- cmd/cleanup-controller/validate/validate.go | 96 --------------------- pkg/validation/cleanuppolicy/validate.go | 76 ++++++++++++++++ 3 files changed, 80 insertions(+), 100 deletions(-) delete mode 100644 cmd/cleanup-controller/validate/validate.go create mode 100644 pkg/validation/cleanuppolicy/validate.go diff --git a/cmd/cleanup-controller/handlers.go b/cmd/cleanup-controller/handlers.go index c338b73cbb..b557642586 100644 --- a/cmd/cleanup-controller/handlers.go +++ b/cmd/cleanup-controller/handlers.go @@ -5,9 +5,9 @@ import ( "time" "github.com/go-logr/logr" - "github.com/kyverno/kyverno/cmd/cleanup-controller/validate" "github.com/kyverno/kyverno/pkg/clients/dclient" admissionutils "github.com/kyverno/kyverno/pkg/utils/admission" + validation "github.com/kyverno/kyverno/pkg/validation/cleanuppolicy" admissionv1 "k8s.io/api/admission/v1" ) @@ -27,9 +27,9 @@ func (h *cleanupPolicyHandlers) Validate(ctx context.Context, logger logr.Logger logger.Error(err, "failed to unmarshal policies from admission request") return admissionutils.Response(request.UID, err) } - err = validate.ValidateCleanupPolicy(logger, policy, h.client, false) - if err != nil { + if err := validation.Validate(ctx, logger, h.client, policy); err != nil { logger.Error(err, "policy validation errors") + return admissionutils.Response(request.UID, err) } - return admissionutils.Response(request.UID, err) + return nil } diff --git a/cmd/cleanup-controller/validate/validate.go b/cmd/cleanup-controller/validate/validate.go deleted file mode 100644 index 205f074a89..0000000000 --- a/cmd/cleanup-controller/validate/validate.go +++ /dev/null @@ -1,96 +0,0 @@ -package validate - -import ( - "fmt" - - "github.com/go-logr/logr" - kyvernov1alpha1 "github.com/kyverno/kyverno/api/kyverno/v1alpha1" - "github.com/kyverno/kyverno/pkg/clients/dclient" - "github.com/kyverno/kyverno/pkg/engine/variables" - "github.com/kyverno/kyverno/pkg/policy/generate" - "golang.org/x/net/context" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/sets" - "k8s.io/client-go/discovery" -) - -// Cleanup provides implementation to validate permission for using DELETE operation by CleanupPolicy -type Cleanup struct { - // rule to hold CleanupPolicy specifications - spec kyvernov1alpha1.CleanupPolicySpec - // authCheck to check access for operations - authCheck generate.Operations - // logger - log logr.Logger -} - -// NewCleanup returns a new instance of Cleanup validation checker -func NewCleanup(client dclient.Interface, cleanup kyvernov1alpha1.CleanupPolicySpec, log logr.Logger) *Cleanup { - c := Cleanup{ - spec: cleanup, - authCheck: generate.NewAuth(client, log), - log: log, - } - - return &c -} - -// canIDelete returns a error if kyverno cannot perform operations -func (c *Cleanup) CanIDelete(ctx context.Context, kind, namespace string) error { - // Skip if there is variable defined - authCheck := c.authCheck - if !variables.IsVariable(kind) && !variables.IsVariable(namespace) { - // DELETE - ok, err := authCheck.CanIDelete(ctx, kind, namespace) - if err != nil { - // machinery error - return err - } - if !ok { - return fmt.Errorf("kyverno does not have permissions to 'delete' resource %s/%s. Update permissions in ClusterRole", kind, namespace) - } - } else { - c.log.V(4).Info("name & namespace uses variables, so cannot be resolved. Skipping Auth Checks.") - } - - return nil -} - -// Validate checks the policy and rules declarations for required configurations -func ValidateCleanupPolicy(logger logr.Logger, cleanuppolicy kyvernov1alpha1.CleanupPolicyInterface, client dclient.Interface, mock bool) error { - // namespace := cleanuppolicy.GetNamespace() - var res []*metav1.APIResourceList - clusterResources := sets.NewString() - - // Get all the cluster type kind supported by cluster - res, err := discovery.ServerPreferredResources(client.Discovery().DiscoveryInterface()) - if err != nil { - if discovery.IsGroupDiscoveryFailedError(err) { - err := err.(*discovery.ErrGroupDiscoveryFailed) - for gv, err := range err.Groups { - logger.Error(err, "failed to list api resources", "group", gv) - } - } else { - return err - } - } - for _, resList := range res { - for _, r := range resList.APIResources { - if !r.Namespaced { - clusterResources.Insert(r.Kind) - } - } - } - - if errs := cleanuppolicy.Validate(clusterResources); len(errs) != 0 { - return errs.ToAggregate() - } - - // for kind := range clusterResources { - // checker := NewCleanup(client, *cleanuppolicy.GetSpec(), logging.GlobalLogger()) - // if err := checker.CanIDelete(kind, namespace); err != nil { - // return fmt.Errorf("cannot delete kind %s in namespace %s", kind, namespace) - // } - // } - return nil -} diff --git a/pkg/validation/cleanuppolicy/validate.go b/pkg/validation/cleanuppolicy/validate.go new file mode 100644 index 0000000000..c9c7a5ee1f --- /dev/null +++ b/pkg/validation/cleanuppolicy/validate.go @@ -0,0 +1,76 @@ +package cleanuppolicy + +import ( + "context" + "fmt" + + "github.com/go-logr/logr" + kyvernov1alpha1 "github.com/kyverno/kyverno/api/kyverno/v1alpha1" + "github.com/kyverno/kyverno/pkg/auth" + "github.com/kyverno/kyverno/pkg/clients/dclient" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/client-go/discovery" +) + +// FetchClusteredResources retieves the list of clustered resources +func FetchClusteredResources(logger logr.Logger, client dclient.Interface) (sets.String, error) { + res, err := discovery.ServerPreferredResources(client.Discovery().DiscoveryInterface()) + if err != nil { + if discovery.IsGroupDiscoveryFailedError(err) { + err := err.(*discovery.ErrGroupDiscoveryFailed) + for gv, err := range err.Groups { + logger.Error(err, "failed to list api resources", "group", gv) + } + } else { + return nil, err + } + } + clusterResources := sets.NewString() + for _, resList := range res { + for _, r := range resList.APIResources { + if !r.Namespaced { + clusterResources.Insert(r.Kind) + } + } + } + return clusterResources, nil +} + +// Validate checks policy is valid +func Validate(ctx context.Context, logger logr.Logger, client dclient.Interface, policy kyvernov1alpha1.CleanupPolicyInterface) error { + clusteredResources, err := FetchClusteredResources(logger, client) + if err != nil { + return err + } + if err := validatePolicy(clusteredResources, policy); err != nil { + return err + } + if err := validateAuth(ctx, client, policy); err != nil { + return err + } + return nil +} + +// validatePolicy checks the policy and rules declarations for required configurations +func validatePolicy(clusterResources sets.String, policy kyvernov1alpha1.CleanupPolicyInterface) error { + errs := policy.Validate(clusterResources) + return errs.ToAggregate() +} + +// validateAuth checks the the delete action is allowed +func validateAuth(ctx context.Context, client dclient.Interface, policy kyvernov1alpha1.CleanupPolicyInterface) error { + namespace := policy.GetNamespace() + spec := policy.GetSpec() + kinds := sets.NewString(spec.MatchResources.GetKinds()...) + for kind := range kinds { + checker := auth.NewCanI(client, kind, namespace, "delete") + allowed, err := checker.RunAccessCheck(ctx) + if err != nil { + return err + } + if !allowed { + return fmt.Errorf("cleanup controller has no permission to delete kind %s", kind) + } + } + return nil +}