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.

Sign up for GitHub

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

Jump to bottom

Remove KMS requiring metadata files (closes #4375) #4700

Merged
merged 19 commits into from
Jan 2, 2024
Merged

Remove KMS requiring metadata files (closes #4375) #4700

merged 19 commits into from
Jan 2, 2024

Conversation

keeganwitt
Copy link
Contributor

Pull Request check list

  • Commit conforms to CONTRIBUTING.md?
  • Proper tests/regressions included?
  • Documentation updated?

Affected functionality
Removes requirement to have KMS metadata kept in files.

Description of change
Adds new config options to avoid the need to use metadata files.

Which issue this PR fixes
#4375

@keeganwitt keeganwitt force-pushed the remove_kms_metadata_file_dependencies branch from 22f1702 to ab8706c Compare November 28, 2023 23:47
@keeganwitt
Remove KMS requiring metadata files (closes spiffe#4375)
a6957bb 
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
@keeganwitt keeganwitt force-pushed the remove_kms_metadata_file_dependencies branch from ab8706c to a6957bb Compare November 29, 2023 14:57
@keeganwitt keeganwitt marked this pull request as ready for review November 29, 2023 17:06
@amartinezfayo amartinezfayo mentioned this pull request Dec 1, 2023
Open
amartinezfayo
amartinezfayo reviewed Dec 1, 2023
View reviewed changes
Copy link
Member

@amartinezfayo amartinezfayo left a comment

Choose a reason for hiding this comment

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

Thank you @keeganwitt for this contribution!

I have some concerns, related with the config name, allowing to have both the key_metadata_file and the key_metadata setting at the same time, and the validation of the key_metadata value configured.
I've commented on the AWS plugin, but the same comments apply to the others. Please let me know what you think. Thanks again!

pkg/server/plugin/keymanager/awskms/awskms.go Outdated Show resolved Hide resolved
pkg/server/plugin/keymanager/awskms/awskms.go Outdated Show resolved Hide resolved
pkg/server/plugin/keymanager/awskms/awskms.go Outdated Show resolved Hide resolved
@keeganwitt
PR feedback changes
58085c4 
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
@keeganwitt keeganwitt force-pushed the remove_kms_metadata_file_dependencies branch from ece2cb2 to 58085c4 Compare December 5, 2023 16:05
@keeganwitt
Add Azure key validation
834212d 
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
@keeganwitt
Ensure server ID is always logged
a0bc1e5 
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
@keeganwitt
Replace backticks with single quotes
023e717 
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
amartinezfayo
amartinezfayo reviewed Dec 15, 2023
View reviewed changes
Copy link
Member

@amartinezfayo amartinezfayo left a comment

Choose a reason for hiding this comment

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

Thank you @keeganwitt for your patience. I went over the validations that should be applied in all the cases and left a couple of comments related with that. I also have some suggestions for the documentation (I did it only for the AWS KMS case, but applies to the rest also).
We should be ready to go after this is addressed. Thanks again!

pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go Outdated Show resolved Hide resolved
pkg/server/plugin/keymanager/gcpkms/gcpkms.go Show resolved Hide resolved
doc/plugin_server_keymanager_aws_kms.md Outdated Show resolved Hide resolved
doc/plugin_server_keymanager_aws_kms.md Outdated Show resolved Hide resolved
doc/plugin_server_keymanager_aws_kms.md Outdated Show resolved Hide resolved
@keeganwitt keeganwitt force-pushed the remove_kms_metadata_file_dependencies branch 2 times, most recently from 2834f6a to 3ab3205 Compare December 16, 2023 04:45
keeganwitt and others added 4 commits December 15, 2023 23:46
@keeganwitt @amartinezfayo
Update doc/plugin_server_keymanager_aws_kms.md
0b8c4f3 
Co-authored-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
@keeganwitt @amartinezfayo
Update doc/plugin_server_keymanager_aws_kms.md
0b79dd1 
Co-authored-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
@keeganwitt @amartinezfayo
Update doc/plugin_server_keymanager_aws_kms.md
9be5ede 
Co-authored-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
@keeganwitt
Remove character check for Azure
45b2cb1 
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
@keeganwitt keeganwitt force-pushed the remove_kms_metadata_file_dependencies branch 2 times, most recently from 8a73374 to 8d7ad4d Compare December 16, 2023 05:03
@keeganwitt
Add GCP validation
fb061c0 
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
@keeganwitt keeganwitt force-pushed the remove_kms_metadata_file_dependencies branch from 8d7ad4d to fb061c0 Compare December 17, 2023 00:29
@keeganwitt
GCP validation PR suggestions
e078c06 
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
@keeganwitt keeganwitt force-pushed the remove_kms_metadata_file_dependencies branch from bc27a92 to e078c06 Compare December 19, 2023 02:57
keeganwitt and others added 3 commits December 19, 2023 14:55
@keeganwitt
Fix unit test
3ab2e5a 
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
@keeganwitt
Fix unit test
d6f09c6 
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
@amartinezfayo
Merge branch 'main' into remove_kms_metadata_file_dependencies
e357dd9
amartinezfayo
amartinezfayo reviewed Dec 26, 2023
View reviewed changes
Copy link
Member

@amartinezfayo amartinezfayo left a comment

Choose a reason for hiding this comment

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

Just a couple of final comments:

  • The gcp_kms plugin is missing the deprecation warning log when using key_metadata_file.
  • Could you update server_full.conf with the changes?

Thanks!

@amartinezfayo amartinezfayo mentioned this pull request Dec 27, 2023
Closed
@keeganwitt
Replace deprecated key_metadata_file with key_identifier_file
7dffcfa 
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
@keeganwitt
Mark key_metadata_file as not required
8141cbf 
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
@keeganwitt
Indicate that one of key_identifier_file and key_identifier_file are …
09d54e4 
…required

Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
@keeganwitt
Copy link
Contributor Author

  • The gcp_kms plugin is missing the deprecation warning log when using key_metadata_file.

I think I have this already? https://github.com/spiffe/spire/pull/4700/files#diff-3dc348b71fdca86ef32e199c452c68d0bc8c70438010305d256fd94fa112ef80R1135

  • Could you update server_full.conf with the changes?

Done.

@amartinezfayo
Copy link
Member

I was referring that the log.Warn("'key_metadata_file' is deprecated in favor of 'key_identifier_file' and will be removed in a future version") warning seems to be missing in the gcp_kms plugin. I can see it in the aws_kms and azure_key_vault but not in gcp_kms?

@keeganwitt
Log that key_metadata_file is deprecated
c500465 
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
@keeganwitt
Copy link
Contributor Author

I was referring that the log.Warn("'key_metadata_file' is deprecated in favor of 'key_identifier_file' and will be removed in a future version") warning seems to be missing in the gcp_kms plugin. I can see it in the aws_kms and azure_key_vault but not in gcp_kms?

Ah, gotcha. Added.

amartinezfayo
amartinezfayo approved these changes Jan 2, 2024
View reviewed changes
Copy link
Member

@amartinezfayo amartinezfayo left a comment

Choose a reason for hiding this comment

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

🎉 Thank you @keeganwitt!

@amartinezfayo amartinezfayo merged commit 1148b9f into spiffe:main Jan 2, 2024
32 checks passed
@MarcosDY MarcosDY added this to the 1.9.0 milestone Jan 3, 2024
@keeganwitt keeganwitt deleted the remove_kms_metadata_file_dependencies branch January 5, 2024 19:10
sriyer pushed a commit to spire-vault/spire that referenced this pull request Feb 23, 2024
@keeganwitt @sriyer
Remove KMS requiring metadata files (closes spiffe#4375) (spiffe#4700)
349a6b7 
* Remove KMS requiring metadata files (closes spiffe#4375)

Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
rushi47 pushed a commit to rushi47/spire that referenced this pull request Apr 11, 2024
@keeganwitt @rushi47
Remove KMS requiring metadata files (closes spiffe#4375) (spiffe#4700)
6320622 
* Remove KMS requiring metadata files (closes spiffe#4375)

Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evan2645 evan2645 evan2645 left review comments

@amartinezfayo amartinezfayo amartinezfayo approved these changes

@azdagron azdagron Awaiting requested review from azdagron azdagron is a code owner

@MarcosDY MarcosDY Awaiting requested review from MarcosDY MarcosDY is a code owner

@rturner3 rturner3 Awaiting requested review from rturner3 rturner3 is a code owner

Assignees

@amartinezfayo amartinezfayo

Labels
None yet
Projects
None yet
Milestone
1.9.0
Development

Successfully merging this pull request may close these issues.
4 participants
@keeganwitt @amartinezfayo @evan2645 @MarcosDY