feat: New S3 Backend for using KMS Keys as S3 Encryption Method #205
Conversation
Switch from using AES256 to KMS for storing individual keys and the latest.json file. The SSM Store supports KMS Encryption and allows more segmentation for which keys a user is allowed to read.
Store the latest keys for each service by the KMS key they were written with. Doing so allows for Chamber to use KMS Encryption on the latest file as well as the individual keys in the service.
Store the KMS Key and the version in the latest file. This allows us to combine the latest files together (even though there is a __latest.json file per KMS Key).
Instead of attempting to migrate between KMS Keys and leaving the first KMS's latest file in an inaccurate state, we ask users to delete and recreate the chamber secret with the new key.
Before writing an existing secret with a new KMS Key, ask the user to delete the existing Chamber secret with the current key. Tell them which KMS key the secret is currently encrypted with.
When attempting to read the __latest.json for a KMS Key, treat AccessDenied as the file not existing. We do this because the user may not be able to read keys in the service that are encrypted with a KMS Key they don't have access to. If we get a different kind of error then panic, this isn't my prefered way to handle the error but the S3 pages API doesn't allow to pass back err as a result.
The changes I've made to the __latest.json format (both schema changes and made multiple of these files, one per KMS Key used) are not backwards compadible with the previous implementation of the S3 backend.
Detail how to use it and it's features.
This seems to be more traditional and I was just working around the lack of err in the AWS ListObjectsPages method.
To avoid treating any secret names that start with kms as index files.
I coppied this from the S3Store interface but this was only being used as a proxy for NewS3StoreWithBucket. Since this backend is brand new it can just have the NewS3KMSStoreWithBucket method and doesn't need to expose the legacy method.
There is a lot of overlap bettween the S3Store and S3KMSStore (because I coppied and pasted the entire thing to begin with). I'm removing a lot of the shared code and just using method overriding for the methods that have changes.
Previously we used an environment variable or set a default. However, in order to added a flag to the UI it would be cleaner to pass in into the store interface.
To support making the command line more usable (and removing the setup of environment being the main interface) the S3-KMS backend now allows setting the KMS Key Alias via the command line interface. The environment variable will be prefered.
This reverts commit cbb5b4d. This broke the List operations (because it removed the overriding of the base class. This caused an issue because the List operation now searches for keys in multiple kms key latest index files).
This can be shared with the S3Store interface via function overriding.
…nctions Instead of maintaining two sets of code for the S3KMSStore and S3Store, instead share functions that are similar between the modules (eg, history, read). The biggest differences are in List (Where we index of multiple latest files in the KMS version but only one in the S3Store), Write and Delete (where we must handle writing with different keys).
All kms key aliases need to be prefixed with "alias/"keyname
Allow passing in the KMS Key Alias to write and delete keys with to the SSM Backend as well as the S3KMSStore
|
Hi @nickatsegment! I've implemented the requested changes from #195. We've been running this backend in production for a few months and have been quite happy with the results. |
|
Hey all, any update on this? Happy to help out with any changes that might be needed to get it merged. 😃 |
store/ssmstore.go
Outdated
| } | ||
|
|
||
| // NewSSMStore creates a new SSMStore | ||
| func NewSSMStore(numRetries int) (*SSMStore, error) { | ||
| func NewSSMStore(numRetries int, kmsKeyAlias string) (*SSMStore, error) { |
There was a problem hiding this comment.
It's important to avoid breaking compatibility here. You should make a new function (say NewSSMStoreWithKMSKeyAlias) and mark this function as deprecated. See https://github.com/segmentio/chamber/blob/master/store/s3store.go#L61 for an example.
Adding a non-public field to the struct is fine.
There was a problem hiding this comment.
Actually, do you need this change for the new backend to work? I appreciate the effort, but really this should probably be a separate PR.
There was a problem hiding this comment.
I wanted to make --kms-key-alias=alias/keyname work for the SSM backend too, because I thought it would be really confusing if as a user of that backend, I set that flag and it wasn’t used at all.
In this PR I’ll return an error if --kms-key-alias is used with the SSM backend. I’ll then make a follow up PR to introduce that flag to the SSM backend too.
There was a problem hiding this comment.
Awesome, thanks, that sounds good. Yeah, I always felt that the env var was an anti-pattern. https://github.com/segmentio/chamber/wiki/Style-Guide#avoid-environment-variables-in-library-code
store/ssmstore.go
Outdated
| }, nil | ||
| } | ||
|
|
||
| func (s *SSMStore) KMSKey() string { |
There was a problem hiding this comment.
I doubt this is called anywhere externally, but we need to keep this function for compatibility, until 3.0
Explode when --kms-key-alias is used with other backends that don't support it.
|
@nickatsegment Ready for re-review. |
Previously we were using AWS Parameter Store. However after hitting AWS Rate Limiting on Parameter Store and being refused a limit increase we decided to switch the Chamber S3 Backend. However, a major point of difference between the SSM Backend and the S3 Backend is that the SSM Backend supports AWS KMS Keys.
This PR switches from using AES256 Encryption (read the one key stored by all S3 servers) to using KMS Keys for Encryption (read specific keys that you create via AWS KMS Service, AWS Manages the KMS key but you have control over disabling them when they become compromised and restricting access to them via IAM Roles).
Along the way, I ran into the issue that Chamber is caching the keys and values for each service in a __latest.json file. To make the KMS Encryption worthwhile, I changed the __latest.json be a "latest" file per KMS Key. This differs slightly from the behavior of the SSM Store backend when the user doesn't provide the CHAMBER_KMS_KEY_ALIAS environment variable. In the SSM backend, you as the user are able to read any secrets which you have the KMS Key for. However In the S3 Version (with the changes that I've made, you're not able to see secrets encrypted with a KMS Key that you do not have access to).
Currently, the S3 Bucket looks like:
service-name:
__latest.json
Key Named JSON File (eg, database_name.json)
Key Named JSON File (eg, database_password.json)
Key Named JSON File (eg, database_username.json)
service-name:
__latest.json
Key Named JSON File (eg, database_name.json)
Key Named JSON File (eg, database_password.json)
Key Named JSON File (eg, database_username.json)
With these changes:
service-name:
alias_default_parameter_store_key__latest.json
Key Named JSON File (eg, database_name.json)
Key Named JSON File (eg, database_password.json)
Key Named JSON File (eg, database_username.json)
service-name:
alias_default_parameter_store_key__latest.json
alias_very_secret_kms_key__latest.json
Key Named JSON File (eg, database_name.json)
Key Named JSON File (eg, database_password.json)
Key Named JSON File (eg, database_username.json)
Implementation Decisions:
I've chosen to make chamber list not show secrets that are encrypted with KMS Keys that you do not have access to. This is because multiple KMS Keys can be used within a single chamber service and some readers will only have one key.
chamber write and chamber delete requires CHAMBER_KMS_KEY_ALIAS to be set to the same KMS Key as an existing Chamber secret. This avoids the latest version of a secret being left encrypted in S3 after you move the secret to a new KMS key.