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.

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

Add AWS Secret Engine Root Credential Rotation #5140

Merged
merged 12 commits into from
Sep 26, 2018

Conversation

joelthompson
Copy link
Contributor

@joelthompson joelthompson commented Aug 20, 2018

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).

TODO:

  • Add tests
  • Add docs
  • Add locking around writing/usage of root credentials

I'm a bit unclear about Seal Wrapping -- I think I don't need to do anything special here, and SealWrapStorage refers to paths in storage, but let me know if this assumption is wrong.

Fixes #4385

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 hashicorp#4385
Also fix a typo in the root credential rotation code
And wire the backend up in a bunch of places so the config can get the
lock
@joelthompson joelthompson changed the title WIP: Add AWS Secret Engine Root Credential Rotation Add AWS Secret Engine Root Credential Rotation Aug 21, 2018
@joelthompson
Copy link
Contributor Author

Finished the TODOs, should be ready for review! :)

@jefferai jefferai added this to the 0.11.1 milestone Aug 21, 2018
@jefferai jefferai modified the milestones: 0.11.1, 0.11.2 Sep 5, 2018
builtin/logical/aws/path_config_rotate_root.go Outdated Show resolved Hide resolved
builtin/logical/aws/path_config_rotate_root.go Outdated Show resolved Hide resolved
builtin/logical/aws/path_config_rotate_root.go Outdated Show resolved Hide resolved
website/source/api/secret/aws/index.html.md Outdated Show resolved Hide resolved
vishalnayak
vishalnayak previously approved these changes Sep 14, 2018
vishalnayak
vishalnayak previously approved these changes Sep 17, 2018
Copy link
Member

@vishalnayak vishalnayak left a comment

Choose a reason for hiding this comment

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

LGTM! You might want to remove the WIP from the PR description :-)

@joelthompson
Copy link
Contributor Author

This isn't ready to merge yet. The addition of the client caching and locking significantly complicates things here, and I'm worried the simple solution is vulnerable to a deadlock, so need to think this through more thoroughly.

Copy link
Contributor Author

@joelthompson joelthompson left a comment

Choose a reason for hiding this comment

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

OK, I think this should be ready, except for the one question about an error vs. warning response.

_, err = client.DeleteAccessKey(&deleteAccessKeyInput)
if err != nil {
return nil, errwrap.Wrapf("error deleting old access key: {{err}}", err)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this just be a warning response instead of returning an err? In this event, the access key was successfully rotated, but it just failed to clean up.

Copy link
Member

Choose a reason for hiding this comment

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

It should be an error.

Copy link
Member

@vishalnayak vishalnayak left a comment

Choose a reason for hiding this comment

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

LGTM!

@vishalnayak vishalnayak merged commit d184aa0 into hashicorp:master Sep 26, 2018
@joelthompson joelthompson deleted the aws_secret_root_rotation branch September 26, 2018 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AWS Secrets Engine Key rotation
3 participants