Merge pull request #3210 from KashifSaadat/feature-gate-strict-iam-policies

Automatic merge from submit-queue

Allow the strict IAM policies to be optional

The stricter IAM policies could potentially cause regression for some edge-cases, or may rely on nodeup image changes that haven't yet been deployed / tagged officially (currently the case on master branch since PR https://github.com/kubernetes/kops/pull/3158 was merged in).

This PR just wraps the new IAM policy rules around a cluster spec flag, `EnableStrictIAM`, so will default to the original behaviour (where the S3 policies were completely open). Could also be used to wrap PR https://github.com/kubernetes/kops/pull/3186 if it progresses any further.

- Or we could reject this and have the policies always strict! :)

cmd/kops/create_cluster.go

@@ -833,6 +833,10 @@ func RunCreateCluster(f *util.Factory, out io.Writer, c *CreateClusterOptions) e
 		}
 	}
 
+	cluster.Spec.IAM = &api.IAMSpec{
+		Legacy: false,
+	}
+
 	sshPublicKeys := make(map[string][]byte)
 	if c.SSHPublicKey != "" {
 		c.SSHPublicKey = utils.ExpandPath(c.SSHPublicKey)

cmd/kops/create_cluster_integration_test.go

@@ -18,17 +18,19 @@ package main
 
 import (
 	"bytes"
-	"github.com/golang/glog"
 	"io/ioutil"
+	"path"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/golang/glog"
+
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/kops/cmd/kops/util"
 	"k8s.io/kops/pkg/apis/kops"
 	"k8s.io/kops/pkg/diff"
 	"k8s.io/kops/pkg/testutils"
-	"path"
-	"strings"
-	"testing"
-	"time"
 )
 
 var MagicTimestamp = metav1.Time{Time: time.Date(2017, 1, 1, 0, 0, 0, 0, time.UTC)}
@@ -182,7 +184,7 @@ func runCreateClusterIntegrationTest(t *testing.T, srcDir string, version string
 		diffString := diff.FormatDiff(expectedYAML, actualYAML)
 		t.Logf("diff:\n%s\n", diffString)
 
-		t.Fatalf("YAML differed from expected")
+		t.Fatalf("YAML differed from expected (%s)", path.Join(srcDir, expectedClusterPath))
 	}
 
 }

pkg/apis/kops/cluster.go

@@ -143,6 +143,9 @@ type ClusterSpec struct {
 	// This API component is under construction, will remove this comment
 	// once this API is fully functional.
 	Assets *Assets `json:"assets,omitempty"`
+
+	// IAM field adds control over the IAM security policies applied to resources
+	IAM *IAMSpec `json:"iam,omitempty"`
 }
 
 // FileAssetSpec defines the structure for a file asset
@@ -164,6 +167,11 @@ type Assets struct {
 	FileRepository    *string `json:"fileRepository,omitempty"`
 }
 
+// IAMSpec adds control over the IAM security policies applied to resources
+type IAMSpec struct {
+	Legacy bool `json:"legacy"`
+}
+
 // HookSpec is a definition hook
 type HookSpec struct {
 	// Name is an optional name for the hook, otherwise the name is kops-hook-<index>

pkg/apis/kops/v1alpha1/cluster.go

@@ -252,6 +252,9 @@ type ClusterSpec struct {
 
 	// Alternative locations for files and containers
 	Assets *Assets `json:"assets,omitempty"`
+
+	// IAM field adds control over the IAM security policies applied to resources
+	IAM *IAMSpec `json:"iam,omitempty"`
 }
 
 // FileAssetSpec defines the structure for a file asset
@@ -273,6 +276,11 @@ type Assets struct {
 	FileRepository    *string `json:"fileRepository,omitempty"`
 }
 
+// IAMSpec adds control over the IAM security policies applied to resources
+type IAMSpec struct {
+	Legacy bool `json:"legacy"`
+}
+
 // HookSpec is a definition hook
 type HookSpec struct {
 	// Name is an optional name for the hook, otherwise the name is kops-hook-<index>

pkg/apis/kops/v1alpha1/defaults.go

@@ -75,4 +75,10 @@ func SetDefaults_ClusterSpec(obj *ClusterSpec) {
 		obj.Authorization.AlwaysAllow = &AlwaysAllowAuthorizationSpec{}
 	}
 
+	if obj.IAM == nil {
+		obj.IAM = &IAMSpec{
+			Legacy: true,
+		}
+	}
+
 }

pkg/apis/kops/v1alpha1/zz_generated.conversion.go

@@ -91,6 +91,8 @@ func RegisterConversions(scheme *runtime.Scheme) error {
 		Convert_kops_HTTPProxy_To_v1alpha1_HTTPProxy,
 		Convert_v1alpha1_HookSpec_To_kops_HookSpec,
 		Convert_kops_HookSpec_To_v1alpha1_HookSpec,
+		Convert_v1alpha1_IAMSpec_To_kops_IAMSpec,
+		Convert_kops_IAMSpec_To_v1alpha1_IAMSpec,
 		Convert_v1alpha1_InstanceGroup_To_kops_InstanceGroup,
 		Convert_kops_InstanceGroup_To_v1alpha1_InstanceGroup,
 		Convert_v1alpha1_InstanceGroupList_To_kops_InstanceGroupList,
@@ -700,6 +702,15 @@ func autoConvert_v1alpha1_ClusterSpec_To_kops_ClusterSpec(in *ClusterSpec, out *
 	} else {
 		out.Assets = nil
 	}
+	if in.IAM != nil {
+		in, out := &in.IAM, &out.IAM
+		*out = new(kops.IAMSpec)
+		if err := Convert_v1alpha1_IAMSpec_To_kops_IAMSpec(*in, *out, s); err != nil {
+			return err
+		}
+	} else {
+		out.IAM = nil
+	}
 	return nil
 }
 
@@ -905,6 +916,15 @@ func autoConvert_kops_ClusterSpec_To_v1alpha1_ClusterSpec(in *kops.ClusterSpec,
 	} else {
 		out.Assets = nil
 	}
+	if in.IAM != nil {
+		in, out := &in.IAM, &out.IAM
+		*out = new(IAMSpec)
+		if err := Convert_kops_IAMSpec_To_v1alpha1_IAMSpec(*in, *out, s); err != nil {
+			return err
+		}
+	} else {
+		out.IAM = nil
+	}
 	return nil
 }
 
@@ -1368,6 +1388,26 @@ func Convert_kops_HookSpec_To_v1alpha1_HookSpec(in *kops.HookSpec, out *HookSpec
 	return autoConvert_kops_HookSpec_To_v1alpha1_HookSpec(in, out, s)
 }
 
+func autoConvert_v1alpha1_IAMSpec_To_kops_IAMSpec(in *IAMSpec, out *kops.IAMSpec, s conversion.Scope) error {
+	out.Legacy = in.Legacy
+	return nil
+}
+
+// Convert_v1alpha1_IAMSpec_To_kops_IAMSpec is an autogenerated conversion function.
+func Convert_v1alpha1_IAMSpec_To_kops_IAMSpec(in *IAMSpec, out *kops.IAMSpec, s conversion.Scope) error {
+	return autoConvert_v1alpha1_IAMSpec_To_kops_IAMSpec(in, out, s)
+}
+
+func autoConvert_kops_IAMSpec_To_v1alpha1_IAMSpec(in *kops.IAMSpec, out *IAMSpec, s conversion.Scope) error {
+	out.Legacy = in.Legacy
+	return nil
+}
+
+// Convert_kops_IAMSpec_To_v1alpha1_IAMSpec is an autogenerated conversion function.
+func Convert_kops_IAMSpec_To_v1alpha1_IAMSpec(in *kops.IAMSpec, out *IAMSpec, s conversion.Scope) error {
+	return autoConvert_kops_IAMSpec_To_v1alpha1_IAMSpec(in, out, s)
+}
+
 func autoConvert_v1alpha1_InstanceGroup_To_kops_InstanceGroup(in *InstanceGroup, out *kops.InstanceGroup, s conversion.Scope) error {
 	out.ObjectMeta = in.ObjectMeta
 	if err := Convert_v1alpha1_InstanceGroupSpec_To_kops_InstanceGroupSpec(&in.Spec, &out.Spec, s); err != nil {

pkg/apis/kops/v1alpha2/cluster.go

@@ -168,6 +168,9 @@ type ClusterSpec struct {
 	Hooks []HookSpec `json:"hooks,omitempty"`
 	// Alternative locations for files and containers
 	Assets *Assets `json:"assets,omitempty"`
+
+	// IAM field adds control over the IAM security policies applied to resources
+	IAM *IAMSpec `json:"iam,omitempty"`
 }
 
 // FileAssetSpec defines the structure for a file asset
@@ -189,6 +192,11 @@ type Assets struct {
 	FileRepository    *string `json:"fileRepository,omitempty"`
 }
 
+// IAMSpec adds control over the IAM security policies applied to resources
+type IAMSpec struct {
+	Legacy bool `json:"legacy"`
+}
+
 // HookSpec is a definition hook
 type HookSpec struct {
 	// Name is an optional name for the hook, otherwise the name is kops-hook-<index>

pkg/apis/kops/v1alpha2/defaults.go

@@ -74,4 +74,10 @@ func SetDefaults_ClusterSpec(obj *ClusterSpec) {
 		// Before the Authorization field was introduced, the behaviour was alwaysAllow
 		obj.Authorization.AlwaysAllow = &AlwaysAllowAuthorizationSpec{}
 	}
+
+	if obj.IAM == nil {
+		obj.IAM = &IAMSpec{
+			Legacy: true,
+		}
+	}
 }

pkg/apis/kops/v1alpha2/zz_generated.conversion.go

@@ -95,6 +95,8 @@ func RegisterConversions(scheme *runtime.Scheme) error {
 		Convert_kops_HTTPProxy_To_v1alpha2_HTTPProxy,
 		Convert_v1alpha2_HookSpec_To_kops_HookSpec,
 		Convert_kops_HookSpec_To_v1alpha2_HookSpec,
+		Convert_v1alpha2_IAMSpec_To_kops_IAMSpec,
+		Convert_kops_IAMSpec_To_v1alpha2_IAMSpec,
 		Convert_v1alpha2_InstanceGroup_To_kops_InstanceGroup,
 		Convert_kops_InstanceGroup_To_v1alpha2_InstanceGroup,
 		Convert_v1alpha2_InstanceGroupList_To_kops_InstanceGroupList,
@@ -738,6 +740,15 @@ func autoConvert_v1alpha2_ClusterSpec_To_kops_ClusterSpec(in *ClusterSpec, out *
 	} else {
 		out.Assets = nil
 	}
+	if in.IAM != nil {
+		in, out := &in.IAM, &out.IAM
+		*out = new(kops.IAMSpec)
+		if err := Convert_v1alpha2_IAMSpec_To_kops_IAMSpec(*in, *out, s); err != nil {
+			return err
+		}
+	} else {
+		out.IAM = nil
+	}
 	return nil
 }
 
@@ -958,6 +969,15 @@ func autoConvert_kops_ClusterSpec_To_v1alpha2_ClusterSpec(in *kops.ClusterSpec,
 	} else {
 		out.Assets = nil
 	}
+	if in.IAM != nil {
+		in, out := &in.IAM, &out.IAM
+		*out = new(IAMSpec)
+		if err := Convert_kops_IAMSpec_To_v1alpha2_IAMSpec(*in, *out, s); err != nil {
+			return err
+		}
+	} else {
+		out.IAM = nil
+	}
 	return nil
 }
 
@@ -1466,6 +1486,26 @@ func Convert_kops_HookSpec_To_v1alpha2_HookSpec(in *kops.HookSpec, out *HookSpec
 	return autoConvert_kops_HookSpec_To_v1alpha2_HookSpec(in, out, s)
 }
 
+func autoConvert_v1alpha2_IAMSpec_To_kops_IAMSpec(in *IAMSpec, out *kops.IAMSpec, s conversion.Scope) error {
+	out.Legacy = in.Legacy
+	return nil
+}
+
+// Convert_v1alpha2_IAMSpec_To_kops_IAMSpec is an autogenerated conversion function.
+func Convert_v1alpha2_IAMSpec_To_kops_IAMSpec(in *IAMSpec, out *kops.IAMSpec, s conversion.Scope) error {
+	return autoConvert_v1alpha2_IAMSpec_To_kops_IAMSpec(in, out, s)
+}
+
+func autoConvert_kops_IAMSpec_To_v1alpha2_IAMSpec(in *kops.IAMSpec, out *IAMSpec, s conversion.Scope) error {
+	out.Legacy = in.Legacy
+	return nil
+}
+
+// Convert_kops_IAMSpec_To_v1alpha2_IAMSpec is an autogenerated conversion function.
+func Convert_kops_IAMSpec_To_v1alpha2_IAMSpec(in *kops.IAMSpec, out *IAMSpec, s conversion.Scope) error {
+	return autoConvert_kops_IAMSpec_To_v1alpha2_IAMSpec(in, out, s)
+}
+
 func autoConvert_v1alpha2_InstanceGroup_To_kops_InstanceGroup(in *InstanceGroup, out *kops.InstanceGroup, s conversion.Scope) error {
 	out.ObjectMeta = in.ObjectMeta
 	if err := Convert_v1alpha2_InstanceGroupSpec_To_kops_InstanceGroupSpec(&in.Spec, &out.Spec, s); err != nil {

pkg/model/iam/iam_builder.go

@@ -78,11 +78,18 @@ type IAMPolicyBuilder struct {
 	HostedZoneID string
 }
 
+// BuildAWSIAMPolicy builds a set of IAM policy statements based on the
+// instance group type and IAM Strict setting within the Cluster Spec
 func (b *IAMPolicyBuilder) BuildAWSIAMPolicy() (*IAMPolicy, error) {
 	wildcard := stringorslice.Slice([]string{"*"})
-
 	iamPrefix := b.IAMPrefix()
 
+	// The Legacy IAM setting deploys an open policy (prior to the hardening PRs)
+	legacyIAM := false
+	if b.Cluster.Spec.IAM != nil {
+		legacyIAM = b.Cluster.Spec.IAM.Legacy
+	}
+
 	p := &IAMPolicy{
 		Version: IAMPolicyDefaultVersion,
 	}
@@ -231,7 +238,7 @@ func (b *IAMPolicyBuilder) BuildAWSIAMPolicy() (*IAMPolicy, error) {
 		}
 
 		if s3Path, ok := vfsPath.(*vfs.S3Path); ok {
-			addS3Permissions(p, iamPrefix, s3Path, b.Role)
+			addS3Permissions(p, iamPrefix, s3Path, b.Role, legacyIAM)
 		} else if _, ok := vfsPath.(*vfs.MemFSPath); ok {
 			// Tests -ignore - nothing we can do in terms of IAM policy
 			glog.Warningf("ignoring memfs path %q for IAM policy builder", vfsPath)
@@ -275,7 +282,7 @@ func addRoute53ListHostedZonesPermission(p *IAMPolicy) {
 
 // addS3Permissions updates the IAM Policy with statements granting tailored
 // access to S3 assets, depending on the instance role
-func addS3Permissions(p *IAMPolicy, iamPrefix string, s3Path *vfs.S3Path, role api.InstanceGroupRole) {
+func addS3Permissions(p *IAMPolicy, iamPrefix string, s3Path *vfs.S3Path, role api.InstanceGroupRole, legacyIAM bool) {
 	// Note that the config store may itself be a subdirectory of a bucket
 	iamS3Path := s3Path.Bucket() + "/" + s3Path.Key()
 	iamS3Path = strings.TrimSuffix(iamS3Path, "/")
@@ -297,6 +304,17 @@ func addS3Permissions(p *IAMPolicy, iamPrefix string, s3Path *vfs.S3Path, role a
 		),
 	})
 
+	if legacyIAM {
+		if role == api.InstanceGroupRoleMaster || role == api.InstanceGroupRoleNode {
+			p.Statement = append(p.Statement, &IAMStatement{
+				Effect: IAMStatementEffectAllow,
+				Action: stringorslice.Slice([]string{"s3:*"}),
+				Resource: stringorslice.Of(
+					strings.Join([]string{iamPrefix, ":s3:::", iamS3Path, "/*"}, ""),
+				),
+			})
+		}
+	} else {
 		if role == api.InstanceGroupRoleMaster {
 			p.Statement = append(p.Statement, &IAMStatement{
 				Effect: IAMStatementEffectAllow,
@@ -323,6 +341,7 @@ func addS3Permissions(p *IAMPolicy, iamPrefix string, s3Path *vfs.S3Path, role a
 			})
 		}
 	}
+}
 
 // IAMPrefix returns the prefix for AWS ARNs in the current region, for use with IAM
 // it is arn:aws everywhere but in cn-north and us-gov-west-1

pkg/model/iam/iam_builder_test.go

@@ -97,10 +97,12 @@ func TestS3PolicyGeneration(t *testing.T) {
 
 	grid := []struct {
 		Role      kops.InstanceGroupRole
+		LegacyIAM bool
 		IAMPolicy IAMPolicy
 	}{
 		{
 			Role:      "Master",
+			LegacyIAM: false,
 			IAMPolicy: IAMPolicy{
 				Statement: append(defaultS3Statements, &IAMStatement{
 					Effect: IAMStatementEffectAllow,
@@ -113,8 +115,24 @@ func TestS3PolicyGeneration(t *testing.T) {
 				}),
 			},
 		},
+		{
+			Role:      "Master",
+			LegacyIAM: true,
+			IAMPolicy: IAMPolicy{
+				Statement: append(defaultS3Statements, &IAMStatement{
+					Effect: IAMStatementEffectAllow,
+					Action: stringorslice.Slice([]string{
+						"s3:*",
+					}),
+					Resource: stringorslice.Of(
+						"arn:aws:s3:::bucket-name/cluster-name.k8s.local/*",
+					),
+				}),
+			},
+		},
 		{
 			Role:      "Node",
+			LegacyIAM: false,
 			IAMPolicy: IAMPolicy{
 				Statement: append(defaultS3Statements, &IAMStatement{
 					Effect: IAMStatementEffectAllow,
@@ -135,8 +153,31 @@ func TestS3PolicyGeneration(t *testing.T) {
 				}),
 			},
 		},
+		{
+			Role:      "Node",
+			LegacyIAM: true,
+			IAMPolicy: IAMPolicy{
+				Statement: append(defaultS3Statements, &IAMStatement{
+					Effect: IAMStatementEffectAllow,
+					Action: stringorslice.Slice([]string{
+						"s3:*",
+					}),
+					Resource: stringorslice.Of(
+						"arn:aws:s3:::bucket-name/cluster-name.k8s.local/*",
+					),
+				}),
+			},
+		},
 		{
 			Role:      "Bastion",
+			LegacyIAM: false,
+			IAMPolicy: IAMPolicy{
+				Statement: defaultS3Statements,
+			},
+		},
+		{
+			Role:      "Bastion",
+			LegacyIAM: true,
 			IAMPolicy: IAMPolicy{
 				Statement: defaultS3Statements,
 			},
@@ -157,7 +198,7 @@ func TestS3PolicyGeneration(t *testing.T) {
 			continue
 		}
 
-		addS3Permissions(ip, "arn:aws", s3Path, x.Role)
+		addS3Permissions(ip, "arn:aws", s3Path, x.Role, x.LegacyIAM)
 
 		expectedPolicy, err := x.IAMPolicy.AsJSON()
 		if err != nil {

tests/integration/create_cluster/complex/expected-v1alpha2.yaml

@@ -20,6 +20,8 @@ spec:
     - instanceGroup: master-us-test-1a
       name: a
     name: events
+  iam:
+    legacy: false
   kubernetesApiAccess:
   - 0.0.0.0/0
   kubernetesVersion: v1.7.1

tests/integration/create_cluster/ha/expected-v1alpha1.yaml

@@ -30,6 +30,8 @@ spec:
     - name: c
       zone: us-test-1c
     name: events
+  iam:
+    legacy: false
   kubernetesVersion: v1.6.0-alpha.3
   masterPublicName: api.ha.example.com
   networkCIDR: 172.20.0.0/16

tests/integration/create_cluster/ha/expected-v1alpha2.yaml

@@ -28,6 +28,8 @@ spec:
     - instanceGroup: master-us-test-1c
       name: c
     name: events
+  iam:
+    legacy: false
   kubernetesApiAccess:
   - 0.0.0.0/0
   kubernetesVersion: v1.6.0-alpha.3

tests/integration/create_cluster/ha_encrypt/expected-v1alpha1.yaml

@@ -36,6 +36,8 @@ spec:
       name: c
       zone: us-test-1c
     name: events
+  iam:
+    legacy: false
   kubernetesVersion: v1.6.0-alpha.3
   masterPublicName: api.ha.example.com
   networkCIDR: 172.20.0.0/16

tests/integration/create_cluster/ha_encrypt/expected-v1alpha2.yaml

@@ -34,6 +34,8 @@ spec:
       instanceGroup: master-us-test-1c
       name: c
     name: events
+  iam:
+    legacy: false
   kubernetesApiAccess:
   - 0.0.0.0/0
   kubernetesVersion: v1.6.0-alpha.3

tests/integration/create_cluster/ha_shared_zones/expected-v1alpha2.yaml

@@ -36,6 +36,8 @@ spec:
     - instanceGroup: master-us-test-1a-3
       name: a-3
     name: events
+  iam:
+    legacy: false
   kubernetesApiAccess:
   - 0.0.0.0/0
   kubernetesVersion: v1.4.8

tests/integration/create_cluster/minimal/expected-v1alpha1.yaml

@@ -22,6 +22,8 @@ spec:
     - name: a
       zone: us-test-1a
     name: events
+  iam:
+    legacy: false
   kubernetesVersion: v1.4.8
   masterPublicName: api.minimal.example.com
   networkCIDR: 172.20.0.0/16

tests/integration/create_cluster/minimal/expected-v1alpha2.yaml

@@ -20,6 +20,8 @@ spec:
     - instanceGroup: master-us-test-1a
       name: a
     name: events
+  iam:
+    legacy: false
   kubernetesApiAccess:
   - 0.0.0.0/0
   kubernetesVersion: v1.4.8

tests/integration/create_cluster/ngwspecified/expected-v1alpha1.yaml

@@ -23,6 +23,8 @@ spec:
     - name: a
       zone: us-test-1a
     name: events
+  iam:
+    legacy: false
   kubernetesVersion: v1.4.8
   masterPublicName: api.private.example.com
   networkCIDR: 172.20.0.0/16

tests/integration/create_cluster/ngwspecified/expected-v1alpha2.yaml

@@ -21,6 +21,8 @@ spec:
     - instanceGroup: master-us-test-1a
       name: a
     name: events
+  iam:
+    legacy: false
   kubernetesApiAccess:
   - 0.0.0.0/0
   kubernetesVersion: v1.4.8

tests/integration/create_cluster/private/expected-v1alpha1.yaml

@@ -27,6 +27,8 @@ spec:
     - name: a
       zone: us-test-1a
     name: events
+  iam:
+    legacy: false
   kubernetesVersion: v1.4.8
   masterPublicName: api.private.example.com
   networkCIDR: 172.20.0.0/16

tests/integration/create_cluster/private/expected-v1alpha2.yaml

@@ -25,6 +25,8 @@ spec:
     - instanceGroup: master-us-test-1a
       name: a
     name: events
+  iam:
+    legacy: false
   kubernetesApiAccess:
   - 0.0.0.0/0
   kubernetesVersion: v1.4.8