-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat(bigtable): Customer Managed Encryption (CMEK) #3899
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial thoughts, happy to discuss further!
CONTRIBUTING.md
Outdated
--keyring $MY_KEYRING \ | ||
--location $MY_LOCATION \ | ||
--role roles/cloudkms.cryptoKeyEncrypterDecrypter \ | ||
--member allAuthenticatedUsers \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this true? Wouldn't you want this to point to a service account?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured that, as this encryption key isn't really a secret, opening it up to all authn'd users was fine. I could be mistaken but I also think adding a service account here would be another dependency/setup step to being a contributor.
@tritone do you know what gsutil kms
is doing behind the scenes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm not exactly sure what the question is here but here's the code: https://github.com/GoogleCloudPlatform/gsutil/blob/7845859b924004d16db0408335fd434424f6c8d7/gslib/commands/kms.py
Looks like it requests a service account and creates one if it doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you re-use the service account used elsewhere in the setup? GCLOUD_TESTS_GOLANG_KEY
should already point to a service account json; is there a gcloud command to grab the email from that? I see https://cloud.google.com/sdk/gcloud/reference/iam/service-accounts/list . I just don't know enough about IAM to be confident that adding this binding isn't some kind of security risk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the complication here is we store the path to the key, but the email you need (the member id) for the service account is within the json.
"client_email": "{PROJECT}@{PROJECT}.iam.gserviceaccount.com",
I could infer this as it generally follows that form, but it may not. I was avoiding adding another EnvVar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are still 1 or 2 open comments but this LGTM, I'll let someone with bigtable knowledge sign off though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with some nits (and my one comment about key creation is still outstanding)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple little questions, generally looking good!
CONTRIBUTING.md
Outdated
--keyring $MY_KEYRING \ | ||
--location $MY_LOCATION \ | ||
--role roles/cloudkms.cryptoKeyEncrypterDecrypter \ | ||
--member allAuthenticatedUsers \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm not exactly sure what the question is here but here's the code: https://github.com/GoogleCloudPlatform/gsutil/blob/7845859b924004d16db0408335fd434424f6c8d7/gslib/commands/kms.py
Looks like it requests a service account and creates one if it doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending resolution for @kolea2 's question re: contributing setup. Also, make sure to run all integration tests to ensure that your refactor with the views is not breaking anything (nice change btw!).
CONTRIBUTING.md
Outdated
--keyring $MY_KEYRING \ | ||
--location $MY_LOCATION \ | ||
--role roles/cloudkms.cryptoKeyEncrypterDecrypter \ | ||
--member allAuthenticatedUsers \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you re-use the service account used elsewhere in the setup? GCLOUD_TESTS_GOLANG_KEY
should already point to a service account json; is there a gcloud command to grab the email from that? I see https://cloud.google.com/sdk/gcloud/reference/iam/service-accounts/list . I just don't know enough about IAM to be confident that adding this binding isn't some kind of security risk.
No description provided.