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

feat(bigtable): Customer Managed Encryption (CMEK) #3899

Merged
merged 33 commits into from Apr 29, 2021

Conversation

crwilcox
Copy link
Contributor

@crwilcox crwilcox commented Apr 7, 2021

No description provided.

@crwilcox crwilcox requested review from tritone and codyoss April 7, 2021 17:32
@product-auto-label product-auto-label bot added the api: bigtable Issues related to the Bigtable API. label Apr 7, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Apr 7, 2021
bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/integration_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@tritone tritone left a 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!

bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/integration_test.go Outdated Show resolved Hide resolved
bigtable/integration_test.go Show resolved Hide resolved
@crwilcox crwilcox added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 16, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 16, 2021
@crwilcox crwilcox marked this pull request as ready for review April 16, 2021 17:44
@crwilcox crwilcox requested a review from kolea2 as a code owner April 16, 2021 17:44
@crwilcox crwilcox requested a review from a team as a code owner April 16, 2021 17:44
CONTRIBUTING.md Outdated
--keyring $MY_KEYRING \
--location $MY_LOCATION \
--role roles/cloudkms.cryptoKeyEncrypterDecrypter \
--member allAuthenticatedUsers \
Copy link
Contributor

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?

Copy link
Contributor Author

@crwilcox crwilcox Apr 20, 2021

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

bigtable/integration_test.go Outdated Show resolved Hide resolved
bigtable/admin.go Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Show resolved Hide resolved
Copy link
Member

@codyoss codyoss left a 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.

Copy link
Contributor

@kolea2 kolea2 left a 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)

bigtable/integration_test.go Outdated Show resolved Hide resolved
bigtable/integration_test.go Show resolved Hide resolved
Copy link
Contributor

@tritone tritone left a 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 \
Copy link
Contributor

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.

bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
Copy link
Contributor

@tritone tritone left a 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 \
Copy link
Contributor

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.

@crwilcox crwilcox merged commit e9684ab into googleapis:master Apr 29, 2021
@crwilcox crwilcox deleted the bigtable-cmek branch October 29, 2021 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants