fix assume role tgb

Author: zac-nixon

docs/guide/targetgroupbinding/targetgroupbinding.md

@@ -112,12 +112,110 @@ spec:
 ### AssumeRole
 
 Sometimes the AWS LoadBalancer controller needs to manipulate target groups from different AWS accounts.
-The way to do that is assuming a role from such account. The following spec fields help you with that.
+The way to do that is assuming a role from such an account. The following spec fields help you with that.
 
 * `iamRoleArnToAssume`: the ARN that you need to assume
 * `assumeRoleExternalId`: the external ID for the assume role operation. Optional, but recommended. It helps you to prevent the confused deputy problem ( https://docs.aws.amazon.com/IAM/latest/UserGuide/confused-deputy.html )
 
 
+```yaml
+apiVersion: elbv2.k8s.aws/v1beta1
+kind: TargetGroupBinding
+metadata:
+  name: peered-tg
+  namespace: nlb-game-2048-1
+spec:
+  assumeRoleExternalId: very-secret-string-2
+  iamRoleArnToAssume: arn:aws:iam::155642222660:role/tg-management-role
+  networking:
+    ingress:
+    - from:
+      - securityGroup:
+          groupID: sg-0b6a41a2fd959623f
+      ports:
+      - port: 80
+        protocol: TCP
+  serviceRef:
+    name: service-2048
+    port: 80
+  targetGroupARN: arn:aws:elasticloadbalancing:us-west-2:155642222660:targetgroup/peered-tg/6a4ecf7bfae473c1
+```
+
+In the following examples, we will refer to Cluster Owner (CO) and Target Group Owner (TGO) accounts.
+
+First, in the TGO account creates a role that will allow the AWS LBC in the CO account to assume it.
+For improved security, we only allow the AWS LBC role in CO account to assume the role.
+
+```json
+{
+    "Version": "2012-10-17",
+    "Statement": [
+        {
+            "Sid": "",
+            "Effect": "Allow",
+            "Principal": {
+                "AWS": "arn:aws:iam::565768096483:role/eksctl-awslbc-loadtest-addon-iamserviceaccoun-Role1-13RdJCMqV6p2"
+            },
+            "Action": "sts:AssumeRole",
+            "Condition": {
+                "StringEquals": {
+                    "sts:ExternalId": "very-secret-string"
+                }
+            }
+        }
+    ]
+}
+```
+
+Next, still in the TGO account we need to add the following permissions to the Role created in the first step.
+
+```json
+{
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Sid": "VisualEditor0",
+      "Effect": "Allow",
+      "Action": [
+        "elasticloadbalancing:RegisterTargets",
+        "elasticloadbalancing:DeregisterTargets"
+      ],
+      "Resource": [
+        "arn:aws:elasticloadbalancing:us-west-2:155642222660:targetgroup/tg1/*",
+        "arn:aws:elasticloadbalancing:us-west-2:155642222660:targetgroup/tg2/*"
+        // add more here //
+      ]
+    },
+    {
+      "Sid": "VisualEditor1",
+      "Effect": "Allow",
+      "Action": [
+        "elasticloadbalancing:DescribeTargetGroups",
+        "elasticloadbalancing:DescribeTargetHealth"
+      ],
+      "Resource": "*"
+    }
+  ]
+}
+```
+
+
+Next, in the CO account, we need to allow the AWS LBC to perform the AssumeRole call.
+By default, this permission is not a part of the standard IAM policy that is vended with the LBC installation scripts.
+For improved security, it is possible to scope the AssumeRole permissions down to only roles that you know ahead of time the
+LBC will need to Assume.
+
+```json
+        {
+            "Effect": "Allow",
+            "Action": [
+                "sts:AssumeRole"
+            ],
+            "Resource": "*"
+        }
+```
+
+
 ## Sample YAML
 
 ```yaml

helm/aws-load-balancer-controller/crds/crds.yaml

@@ -317,6 +317,15 @@ spec:
           spec:
             description: TargetGroupBindingSpec defines the desired state of TargetGroupBinding
             properties:
+              assumeRoleExternalId:
+                description: IAM Role ARN to assume when calling AWS APIs. Needed
+                  to assume a role in another account and prevent the confused deputy
+                  problem. https://docs.aws.amazon.com/IAM/latest/UserGuide/confused-deputy.html
+                type: string
+              iamRoleArnToAssume:
+                description: IAM Role ARN to assume when calling AWS APIs. Useful
+                  if the target group is in a different AWS account
+                type: string
               multiClusterTargetGroup:
                 description: MultiClusterTargetGroup Denotes if the TargetGroup is
                   shared among multiple clusters
@@ -494,6 +503,15 @@ spec:
           spec:
             description: TargetGroupBindingSpec defines the desired state of TargetGroupBinding
             properties:
+              assumeRoleExternalId:
+                description: IAM Role ARN to assume when calling AWS APIs. Needed
+                  to assume a role in another account and prevent the confused deputy
+                  problem. https://docs.aws.amazon.com/IAM/latest/UserGuide/confused-deputy.html
+                type: string
+              iamRoleArnToAssume:
+                description: IAM Role ARN to assume when calling AWS APIs. Useful
+                  if the target group is in a different AWS account
+                type: string
               ipAddressType:
                 description: ipAddressType specifies whether the target group is of
                   type IPv4 or IPv6. If unspecified, it will be automatically inferred.

pkg/aws/aws_config.go

@@ -0,0 +1,81 @@
+package aws
+
+import (
+	"context"
+	"github.com/aws/aws-sdk-go-v2/aws"
+	awsmiddleware "github.com/aws/aws-sdk-go-v2/aws/middleware"
+	"github.com/aws/aws-sdk-go-v2/aws/ratelimit"
+	"github.com/aws/aws-sdk-go-v2/aws/retry"
+	"github.com/aws/aws-sdk-go-v2/config"
+	"github.com/aws/aws-sdk-go-v2/feature/ec2/imds"
+	smithymiddleware "github.com/aws/smithy-go/middleware"
+	"sigs.k8s.io/aws-load-balancer-controller/pkg/aws/throttle"
+	awsmetrics "sigs.k8s.io/aws-load-balancer-controller/pkg/metrics/aws"
+	"sigs.k8s.io/aws-load-balancer-controller/pkg/version"
+)
+
+const (
+	userAgent = "elbv2.k8s.aws"
+)
+
+func NewAWSConfigGenerator(cfg CloudConfig, ec2IMDSEndpointMode imds.EndpointModeState, metricsCollector *awsmetrics.Collector) AWSConfigGenerator {
+	return &awsConfigGeneratorImpl{
+		cfg:                 cfg,
+		ec2IMDSEndpointMode: ec2IMDSEndpointMode,
+		metricsCollector:    metricsCollector,
+	}
+
+}
+
+// AWSConfigGenerator is responsible for generating an aws config based on the running environment
+type AWSConfigGenerator interface {
+	GenerateAWSConfig(optFns ...func(*config.LoadOptions) error) (aws.Config, error)
+}
+
+type awsConfigGeneratorImpl struct {
+	cfg                 CloudConfig
+	ec2IMDSEndpointMode imds.EndpointModeState
+	metricsCollector    *awsmetrics.Collector
+}
+
+func (gen *awsConfigGeneratorImpl) GenerateAWSConfig(optFns ...func(*config.LoadOptions) error) (aws.Config, error) {
+
+	defaultOpts := []func(*config.LoadOptions) error{
+		config.WithRegion(gen.cfg.Region),
+		config.WithRetryer(func() aws.Retryer {
+			return retry.NewStandard(func(o *retry.StandardOptions) {
+				o.RateLimiter = ratelimit.None
+				o.MaxAttempts = gen.cfg.MaxRetries
+			})
+		}),
+		config.WithEC2IMDSEndpointMode(gen.ec2IMDSEndpointMode),
+		config.WithAPIOptions([]func(stack *smithymiddleware.Stack) error{
+			awsmiddleware.AddUserAgentKeyValue(userAgent, version.GitVersion),
+		}),
+	}
+
+	defaultOpts = append(defaultOpts, optFns...)
+
+	awsConfig, err := config.LoadDefaultConfig(context.TODO(),
+		defaultOpts...,
+	)
+
+	if err != nil {
+		return aws.Config{}, err
+	}
+
+	if gen.cfg.ThrottleConfig != nil {
+		throttler := throttle.NewThrottler(gen.cfg.ThrottleConfig)
+		awsConfig.APIOptions = append(awsConfig.APIOptions, func(stack *smithymiddleware.Stack) error {
+			return throttle.WithSDKRequestThrottleMiddleware(throttler)(stack)
+		})
+	}
+
+	if gen.metricsCollector != nil {
+		awsConfig.APIOptions = awsmetrics.WithSDKMetricCollector(gen.metricsCollector, awsConfig.APIOptions)
+	}
+
+	return awsConfig, nil
+}
+
+var _ AWSConfigGenerator = &awsConfigGeneratorImpl{}

pkg/aws/cloud.go

@@ -10,18 +10,11 @@ import (
 	"sync"
 	"time"
 
-	awsmiddleware "github.com/aws/aws-sdk-go-v2/aws/middleware"
-	"github.com/aws/aws-sdk-go-v2/aws/ratelimit"
-	"github.com/aws/aws-sdk-go-v2/aws/retry"
 	"github.com/aws/aws-sdk-go-v2/config"
 	"github.com/aws/aws-sdk-go-v2/credentials"
 	ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"
 	"github.com/aws/aws-sdk-go-v2/service/sts"
 
-	smithymiddleware "github.com/aws/smithy-go/middleware"
-	"sigs.k8s.io/aws-load-balancer-controller/pkg/aws/throttle"
-	"sigs.k8s.io/aws-load-balancer-controller/pkg/version"
-
 	"github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/aws/aws-sdk-go-v2/feature/ec2/imds"
 	"github.com/aws/aws-sdk-go-v2/service/ec2"
@@ -35,7 +28,6 @@ import (
 )
 
 const (
-	userAgent          = "elbv2.k8s.aws"
 	cacheTTLBufferTime = 30 * time.Second
 )
 
@@ -81,29 +73,11 @@ func NewCloud(cfg CloudConfig, clusterName string, metricsCollector *aws_metrics
 		}
 		cfg.Region = region
 	}
-	awsConfig, err := config.LoadDefaultConfig(context.TODO(),
-		config.WithRegion(cfg.Region),
-		config.WithRetryer(func() aws.Retryer {
-			return retry.NewStandard(func(o *retry.StandardOptions) {
-				o.RateLimiter = ratelimit.None
-				o.MaxAttempts = cfg.MaxRetries
-			})
-		}),
-		config.WithEC2IMDSEndpointMode(ec2IMDSEndpointMode),
-		config.WithAPIOptions([]func(stack *smithymiddleware.Stack) error{
-			awsmiddleware.AddUserAgentKeyValue(userAgent, version.GitVersion),
-		}),
-	)
-
-	if cfg.ThrottleConfig != nil {
-		throttler := throttle.NewThrottler(cfg.ThrottleConfig)
-		awsConfig.APIOptions = append(awsConfig.APIOptions, func(stack *smithymiddleware.Stack) error {
-			return throttle.WithSDKRequestThrottleMiddleware(throttler)(stack)
-		})
-	}
 
-	if metricsCollector != nil {
-		awsConfig.APIOptions = aws_metrics.WithSDKMetricCollector(metricsCollector, awsConfig.APIOptions)
+	awsConfigGenerator := NewAWSConfigGenerator(cfg, ec2IMDSEndpointMode, metricsCollector)
+	awsConfig, err := awsConfigGenerator.GenerateAWSConfig()
+	if err != nil {
+		return nil, errors.Wrap(err, "Unable to generate AWS config")
 	}
 
 	if awsClientsProvider == nil {
@@ -132,6 +106,8 @@ func NewCloud(cfg CloudConfig, clusterName string, metricsCollector *aws_metrics
 		shield:      services.NewShield(awsClientsProvider),
 		rgt:         services.NewRGT(awsClientsProvider),
 
+		awsConfigGenerator: awsConfigGenerator,
+
 		assumeRoleElbV2Cache: cache.NewExpiring(),
 
 		awsClientsProvider: awsClientsProvider,
@@ -229,6 +205,8 @@ type defaultCloud struct {
 
 	clusterName string
 
+	awsConfigGenerator AWSConfigGenerator
+
 	// A cache holding elbv2 clients that are assuming a role.
 	assumeRoleElbV2Cache *cache.Expiring
 	// assumeRoleElbV2CacheMutex protects assumeRoleElbV2Cache
@@ -251,31 +229,33 @@ func (c *defaultCloud) GetAssumedRoleELBV2(ctx context.Context, assumeRoleArn st
 	if exists {
 		return assumedRoleELBV2.(services.ELBV2), nil
 	}
-	c.logger.Info("awsCloud", "method", "GetAssumedRoleELBV2", "AssumeRoleArn", assumeRoleArn, "externalId", externalId)
+	c.logger.Info("Constructing new elbv2 client", "AssumeRoleArn", assumeRoleArn, "externalId", externalId)
 
-	existingAwsConfig, _ := c.awsClientsProvider.GetAWSConfig(ctx, "GetAWSConfigForIAMRoleImpersonation")
+	stsClient, err := c.awsClientsProvider.GetSTSClient(ctx, "AssumeRole")
+	if err != nil {
+		// This should never happen, but let's be forward-looking.
+		return nil, err
+	}
 
-	sourceAccount := sts.NewFromConfig(*existingAwsConfig)
-	response, err := sourceAccount.AssumeRole(ctx, &sts.AssumeRoleInput{
+	response, err := stsClient.AssumeRole(ctx, &sts.AssumeRoleInput{
 		RoleArn:         aws.String(assumeRoleArn),
 		RoleSessionName: aws.String(generateAssumeRoleSessionName(c.clusterName)),
 		ExternalId:      aws.String(externalId),
 	})
 	if err != nil {
-		c.logger.Error(err, "Unable to assume target role, %v")
+		c.logger.Error(err, "Unable to assume target role", "roleArn", assumeRoleArn)
 		return nil, err
 	}
 	assumedRoleCreds := response.Credentials
 	newCreds := credentials.NewStaticCredentialsProvider(*assumedRoleCreds.AccessKeyId, *assumedRoleCreds.SecretAccessKey, *assumedRoleCreds.SessionToken)
-	newAwsConfig, err := config.LoadDefaultConfig(ctx, config.WithRegion(c.cfg.Region), config.WithCredentialsProvider(newCreds))
+	newAwsConfig, err := c.awsConfigGenerator.GenerateAWSConfig(config.WithCredentialsProvider(newCreds))
 	if err != nil {
-		c.logger.Error(err, "Unable to load static credentials for service client config, %v. Attempting to use default client")
+		c.logger.Error(err, "Create new service client config service client config", "roleArn", assumeRoleArn)
 		return nil, err
 	}
 
 	cacheTTL := assumedRoleCreds.Expiration.Sub(time.Now())
-	existingAwsConfig.Credentials = newAwsConfig.Credentials
-	elbv2WithAssumedRole := services.NewELBV2(c.awsClientsProvider, c)
+	elbv2WithAssumedRole := services.NewELBV2FromStaticClient(c.awsClientsProvider.GenerateNewELBv2Client(newAwsConfig), c)
 
 	c.assumeRoleElbV2CacheMutex.Lock()
 	defer c.assumeRoleElbV2CacheMutex.Unlock()