Merge pull request #3158 from KashifSaadat/node-iam-policy-updates

Automatic merge from submit-queue

Tighten down S3 IAM policy statements

This PR contains updates to:
- Remove default `s3:*` IAM policy for master and compute nodes
- Allow all nodes to list bucket contents
- Allow master nodes to get all bucket contents
- Allow compute nodes to get specific bucket contents (certain private key files are disallowed)
- Adds unit tests around the S3 policy build function

pkg/model/iam/iam_builder.go

@@ -231,26 +231,7 @@ func (b *IAMPolicyBuilder) BuildAWSIAMPolicy() (*IAMPolicy, error) {
 		}
 
 		if s3Path, ok := vfsPath.(*vfs.S3Path); ok {
-			// Note that the config store may itself be a subdirectory of a bucket
-			iamS3Path := s3Path.Bucket() + "/" + s3Path.Key()
-			iamS3Path = strings.TrimSuffix(iamS3Path, "/")
-
-			p.Statement = append(p.Statement, &IAMStatement{
-				Effect: IAMStatementEffectAllow,
-				Action: stringorslice.Slice([]string{"s3:*"}),
-				Resource: stringorslice.Of(
-					iamPrefix+":s3:::"+iamS3Path,
-					iamPrefix+":s3:::"+iamS3Path+"/*",
-				),
-			})
-
-			p.Statement = append(p.Statement, &IAMStatement{
-				Effect: IAMStatementEffectAllow,
-				Action: stringorslice.Of("s3:GetBucketLocation", "s3:ListBucket"),
-				Resource: stringorslice.Slice([]string{
-					iamPrefix + ":s3:::" + s3Path.Bucket(),
-				}),
-			})
+			addS3Permissions(p, iamPrefix, s3Path, b.Role)
 		} 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)
@@ -292,6 +273,57 @@ 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) {
+	// Note that the config store may itself be a subdirectory of a bucket
+	iamS3Path := s3Path.Bucket() + "/" + s3Path.Key()
+	iamS3Path = strings.TrimSuffix(iamS3Path, "/")
+
+	p.Statement = append(p.Statement, &IAMStatement{
+		Effect: IAMStatementEffectAllow,
+		Action: stringorslice.Of("s3:GetBucketLocation", "s3:ListBucket"),
+		Resource: stringorslice.Slice([]string{
+			strings.Join([]string{iamPrefix, ":s3:::", s3Path.Bucket()}, ""),
+		}),
+	})
+
+	p.Statement = append(p.Statement, &IAMStatement{
+		Effect: IAMStatementEffectAllow,
+		Action: stringorslice.Slice([]string{"s3:List*"}),
+		Resource: stringorslice.Of(
+			strings.Join([]string{iamPrefix, ":s3:::", iamS3Path}, ""),
+			strings.Join([]string{iamPrefix, ":s3:::", iamS3Path, "/*"}, ""),
+		),
+	})
+
+	if role == api.InstanceGroupRoleMaster {
+		p.Statement = append(p.Statement, &IAMStatement{
+			Effect: IAMStatementEffectAllow,
+			Action: stringorslice.Slice([]string{"s3:Get*"}),
+			Resource: stringorslice.Of(
+				strings.Join([]string{iamPrefix, ":s3:::", iamS3Path, "/*"}, ""),
+			),
+		})
+	} else if role == api.InstanceGroupRoleNode {
+		p.Statement = append(p.Statement, &IAMStatement{
+			Effect: IAMStatementEffectAllow,
+			Action: stringorslice.Slice([]string{"s3:Get*"}),
+			Resource: stringorslice.Of(
+				strings.Join([]string{iamPrefix, ":s3:::", iamS3Path, "/addons/*"}, ""),
+				strings.Join([]string{iamPrefix, ":s3:::", iamS3Path, "/instancegroup/*"}, ""),
+				strings.Join([]string{iamPrefix, ":s3:::", iamS3Path, "/pki/issued/*"}, ""),
+				strings.Join([]string{iamPrefix, ":s3:::", iamS3Path, "/pki/ssh/*"}, ""),
+				strings.Join([]string{iamPrefix, ":s3:::", iamS3Path, "/pki/private/kube-proxy/*"}, ""),
+				strings.Join([]string{iamPrefix, ":s3:::", iamS3Path, "/pki/private/kubelet/*"}, ""),
+				strings.Join([]string{iamPrefix, ":s3:::", iamS3Path, "/secrets/*"}, ""),
+				strings.Join([]string{iamPrefix, ":s3:::", iamS3Path, "/cluster.spec"}, ""),
+				strings.Join([]string{iamPrefix, ":s3:::", iamS3Path, "/config"}, ""),
+			),
+		})
+	}
+}
+
 // 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
 func (b *IAMPolicyBuilder) IAMPrefix() string {

pkg/model/iam/iam_builder_test.go

@@ -20,7 +20,10 @@ import (
 	"encoding/json"
 	"testing"
 
+	"k8s.io/kops/pkg/apis/kops"
+	"k8s.io/kops/pkg/diff"
 	"k8s.io/kops/pkg/util/stringorslice"
+	"k8s.io/kops/util/pkg/vfs"
 )
 
 func TestRoundTrip(t *testing.T) {
@@ -46,13 +49,13 @@ func TestRoundTrip(t *testing.T) {
 		},
 	}
 	for _, g := range grid {
-		actualJson, err := json.Marshal(g.IAM)
+		actualJSON, err := json.Marshal(g.IAM)
 		if err != nil {
 			t.Errorf("error encoding IAM %s to json: %v", g.IAM, err)
 		}
 
-		if g.JSON != string(actualJson) {
-			t.Errorf("Unexpected JSON encoding.  Actual=%q, Expected=%q", string(actualJson), g.JSON)
+		if g.JSON != string(actualJSON) {
+			t.Errorf("Unexpected JSON encoding.  Actual=%q, Expected=%q", string(actualJSON), g.JSON)
 		}
 
 		parsed := &IAMStatement{}
@@ -67,3 +70,111 @@ func TestRoundTrip(t *testing.T) {
 
 	}
 }
+
+func TestS3PolicyGeneration(t *testing.T) {
+	defaultS3Statements := []*IAMStatement{
+		{
+			Effect: IAMStatementEffectAllow,
+			Action: stringorslice.Of(
+				"s3:GetBucketLocation",
+				"s3:ListBucket",
+			),
+			Resource: stringorslice.Slice([]string{
+				"arn:aws:s3:::bucket-name",
+			}),
+		},
+		{
+			Effect: IAMStatementEffectAllow,
+			Action: stringorslice.Slice([]string{
+				"s3:List*",
+			}),
+			Resource: stringorslice.Slice([]string{
+				"arn:aws:s3:::bucket-name/cluster-name.k8s.local",
+				"arn:aws:s3:::bucket-name/cluster-name.k8s.local/*",
+			}),
+		},
+	}
+
+	grid := []struct {
+		Role      kops.InstanceGroupRole
+		IAMPolicy IAMPolicy
+	}{
+		{
+			Role: "Master",
+			IAMPolicy: IAMPolicy{
+				Statement: append(defaultS3Statements, &IAMStatement{
+					Effect: IAMStatementEffectAllow,
+					Action: stringorslice.Slice([]string{
+						"s3:Get*",
+					}),
+					Resource: stringorslice.Of(
+						"arn:aws:s3:::bucket-name/cluster-name.k8s.local/*",
+					),
+				}),
+			},
+		},
+		{
+			Role: "Node",
+			IAMPolicy: IAMPolicy{
+				Statement: append(defaultS3Statements, &IAMStatement{
+					Effect: IAMStatementEffectAllow,
+					Action: stringorslice.Slice([]string{
+						"s3:Get*",
+					}),
+					Resource: stringorslice.Slice([]string{
+						"arn:aws:s3:::bucket-name/cluster-name.k8s.local/addons/*",
+						"arn:aws:s3:::bucket-name/cluster-name.k8s.local/instancegroup/*",
+						"arn:aws:s3:::bucket-name/cluster-name.k8s.local/pki/issued/*",
+						"arn:aws:s3:::bucket-name/cluster-name.k8s.local/pki/ssh/*",
+						"arn:aws:s3:::bucket-name/cluster-name.k8s.local/pki/private/kube-proxy/*",
+						"arn:aws:s3:::bucket-name/cluster-name.k8s.local/pki/private/kubelet/*",
+						"arn:aws:s3:::bucket-name/cluster-name.k8s.local/secrets/*",
+						"arn:aws:s3:::bucket-name/cluster-name.k8s.local/cluster.spec",
+						"arn:aws:s3:::bucket-name/cluster-name.k8s.local/config",
+					}),
+				}),
+			},
+		},
+		{
+			Role: "Bastion",
+			IAMPolicy: IAMPolicy{
+				Statement: defaultS3Statements,
+			},
+		},
+	}
+
+	for i, x := range grid {
+		ip := &IAMPolicy{}
+
+		vfsPath, err := vfs.Context.BuildVfsPath("s3://bucket-name/cluster-name.k8s.local")
+		if err != nil {
+			t.Errorf("case %d failed to build Vfs Path. error: %s", i, err)
+			continue
+		}
+		s3Path, ok := vfsPath.(*vfs.S3Path)
+		if !ok {
+			t.Errorf("case %d failed to build S3 Path.", i)
+			continue
+		}
+
+		addS3Permissions(ip, "arn:aws", s3Path, x.Role)
+
+		expectedPolicy, err := x.IAMPolicy.AsJSON()
+		if err != nil {
+			t.Errorf("case %d failed to convert expected IAM Policy to JSON. Error: %q", i, err)
+			continue
+		}
+		actualPolicy, err := ip.AsJSON()
+		if err != nil {
+			t.Errorf("case %d failed to convert generated IAM Policy to JSON. Error: %q", i, err)
+			continue
+		}
+
+		if expectedPolicy != actualPolicy {
+			diffString := diff.FormatDiff(expectedPolicy, actualPolicy)
+			t.Logf("diff:\n%s\n", diffString)
+			t.Errorf("case %d failed, policy output differed from expected.", i)
+			continue
+		}
+	}
+}

upup/pkg/fi/vfs_castore.go

@@ -24,14 +24,16 @@ import (
 	"crypto/x509"
 	"crypto/x509/pkix"
 	"fmt"
-	"github.com/golang/glog"
-	"golang.org/x/crypto/ssh"
-	"k8s.io/kops/util/pkg/vfs"
 	"math/big"
 	"os"
 	"strings"
 	"sync"
 	"time"
+
+	"github.com/golang/glog"
+	"golang.org/x/crypto/ssh"
+
+	"k8s.io/kops/util/pkg/vfs"
 )
 
 type VFSCAStore struct {
@@ -300,19 +302,11 @@ func (c *VFSCAStore) FindKeypair(id string) (*Certificate, *PrivateKey, error) {
 func (c *VFSCAStore) FindCert(id string) (*Certificate, error) {
 	var certs *certificates
 
-	if id == CertificateId_CA {
-		caCertificates, _, err := c.readCAKeypairs()
-		if err != nil {
-			return nil, err
-		}
-		certs = caCertificates
-	} else {
-		var err error
-		p := c.buildCertificatePoolPath(id)
-		certs, err = c.loadCertificates(p)
-		if err != nil {
-			return nil, err
-		}
+	var err error
+	p := c.buildCertificatePoolPath(id)
+	certs, err = c.loadCertificates(p)
+	if err != nil {
+		return nil, fmt.Errorf("error in 'FindCert' attempting to load cert %q: %v", id, err)
 	}
 
 	var cert *Certificate
@@ -326,19 +320,11 @@ func (c *VFSCAStore) FindCert(id string) (*Certificate, error) {
 func (c *VFSCAStore) FindCertificatePool(id string) (*CertificatePool, error) {
 	var certs *certificates
 
-	if id == CertificateId_CA {
-		caCertificates, _, err := c.readCAKeypairs()
-		if err != nil {
-			return nil, err
-		}
-		certs = caCertificates
-	} else {
-		var err error
-		p := c.buildCertificatePoolPath(id)
-		certs, err = c.loadCertificates(p)
-		if err != nil {
-			return nil, err
-		}
+	var err error
+	p := c.buildCertificatePoolPath(id)
+	certs, err = c.loadCertificates(p)
+	if err != nil {
+		return nil, fmt.Errorf("error in 'FindCertificatePool' attempting to load cert %q: %v", id, err)
 	}
 
 	pool := &CertificatePool{}