Skip to content

[Storage] KMS integration #3196

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
Merged
frankyn merged 14 commits into from
May 21, 2018
Merged

[Storage] KMS integration #3196

frankyn merged 14 commits into from
May 21, 2018

Conversation

@frankyn
Copy link
Contributor

@frankyn frankyn commented Apr 26, 2018 •
edited
Loading

Original author: @rossjudson
cc: @danoscarmike @rossjudson

Expected work items:

  • Define KMS keyring for integration tests
  • Allow configuration of default_kms_key_name in bucket
  • Allow specification of kms_key_name on object insert
  • Allow specification of kms_key_name on object rewrite
  • Test: Ensure kms_key_name is visible in object listings.
  • Test: Insert an object with a Cloud KMS key then fetch the object and verify contents and metadata (should include kms_key_name).
  • Test: Create a bucket with a default Cloud KMS key, insert an object, then fetch the object and verify its contents and metadata (should include kms_key_name).
  • Test: Update the default_kms_key_name of a bucket to a different key name.
  • Test: Update the bucket to remove default_kms_key_name encryption.
  • Test: Migrate CSEK to Cloud KMS key, download object and verify contents are decrypted.
  • Test: Test defaultKMSKeyName can be selected using BucketField.ENCRYPTION.
  • Test: Test kmsKeyName can be selected using BlobField.KMS_KEY_NAME.
  • Test: Write unit tests for BlobWriteOption and BlobTargetOption.
  • Nit: Remove Alpha and Beta annotations for GA features (Bucket level IAM and Requester Pays).
  • Add Beta annotations to KMS integration methods.
  • Code Review

@frankyn frankyn requested a review from pongad as a code owner April 26, 2018 17:09
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 26, 2018
pongad
pongad reviewed Apr 26, 2018
View reviewed changes
google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java
}
Encryption encryption = bucketPb.getEncryption();
if (encryption != null && encryption.getDefaultKmsKeyName() != null && !encryption.getDefaultKmsKeyName().isEmpty()) {
builder.setDefaultKmsKeyName(encryption.getDefaultKmsKeyName());

This comment was marked as spam.

Sign in to view

google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java
}

/**
* Returns the default Cloud KMS key to be applied to newly inserted objects in this bucket.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@googlebot
Copy link

googlebot commented May 1, 2018

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@googlebot googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels May 1, 2018
@frankyn frankyn force-pushed the kms-integration branch from e847668 to 56b34df Compare May 1, 2018 08:18
@googlebot
Copy link

googlebot commented May 1, 2018

CLAs look good, thanks!

@googlebot googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels May 1, 2018
frankyn
frankyn commented May 1, 2018
View reviewed changes
google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java Outdated
.decode("H4sIAAAAAAAAAPNIzcnJV3DPz0/PSVVwzskvTVEILskvSkxPVQQA/LySchsAAAA=");
private static final Map<String, String> BUCKET_LABELS = ImmutableMap.of("label1", "value1");
private static final String SERVICE_ACCOUNT_EMAIL = "[email protected]";
private static final String KMS_KEY_NAME = ""; // use real KMS KEY associated to the test project.

This comment was marked as spam.

Sign in to view

@googlebot googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels May 1, 2018
@frankyn frankyn force-pushed the kms-integration branch from f47fe80 to 47701ed Compare May 1, 2018 08:43
@googlebot
Copy link

googlebot commented May 1, 2018

CLAs look good, thanks!

@googlebot googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels May 1, 2018
@googleapis googleapis deleted a comment from googlebot May 1, 2018
@frankyn frankyn changed the title [Storage] Adding initial PR for KMS integration [Storage] KMS integration May 7, 2018
@frankyn frankyn force-pushed the kms-integration branch from d4afcd1 to 8d042d2 Compare May 7, 2018 07:34
@pongad pongad requested a review from garrettjonesgoogle May 7, 2018 16:37
@pongad
Copy link
Contributor

pongad commented May 7, 2018

Also adding @garrettjonesgoogle

garrettjonesgoogle
garrettjonesgoogle reviewed May 7, 2018
View reviewed changes
google-cloud-storage/pom.xml Outdated
<dependency>
<groupId>com.google.apis</groupId>
<artifactId>google-api-services-storage</artifactId>
<version>v1-rev125-1.23.0</version>

This comment was marked as spam.

Sign in to view

google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java Outdated
}

@Override
public Builder setKmsKeyName(String kmsKeyName) {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-storage/src/main/java/com/google/cloud/storage/Bucket.java
* 'requester_pays' flag.
*/
@GcpLaunchStage.Alpha
public static BucketSourceOption userProject(String userProject) {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java Outdated
import java.util.List;
import java.util.Map;

import jdk.nashorn.internal.runtime.regexp.joni.exception.ValueException;

This comment was marked as spam.

Sign in to view

@frankyn
Copy link
Contributor Author

frankyn commented May 8, 2018 •
edited
Loading

@pongad and @garrettjonesgoogle, I realized yesterday afternoon that I confused myself on API surface for this feature in context of inserting an object with a KMS KeyName. On objects.insert kmsKeyName is set in request URL query parameters and only available in resource body post-operation.

I updated the PR/design to use BlobTargetOption and BlobWriteOption and set setKmsKeyName in BlobInfo as a private method.

More context and background on the operation:
https://cloud.google.com/storage/docs/encryption/using-customer-managed-keys#add-object-key

@pongad
Copy link
Contributor

pongad commented May 16, 2018

Oh dear I'm sorry I dropped the ball on this. We moved a bunch of files. Could you merge from master?

There are some lint warnings that are worth fixing, but I'm fine fixing those in a separate PR.

@frankyn frankyn force-pushed the kms-integration branch 2 times, most recently from 99a7491 to 619c6e4 Compare May 21, 2018 18:58
pongad
pongad reviewed May 21, 2018
View reviewed changes
Copy link
Contributor

@pongad pongad left a comment

Choose a reason for hiding this comment

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

One nit, but LGTM otherwise

...le-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java Outdated
@Override
public Builder setDefaultKmsKeyName(String defaultKmsKeyName) {
this.defaultKmsKeyName = defaultKmsKeyName != null
? new String(defaultKmsKeyName) : Data.<String>nullOf(String.class);

This comment was marked as spam.

Sign in to view

@pongad
Copy link
Contributor

pongad commented May 21, 2018

@garrettjonesgoogle Do you also want to take a look?

garrettjonesgoogle
garrettjonesgoogle approved these changes May 21, 2018
View reviewed changes
Copy link
Member

@garrettjonesgoogle garrettjonesgoogle left a comment

Choose a reason for hiding this comment

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

LGTM

@frankyn
Copy link
Contributor Author

frankyn commented May 21, 2018

All checks passed merging now.

@frankyn frankyn merged commit 58f0c89 into master May 21, 2018
@frankyn frankyn deleted the kms-integration branch May 21, 2018 23:49
@danoscarmike
Copy link
Contributor

danoscarmike commented May 22, 2018

🎉 Thanks Frank, Michael and Garrett!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@pongad pongad pongad left review comments

@garrettjonesgoogle garrettjonesgoogle garrettjonesgoogle approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@frankyn @googlebot @pongad @danoscarmike @garrettjonesgoogle