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

Support for access key rotation #60

Open
kimvanwyk opened this issue Jan 19, 2022 · 16 comments
Open

Support for access key rotation #60

kimvanwyk opened this issue Jan 19, 2022 · 16 comments
Labels
enhancement New feature or request

Comments

@kimvanwyk
Copy link

Currently, when issuing create for an existing bucket and user, a new access key and secret key is created for that user and added to the user's set of access keys. For some security-related use cases though it would be helpful to be able to rotate access keys, so only the most recently added key is valid. Perhaps a switch like "--rotate-access-key" or similar could be added, which would deactivate (and possibly then remove) the user's existing access keys when adding a new one?

@simonw simonw added the enhancement New feature or request label Jan 19, 2022
@simonw
Copy link
Owner

simonw commented Jan 19, 2022

This is a good idea - I hadn't considered key rotation at all.

It looks like there's an update_access_key() method at https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/iam.html#IAM.Client.update_access_key which is designed for this:

Changes the status of the specified access key from Active to Inactive, or vice versa. This operation can be used to disable a user's key as part of a key rotation workflow.

[...]

For information about rotating keys, see Managing keys and certificates in the IAM User Guide .

Used like this:

response = client.update_access_key(
    UserName='string',
    AccessKeyId='string',
    Status='Active'|'Inactive'
)

@simonw
Copy link
Owner

simonw commented Jan 19, 2022

I'm going to go with --deactivate-existing-keys as the option for this - I think that's the right level of clarity for what will happen. The problem with the term "rotate" is that it could be misinterpreted as indicating that keys will be automatically rotated in some way, when that's still a step that the user has to manually perform afterwards.

@simonw
Copy link
Owner

simonw commented Jan 19, 2022

Oh interesting - an IAM user is only allowed a maximum of two access keys according to https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_iam-quotas.html - and that's not a quota you can request to increase.

Amusingly the list_access_keys method at https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/iam.html#IAM.Client.list_access_keys says:

Although each user is limited to a small number of keys, you can still paginate the results using the MaxItems and Marker parameters.

Good for them for opting for API consistency over all else!

@simonw
Copy link
Owner

simonw commented Jan 19, 2022

I'm now thinking that having this as yet another option to s3-credentials create may not make sense - it should probably be a separate command entirely.

And if it's a separate command, I'm OK using the "rotate" terminology, especially now that I know the user can only have two keys.

Maybe this then:

s3-credentials rotate name-of-user

Which would deactivate their existing key and issue a new one.

(I'm beginning to regret calling this command s3-credentials now - aws-credentials might actually be a better name for it.)

@simonw
Copy link
Owner

simonw commented Jan 19, 2022

I checked to see if AWS already provide a good tool for rotating access keys and as with so many AWS things it's a relatively complex multi-step process: https://aws.amazon.com/blogs/security/how-to-rotate-access-keys-for-iam-users/

@simonw
Copy link
Owner

simonw commented Jan 19, 2022

Interesting challenge: the user is allowed at most two access keys. If you are rotating for the first time this is OK - you mark their existing access key as inactive and create a new one.

But... if you rotate a second time, you won't be able to do this - AWS will not let you create a third access key without first deleting one of the others.

So the command needs to be able to spot this case and either delete the oldest inactive key for you OR give you a prompt that lets you confirm that you would like to do that.

Maybe it prompts you for which one to delete, but if you pass --force it deletes any inactive ones... or the oldest active key otherwise.

@simonw
Copy link
Owner

simonw commented Jan 19, 2022

Having it as a separate command is also better because the concept of "rotating access keys" doesn't apply at all to temporary credentials created using the --duration 15m option (which are made using STS.assume_role()) - so if it was part of s3-credentials create I would have to add validation that throws an error if you attempt to use both --duration and --deactivate-existing-keys at the same time.

@simonw
Copy link
Owner

simonw commented Jan 19, 2022

I should include a full integration test in the implementation, which creates a brand new user, rotates they key a couple of times and then deletes them.

@simonw
Copy link
Owner

simonw commented Jan 19, 2022

I'm thinking that s3-credentials rotate should be a VERY noisy command that expects to usually be run manually, not automatically - so it's OK for it to output a lot of detail about what it's doing.

It should output to stderr though, so that only the final credentials JSON or INI is written to stdin. And it should support --silent.

s3-credentials create works like that already:

def log(message):
if not silent:
click.echo(message, err=True)

@simonw
Copy link
Owner

simonw commented Jan 19, 2022

Oh hang on... the whole point of key rotation usually is that you get to create a new key, then configure things to start using that new key, and THEN deactivate the old key - so you can avoid any downtime to whatever is making the authenticated calls.

Need to think a bit harder about what that would mean for this feature.

Maybe it doesn't make sense to have a single rotate command, since there are other manual steps in between creating the new key and deactivating the old one?

@kimvanwyk
Copy link
Author

I agree, a separate, deliberately invoked and noisy command would be best.

@kimvanwyk
Copy link
Author

kimvanwyk commented Jan 19, 2022

In terms of the steps in rotating, I was first thinking you could deactivate the oldest key, but there's no guarantee only the newer key is in use.

Perhaps a deactivate-oldest-access-key command, which does what it says on the tin. Then the create command can make a fresh access key as usual. It wouldn't however necessarily be obvious that one would need the deactivate-oldest-access-key command before calling the create command

@kimvanwyk
Copy link
Author

kimvanwyk commented Jan 19, 2022

Incidentally, trying to call create with the same username 3 times hits the issue you've noted above, that there can only be 2 active access keys:

s3-credentials create --username USERNAME "BUCKET_NAME"
Attached policy s3.read-write.USERNAME to user USERNAME
Traceback (most recent call last):
  File "/home/kimv/work/data_engineering/sbsa_archive/.venv/bin/s3-credentials", line 8, in <module>
    sys.exit(cli())
  File "/home/kimv/work/data_engineering/sbsa_archive/.venv/lib/python3.9/site-packages/click/core.py", line 1128, in __call__
    return self.main(*args, **kwargs)
  File "/home/kimv/work/data_engineering/sbsa_archive/.venv/lib/python3.9/site-packages/click/core.py", line 1053, in main
    rv = self.invoke(ctx)
  File "/home/kimv/work/data_engineering/sbsa_archive/.venv/lib/python3.9/site-packages/click/core.py", line 1659, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/kimv/work/data_engineering/sbsa_archive/.venv/lib/python3.9/site-packages/click/core.py", line 1395, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/kimv/work/data_engineering/sbsa_archive/.venv/lib/python3.9/site-packages/click/core.py", line 754, in invoke
    return __callback(*args, **kwargs)
  File "/home/kimv/work/data_engineering/sbsa_archive/.venv/lib/python3.9/site-packages/s3_credentials/cli.py", line 457, in create
    response = iam.create_access_key(
  File "/home/kimv/work/data_engineering/sbsa_archive/.venv/lib/python3.9/site-packages/botocore/client.py", line 391, in _api_call
    return self._make_api_call(operation_name, kwargs)
  File "/home/kimv/work/data_engineering/sbsa_archive/.venv/lib/python3.9/site-packages/botocore/client.py", line 719, in _make_api_call
    raise error_class(parsed_response, operation_name)
botocore.errorfactory.LimitExceededException: An error occurred (LimitExceeded) when calling the CreateAccessKey operation: Cannot exceed quota for AccessKeysPerUser: 2

Deactivating one of the keys wasn't enough - I got the same AccessKeysPerUser quota error when running the create command.

After I deleted an existing access key the create command worked as expected.

@simonw
Copy link
Owner

simonw commented Jan 19, 2022

I'm coming back around to the idea of an interactive rotate command - one which outputs the new credentials and says "now go and start using those, then hit ENTER when you are ready to deactivate the old one."

@kimvanwyk
Copy link
Author

I'm coming back around to the idea of an interactive rotate command - one which outputs the new credentials and says "now go and start using those, then hit ENTER when you are ready to deactivate the old one."

This would be a good approach. It would presumably need to prompt for a set of credentials to delete first if the user already has 2, before it can make new credentials.

@jonathankretzmer
Copy link

I'm coming back around to the idea of an interactive rotate command - one which outputs the new credentials and says "now go and start using those, then hit ENTER when you are ready to deactivate the old one."

This would be a good approach. It would presumably need to prompt for a set of credentials to delete first if the user already has 2, before it can make new credentials.

Perhaps by providing a portion of the key to string match, a hash, or to automatically select the oldest. Providing such through the command line would then help with fully automated use-cases.

As an aside, great tool @simonw :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants