diff --git a/.gitignore b/.gitignore index 9366f4ac3..06000d4c4 100644 --- a/.gitignore +++ b/.gitignore @@ -16,3 +16,5 @@ *.DS_Store .AppleDouble .LSOverride + +.idea/* diff --git a/cmd/operator-verify/manifests/cmd.go b/cmd/operator-verify/manifests/cmd.go index fd40ee6fd..0f9694aa8 100644 --- a/cmd/operator-verify/manifests/cmd.go +++ b/cmd/operator-verify/manifests/cmd.go @@ -22,6 +22,7 @@ validation library.`, } rootCmd.Flags().Bool("operatorhub_validate", false, "enable optional UI validation for operatorhub.io") + rootCmd.Flags().Bool("object_validate", false, "enable optional bundle object validation") return rootCmd } @@ -40,10 +41,18 @@ func manifestsFunc(cmd *cobra.Command, args []string) { log.Fatalf("Unable to parse operatorhub_validate parameter") } + bundleObjectValidate, err := cmd.Flags().GetBool("object_validate") + if err != nil { + log.Fatalf("Unable to parse object_validate parameter: %w", err) + } + validators := validation.DefaultBundleValidators if operatorHubValidate { validators = validators.WithValidators(validation.OperatorHubValidator) } + if bundleObjectValidate { + validators = validators.WithValidators(validation.ObjectValidator) + } results := validators.Validate(bundle.ObjectsToValidate()...) nonEmptyResults := []errors.ManifestResult{} diff --git a/pkg/validation/errors/error.go b/pkg/validation/errors/error.go index 33e6f253c..4ff55c0dd 100644 --- a/pkg/validation/errors/error.go +++ b/pkg/validation/errors/error.go @@ -100,6 +100,7 @@ const ( ErrorInvalidManifestStructure ErrorType = "ManifestStructureNotValid" ErrorInvalidBundle ErrorType = "BundleNotValid" ErrorInvalidPackageManifest ErrorType = "PackageManifestNotValid" + ErrorObjectFailedValidation ErrorType = "ObjectFailedValidation" ) func NewError(t ErrorType, detail, field string, v interface{}) Error { @@ -230,3 +231,15 @@ func WarnInvalidOperation(detail string, value interface{}) Error { func invalidOperation(lvl Level, detail string, value interface{}) Error { return Error{ErrorInvalidOperation, lvl, "", value, detail} } + +func ErrInvalidObject(value interface{}, detail string) Error { + return invalidObject(LevelError, detail, value) +} + +func invalidObject(lvl Level, detail string, value interface{}) Error { + return Error{ErrorObjectFailedValidation, lvl, "", value, detail} +} + +func WarnInvalidObject(detail string, value interface{}) Error { + return failedValidation(LevelWarn, detail, value) +} diff --git a/pkg/validation/internal/object.go b/pkg/validation/internal/object.go new file mode 100644 index 000000000..4854bbb1c --- /dev/null +++ b/pkg/validation/internal/object.go @@ -0,0 +1,197 @@ +package internal + +import ( + "encoding/json" + "github.com/operator-framework/api/pkg/validation/errors" + interfaces "github.com/operator-framework/api/pkg/validation/interfaces" + + policyv1beta1 "k8s.io/api/policy/v1beta1" + rbacv1 "k8s.io/api/rbac/v1" + schedulingv1 "k8s.io/api/scheduling/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +var ObjectValidator interfaces.Validator = interfaces.ValidatorFunc(validateObjects) + +const ( + PodDisruptionBudgetKind = "PodDisruptionBudget" + PriorityClassKind = "PriorityClass" + RoleKind = "Role" + ClusterRoleKind = "ClusterRole" + PodDisruptionBudgetAPIGroup = "policy" + SCCAPIGroup = "security.openshift.io" +) + +// defaultSCCs is a map of the default Security Context Constraints present as of OpenShift 4.5. +// See https://docs.openshift.com/container-platform/4.5/authentication/managing-security-context-constraints.html#security-context-constraints-about_configuring-internal-oauth +var defaultSCCs = map[string]struct{}{ + "privileged": {}, + "restricted": {}, + "anyuid": {}, + "hostaccess": {}, + "hostmount-anyuid": {}, + "hostnetwork": {}, + "node-exporter": {}, + "nonroot": {}, +} + +func validateObjects(objs ...interface{}) (results []errors.ManifestResult) { + for _, obj := range objs { + switch u := obj.(type) { + case *unstructured.Unstructured: + switch u.GroupVersionKind().Kind { + case PodDisruptionBudgetKind: + results = append(results, validatePDB(u)) + case PriorityClassKind: + results = append(results, validatePriorityClass(u)) + case RoleKind: + results = append(results, validateRBAC(u)) + case ClusterRoleKind: + results = append(results, validateRBAC(u)) + } + } + } + return results +} + +// validatePDB checks the PDB to ensure the minimum and maximum budgets are set to reasonable levels. +// See https://github.com/operator-framework/operator-lifecycle-manager/blob/master/doc/design/adding-pod-disruption-budgets.md#limitations-on-pod-disruption-budgets +func validatePDB(u *unstructured.Unstructured) (result errors.ManifestResult) { + pdb := policyv1beta1.PodDisruptionBudget{} + + b, err := u.MarshalJSON() + if err != nil { + result.Add(errors.ErrInvalidParse("error converting unstructured", err)) + return + } + + err = json.Unmarshal(b, &pdb) + if err != nil { + result.Add(errors.ErrInvalidParse("error unmarshaling poddisruptionbudget", err)) + return + } + + /* + maxUnavailable field cannot be set to 0 or 0%. + minAvailable field cannot be set to 100%. + */ + + maxUnavailable := pdb.Spec.MaxUnavailable + if maxUnavailable != nil && (maxUnavailable.IntVal == 0 || maxUnavailable.StrVal == "0%") { + result.Add(errors.ErrInvalidObject(pdb, "maxUnavailable field cannot be set to 0 or 0%")) + } + + minAvailable := pdb.Spec.MinAvailable + if minAvailable != nil && minAvailable.StrVal == "100%" { + result.Add(errors.ErrInvalidObject(pdb, "minAvailable field cannot be set to 100%")) + } + + return +} + +// validatePriorityClass checks the PriorityClass object to ensure globalDefault is set to false. +// See https://github.com/operator-framework/operator-lifecycle-manager/blob/master/doc/design/adding-priority-classes.md +func validatePriorityClass(u *unstructured.Unstructured) (result errors.ManifestResult) { + pc := schedulingv1.PriorityClass{} + + b, err := u.MarshalJSON() + if err != nil { + result.Add(errors.ErrInvalidParse("error converting unstructured", err)) + return + } + + err = json.Unmarshal(b, &pc) + if err != nil { + result.Add(errors.ErrInvalidParse("error unmarshaling priorityclass", err)) + return + } + + if pc.GlobalDefault { + result.Add(errors.ErrInvalidObject(pc, "globalDefault field cannot be set to true")) + } + + return +} + +func validateRBAC(u *unstructured.Unstructured) (result errors.ManifestResult) { + var policyRules []rbacv1.PolicyRule + + b, err := u.MarshalJSON() + if err != nil { + result.Add(errors.ErrInvalidParse("error converting unstructured", err)) + return + } + + switch u.GroupVersionKind().Kind { + case RoleKind: + role := rbacv1.Role{} + err = json.Unmarshal(b, &role) + if err != nil { + result.Add(errors.ErrInvalidParse("error unmarshaling role", err)) + return + } + policyRules = role.Rules + case ClusterRoleKind: + clusterrole := rbacv1.ClusterRole{} + err = json.Unmarshal(b, &clusterrole) + if err != nil { + result.Add(errors.ErrInvalidParse("error unmarshaling clusterrole", err)) + return + } + policyRules = clusterrole.Rules + } + + return audit(policyRules) +} + +// audit checks the provided rbac policies against prescribed limitations. +// If permission is granted to create/modify a PDB, a warning is returned. +// If permission is granted to modify default SCCs in OpenShift, an error is returned. +func audit(policies []rbacv1.PolicyRule) (result errors.ManifestResult) { + // check for permission to modify/create PDBs + for _, rule := range policies { + if contains(rule.APIGroups, PodDisruptionBudgetAPIGroup) && + contains(rule.Resources, "poddisruptionbudgets") && + contains(rule.Verbs, rbacv1.VerbAll, "create", "update", "patch") { + result.Add(errors.WarnInvalidObject("RBAC includes permission to create/update poddisruptionbudgets, which could impact cluster stability", rule)) + } + } + + // check sccs for modifying default known SCCs + for _, rule := range policies { + if contains(rule.APIGroups, SCCAPIGroup) && + contains(rule.Resources, "securitycontextconstraints") && + contains(rule.Verbs, rbacv1.VerbAll, "delete", "update", "patch") && + containsDefaults(rule.ResourceNames, defaultSCCs) { + result.Add(errors.ErrInvalidObject(rule, "RBAC includes permission to modify default securitycontextconstraints, which could impact cluster stability")) + } + } + + return +} + +// contains returns true if at least one item is present in the array +func contains(slice []string, items ...string) bool { + set := make(map[string]struct{}, len(slice)) + for _, s := range slice { + set[s] = struct{}{} + } + + for _, item := range items { + if _, ok := set[item]; ok { + return true + } + } + + return false +} + +// containsDefaults returns true if at least one item is present as a key in the map +func containsDefaults(slice []string, defaults map[string]struct{}) bool { + for _, s := range slice { + if _, ok := defaults[s]; ok { + return true + } + } + return false +} diff --git a/pkg/validation/internal/object_test.go b/pkg/validation/internal/object_test.go new file mode 100644 index 000000000..170c2cc9d --- /dev/null +++ b/pkg/validation/internal/object_test.go @@ -0,0 +1,107 @@ +package internal + +import ( + "github.com/ghodss/yaml" + "io/ioutil" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "testing" +) + +func TestValidateObject(t *testing.T) { + var table = []struct { + description string + path string + error bool + warning bool + detail string + }{ + { + description: "valid PDB", + path: "./testdata/objects/valid_pdb.yaml", + }, + { + description: "invalid PDB - minAvailable set to 100%", + path: "./testdata/objects/invalid_pdb_minAvailable.yaml", + error: true, + detail: "minAvailable field cannot be set to 100%", + }, + { + description: "invalid PDB - maxUnavailable set to 0", + path: "./testdata/objects/invalid_pdb_maxUnavailable.yaml", + error: true, + detail: "maxUnavailable field cannot be set to 0 or 0%", + }, + { + description: "valid priorityclass", + path: "./testdata/objects/valid_priorityclass.yaml", + }, + { + description: "invalid priorityclass - global default set to true", + path: "./testdata/objects/invalid_priorityclass.yaml", + error: true, + detail: "globalDefault field cannot be set to true", + }, + { + description: "valid pdb role", + path: "./testdata/objects/valid_role_get_pdb.yaml", + }, + { + description: "invalid role - modify pdb", + path: "./testdata/objects/invalid_role_create_pdb.yaml", + warning: true, + detail: "RBAC includes permission to create/update poddisruptionbudgets, which could impact cluster stability", + }, + { + description: "valid scc role", + path: "./testdata/objects/valid_role_get_scc.yaml", + }, + { + description: "invalid scc role - modify default scc", + path: "./testdata/objects/invalid_role_modify_scc.yaml", + error: true, + detail: "RBAC includes permission to modify default securitycontextconstraints, which could impact cluster stability", + }, + } + + for _, tt := range table { + t.Log(tt.description) + + u := unstructured.Unstructured{} + o, err := ioutil.ReadFile(tt.path) + if err != nil { + t.Fatalf("reading yaml object file: %s", err) + } + if err := yaml.Unmarshal(o, &u); err != nil { + t.Fatalf("unmarshalling object at path %s: %v", tt.path, err) + } + + results := ObjectValidator.Validate(&u) + + // check errors + if len(results[0].Errors) > 0 && tt.error == false { + t.Fatalf("received errors %#v when no validation error expected for %s", results, tt.path) + } + if len(results[0].Errors) == 0 && tt.error == true { + t.Fatalf("received no errors when validation error expected for %s", tt.path) + } + if len(results[0].Errors) > 0 { + if results[0].Errors[0].Detail != tt.detail { + t.Fatalf("expected validation error detail %s, got %s", tt.detail, results[0].Errors[0].Detail) + } + } + + // check warnings + if len(results[0].Warnings) > 0 && tt.warning == false { + t.Fatalf("received errors %#v when no validation warning expected for %s", results, tt.path) + } + if len(results[0].Warnings) == 0 && tt.warning == true { + t.Fatalf("received no errors when validation warning expected for %s", tt.path) + } + if len(results[0].Warnings) > 0 { + if results[0].Warnings[0].Detail != tt.detail { + t.Fatalf("expected validation warning detail %s, got %s", tt.detail, results[0].Warnings[0].Detail) + } + } + } + +} diff --git a/pkg/validation/internal/testdata/objects/invalid_pdb_maxUnavailable.yaml b/pkg/validation/internal/testdata/objects/invalid_pdb_maxUnavailable.yaml new file mode 100644 index 000000000..94586ef91 --- /dev/null +++ b/pkg/validation/internal/testdata/objects/invalid_pdb_maxUnavailable.yaml @@ -0,0 +1,6 @@ +apiVersion: policy/v1beta1 +kind: PodDisruptionBudget +metadata: + name: busybox-pdb +spec: + maxUnavailable: 0 diff --git a/pkg/validation/internal/testdata/objects/invalid_pdb_minAvailable.yaml b/pkg/validation/internal/testdata/objects/invalid_pdb_minAvailable.yaml new file mode 100644 index 000000000..f6051696e --- /dev/null +++ b/pkg/validation/internal/testdata/objects/invalid_pdb_minAvailable.yaml @@ -0,0 +1,6 @@ +apiVersion: policy/v1beta1 +kind: PodDisruptionBudget +metadata: + name: busybox-pdb +spec: + minAvailable: 100% diff --git a/pkg/validation/internal/testdata/objects/invalid_priorityclass.yaml b/pkg/validation/internal/testdata/objects/invalid_priorityclass.yaml new file mode 100644 index 000000000..f20bbcd53 --- /dev/null +++ b/pkg/validation/internal/testdata/objects/invalid_priorityclass.yaml @@ -0,0 +1,6 @@ +apiVersion: scheduling.k8s.io/v1 +kind: PriorityClass +metadata: + name: super-priority +value: 1000 +globalDefault: true diff --git a/pkg/validation/internal/testdata/objects/invalid_role_create_pdb.yaml b/pkg/validation/internal/testdata/objects/invalid_role_create_pdb.yaml new file mode 100644 index 000000000..5c72caf88 --- /dev/null +++ b/pkg/validation/internal/testdata/objects/invalid_role_create_pdb.yaml @@ -0,0 +1,9 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + namespace: default + name: pdb-modifier +rules: + - apiGroups: ["policy"] + resources: ["poddisruptionbudgets"] + verbs: ["create", "list"] diff --git a/pkg/validation/internal/testdata/objects/invalid_role_modify_scc.yaml b/pkg/validation/internal/testdata/objects/invalid_role_modify_scc.yaml new file mode 100644 index 000000000..532f5e21b --- /dev/null +++ b/pkg/validation/internal/testdata/objects/invalid_role_modify_scc.yaml @@ -0,0 +1,14 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: invalid-scc + namespace: namespace +rules: + - apiGroups: + - security.openshift.io + resourceNames: + - privileged + resources: + - securitycontextconstraints + verbs: + - update diff --git a/pkg/validation/internal/testdata/objects/valid_pdb.yaml b/pkg/validation/internal/testdata/objects/valid_pdb.yaml new file mode 100644 index 000000000..bdb699b2a --- /dev/null +++ b/pkg/validation/internal/testdata/objects/valid_pdb.yaml @@ -0,0 +1,9 @@ +apiVersion: policy/v1beta1 +kind: PodDisruptionBudget +metadata: + name: busybox-pdb +spec: + minAvailable: 2 + selector: + matchLabels: + app: busybox diff --git a/pkg/validation/internal/testdata/objects/valid_priorityclass.yaml b/pkg/validation/internal/testdata/objects/valid_priorityclass.yaml new file mode 100644 index 000000000..39e0f8d5a --- /dev/null +++ b/pkg/validation/internal/testdata/objects/valid_priorityclass.yaml @@ -0,0 +1,6 @@ +apiVersion: scheduling.k8s.io/v1 +kind: PriorityClass +metadata: + name: super-priority +value: 1000 +globalDefault: false diff --git a/pkg/validation/internal/testdata/objects/valid_role_get_pdb.yaml b/pkg/validation/internal/testdata/objects/valid_role_get_pdb.yaml new file mode 100644 index 000000000..8bc7305fd --- /dev/null +++ b/pkg/validation/internal/testdata/objects/valid_role_get_pdb.yaml @@ -0,0 +1,9 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + namespace: default + name: pdb-reader +rules: + - apiGroups: ["policy"] + resources: ["poddisruptionbudgets"] + verbs: ["get", "list"] diff --git a/pkg/validation/internal/testdata/objects/valid_role_get_scc.yaml b/pkg/validation/internal/testdata/objects/valid_role_get_scc.yaml new file mode 100644 index 000000000..c0f748fa2 --- /dev/null +++ b/pkg/validation/internal/testdata/objects/valid_role_get_scc.yaml @@ -0,0 +1,14 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: role-name + namespace: namespace +rules: + - apiGroups: + - security.openshift.io + resourceNames: + - scc-name + resources: + - securitycontextconstraints + verbs: + - use diff --git a/pkg/validation/validation.go b/pkg/validation/validation.go index 656e4d7da..30f782404 100644 --- a/pkg/validation/validation.go +++ b/pkg/validation/validation.go @@ -31,6 +31,10 @@ var BundleValidator = internal.BundleValidator // for OperatorHub.io requirements. var OperatorHubValidator = internal.OperatorHubValidator +// Object Validator validates various custom objects in the bundle like PDBs and SCCs. +// Object validation is optional and not a default-level validation. +var ObjectValidator = internal.ObjectValidator + // AllValidators implements Validator to validate all Operator manifest types. var AllValidators = interfaces.Validators{ PackageManifestValidator, @@ -38,6 +42,7 @@ var AllValidators = interfaces.Validators{ CustomResourceDefinitionValidator, BundleValidator, OperatorHubValidator, + ObjectValidator, } var DefaultBundleValidators = interfaces.Validators{