From 649c4e4fe8046c858b74ffcd412b82a24ead121d Mon Sep 17 00:00:00 2001 From: Joel Thompson Date: Sun, 19 Aug 2018 23:43:21 -0400 Subject: [PATCH 1/8] Add AWS Secret Engine Root Credential Rotation This allows the AWS Secret Engine to rotate its credentials used to access AWS. This will only work when the AWS Secret Engine has been provided explicit IAM credentials via the config/root endpoint, and further, when the IAM credentials provided are the only access key on the IAM user associated wtih the access key (because AWS allows a maximum of 2 access keys per user). Fixes #4385 --- builtin/logical/aws/backend.go | 1 + .../logical/aws/path_config_rotate_root.go | 114 ++++++++++++++++++ 2 files changed, 115 insertions(+) create mode 100644 builtin/logical/aws/path_config_rotate_root.go diff --git a/builtin/logical/aws/backend.go b/builtin/logical/aws/backend.go index 1c1f04b6807f..00a2f21596f7 100644 --- a/builtin/logical/aws/backend.go +++ b/builtin/logical/aws/backend.go @@ -34,6 +34,7 @@ func Backend() *backend { Paths: []*framework.Path{ pathConfigRoot(), + pathConfigRotateRoot(), pathConfigLease(&b), pathRoles(&b), pathListRoles(&b), diff --git a/builtin/logical/aws/path_config_rotate_root.go b/builtin/logical/aws/path_config_rotate_root.go new file mode 100644 index 000000000000..7f3b3a89b442 --- /dev/null +++ b/builtin/logical/aws/path_config_rotate_root.go @@ -0,0 +1,114 @@ +package aws + +import ( + "context" + "fmt" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/iam" + "github.com/hashicorp/vault/logical" + "github.com/hashicorp/vault/logical/framework" +) + +func pathConfigRotateRoot() *framework.Path { + return &framework.Path{ + Pattern: "config/rotate-root", + Callbacks: map[logical.Operation]framework.OperationFunc{ + logical.UpdateOperation: pathConfigRotateRootUpdate, + }, + + HelpSynopsis: pathConfigRotateRootHelpSyn, + HelpDescription: pathConfigRotateRootHelpDesc, + } +} + +func pathConfigRotateRootUpdate(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { + // TODO: Add locking around reading/writing the config path/rootConfig + rawRootConfig, err := req.Storage.Get(ctx, "config/root") + if err != nil { + return nil, err + } + if rawRootConfig == nil { + return logical.ErrorResponse("no configuration found for config/root"), nil + } + var config rootConfig + if err := rawRootConfig.DecodeJSON(&config); err != nil { + return logical.ErrorResponse(fmt.Sprintf("error reading root configuration: %v", err)), nil + } + + if config.AccessKey == "" || config.SecretKey == "" { + return logical.ErrorResponse("cannot call config/rotate-root when either access_key or secret_key is empty"), nil + } + + client, err := clientIAM(ctx, req.Storage) + if err == nil { + return logical.ErrorResponse(fmt.Sprintf("error retrieving IAM client: %v", err)), nil + } + if client == nil { + return logical.ErrorResponse("nil IAM client"), nil + } + var getUserInput iam.GetUserInput // empty input means get current user + getUserRes, err := client.GetUser(&getUserInput) + if err != nil { + return logical.ErrorResponse(fmt.Sprintf("error calling GetUser: %v", err)), nil + } + if getUserRes == nil { + return logical.ErrorResponse("nil response from GetUser"), nil + } + if getUserRes.User == nil { + return logical.ErrorResponse("nil user returned from GetUser"), nil + } + if getUserRes.User.UserName == nil { + return logical.ErrorResponse("nil UserName returnjd from GetUser"), nil + } + + createAccessKeyInput := iam.CreateAccessKeyInput{ + UserName: getUserRes.User.UserName, + } + createAccessKeyRes, err := client.CreateAccessKey(&createAccessKeyInput) + if err != nil { + return logical.ErrorResponse(fmt.Sprintf("error calling CreateAccessKey: %v", err)), nil + } + if createAccessKeyRes.AccessKey == nil { + return logical.ErrorResponse("nil response from CreateAccessKey"), nil + } + if createAccessKeyRes.AccessKey.AccessKeyId == nil || createAccessKeyRes.AccessKey.SecretAccessKey == nil { + return logical.ErrorResponse("nil AccessKeyId or SecretAccessKey returned from CreateAccessKey"), nil + } + + oldAccessKey := config.AccessKey + + config.AccessKey = *createAccessKeyRes.AccessKey.AccessKeyId + config.SecretKey = *createAccessKeyRes.AccessKey.SecretAccessKey + + newEntry, err := logical.StorageEntryJSON("config/root", config) + if err != nil { + return logical.ErrorResponse(fmt.Sprintf("error generating new root config: %v", err)), nil + } + // TODO: Should we return the full error message? Are there any scenarios in which it could expose + // the underlying creds? (The idea of rotate-root is that it should never expose credentials.) + if err := req.Storage.Put(ctx, newEntry); err != nil { + return logical.ErrorResponse(fmt.Sprintf("error saving new root config: %v", err)), nil + } + + deleteAccessKeyInput := iam.DeleteAccessKeyInput{ + AccessKeyId: aws.String(oldAccessKey), + UserName: getUserRes.User.UserName, + } + _, err = client.DeleteAccessKey(&deleteAccessKeyInput) + if err != nil { + return logical.ErrorResponse(fmt.Sprintf("error deleting old access key: %v", err)), nil + } + + return nil, nil +} + +const pathConfigRotateRootHelpSyn = ` +Request to rotate the AWS credentials used by Vault +` + +const pathConfigRotateRootHelpDesc = ` +This path attempts to rotate the AWS credentials used by Vault for this mount. +It is only valid if Vault has been configured to use AWS IAM credentials via the +config/root endpoint. +` From 21fd1cfe3a5c0b2fb678e9a955cfc801aa53bb44 Mon Sep 17 00:00:00 2001 From: Joel Thompson Date: Mon, 20 Aug 2018 22:10:52 -0400 Subject: [PATCH 2/8] Add test for AWS root credential rotation Also fix a typo in the root credential rotation code --- builtin/logical/aws/backend_test.go | 39 +++++++++++++++++++ .../logical/aws/path_config_rotate_root.go | 8 +++- 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/builtin/logical/aws/backend_test.go b/builtin/logical/aws/backend_test.go index 85bc70af35f4..cfc183693319 100644 --- a/builtin/logical/aws/backend_test.go +++ b/builtin/logical/aws/backend_test.go @@ -57,6 +57,7 @@ func TestBackend_basicSTS(t *testing.T) { Backend: getBackend(t), Steps: []logicaltest.TestStep{ testAccStepConfigWithCreds(t, accessKey), + testAccStepRotateRoot(accessKey), testAccStepWritePolicy(t, "test", testDynamoPolicy), testAccStepRead(t, "sts", "test", []credentialTestFunc{listDynamoTablesTest}), testAccStepWriteArnPolicyRef(t, "test", ec2PolicyArn), @@ -368,6 +369,44 @@ func testAccStepConfigWithCreds(t *testing.T, accessKey *awsAccessKey) logicalte } } +func testAccStepRotateRoot(oldAccessKey *awsAccessKey) logicaltest.TestStep { + return logicaltest.TestStep{ + Operation: logical.UpdateOperation, + Path: "config/rotate-root", + Check: func(resp *logical.Response) error { + if resp == nil { + return fmt.Errorf("received nil response from config/rotate-root") + } + newAccessKeyId := resp.Data["access_key"].(string) + if newAccessKeyId == oldAccessKey.AccessKeyId { + return fmt.Errorf("rotate-root didn't rotate access key") + } + awsConfig := &aws.Config{ + Region: aws.String("us-east-1"), + HTTPClient: cleanhttp.DefaultClient(), + Credentials: credentials.NewStaticCredentials(oldAccessKey.AccessKeyId, oldAccessKey.SecretAccessKey, ""), + } + // sigh.... + oldAccessKey.AccessKeyId = newAccessKeyId + log.Println("[WARN] Sleeping for 10 seconds waiting for AWS...") + time.Sleep(10 * time.Second) + svc := sts.New(session.New(awsConfig)) + params := &sts.GetCallerIdentityInput{} + _, err := svc.GetCallerIdentity(params) + if err == nil { + return fmt.Errorf("bad: old credentials succeeded after rotate") + } + if aerr, ok := err.(awserr.Error); ok { + if aerr.Code() != "InvalidClientTokenId" { + return fmt.Errorf("Unknown error returned from AWS: %#v", aerr) + } + return nil + } + return err + }, + } +} + func testAccStepRead(t *testing.T, path, name string, credentialTests []credentialTestFunc) logicaltest.TestStep { return logicaltest.TestStep{ Operation: logical.ReadOperation, diff --git a/builtin/logical/aws/path_config_rotate_root.go b/builtin/logical/aws/path_config_rotate_root.go index 7f3b3a89b442..e73e7250742d 100644 --- a/builtin/logical/aws/path_config_rotate_root.go +++ b/builtin/logical/aws/path_config_rotate_root.go @@ -41,7 +41,7 @@ func pathConfigRotateRootUpdate(ctx context.Context, req *logical.Request, data } client, err := clientIAM(ctx, req.Storage) - if err == nil { + if err != nil { return logical.ErrorResponse(fmt.Sprintf("error retrieving IAM client: %v", err)), nil } if client == nil { @@ -100,7 +100,11 @@ func pathConfigRotateRootUpdate(ctx context.Context, req *logical.Request, data return logical.ErrorResponse(fmt.Sprintf("error deleting old access key: %v", err)), nil } - return nil, nil + return &logical.Response{ + Data: map[string]interface{}{ + "access_key": config.AccessKey, + }, + }, nil } const pathConfigRotateRootHelpSyn = ` From 075c9058692195a6b220053d817a4f92d2a35caa Mon Sep 17 00:00:00 2001 From: Joel Thompson Date: Mon, 20 Aug 2018 22:27:07 -0400 Subject: [PATCH 3/8] Add docs for AWS root rotation --- website/source/api/secret/aws/index.html.md | 41 +++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/website/source/api/secret/aws/index.html.md b/website/source/api/secret/aws/index.html.md index 8b4f97845396..330557653241 100644 --- a/website/source/api/secret/aws/index.html.md +++ b/website/source/api/secret/aws/index.html.md @@ -80,6 +80,47 @@ $ curl \ http://127.0.0.1:8200/v1/aws/config/root ``` +## Rotate Root IAM Credentials + +When you have configured Vault with static credentials, you can use this +endpoint to have Vault rotate the access key it used. Note that, due to AWS +eventual consistency, after calling this endpoint, subsequent calls from Vault +to AWS may fail for a few seconds until AWS becomes consistent again. + + +In order to call this endpoint, Vault's AWS access key MUST be the only access +key on the IAM user; otherwise, generation of a new access key will fail. Once +this method is called, Vault will now be the only entity that knows the AWS +secret key is uses to access AWS. + +| Method | Path | Produces | +| :------- | :--------------------------- | :--------------------- | +| `POST` | `/aws/config/rotate-root` | `200 application/json` | + +### Parameters + +There are no parameters to this operation. + +### Sample Request + +```$ curl \ + --header "X-Vault-Token: ..." \ + --request POST \ + http://127.0.0.1:8211/v1/aws/config/rotate-root +``` + +### Sample Response + +```json +{ + "data": { + "access_key": "AKIA..." + } +} +``` + +The new access key Vault uses is returned by this operation. + ## Configure Lease This endpoint configures lease settings for the AWS secrets engine. It is From 7f79d830b3630aa09e21a59436c60d2ec5ecf932 Mon Sep 17 00:00:00 2001 From: Joel Thompson Date: Mon, 20 Aug 2018 22:52:23 -0400 Subject: [PATCH 4/8] Add locks around reading and writing config/root And wire the backend up in a bunch of places so the config can get the lock --- builtin/logical/aws/backend.go | 7 ++++-- builtin/logical/aws/client.go | 12 +++++---- builtin/logical/aws/path_config_root.go | 9 ++++--- .../logical/aws/path_config_rotate_root.go | 25 +++++++++++-------- builtin/logical/aws/path_user.go | 4 +-- builtin/logical/aws/rollback.go | 8 +++--- builtin/logical/aws/secret_access_keys.go | 12 ++++----- 7 files changed, 44 insertions(+), 33 deletions(-) diff --git a/builtin/logical/aws/backend.go b/builtin/logical/aws/backend.go index 00a2f21596f7..9000340b9e38 100644 --- a/builtin/logical/aws/backend.go +++ b/builtin/logical/aws/backend.go @@ -33,8 +33,8 @@ func Backend() *backend { }, Paths: []*framework.Path{ - pathConfigRoot(), - pathConfigRotateRoot(), + pathConfigRoot(&b), + pathConfigRotateRoot(&b), pathConfigLease(&b), pathRoles(&b), pathListRoles(&b), @@ -58,6 +58,9 @@ type backend struct { // Mutex to protect access to reading and writing policies roleMutex sync.RWMutex + + // Mutex to protect access to reading and writing root creds + rootMutex sync.RWMutex } const backendHelp = ` diff --git a/builtin/logical/aws/client.go b/builtin/logical/aws/client.go index 6a8ffa26f74c..ccb5f0d819a6 100644 --- a/builtin/logical/aws/client.go +++ b/builtin/logical/aws/client.go @@ -15,11 +15,13 @@ import ( "github.com/hashicorp/vault/logical" ) -func getRootConfig(ctx context.Context, s logical.Storage, clientType string) (*aws.Config, error) { +func (b *backend) getRootConfig(ctx context.Context, s logical.Storage, clientType string) (*aws.Config, error) { credsConfig := &awsutil.CredentialsConfig{} var endpoint string var maxRetries int = aws.UseServiceDefaultRetries + b.rootMutex.RLock() + defer b.rootMutex.RUnlock() entry, err := s.Get(ctx, "config/root") if err != nil { return nil, err @@ -68,8 +70,8 @@ func getRootConfig(ctx context.Context, s logical.Storage, clientType string) (* }, nil } -func clientIAM(ctx context.Context, s logical.Storage) (*iam.IAM, error) { - awsConfig, err := getRootConfig(ctx, s, "iam") +func (b *backend) clientIAM(ctx context.Context, s logical.Storage) (*iam.IAM, error) { + awsConfig, err := b.getRootConfig(ctx, s, "iam") if err != nil { return nil, err } @@ -82,8 +84,8 @@ func clientIAM(ctx context.Context, s logical.Storage) (*iam.IAM, error) { return client, nil } -func clientSTS(ctx context.Context, s logical.Storage) (*sts.STS, error) { - awsConfig, err := getRootConfig(ctx, s, "sts") +func (b *backend) clientSTS(ctx context.Context, s logical.Storage) (*sts.STS, error) { + awsConfig, err := b.getRootConfig(ctx, s, "sts") if err != nil { return nil, err } diff --git a/builtin/logical/aws/path_config_root.go b/builtin/logical/aws/path_config_root.go index d3503410a5d1..ab9632cde2a6 100644 --- a/builtin/logical/aws/path_config_root.go +++ b/builtin/logical/aws/path_config_root.go @@ -8,7 +8,7 @@ import ( "github.com/hashicorp/vault/logical/framework" ) -func pathConfigRoot() *framework.Path { +func pathConfigRoot(b *backend) *framework.Path { return &framework.Path{ Pattern: "config/root", Fields: map[string]*framework.FieldSchema{ @@ -42,7 +42,7 @@ func pathConfigRoot() *framework.Path { }, Callbacks: map[logical.Operation]framework.OperationFunc{ - logical.UpdateOperation: pathConfigRootWrite, + logical.UpdateOperation: b.pathConfigRootWrite, }, HelpSynopsis: pathConfigRootHelpSyn, @@ -50,12 +50,15 @@ func pathConfigRoot() *framework.Path { } } -func pathConfigRootWrite(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { +func (b *backend) pathConfigRootWrite(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { region := data.Get("region").(string) iamendpoint := data.Get("iam_endpoint").(string) stsendpoint := data.Get("sts_endpoint").(string) maxretries := data.Get("max_retries").(int) + b.rootMutex.Lock() + defer b.rootMutex.Unlock() + entry, err := logical.StorageEntryJSON("config/root", rootConfig{ AccessKey: data.Get("access_key").(string), SecretKey: data.Get("secret_key").(string), diff --git a/builtin/logical/aws/path_config_rotate_root.go b/builtin/logical/aws/path_config_rotate_root.go index e73e7250742d..5fa160dd2783 100644 --- a/builtin/logical/aws/path_config_rotate_root.go +++ b/builtin/logical/aws/path_config_rotate_root.go @@ -10,11 +10,11 @@ import ( "github.com/hashicorp/vault/logical/framework" ) -func pathConfigRotateRoot() *framework.Path { +func pathConfigRotateRoot(b *backend) *framework.Path { return &framework.Path{ Pattern: "config/rotate-root", Callbacks: map[logical.Operation]framework.OperationFunc{ - logical.UpdateOperation: pathConfigRotateRootUpdate, + logical.UpdateOperation: b.pathConfigRotateRootUpdate, }, HelpSynopsis: pathConfigRotateRootHelpSyn, @@ -22,8 +22,18 @@ func pathConfigRotateRoot() *framework.Path { } } -func pathConfigRotateRootUpdate(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { - // TODO: Add locking around reading/writing the config path/rootConfig +func (b *backend) pathConfigRotateRootUpdate(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { + // have to get the client config first because that takes out a read lock + client, err := b.clientIAM(ctx, req.Storage) + if err != nil { + return logical.ErrorResponse(fmt.Sprintf("error retrieving IAM client: %v", err)), nil + } + if client == nil { + return logical.ErrorResponse("nil IAM client"), nil + } + + b.rootMutex.Lock() + defer b.rootMutex.Unlock() rawRootConfig, err := req.Storage.Get(ctx, "config/root") if err != nil { return nil, err @@ -40,13 +50,6 @@ func pathConfigRotateRootUpdate(ctx context.Context, req *logical.Request, data return logical.ErrorResponse("cannot call config/rotate-root when either access_key or secret_key is empty"), nil } - client, err := clientIAM(ctx, req.Storage) - if err != nil { - return logical.ErrorResponse(fmt.Sprintf("error retrieving IAM client: %v", err)), nil - } - if client == nil { - return logical.ErrorResponse("nil IAM client"), nil - } var getUserInput iam.GetUserInput // empty input means get current user getUserRes, err := client.GetUser(&getUserInput) if err != nil { diff --git a/builtin/logical/aws/path_user.go b/builtin/logical/aws/path_user.go index 0d541546f70f..7030f502ecc0 100644 --- a/builtin/logical/aws/path_user.go +++ b/builtin/logical/aws/path_user.go @@ -111,7 +111,7 @@ func (b *backend) pathCredsRead(ctx context.Context, req *logical.Request, d *fr } } -func pathUserRollback(ctx context.Context, req *logical.Request, _kind string, data interface{}) error { +func (b *backend) pathUserRollback(ctx context.Context, req *logical.Request, _kind string, data interface{}) error { var entry walUser if err := mapstructure.Decode(data, &entry); err != nil { return err @@ -119,7 +119,7 @@ func pathUserRollback(ctx context.Context, req *logical.Request, _kind string, d username := entry.UserName // Get the client - client, err := clientIAM(ctx, req.Storage) + client, err := b.clientIAM(ctx, req.Storage) if err != nil { return err } diff --git a/builtin/logical/aws/rollback.go b/builtin/logical/aws/rollback.go index 92130bceb386..570244c36323 100644 --- a/builtin/logical/aws/rollback.go +++ b/builtin/logical/aws/rollback.go @@ -9,11 +9,11 @@ import ( "github.com/hashicorp/vault/logical/framework" ) -var walRollbackMap = map[string]framework.WALRollbackFunc{ - "user": pathUserRollback, -} - func (b *backend) walRollback(ctx context.Context, req *logical.Request, kind string, data interface{}) error { + walRollbackMap := map[string]framework.WALRollbackFunc{ + "user": b.pathUserRollback, + } + if !b.System().LocalMount() && b.System().ReplicationState().HasState(consts.ReplicationPerformancePrimary) { return nil } diff --git a/builtin/logical/aws/secret_access_keys.go b/builtin/logical/aws/secret_access_keys.go index 57e1105da69b..98e1a08cae71 100644 --- a/builtin/logical/aws/secret_access_keys.go +++ b/builtin/logical/aws/secret_access_keys.go @@ -37,7 +37,7 @@ func secretAccessKeys(b *backend) *framework.Secret { }, Renew: b.secretAccessKeysRenew, - Revoke: secretAccessKeysRevoke, + Revoke: b.secretAccessKeysRevoke, } } @@ -67,7 +67,7 @@ func genUsername(displayName, policyName, userType string) (ret string, warning func (b *backend) secretTokenCreate(ctx context.Context, s logical.Storage, displayName, policyName, policy string, lifeTimeInSeconds int64) (*logical.Response, error) { - STSClient, err := clientSTS(ctx, s) + STSClient, err := b.clientSTS(ctx, s) if err != nil { return logical.ErrorResponse(err.Error()), nil } @@ -112,7 +112,7 @@ func (b *backend) secretTokenCreate(ctx context.Context, s logical.Storage, func (b *backend) assumeRole(ctx context.Context, s logical.Storage, displayName, roleName, roleArn, policy string, lifeTimeInSeconds int64) (*logical.Response, error) { - STSClient, err := clientSTS(ctx, s) + STSClient, err := b.clientSTS(ctx, s) if err != nil { return logical.ErrorResponse(err.Error()), nil } @@ -161,7 +161,7 @@ func (b *backend) secretAccessKeysCreate( ctx context.Context, s logical.Storage, displayName, policyName string, role *awsRoleEntry) (*logical.Response, error) { - client, err := clientIAM(ctx, s) + client, err := b.clientIAM(ctx, s) if err != nil { return logical.ErrorResponse(err.Error()), nil } @@ -281,7 +281,7 @@ func (b *backend) secretAccessKeysRenew(ctx context.Context, req *logical.Reques return resp, nil } -func secretAccessKeysRevoke(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { +func (b *backend) secretAccessKeysRevoke(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { // STS cleans up after itself so we can skip this if is_sts internal data // element set to true. If is_sts is not set, assumes old version @@ -309,7 +309,7 @@ func secretAccessKeysRevoke(ctx context.Context, req *logical.Request, d *framew } // Use the user rollback mechanism to delete this user - err := pathUserRollback(ctx, req, "user", map[string]interface{}{ + err := b.pathUserRollback(ctx, req, "user", map[string]interface{}{ "username": username, }) if err != nil { From 63efb7e821aebb4a809fd039dfa3cf3194e6164b Mon Sep 17 00:00:00 2001 From: Joel Thompson Date: Thu, 13 Sep 2018 21:04:47 -0400 Subject: [PATCH 5/8] Respond to PR feedback --- .../logical/aws/path_config_rotate_root.go | 33 +++++++++---------- website/source/api/secret/aws/index.html.md | 2 +- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/builtin/logical/aws/path_config_rotate_root.go b/builtin/logical/aws/path_config_rotate_root.go index 5fa160dd2783..11ef36ae0a03 100644 --- a/builtin/logical/aws/path_config_rotate_root.go +++ b/builtin/logical/aws/path_config_rotate_root.go @@ -6,6 +6,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/iam" + "github.com/hashicorp/errwrap" "github.com/hashicorp/vault/logical" "github.com/hashicorp/vault/logical/framework" ) @@ -26,10 +27,10 @@ func (b *backend) pathConfigRotateRootUpdate(ctx context.Context, req *logical.R // have to get the client config first because that takes out a read lock client, err := b.clientIAM(ctx, req.Storage) if err != nil { - return logical.ErrorResponse(fmt.Sprintf("error retrieving IAM client: %v", err)), nil + return nil, err } if client == nil { - return logical.ErrorResponse("nil IAM client"), nil + return nil, fmt.Errorf("nil IAM client") } b.rootMutex.Lock() @@ -39,30 +40,30 @@ func (b *backend) pathConfigRotateRootUpdate(ctx context.Context, req *logical.R return nil, err } if rawRootConfig == nil { - return logical.ErrorResponse("no configuration found for config/root"), nil + return nil, fmt.Errorf("No configuration found for config/root") } var config rootConfig if err := rawRootConfig.DecodeJSON(&config); err != nil { - return logical.ErrorResponse(fmt.Sprintf("error reading root configuration: %v", err)), nil + return nil, errwrap.Wrapf("Error reading root configuration: {{err}}", err) } if config.AccessKey == "" || config.SecretKey == "" { - return logical.ErrorResponse("cannot call config/rotate-root when either access_key or secret_key is empty"), nil + return logical.ErrorResponse("Cannot call config/rotate-root when either access_key or secret_key is empty"), nil } var getUserInput iam.GetUserInput // empty input means get current user getUserRes, err := client.GetUser(&getUserInput) if err != nil { - return logical.ErrorResponse(fmt.Sprintf("error calling GetUser: %v", err)), nil + return nil, errwrap.Wrapf("error calling GetUser: {{err}}", err) } if getUserRes == nil { - return logical.ErrorResponse("nil response from GetUser"), nil + return nil, fmt.Errorf("nil response from GetUser") } if getUserRes.User == nil { - return logical.ErrorResponse("nil user returned from GetUser"), nil + return nil, fmt.Errorf("nil user returned from GetUser") } if getUserRes.User.UserName == nil { - return logical.ErrorResponse("nil UserName returnjd from GetUser"), nil + return nil, fmt.Errorf("nil UserName returned from GetUser") } createAccessKeyInput := iam.CreateAccessKeyInput{ @@ -70,13 +71,13 @@ func (b *backend) pathConfigRotateRootUpdate(ctx context.Context, req *logical.R } createAccessKeyRes, err := client.CreateAccessKey(&createAccessKeyInput) if err != nil { - return logical.ErrorResponse(fmt.Sprintf("error calling CreateAccessKey: %v", err)), nil + return nil, errwrap.Wrapf("error calling CreateAccessKey: {{err}}", err) } if createAccessKeyRes.AccessKey == nil { - return logical.ErrorResponse("nil response from CreateAccessKey"), nil + return nil, fmt.Errorf("nil response from CreateAccessKey") } if createAccessKeyRes.AccessKey.AccessKeyId == nil || createAccessKeyRes.AccessKey.SecretAccessKey == nil { - return logical.ErrorResponse("nil AccessKeyId or SecretAccessKey returned from CreateAccessKey"), nil + return nil, fmt.Errorf("nil AccessKeyId or SecretAccessKey returned from CreateAccessKey") } oldAccessKey := config.AccessKey @@ -86,12 +87,10 @@ func (b *backend) pathConfigRotateRootUpdate(ctx context.Context, req *logical.R newEntry, err := logical.StorageEntryJSON("config/root", config) if err != nil { - return logical.ErrorResponse(fmt.Sprintf("error generating new root config: %v", err)), nil + return nil, errwrap.Wrapf("error generating new config/root JSON: {{err}}", err) } - // TODO: Should we return the full error message? Are there any scenarios in which it could expose - // the underlying creds? (The idea of rotate-root is that it should never expose credentials.) if err := req.Storage.Put(ctx, newEntry); err != nil { - return logical.ErrorResponse(fmt.Sprintf("error saving new root config: %v", err)), nil + return nil, errwrap.Wrapf("error saving new config/root: {{err}}", err) } deleteAccessKeyInput := iam.DeleteAccessKeyInput{ @@ -100,7 +99,7 @@ func (b *backend) pathConfigRotateRootUpdate(ctx context.Context, req *logical.R } _, err = client.DeleteAccessKey(&deleteAccessKeyInput) if err != nil { - return logical.ErrorResponse(fmt.Sprintf("error deleting old access key: %v", err)), nil + return nil, errwrap.Wrapf("Error deleting old access key: {{err}}", err) } return &logical.Response{ diff --git a/website/source/api/secret/aws/index.html.md b/website/source/api/secret/aws/index.html.md index 330557653241..fd246dda88e6 100644 --- a/website/source/api/secret/aws/index.html.md +++ b/website/source/api/secret/aws/index.html.md @@ -91,7 +91,7 @@ to AWS may fail for a few seconds until AWS becomes consistent again. In order to call this endpoint, Vault's AWS access key MUST be the only access key on the IAM user; otherwise, generation of a new access key will fail. Once this method is called, Vault will now be the only entity that knows the AWS -secret key is uses to access AWS. +secret key is used to access AWS. | Method | Path | Produces | | :------- | :--------------------------- | :--------------------- | From b86f6cc97ce4432fcb20416878b8d6c955181645 Mon Sep 17 00:00:00 2001 From: Joel Thompson Date: Fri, 14 Sep 2018 20:13:54 -0400 Subject: [PATCH 6/8] Fix casing in error messages --- builtin/logical/aws/path_config_rotate_root.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/logical/aws/path_config_rotate_root.go b/builtin/logical/aws/path_config_rotate_root.go index 11ef36ae0a03..d46e711469ef 100644 --- a/builtin/logical/aws/path_config_rotate_root.go +++ b/builtin/logical/aws/path_config_rotate_root.go @@ -40,11 +40,11 @@ func (b *backend) pathConfigRotateRootUpdate(ctx context.Context, req *logical.R return nil, err } if rawRootConfig == nil { - return nil, fmt.Errorf("No configuration found for config/root") + return nil, fmt.Errorf("no configuration found for config/root") } var config rootConfig if err := rawRootConfig.DecodeJSON(&config); err != nil { - return nil, errwrap.Wrapf("Error reading root configuration: {{err}}", err) + return nil, errwrap.Wrapf("error reading root configuration: {{err}}", err) } if config.AccessKey == "" || config.SecretKey == "" { @@ -99,7 +99,7 @@ func (b *backend) pathConfigRotateRootUpdate(ctx context.Context, req *logical.R } _, err = client.DeleteAccessKey(&deleteAccessKeyInput) if err != nil { - return nil, errwrap.Wrapf("Error deleting old access key: {{err}}", err) + return nil, errwrap.Wrapf("error deleting old access key: {{err}}", err) } return &logical.Response{ From 8e23e0cc20080f189894c2403daecc9be82aa936 Mon Sep 17 00:00:00 2001 From: Joel Thompson Date: Thu, 20 Sep 2018 21:00:32 -0400 Subject: [PATCH 7/8] Fix merge errors --- builtin/logical/aws/backend.go | 4 ++-- builtin/logical/aws/backend_test.go | 8 ++++---- builtin/logical/aws/client.go | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/builtin/logical/aws/backend.go b/builtin/logical/aws/backend.go index 5a534557761f..a895d54cde8b 100644 --- a/builtin/logical/aws/backend.go +++ b/builtin/logical/aws/backend.go @@ -102,7 +102,7 @@ func (b *backend) clientIAM(ctx context.Context, s logical.Storage) (iamiface.IA return b.iamClient, nil } - iamClient, err := clientIAM(ctx, s) + iamClient, err := b.nonCachedClientIAM(ctx, s) if err != nil { return nil, err } @@ -129,7 +129,7 @@ func (b *backend) clientSTS(ctx context.Context, s logical.Storage) (stsiface.ST return b.stsClient, nil } - stsClient, err := clientSTS(ctx, s) + stsClient, err := b.nonCachedClientSTS(ctx, s) if err != nil { return nil, err } diff --git a/builtin/logical/aws/backend_test.go b/builtin/logical/aws/backend_test.go index 05f75e6f68e5..14982d4605a5 100644 --- a/builtin/logical/aws/backend_test.go +++ b/builtin/logical/aws/backend_test.go @@ -442,17 +442,17 @@ func testAccStepRotateRoot(oldAccessKey *awsAccessKey) logicaltest.TestStep { if resp == nil { return fmt.Errorf("received nil response from config/rotate-root") } - newAccessKeyId := resp.Data["access_key"].(string) - if newAccessKeyId == oldAccessKey.AccessKeyId { + newAccessKeyID := resp.Data["access_key"].(string) + if newAccessKeyID == oldAccessKey.AccessKeyID { return fmt.Errorf("rotate-root didn't rotate access key") } awsConfig := &aws.Config{ Region: aws.String("us-east-1"), HTTPClient: cleanhttp.DefaultClient(), - Credentials: credentials.NewStaticCredentials(oldAccessKey.AccessKeyId, oldAccessKey.SecretAccessKey, ""), + Credentials: credentials.NewStaticCredentials(oldAccessKey.AccessKeyID, oldAccessKey.SecretAccessKey, ""), } // sigh.... - oldAccessKey.AccessKeyId = newAccessKeyId + oldAccessKey.AccessKeyID = newAccessKeyID log.Println("[WARN] Sleeping for 10 seconds waiting for AWS...") time.Sleep(10 * time.Second) svc := sts.New(session.New(awsConfig)) diff --git a/builtin/logical/aws/client.go b/builtin/logical/aws/client.go index ccb5f0d819a6..42c7639b6f6a 100644 --- a/builtin/logical/aws/client.go +++ b/builtin/logical/aws/client.go @@ -70,7 +70,7 @@ func (b *backend) getRootConfig(ctx context.Context, s logical.Storage, clientTy }, nil } -func (b *backend) clientIAM(ctx context.Context, s logical.Storage) (*iam.IAM, error) { +func (b *backend) nonCachedClientIAM(ctx context.Context, s logical.Storage) (*iam.IAM, error) { awsConfig, err := b.getRootConfig(ctx, s, "iam") if err != nil { return nil, err @@ -84,7 +84,7 @@ func (b *backend) clientIAM(ctx context.Context, s logical.Storage) (*iam.IAM, e return client, nil } -func (b *backend) clientSTS(ctx context.Context, s logical.Storage) (*sts.STS, error) { +func (b *backend) nonCachedClientSTS(ctx context.Context, s logical.Storage) (*sts.STS, error) { awsConfig, err := b.getRootConfig(ctx, s, "sts") if err != nil { return nil, err From f8f020f815085e51900854a78acf4d62c89ed1f0 Mon Sep 17 00:00:00 2001 From: Joel Thompson Date: Thu, 20 Sep 2018 21:58:32 -0400 Subject: [PATCH 8/8] Fix locking bugs --- builtin/logical/aws/backend.go | 8 +++----- builtin/logical/aws/client.go | 13 ++++++------- builtin/logical/aws/path_config_root.go | 6 ++---- builtin/logical/aws/path_config_rotate_root.go | 8 ++++++-- 4 files changed, 17 insertions(+), 18 deletions(-) diff --git a/builtin/logical/aws/backend.go b/builtin/logical/aws/backend.go index a895d54cde8b..1c621f119924 100644 --- a/builtin/logical/aws/backend.go +++ b/builtin/logical/aws/backend.go @@ -61,9 +61,7 @@ type backend struct { // Mutex to protect access to reading and writing policies roleMutex sync.RWMutex - // Mutex to protect access to reading and writing root creds - rootMutex sync.RWMutex - // Mutex to protect access to iam/sts clients + // Mutex to protect access to iam/sts clients and client configs clientMutex sync.RWMutex // iamClient and stsClient hold configured iam and sts clients for reuse, and @@ -102,7 +100,7 @@ func (b *backend) clientIAM(ctx context.Context, s logical.Storage) (iamiface.IA return b.iamClient, nil } - iamClient, err := b.nonCachedClientIAM(ctx, s) + iamClient, err := nonCachedClientIAM(ctx, s) if err != nil { return nil, err } @@ -129,7 +127,7 @@ func (b *backend) clientSTS(ctx context.Context, s logical.Storage) (stsiface.ST return b.stsClient, nil } - stsClient, err := b.nonCachedClientSTS(ctx, s) + stsClient, err := nonCachedClientSTS(ctx, s) if err != nil { return nil, err } diff --git a/builtin/logical/aws/client.go b/builtin/logical/aws/client.go index 42c7639b6f6a..8c56b8d1b2d2 100644 --- a/builtin/logical/aws/client.go +++ b/builtin/logical/aws/client.go @@ -15,13 +15,12 @@ import ( "github.com/hashicorp/vault/logical" ) -func (b *backend) getRootConfig(ctx context.Context, s logical.Storage, clientType string) (*aws.Config, error) { +// NOTE: The caller is required to ensure that b.clientMutex is at least read locked +func getRootConfig(ctx context.Context, s logical.Storage, clientType string) (*aws.Config, error) { credsConfig := &awsutil.CredentialsConfig{} var endpoint string var maxRetries int = aws.UseServiceDefaultRetries - b.rootMutex.RLock() - defer b.rootMutex.RUnlock() entry, err := s.Get(ctx, "config/root") if err != nil { return nil, err @@ -70,8 +69,8 @@ func (b *backend) getRootConfig(ctx context.Context, s logical.Storage, clientTy }, nil } -func (b *backend) nonCachedClientIAM(ctx context.Context, s logical.Storage) (*iam.IAM, error) { - awsConfig, err := b.getRootConfig(ctx, s, "iam") +func nonCachedClientIAM(ctx context.Context, s logical.Storage) (*iam.IAM, error) { + awsConfig, err := getRootConfig(ctx, s, "iam") if err != nil { return nil, err } @@ -84,8 +83,8 @@ func (b *backend) nonCachedClientIAM(ctx context.Context, s logical.Storage) (*i return client, nil } -func (b *backend) nonCachedClientSTS(ctx context.Context, s logical.Storage) (*sts.STS, error) { - awsConfig, err := b.getRootConfig(ctx, s, "sts") +func nonCachedClientSTS(ctx context.Context, s logical.Storage) (*sts.STS, error) { + awsConfig, err := getRootConfig(ctx, s, "sts") if err != nil { return nil, err } diff --git a/builtin/logical/aws/path_config_root.go b/builtin/logical/aws/path_config_root.go index e7b945cfe393..51e353787b38 100644 --- a/builtin/logical/aws/path_config_root.go +++ b/builtin/logical/aws/path_config_root.go @@ -56,8 +56,8 @@ func (b *backend) pathConfigRootWrite(ctx context.Context, req *logical.Request, stsendpoint := data.Get("sts_endpoint").(string) maxretries := data.Get("max_retries").(int) - b.rootMutex.Lock() - defer b.rootMutex.Unlock() + b.clientMutex.Lock() + defer b.clientMutex.Unlock() entry, err := logical.StorageEntryJSON("config/root", rootConfig{ AccessKey: data.Get("access_key").(string), @@ -77,8 +77,6 @@ func (b *backend) pathConfigRootWrite(ctx context.Context, req *logical.Request, // clear possible cached IAM / STS clients after successfully updating // config/root - b.clientMutex.Lock() - defer b.clientMutex.Unlock() b.iamClient = nil b.stsClient = nil diff --git a/builtin/logical/aws/path_config_rotate_root.go b/builtin/logical/aws/path_config_rotate_root.go index d46e711469ef..a69c342e0b47 100644 --- a/builtin/logical/aws/path_config_rotate_root.go +++ b/builtin/logical/aws/path_config_rotate_root.go @@ -33,8 +33,9 @@ func (b *backend) pathConfigRotateRootUpdate(ctx context.Context, req *logical.R return nil, fmt.Errorf("nil IAM client") } - b.rootMutex.Lock() - defer b.rootMutex.Unlock() + b.clientMutex.Lock() + defer b.clientMutex.Unlock() + rawRootConfig, err := req.Storage.Get(ctx, "config/root") if err != nil { return nil, err @@ -93,6 +94,9 @@ func (b *backend) pathConfigRotateRootUpdate(ctx context.Context, req *logical.R return nil, errwrap.Wrapf("error saving new config/root: {{err}}", err) } + b.iamClient = nil + b.stsClient = nil + deleteAccessKeyInput := iam.DeleteAccessKeyInput{ AccessKeyId: aws.String(oldAccessKey), UserName: getUserRes.User.UserName,