fix: cache aws auth client by account id (#9981) (#10107)

* fix aws auth client cache to use accound ID

* return error if no sts config found

* cache ec2 clients by account ID, region, and role

* add changelog

* fix log syntax

Co-authored-by: John-Michael Faircloth <[email protected]>

Author: hc-github-team-secure-vault-core

builtin/credential/aws/backend.go

@@ -73,17 +73,19 @@ type backend struct {
 	// of tidyCooldownPeriod.
 	nextTidyTime time.Time
 
-	// Map to hold the EC2 client objects indexed by region and STS role.
+	// Map to hold the EC2 client objects indexed by a composite key of Account
+	// ID, region, and STS role.
 	// This avoids the overhead of creating a client object for every login request.
 	// When the credentials are modified or deleted, all the cached client objects
 	// will be flushed. The empty STS role signifies the master account
-	EC2ClientsMap map[string]map[string]*ec2.EC2
+	EC2ClientsMap map[clientKey]*ec2.EC2
 
-	// Map to hold the IAM client objects indexed by region and STS role.
+	// Map to hold the IAM client objects indexed by a composite key of Account
+	// ID, region, and STS role.
 	// This avoids the overhead of creating a client object for every login request.
 	// When the credentials are modified or deleted, all the cached client objects
 	// will be flushed. The empty STS role signifies the master account
-	IAMClientsMap map[string]map[string]*iam.IAM
+	IAMClientsMap map[clientKey]*iam.IAM
 
 	// Map to associate a partition to a random region in that partition. Users of
 	// this don't care what region in the partition they use, but there is some client
@@ -117,13 +119,31 @@ type backend struct {
 	deprecatedTerms *strings.Replacer
 }
 
+// clientKey is a composite key for caching IAM or EC2 clients.
+type clientKey struct {
+	AccountID string
+	Region    string
+	STSRole   string // Use an empty string for the master account
+}
+
+func (c clientKey) String() string {
+	// For clarity in logs, we explicitly state when the master account is
+	// being used instead of showing an empty string for the role.
+	rolePart := c.STSRole
+	if rolePart == "" {
+		rolePart = "<master-account>"
+	}
+
+	return fmt.Sprintf("%s/%s/%s", c.AccountID, c.Region, rolePart)
+}
+
 func Backend(_ *logical.BackendConfig) (*backend, error) {
 	b := &backend{
 		// Setting the periodic func to be run once in an hour.
 		// If there is a real need, this can be made configurable.
 		tidyCooldownPeriod:     time.Hour,
-		EC2ClientsMap:          make(map[string]map[string]*ec2.EC2),
-		IAMClientsMap:          make(map[string]map[string]*iam.IAM),
+		EC2ClientsMap:          make(map[clientKey]*ec2.EC2),
+		IAMClientsMap:          make(map[clientKey]*iam.IAM),
 		iamUserIdToArnCache:    cache.New(7*24*time.Hour, 24*time.Hour),
 		tidyDenyListCASGuard:   new(uint32),
 		tidyAccessListCASGuard: new(uint32),

builtin/credential/aws/client.go

@@ -175,21 +175,15 @@ func (b *backend) getClientConfig(ctx context.Context, s logical.Storage, region
 // the cached EC2 client objects will be flushed. Config mutex lock should be
 // acquired for write operation before calling this method.
 func (b *backend) flushCachedEC2Clients() {
-	// deleting items in map during iteration is safe
-	for region := range b.EC2ClientsMap {
-		delete(b.EC2ClientsMap, region)
-	}
+	b.EC2ClientsMap = make(map[clientKey]*ec2.EC2)
 }
 
 // flushCachedIAMClients deletes all the cached iam client objects from the
 // backend. If the client credentials configuration is deleted or updated in
 // the backend, all the cached IAM client objects will be flushed. Config mutex
 // lock should be acquired for write operation before calling this method.
 func (b *backend) flushCachedIAMClients() {
-	// deleting items in map during iteration is safe
-	for region := range b.IAMClientsMap {
-		delete(b.IAMClientsMap, region)
-	}
+	b.IAMClientsMap = make(map[clientKey]*iam.IAM)
 }
 
 // Gets an entry out of the user ID cache
@@ -221,6 +215,11 @@ func (b *backend) stsRoleForAccount(ctx context.Context, s logical.Storage, acco
 	if sts != nil {
 		return sts.StsRole, sts.ExternalID, nil
 	}
+
+	// Return an error if there's no STS config for an account which is not the default one
+	if b.defaultAWSAccountID != "" && b.defaultAWSAccountID != accountID {
+		return "", "", fmt.Errorf("no STS configuration found for account ID %q", accountID)
+	}
 	return "", "", nil
 }
 
@@ -231,10 +230,16 @@ func (b *backend) clientEC2(ctx context.Context, s logical.Storage, region, acco
 		return nil, err
 	}
 	b.configMutex.RLock()
-	if b.EC2ClientsMap[region] != nil && b.EC2ClientsMap[region][stsRole] != nil {
+
+	key := clientKey{
+		AccountID: accountID,
+		Region:    region,
+		STSRole:   stsRole,
+	}
+	if cachedClient, ok := b.EC2ClientsMap[key]; ok {
 		defer b.configMutex.RUnlock()
 		// If the client object was already created, return it
-		return b.EC2ClientsMap[region][stsRole], nil
+		return cachedClient, nil
 	}
 
 	// Release the read lock and acquire the write lock
@@ -243,8 +248,8 @@ func (b *backend) clientEC2(ctx context.Context, s logical.Storage, region, acco
 	defer b.configMutex.Unlock()
 
 	// If the client gets created while switching the locks, return it
-	if b.EC2ClientsMap[region] != nil && b.EC2ClientsMap[region][stsRole] != nil {
-		return b.EC2ClientsMap[region][stsRole], nil
+	if cachedClient, ok := b.EC2ClientsMap[key]; ok {
+		return cachedClient, nil
 	}
 
 	// Create an AWS config object using a chain of providers
@@ -267,13 +272,9 @@ func (b *backend) clientEC2(ctx context.Context, s logical.Storage, region, acco
 	if client == nil {
 		return nil, fmt.Errorf("could not obtain ec2 client")
 	}
-	if _, ok := b.EC2ClientsMap[region]; !ok {
-		b.EC2ClientsMap[region] = map[string]*ec2.EC2{stsRole: client}
-	} else {
-		b.EC2ClientsMap[region][stsRole] = client
-	}
 
-	return b.EC2ClientsMap[region][stsRole], nil
+	b.EC2ClientsMap[key] = client
+	return b.EC2ClientsMap[key], nil
 }
 
 // clientIAM creates a client to interact with AWS IAM API
@@ -283,27 +284,34 @@ func (b *backend) clientIAM(ctx context.Context, s logical.Storage, region, acco
 		return nil, err
 	}
 	if stsRole == "" {
-		b.Logger().Debug(fmt.Sprintf("no stsRole found for %s", accountID))
+		b.Logger().Debug("no stsRole found for account", "accountID", accountID)
 	} else {
-		b.Logger().Debug(fmt.Sprintf("found stsRole %s for account %s", stsRole, accountID))
+		b.Logger().Debug("found stsRole for account", "stsRole", stsRole, "accountID", accountID)
 	}
 	b.configMutex.RLock()
-	if b.IAMClientsMap[region] != nil && b.IAMClientsMap[region][stsRole] != nil {
+
+	key := clientKey{
+		AccountID: accountID,
+		Region:    region,
+		STSRole:   stsRole,
+	}
+	if cachedClient, ok := b.IAMClientsMap[key]; ok {
 		defer b.configMutex.RUnlock()
 		// If the client object was already created, return it
-		b.Logger().Debug(fmt.Sprintf("returning cached client for region %s and stsRole %s", region, stsRole))
-		return b.IAMClientsMap[region][stsRole], nil
+		b.Logger().Debug("returning cached client for key", "key", key)
+		return cachedClient, nil
 	}
-	b.Logger().Debug(fmt.Sprintf("no cached client for region %s and stsRole %s", region, stsRole))
+	b.Logger().Debug("no cached client for key", "key", key)
 
 	// Release the read lock and acquire the write lock
 	b.configMutex.RUnlock()
 	b.configMutex.Lock()
 	defer b.configMutex.Unlock()
 
 	// If the client gets created while switching the locks, return it
-	if b.IAMClientsMap[region] != nil && b.IAMClientsMap[region][stsRole] != nil {
-		return b.IAMClientsMap[region][stsRole], nil
+	if cachedClient, ok := b.IAMClientsMap[key]; ok {
+		b.Logger().Debug("returning cached client for key", "key", key)
+		return cachedClient, nil
 	}
 
 	// Create an AWS config object using a chain of providers
@@ -326,12 +334,8 @@ func (b *backend) clientIAM(ctx context.Context, s logical.Storage, region, acco
 	if client == nil {
 		return nil, fmt.Errorf("could not obtain iam client")
 	}
-	if _, ok := b.IAMClientsMap[region]; !ok {
-		b.IAMClientsMap[region] = map[string]*iam.IAM{stsRole: client}
-	} else {
-		b.IAMClientsMap[region][stsRole] = client
-	}
-	return b.IAMClientsMap[region][stsRole], nil
+	b.IAMClientsMap[key] = client
+	return b.IAMClientsMap[key], nil
 }
 
 // PluginIdentityTokenFetcher fetches plugin identity tokens from Vault. It is provided

builtin/credential/aws/path_config_rotate_root.go

@@ -10,7 +10,6 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/credentials"
 	"github.com/aws/aws-sdk-go/aws/session"
-	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/aws/aws-sdk-go/service/iam"
 	"github.com/aws/aws-sdk-go/service/iam/iamiface"
 	"github.com/hashicorp/go-cleanhttp"
@@ -181,8 +180,8 @@ func (b *backend) rotateRoot(ctx context.Context, req *logical.Request) (*logica
 
 	// Previous cached clients need to be cleared because they may have been made using
 	// the soon-to-be-obsolete credentials.
-	b.IAMClientsMap = make(map[string]map[string]*iam.IAM)
-	b.EC2ClientsMap = make(map[string]map[string]*ec2.EC2)
+	b.flushCachedIAMClients()
+	b.flushCachedEC2Clients()
 
 	// Now to clean up the old key.
 	deleteAccessKeyInput := iam.DeleteAccessKeyInput{

changelog/_9981.txt

@@ -0,0 +1,3 @@
+```release-note:security
+auth/aws: fix an issue where a user may be able to bypass authentication to Vault due to incorrect caching of the AWS client
+```