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

keystore: add support for custom kms tags #48132

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

dboslee
Copy link
Contributor

@dboslee dboslee commented Oct 30, 2024

Adds support for configuring custom tags which replace the default TeleportCluster tag. This will be used by cloud. Its important to note that changing this configuration for a Teleport cluster already running on AWS KMS could orphan keys as Teleport will only manage keys with the configured set of tags.

Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-48132.d3pp5qlev8mo18.amplifyapp.com

lib/utils/utils.go Outdated Show resolved Hide resolved
lib/auth/keystore/aws_kms_test.go Outdated Show resolved Hide resolved
lib/auth/keystore/aws_kms_test.go Show resolved Hide resolved
Comment on lines +404 to 411
// All tags must match for this key to be considered for deletion.
for k, v := range a.tags {
if !slices.ContainsFunc(output.Tags, func(tag kmstypes.Tag) bool {
return aws.ToString(tag.TagKey) == k && aws.ToString(tag.TagValue) == v
}) {
return nil
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please update the tests so TestAWSKMS_DeleteUnusedKeys runs with and without custom tags set? you can turn that whole test into a subtest that takes a tags param I guess

And I want to stress it's going to be very important that some tag that's unique for the cluster gets set. For example, I would hate for some customer to stumble upon this config setting and set their own tag like owner: me - and then every KMS key with that tag will get deleted by this. Can we actually forcibly prevent this somehow? Maybe require at least one tag value to include the cluster name, or add the default TeleportCluster tag if none of the configured tags include the cluster name

Copy link
Contributor Author

@dboslee dboslee Nov 1, 2024

Choose a reason for hiding this comment

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

Let me think about this some more. Perhaps the best path is to keep the TeleportCluster tag and have the tags field allow for additional tags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think of 730d1dc

It will add the TeleportCluster tag if its not specified in the tag configuration.

lib/config/fileconf.go Outdated Show resolved Hide resolved
@dboslee dboslee added the no-changelog Indicates that a PR does not require a changelog entry label Nov 5, 2024
lib/auth/keystore/aws_kms.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a PR does not require a changelog entry size/md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants