Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix hardcoded ARN partitions #12638

Merged
merged 4 commits into from
Oct 30, 2021
Merged

Fix hardcoded ARN partitions #12638

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5d82ce0
Update cloudmock and integration test inputs to use aws-test partition
rifelpet Oct 29, 2021
c734f5c
Update IAMBuilder to include the current partition in ARNs
rifelpet Oct 29, 2021
8dc11bd
./hack/update-expected.sh
rifelpet Oct 29, 2021
0bd7348
Fix ARN partition in SQS queue policy
rifelpet Oct 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cloudmock/aws/mockelbv2/loadbalancers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ func (m *MockELBV2) CreateLoadBalancer(request *elbv2.CreateLoadBalancerInput) (
lb.VpcId = aws.String("vpc-1")

m.lbCount++
arn := fmt.Sprintf("arn:aws:elasticloadbalancing:us-test-1:000000000000:loadbalancer/net/%v/%v", aws.StringValue(request.Name), m.lbCount)
arn := fmt.Sprintf("arn:aws-test:elasticloadbalancing:us-test-1:000000000000:loadbalancer/net/%v/%v", aws.StringValue(request.Name), m.lbCount)

lb.LoadBalancerArn = aws.String(arn)

Expand Down
2 changes: 1 addition & 1 deletion cloudmock/aws/mockelbv2/targetgroups.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ func (m *MockELBV2) CreateTargetGroup(request *elbv2.CreateTargetGroupInput) (*e
}

m.tgCount++
arn := fmt.Sprintf("arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/%v/%v", aws.StringValue(request.Name), m.tgCount)
arn := fmt.Sprintf("arn:aws-test:elasticloadbalancing:us-test-1:000000000000:targetgroup/%v/%v", aws.StringValue(request.Name), m.tgCount)
tg.TargetGroupArn = aws.String(arn)

if m.TargetGroups == nil {
Expand Down
2 changes: 1 addition & 1 deletion cloudmock/aws/mockeventbridge/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func (m *MockEventBridge) PutRule(input *eventbridge.PutRuleInput) (*eventbridge
defer m.mutex.Unlock()

name := *input.Name
arn := "arn:aws:events:us-east-1:012345678901:rule/" + name
arn := "arn:aws-test:events:us-east-1:012345678901:rule/" + name

rule := &eventbridge.Rule{
Arn: &arn,
Expand Down
2 changes: 1 addition & 1 deletion cloudmock/aws/mockiam/oidcprovider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ func (m *MockIAM) CreateOpenIDConnectProvider(request *iam.CreateOpenIDConnectPr

klog.Infof("CreateOpenIDConnectProvider: %v", request)

arn := fmt.Sprintf("arn:aws:iam::0000000000:oidc-provider/%s", *request.Url)
arn := fmt.Sprintf("arn:aws-test:iam::0000000000:oidc-provider/%s", *request.Url)

p := &iam.GetOpenIDConnectProviderOutput{
ClientIDList: request.ClientIDList,
Expand Down
2 changes: 1 addition & 1 deletion cloudmock/aws/mocksqs/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func (m *MockSQS) CreateQueue(input *sqs.CreateQueueInput) (*sqs.CreateQueueOutp
tags: input.Tags,
}

arn := fmt.Sprintf("arn:aws:sqs:us-test-1:000000000000:queue/%v", aws.StringValue(input.QueueName))
arn := fmt.Sprintf("arn:aws-test:sqs:us-test-1:000000000000:queue/%v", aws.StringValue(input.QueueName))
queue.attributes["QueueArn"] = &arn

m.Queues[name] = queue
Expand Down
7 changes: 4 additions & 3 deletions pkg/model/awsmodel/iam.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ func (b *IAMModelBuilder) buildIAMRolePolicy(role iam.Subject, iamName string, i
Cluster: b.Cluster,
Role: role,
Region: b.Region,
Partition: b.AWSPartition,
UseServiceAccountExternalPermisssions: b.UseServiceAccountExternalPermissions(),
},
}
Expand Down Expand Up @@ -405,7 +406,7 @@ func IAMServiceEC2(region string) string {
return "ec2.amazonaws.com"
}

func formatAWSIAMStatement(accountId, oidcProvider, namespace, name string) (*iam.Statement, error) {
func formatAWSIAMStatement(accountId, partition, oidcProvider, namespace, name string) (*iam.Statement, error) {
// disallow wildcard in the service account name
if strings.Contains(name, "*") {
return nil, fmt.Errorf("service account name cannot contain a wildcard %s", name)
Expand All @@ -420,7 +421,7 @@ func formatAWSIAMStatement(accountId, oidcProvider, namespace, name string) (*ia
return &iam.Statement{
Effect: "Allow",
Principal: iam.Principal{
Federated: "arn:aws:iam::" + accountId + ":oidc-provider/" + oidcProvider,
Federated: "arn:" + partition + ":iam::" + accountId + ":oidc-provider/" + oidcProvider,
},
Action: stringorslice.String("sts:AssumeRoleWithWebIdentity"),
Condition: map[string]interface{}{
Expand All @@ -439,7 +440,7 @@ func (b *IAMModelBuilder) buildAWSIAMRolePolicy(role iam.Subject) (fi.Resource,
if ok {
oidcProvider := strings.TrimPrefix(*b.Cluster.Spec.KubeAPIServer.ServiceAccountIssuer, "https://")

statement, err := formatAWSIAMStatement(b.AWSAccountID, oidcProvider, serviceAccount.Namespace, serviceAccount.Name)
statement, err := formatAWSIAMStatement(b.AWSAccountID, b.AWSPartition, oidcProvider, serviceAccount.Namespace, serviceAccount.Name)
if err != nil {
return nil, err
}
Expand Down
10 changes: 7 additions & 3 deletions pkg/model/awsmodel/iam_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ func TestIAMServiceEC2(t *testing.T) {
func Test_formatAWSIAMStatement(t *testing.T) {
type args struct {
acountId string
partition string
oidcProvider string
namespace string
name string
Expand All @@ -58,6 +59,7 @@ func Test_formatAWSIAMStatement(t *testing.T) {
name: "namespace and name without wildcard",
args: args{
acountId: "0123456789",
partition: "aws-test",
oidcProvider: "oidc-test",
namespace: "test",
name: "test",
Expand All @@ -66,7 +68,7 @@ func Test_formatAWSIAMStatement(t *testing.T) {
want: &iam.Statement{
Effect: "Allow",
Principal: iam.Principal{
Federated: "arn:aws:iam::0123456789:oidc-provider/oidc-test",
Federated: "arn:aws-test:iam::0123456789:oidc-provider/oidc-test",
},
Action: stringorslice.String("sts:AssumeRoleWithWebIdentity"),
Condition: map[string]interface{}{
Expand All @@ -80,6 +82,7 @@ func Test_formatAWSIAMStatement(t *testing.T) {
name: "name contains wildcard",
args: args{
acountId: "0123456789",
partition: "aws-test",
oidcProvider: "oidc-test",
namespace: "test",
name: "test-*",
Expand All @@ -90,6 +93,7 @@ func Test_formatAWSIAMStatement(t *testing.T) {
name: "namespace contains wildcard",
args: args{
acountId: "0123456789",
partition: "aws-test",
oidcProvider: "oidc-test",
namespace: "test-*",
name: "test",
Expand All @@ -98,7 +102,7 @@ func Test_formatAWSIAMStatement(t *testing.T) {
want: &iam.Statement{
Effect: "Allow",
Principal: iam.Principal{
Federated: "arn:aws:iam::0123456789:oidc-provider/oidc-test",
Federated: "arn:aws-test:iam::0123456789:oidc-provider/oidc-test",
},
Action: stringorslice.String("sts:AssumeRoleWithWebIdentity"),
Condition: map[string]interface{}{
Expand All @@ -111,7 +115,7 @@ func Test_formatAWSIAMStatement(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := formatAWSIAMStatement(tt.args.acountId, tt.args.oidcProvider, tt.args.namespace, tt.args.name)
got, err := formatAWSIAMStatement(tt.args.acountId, tt.args.partition, tt.args.oidcProvider, tt.args.namespace, tt.args.name)
if (err != nil) != tt.wantErr {
t.Errorf("formatAWSIAMStatement() error = %v, wantErr %v", err, tt.wantErr)
return
Expand Down
2 changes: 1 addition & 1 deletion pkg/model/components/addonmanifests/awscloudcontrollermanager/iam.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ var _ iam.Subject = &ServiceAccount{}
func (r *ServiceAccount) BuildAWSPolicy(b *iam.PolicyBuilder) (*iam.Policy, error) {
clusterName := b.Cluster.ObjectMeta.Name
p := iam.NewPolicy(clusterName)
iam.AddCCMPermissions(p, b.Cluster.Spec.Networking.Kubenet != nil)
iam.AddCCMPermissions(p, b.Partition, b.Cluster.Spec.Networking.Kubenet != nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to pass the partition into NewPolicy() instead of having to pass it to every Add*Permissions() call?

iam.AddLegacyCCMPermissions(p)
return p, nil
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/model/components/addonmanifests/awsebscsidriver/iam.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func (r *ServiceAccount) BuildAWSPolicy(b *iam.PolicyBuilder) (*iam.Policy, erro
p := iam.NewPolicy(clusterName)

addSnapshotControllerPermissions := b.Cluster.Spec.SnapshotController != nil && fi.BoolValue(b.Cluster.Spec.SnapshotController.Enabled)
iam.AddAWSEBSCSIDriverPermissions(p, addSnapshotControllerPermissions)
iam.AddAWSEBSCSIDriverPermissions(p, b.Partition, addSnapshotControllerPermissions)

return p, nil
}
Expand Down
1 change: 0 additions & 1 deletion pkg/model/iam/BUILD.bazel
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

57 changes: 22 additions & 35 deletions pkg/model/iam/iam_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import (
"sort"
"strings"

"github.com/aws/aws-sdk-go/aws/endpoints"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/klog/v2"
"k8s.io/kops/pkg/apis/kops"
Expand Down Expand Up @@ -247,6 +246,7 @@ type PolicyBuilder struct {
HostedZoneID string
KMSKeys []string
Region string
Partition string
ResourceARN *string
Role Subject
UseServiceAccountExternalPermisssions bool
Expand Down Expand Up @@ -303,7 +303,7 @@ func (r *NodeRoleAPIServer) BuildAWSPolicy(b *PolicyBuilder) (*Policy, error) {
}

if b.Cluster.Spec.Networking != nil && b.Cluster.Spec.Networking.AmazonVPC != nil {
addAmazonVPCCNIPermissions(p, b.IAMPrefix())
addAmazonVPCCNIPermissions(p, b.Partition)
}

if b.Cluster.Spec.Networking != nil && b.Cluster.Spec.Networking.Cilium != nil && b.Cluster.Spec.Networking.Cilium.Ipam == kops.CiliumIpamEni {
Expand Down Expand Up @@ -340,17 +340,17 @@ func (r *NodeRoleMaster) BuildAWSPolicy(b *PolicyBuilder) (*Policy, error) {

// If cluster does not use external CCM, the master IAM Role needs CCM permissions
if b.Cluster.Spec.ExternalCloudControllerManager == nil {
AddCCMPermissions(p, b.Cluster.Spec.Networking.Kubenet != nil)
AddCCMPermissions(p, b.Partition, b.Cluster.Spec.Networking.Kubenet != nil)
AddLegacyCCMPermissions(p)
}

if !b.UseServiceAccountExternalPermisssions {
esc := b.Cluster.Spec.SnapshotController != nil &&
fi.BoolValue(b.Cluster.Spec.SnapshotController.Enabled)
AddAWSEBSCSIDriverPermissions(p, esc)
AddAWSEBSCSIDriverPermissions(p, b.Partition, esc)

if b.Cluster.Spec.ExternalCloudControllerManager != nil {
AddCCMPermissions(p, b.Cluster.Spec.Networking.Kubenet != nil)
AddCCMPermissions(p, b.Partition, b.Cluster.Spec.Networking.Kubenet != nil)
AddLegacyCCMPermissions(p)
}

Expand All @@ -370,7 +370,7 @@ func (r *NodeRoleMaster) BuildAWSPolicy(b *PolicyBuilder) (*Policy, error) {
}

if b.Cluster.Spec.Networking != nil && b.Cluster.Spec.Networking.AmazonVPC != nil {
addAmazonVPCCNIPermissions(p, b.IAMPrefix())
addAmazonVPCCNIPermissions(p, b.Partition)
}

if b.Cluster.Spec.Networking != nil && b.Cluster.Spec.Networking.Cilium != nil && b.Cluster.Spec.Networking.Cilium.Ipam == kops.CiliumIpamEni {
Expand Down Expand Up @@ -400,7 +400,7 @@ func (r *NodeRoleNode) BuildAWSPolicy(b *PolicyBuilder) (*Policy, error) {
}

if b.Cluster.Spec.Networking != nil && b.Cluster.Spec.Networking.AmazonVPC != nil {
addAmazonVPCCNIPermissions(p, b.IAMPrefix())
addAmazonVPCCNIPermissions(p, b.Partition)
}

if b.Cluster.Spec.Networking != nil && b.Cluster.Spec.Networking.Calico != nil && b.Cluster.Spec.Networking.Calico.AWSSrcDstCheck != "DoNothing" {
Expand All @@ -421,19 +421,6 @@ func (r *NodeRoleBastion) BuildAWSPolicy(b *PolicyBuilder) (*Policy, error) {
return p, nil
}

// IAMPrefix returns the prefix for AWS ARNs in the current region, for use with IAM
// it is arn:aws in the default aws partition but different in other isolated or non-standard partitions
func (b *PolicyBuilder) IAMPrefix() string {
partitions := endpoints.DefaultPartitions()
for _, p := range partitions {
if _, ok := p.Regions()[b.Region]; ok {
arn := "arn:" + p.ID()
return arn
}
}
return "arn:aws"
}

// AddS3Permissions builds an IAM Policy, with statements granting tailored
// access to S3 assets, depending on the instance group or service-account role
func (b *PolicyBuilder) AddS3Permissions(p *Policy) (*Policy, error) {
Expand Down Expand Up @@ -556,7 +543,7 @@ func (b *PolicyBuilder) AddS3Permissions(p *Policy) (*Policy, error) {
"s3:ListBucketVersions",
),
Resource: stringorslice.Slice([]string{
strings.Join([]string{b.IAMPrefix(), ":s3:::", s3Bucket}, ""),
fmt.Sprintf("arn:%v:s3:::%v", b.Partition, s3Bucket),
}),
})
}
Expand All @@ -574,7 +561,7 @@ func (b *PolicyBuilder) buildS3WriteStatements(p *Policy, iamS3Path string) {
"s3:PutObject",
}),
Resource: stringorslice.Of(
strings.Join([]string{b.IAMPrefix(), ":s3:::", iamS3Path, "/*"}, ""),
fmt.Sprintf("arn:%v:s3:::%v/*", b.Partition, iamS3Path),
),
})

Expand All @@ -592,7 +579,7 @@ func (b *PolicyBuilder) buildS3GetStatements(p *Policy, iamS3Path string) error

// Add the prefix for IAM
for i, r := range resources {
resources[i] = b.IAMPrefix() + ":s3:::" + iamS3Path + r
resources[i] = fmt.Sprintf("arn:%v:s3:::%v%v", b.Partition, iamS3Path, r)
}

p.Statement = append(p.Statement, &Statement{
Expand Down Expand Up @@ -809,7 +796,7 @@ func AddLegacyCCMPermissions(p *Policy) {
)
}

func AddCCMPermissions(p *Policy, cloudRoutes bool) {
func AddCCMPermissions(p *Policy, partition string, cloudRoutes bool) {
p.unconditionalAction.Insert(
"autoscaling:DescribeAutoScalingGroups",
"autoscaling:DescribeTags",
Expand Down Expand Up @@ -878,8 +865,8 @@ func AddCCMPermissions(p *Policy, cloudRoutes bool) {
),
Resource: stringorslice.Slice(
[]string{
"arn:aws:ec2:*:*:volume/*",
"arn:aws:ec2:*:*:snapshot/*",
fmt.Sprintf("arn:%v:ec2:*:*:volume/*", partition),
fmt.Sprintf("arn:%v:ec2:*:*:snapshot/*", partition),
},
),
Condition: Condition{
Expand Down Expand Up @@ -943,7 +930,7 @@ func AddClusterAutoscalerPermissions(p *Policy) {
}

// AddAWSEBSCSIDriverPermissions appens policy statements that the AWS EBS CSI Driver needs to operate.
func AddAWSEBSCSIDriverPermissions(p *Policy, appendSnapshotPermissions bool) {
func AddAWSEBSCSIDriverPermissions(p *Policy, partition string, appendSnapshotPermissions bool) {

if appendSnapshotPermissions {
addSnapshotPersmissions(p)
Expand Down Expand Up @@ -975,8 +962,8 @@ func AddAWSEBSCSIDriverPermissions(p *Policy, appendSnapshotPermissions bool) {
),
Resource: stringorslice.Slice(
[]string{
"arn:aws:ec2:*:*:volume/*",
"arn:aws:ec2:*:*:snapshot/*",
fmt.Sprintf("arn:%v:ec2:*:*:volume/*", partition),
fmt.Sprintf("arn:%v:ec2:*:*:snapshot/*", partition),
},
),
Condition: Condition{
Expand All @@ -996,8 +983,8 @@ func AddAWSEBSCSIDriverPermissions(p *Policy, appendSnapshotPermissions bool) {
),
Resource: stringorslice.Slice(
[]string{
"arn:aws:ec2:*:*:volume/*",
"arn:aws:ec2:*:*:snapshot/*",
fmt.Sprintf("arn:%v:ec2:*:*:volume/*", partition),
fmt.Sprintf("arn:%v:ec2:*:*:snapshot/*", partition),
},
),
Condition: Condition{
Expand Down Expand Up @@ -1038,13 +1025,13 @@ func AddDNSControllerPermissions(b *PolicyBuilder, p *Policy) {
Action: stringorslice.Of("route53:ChangeResourceRecordSets",
"route53:ListResourceRecordSets",
"route53:GetHostedZone"),
Resource: stringorslice.Slice([]string{b.IAMPrefix() + ":route53:::hostedzone/" + hostedZoneID}),
Resource: stringorslice.Slice([]string{fmt.Sprintf("arn:%v:route53:::hostedzone/%v", b.Partition, hostedZoneID)}),
})

p.Statement = append(p.Statement, &Statement{
Effect: StatementEffectAllow,
Action: stringorslice.Slice([]string{"route53:GetChange"}),
Resource: stringorslice.Slice([]string{b.IAMPrefix() + ":route53:::change/*"}),
Resource: stringorslice.Slice([]string{fmt.Sprintf("arn:%v:route53:::change/*", b.Partition)}),
})

wildcard := stringorslice.Slice([]string{"*"})
Expand Down Expand Up @@ -1113,7 +1100,7 @@ func addCiliumEniPermissions(p *Policy) {
)
}

func addAmazonVPCCNIPermissions(p *Policy, iamPrefix string) {
func addAmazonVPCCNIPermissions(p *Policy, partition string) {
p.unconditionalAction.Insert(
"ec2:AssignPrivateIpAddresses",
"ec2:AttachNetworkInterface",
Expand All @@ -1134,7 +1121,7 @@ func addAmazonVPCCNIPermissions(p *Policy, iamPrefix string) {
"ec2:CreateTags",
}),
Resource: stringorslice.Slice([]string{
strings.Join([]string{iamPrefix, ":ec2:*:*:network-interface/*"}, ""),
strings.Join([]string{"arn:", partition, ":ec2:*:*:network-interface/*"}, ""),
})},
)
}
Expand Down
22 changes: 2 additions & 20 deletions pkg/model/iam/iam_builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,25 +30,6 @@ import (
"k8s.io/kops/upup/pkg/fi"
)

func TestIAMPrefix(t *testing.T) {
var expectations = map[string]string{
"us-east-1": "arn:aws",
"us-iso-east-1": "arn:aws-iso",
"us-isob-east-1": "arn:aws-iso-b",
"us-gov-east-1": "arn:aws-us-gov",
"randomunknown": "arn:aws",
"cn-north-1": "arn:aws-cn",
"cn-northwest-1": "arn:aws-cn",
}

for region, expect := range expectations {
arn := (&PolicyBuilder{Region: region}).IAMPrefix()
if arn != expect {
t.Errorf("expected %s for %s, received %s", expect, region, arn)
}
}
}

func TestRoundTrip(t *testing.T) {
grid := []struct {
IAM *Statement
Expand Down Expand Up @@ -192,7 +173,8 @@ func TestPolicyGeneration(t *testing.T) {
},
},
},
Role: x.Role,
Role: x.Role,
Partition: "aws-test",
}
b.Cluster.SetName("iam-builder-test.k8s.local")

Expand Down
Loading