From 8f15a58e8c9fcebeefa731c2cede74d14fb74b5f Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Fri, 27 Jul 2018 14:42:43 -0400 Subject: [PATCH] Validate IAM additionalPolicies We now validate them with the cluster, so we should give early and clear feedback if the IAM policy is not valid. --- pkg/apis/kops/instancegroup.go | 1 + pkg/apis/kops/validation/BUILD.bazel | 1 + pkg/apis/kops/validation/validation.go | 46 ++++++++++++++++++- pkg/apis/kops/validation/validation_test.go | 50 +++++++++++++++++++++ pkg/model/iam.go | 5 +-- pkg/model/iam/BUILD.bazel | 5 ++- pkg/model/iam/types.go | 31 +++++++++++++ 7 files changed, 134 insertions(+), 5 deletions(-) create mode 100644 pkg/model/iam/types.go diff --git a/pkg/apis/kops/instancegroup.go b/pkg/apis/kops/instancegroup.go index a86bd17306..2b55e9da0d 100644 --- a/pkg/apis/kops/instancegroup.go +++ b/pkg/apis/kops/instancegroup.go @@ -60,6 +60,7 @@ const ( InstanceGroupRoleBastion InstanceGroupRole = "Bastion" ) +// AllInstanceGroupRoles is a slice of all valid InstanceGroupRole values var AllInstanceGroupRoles = []InstanceGroupRole{ InstanceGroupRoleNode, InstanceGroupRoleMaster, diff --git a/pkg/apis/kops/validation/BUILD.bazel b/pkg/apis/kops/validation/BUILD.bazel index a3098cfebc..086f9b8698 100644 --- a/pkg/apis/kops/validation/BUILD.bazel +++ b/pkg/apis/kops/validation/BUILD.bazel @@ -18,6 +18,7 @@ go_library( "//pkg/apis/kops/util:go_default_library", "//pkg/featureflag:go_default_library", "//pkg/model/components:go_default_library", + "//pkg/model/iam:go_default_library", "//upup/pkg/fi:go_default_library", "//upup/pkg/fi/cloudup/awsup:go_default_library", "//vendor/github.com/blang/semver:go_default_library", diff --git a/pkg/apis/kops/validation/validation.go b/pkg/apis/kops/validation/validation.go index 4433e5e9ed..9b01e81534 100644 --- a/pkg/apis/kops/validation/validation.go +++ b/pkg/apis/kops/validation/validation.go @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/kops/pkg/apis/kops" + "k8s.io/kops/pkg/model/iam" ) var validDockerConfigStorageValues = []string{"aufs", "btrfs", "devicemapper", "overlay", "overlay2", "zfs"} @@ -54,7 +55,7 @@ func newValidateCluster(cluster *kops.Cluster) field.ErrorList { func validateClusterSpec(spec *kops.ClusterSpec, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - allErrs = append(allErrs, validateSubnets(spec.Subnets, field.NewPath("spec"))...) + allErrs = append(allErrs, validateSubnets(spec.Subnets, fieldPath.Child("subnets"))...) // SSHAccess for i, cidr := range spec.SSHAccess { @@ -95,6 +96,13 @@ func validateClusterSpec(spec *kops.ClusterSpec, fieldPath *field.Path) field.Er allErrs = append(allErrs, validateNetworking(spec.Networking, fieldPath.Child("networking"))...) } + // IAM additionalPolicies + if spec.AdditionalPolicies != nil { + for k, v := range *spec.AdditionalPolicies { + allErrs = append(allErrs, validateAdditionalPolicy(k, v, fieldPath.Child("additionalPolicies"))...) + } + } + return allErrs } @@ -250,3 +258,39 @@ func validateNetworkingFlannel(v *kops.FlannelNetworkingSpec, fldPath *field.Pat return allErrs } + +func validateAdditionalPolicy(role string, policy string, fldPath *field.Path) field.ErrorList { + errs := field.ErrorList{} + + valid := sets.NewString() + for _, r := range kops.AllInstanceGroupRoles { + k := strings.ToLower(string(r)) + valid.Insert(k) + } + if !valid.Has(role) { + message := fmt.Sprintf("role is not known (valid values: %s)", strings.Join(valid.List(), ",")) + errs = append(errs, field.Invalid(fldPath, role, message)) + } + + statements, err := iam.ParseStatements(policy) + if err != nil { + errs = append(errs, field.Invalid(fldPath.Key(role), policy, "policy was not valid JSON: "+err.Error())) + } + + // Trivial validation of policy, mostly to make sure it isn't some other random object + for i, statement := range statements { + fldEffect := fldPath.Key(role).Index(i).Child("Effect") + switch statement.Effect { + case "Allow", "Deny": + //valid + + case "": + errs = append(errs, field.Required(fldEffect, "Effect must be specified for IAM policy")) + + default: + errs = append(errs, field.Invalid(fldEffect, statement.Effect, "Effect must be 'Allow' or 'Deny'")) + } + } + + return errs +} diff --git a/pkg/apis/kops/validation/validation_test.go b/pkg/apis/kops/validation/validation_test.go index bd759552f6..24c264ee82 100644 --- a/pkg/apis/kops/validation/validation_test.go +++ b/pkg/apis/kops/validation/validation_test.go @@ -241,3 +241,53 @@ func Test_Validate_Networking_Flannel(t *testing.T) { testErrors(t, g.Input, errs, g.ExpectedErrors) } } + +func Test_Validate_AdditionalPolicies(t *testing.T) { + grid := []struct { + Input map[string]string + ExpectedErrors []string + }{ + { + Input: map[string]string{}, + }, + { + Input: map[string]string{ + "master": `[ { "Action": [ "s3:GetObject" ], "Resource": [ "*" ], "Effect": "Allow" } ]`, + }, + }, + { + Input: map[string]string{ + "notarole": `[ { "Action": [ "s3:GetObject" ], "Resource": [ "*" ], "Effect": "Allow" } ]`, + }, + ExpectedErrors: []string{"Invalid value::spec.additionalPolicies"}, + }, + { + Input: map[string]string{ + "master": `badjson`, + }, + ExpectedErrors: []string{"Invalid value::spec.additionalPolicies[master]"}, + }, + { + Input: map[string]string{ + "master": `[ { "Action": [ "s3:GetObject" ], "Resource": [ "*" ] } ]`, + }, + ExpectedErrors: []string{"Required value::spec.additionalPolicies[master][0].Effect"}, + }, + { + Input: map[string]string{ + "master": `[ { "Action": [ "s3:GetObject" ], "Resource": [ "*" ], "Effect": "allow" } ]`, + }, + ExpectedErrors: []string{"Invalid value::spec.additionalPolicies[master][0].Effect"}, + }, + } + for _, g := range grid { + clusterSpec := &kops.ClusterSpec{ + AdditionalPolicies: &g.Input, + Subnets: []kops.ClusterSubnetSpec{ + {Name: "subnet1"}, + }, + } + errs := validateClusterSpec(clusterSpec, field.NewPath("spec")) + testErrors(t, g.Input, errs, g.ExpectedErrors) + } +} diff --git a/pkg/model/iam.go b/pkg/model/iam.go index 777dbd84fc..ce3ad7b822 100644 --- a/pkg/model/iam.go +++ b/pkg/model/iam.go @@ -17,7 +17,6 @@ limitations under the License. package model import ( - "encoding/json" "fmt" "strings" "text/template" @@ -186,8 +185,8 @@ func (b *IAMModelBuilder) buildIAMTasks(igRole kops.InstanceGroupRole, iamName s Version: iam.PolicyDefaultVersion, } - statements := make([]*iam.Statement, 0) - if err := json.Unmarshal([]byte(additionalPolicy), &statements); err != nil { + statements, err := iam.ParseStatements(additionalPolicy) + if err != nil { return fmt.Errorf("additionalPolicy %q is invalid: %v", strings.ToLower(string(igRole)), err) } diff --git a/pkg/model/iam/BUILD.bazel b/pkg/model/iam/BUILD.bazel index f4da6b4606..0b57a12ee6 100644 --- a/pkg/model/iam/BUILD.bazel +++ b/pkg/model/iam/BUILD.bazel @@ -2,7 +2,10 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "go_default_library", - srcs = ["iam_builder.go"], + srcs = [ + "iam_builder.go", + "types.go", + ], importpath = "k8s.io/kops/pkg/model/iam", visibility = ["//visibility:public"], deps = [ diff --git a/pkg/model/iam/types.go b/pkg/model/iam/types.go new file mode 100644 index 0000000000..96d711bacf --- /dev/null +++ b/pkg/model/iam/types.go @@ -0,0 +1,31 @@ +/* +Copyright 2018 The Kubernetes 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 iam + +import ( + "encoding/json" + "fmt" +) + +// ParseStatements parses JSON into a list of Statements +func ParseStatements(policy string) ([]*Statement, error) { + statements := make([]*Statement, 0) + if err := json.Unmarshal([]byte(policy), &statements); err != nil { + return nil, fmt.Errorf("error parsing IAM statements: %v", err) + } + return statements, nil +}