Storage: HMAC service account support #5284

JesseLovelace · 2019-06-03T20:21:33Z

Adds support for HMAC service accounts. Holding off on adding documentation until the general implementation is approved

… hmacsa

jkwlui · 2019-06-18T20:16:10Z

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/HmacKey.java

+  }
+
+  public static class Builder {
+    private String secretKey;


Not super familiar with documenting Java code, but can we add a note saying the secret is only available on creation?

Yeah--I'm planning to add the documentation once the code is otherwise approved

jkwlui

Surface looks good! Thanks Jesse. Please hold off from merging until we're ready to release.

jkwlui · 2019-06-18T20:16:55Z

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/HmacKey.java

+
+  @Override
+  public int hashCode() {
+    return Objects.hash(secretKey,  metadata);


extra space here?

frankyn

Thanks @JesseLovelace, I've added some minor comments.

Could you also run the code through the Google Java linter?

frankyn · 2019-06-18T20:49:55Z

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.io.BaseEncoding;
+import com.google.cloud.storage.HmacKey.HmacKeyMetadata;


I don't think this is necessary.

It is, would need "HmacKey.HmacKeyMetadata" without it

frankyn · 2019-06-18T20:56:47Z

...d-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/StorageRpc.java

   */
  List<ObjectAccessControl> listAcls(String bucket, String object, Long generation);

+  HmacKey createHmacKey(String serviceAccountEmail);


Given the introduction of these new RPCs the NIO library will also need to be updated. https://github.com/googleapis/google-cloud-java/tree/master/google-cloud-clients/google-cloud-contrib/google-cloud-nio

frankyn · 2019-06-18T20:58:53Z

...ud-clients/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java

+  @Test
+  public void testHmacKey() {
+    ServiceAccount serviceAccount =
+        ServiceAccount.of("[email protected]");


This should be an environment variable value.

frankyn · 2019-06-18T21:03:09Z

...e-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java

 import com.google.common.io.BaseEncoding;
 import com.google.common.net.UrlEscapers;
 import com.google.common.primitives.Ints;
+import com.google.cloud.storage.HmacKey.HmacKeyMetadata;


I don't think this is necessary.

It is, would need "HmacKey.HmacKeyMetadata" without it

ah, gotcha. Thanks for clarifying!

frankyn · 2019-06-18T21:04:40Z

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+
+  void deleteHmacKey(String accessId);
+
+  HmacKeyMetadata updateHmacKeyState(


Nice helper method! (no action required).

… hmacsa

frankyn · 2019-06-21T21:17:56Z

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+      ServiceAccount serviceAccount, String pageToken, Long maxResults, boolean showDeletedKeys);

+  /**
+   * Lists HMAC keys for a given service account. Note this returns {@code HmacKeyMetadata} objects,


Lists active HMAC keys for a given service account.

frankyn · 2019-06-21T21:18:34Z

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+   * <pre>{@code
+   * ServiceAccount serviceAccount = ServiceAccount.of("[email protected]");
+   *
+   * Page<HmacKey.HmacKeyMetadata> metadataPage = storage.listHmacKeys(serviceAccount, null, 10);


Could you add the boolean value of true for Deleted keys?

frankyn · 2019-06-21T21:20:42Z

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+   */
  Page<HmacKeyMetadata> listHmacKeys(
-      ServiceAccount serviceAccount, String pageToken, Long maxResults);
+      ServiceAccount serviceAccount, String pageToken, Long maxResults, boolean showDeletedKeys);


Out of curiosity, why the overload for showDeletedKeys?

My reasoning here is that the most common use case is going to be listHmacKeys(serviceAccount), so offering that along with one other signature to be more precise would meet most needs

Gotcha that makes sense. What about defaulting to not showing deleted keys?

That is already the default I believe

API defaults to false if not given.

Thanks @jkwlui and @JesseLovelace!

… hmacsa

codecov · 2019-06-21T21:36:16Z

Codecov Report

Merging #5284 into master will decrease coverage by 3.09%.
The diff coverage is 6.45%.

@@             Coverage Diff              @@
##             master    #5284      +/-   ##
============================================
- Coverage     50.48%   47.38%    -3.1%     
- Complexity    23890    27180    +3290     
============================================
  Files          2258     2523     +265     
  Lines        227482   274574   +47092     
  Branches      24973    31380    +6407     
============================================
+ Hits         114834   130112   +15278     
- Misses       104029   134852   +30823     
- Partials       8619     9610     +991

Impacted Files	Coverage Δ	Complexity Δ
...ain/java/com/google/cloud/storage/StorageImpl.java	`77.81% <0%> (-8.39%)`	`98 <0> (-5)`
...om/google/cloud/storage/spi/v1/HttpStorageRpc.java	`1.64% <0%> (-0.19%)`	`1 <0> (ø)`
...rc/main/java/com/google/cloud/storage/Storage.java	`79.72% <0%> (-6.13%)`	`0 <0> (ø)`
...ogle/cloud/storage/spi/v1/HttpStorageRpcSpans.java	`100% <100%> (ø)`	`3 <0> (ø)`	⬇️
...va/com/google/cloud/storage/spi/v1/StorageRpc.java	`64.38% <100%> (+1.52%)`	`0 <0> (ø)`	⬇️
...ud/storage/contrib/nio/testing/FakeStorageRpc.java	`59.66% <40%> (-4.07%)`	`44 <0> (+3)`
...rc/main/java/com/google/cloud/storage/HmacKey.java	`5.73% <5.73%> (ø)`	`0 <0> (?)`
...n/java/com/google/cloud/bigquery/BigQueryImpl.java	`66.78% <0%> (-13.22%)`	`57% <0%> (ø)`
...a/com/google/cloud/firestore/FirestoreOptions.java	`36.97% <0%> (-7.82%)`	`7% <0%> (ø)`
... and 938 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4f56e8...61187df. Read the comment docs.

… hmacsa

jkwlui · 2019-06-21T23:24:48Z

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+   *
+   * @throws StorageException upon failure
+   */
+  Page<HmacKeyMetadata> listHmacKeys(ServiceAccount serviceAccount);


listHmacKeys should also be able to accept no serviceAccount, which will show HMAC keys from all accounts. Can your implementation accept no serviceAccount?

Listing keys from all accounts can be accomplished by calling this with serviceAccount=null. I'll add a note about it in the documentation

Got it. That would really help, thank you!

… hmacsa

frankyn

Adding an additional request for consistency with the Storage surface and HMACKey RPCs.

frankyn · 2019-07-10T17:40:13Z

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+   * @throws StorageException upon failure
+   */
+  Page<HmacKeyMetadata> listHmacKeys(
+      ServiceAccount serviceAccount, String pageToken, Long maxResults, boolean showDeletedKeys);


Based on other RPCs the Storage library has, the following parameters: serviceAccountEmail, pageToken, showDeletedKeys, and maxResults should be listed under an Options optional parameter because they are optional. The only required value is project_id which is provided by client credentials.

Other HmacKey RPCs should have a similar Options parameter for the value userProject. At the moment it's ignored by the service but it's expected to be available. Given it would change the signature of the method I thought it'd be best to do it now rather than wait until later. (I'm open to opinions here).

… hmacsa

frankyn

One last nit which is to expose userProject in each new RPC method.

frankyn · 2019-07-29T06:06:31Z

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+     * Returns an option to specify whether to show deleted keys in the result. This option is false
+     * by default.
+     */
+    public static ListHmacKeysOption showDeletedKeys(boolean showDeletedKeys) {


userProject should be available based on the discovery document. https://www.googleapis.com/discovery/v1/apis/storage/v1/rest

frankyn · 2019-07-29T06:07:26Z

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+   *
+   * @throws StorageException upon failure
+   */
+  HmacKey createHmacKey(ServiceAccount serviceAccount);


Include options for userProject.

frankyn · 2019-07-29T06:07:34Z

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+   *
+   * @throws StorageException upon failure
+   */
+  HmacKeyMetadata getHmacKey(String accessId);


Include options for userProject.

frankyn · 2019-07-29T06:07:39Z

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+   *
+   * @throws StorageException upon failure
+   */
+  void deleteHmacKey(String accessId);


Include options for userProject.

frankyn · 2019-07-29T06:07:48Z

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+   * @throws StorageException upon failure
+   */
+  HmacKeyMetadata updateHmacKeyState(
+      final HmacKeyMetadata hmacKeyMetadata, final HmacKey.HmacKeyState state);


Include options for userProject.

… hmacsa

frankyn

A few last nits. Thank you for your patience.

frankyn · 2019-08-07T02:57:25Z

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+   *
+   * @throws StorageException upon failure
+   */
+  void deleteHmacKey(String accessId, DeleteHmacKeyOption... options);


I'm open to interpretation on the following. PLMK what you think.
There should use HMACKeyMetadata as a parameter instead of String accessId. This may not happen often so I'm thinking we should not have this overload (provided examples below). The HMACKeyMetadata can then provide the projectId as well. We don't need HmacKeyOption.PROJECT_ID option in this case.

I would expect this to be a common case given the current surface:

// Delete an hmac key. String hmacKeyAccessId = "my-access-id"; HmacKey.HmackeyMetadata hmacKeyMetadata = storage.getHmacKey(hmacKeyAccessId); hmacKeyMetadata = storage.updateHmacKeyState(hmacKeyMetadata, HmacKey.HmacKeyState.INACTIVE); storage.deleteHmacKey(hmacKeyMetadata);

Here's an example that expects the key to be deactivated. A user needs a deactivated key.

// Delete an hmac key has the expectation that hmacKey be deactivated already. // Note: This may not be too often the case. We might want to not have deleteHmacKey(String accessId..). String hmacKeyAccessId = "my-access-id"; storage.deleteHmacKey("my-access-id");

frankyn · 2019-08-07T02:58:57Z

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+    /** Returns an option to specify the Project ID for this request. If not specified,
+     * defaults to Application Default Credentials.
+     */
+    public static UpdateHmacKeyOption projectId(String projectId) {


This isn't necessary please remove it.

frankyn · 2019-08-07T02:59:35Z

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+    /** Returns an option to specify the Project ID for this request. If not specified,
+     * defaults to Application Default Credentials.
+     */
+    public static DeleteHmacKeyOption projectId(String projectId) {


Remove option. Reviewing design again it doesn't make sense. I'd expect a Get, List, and Create requests to provide the projectId for subsequent requests.

… hmacsa

frankyn

A few minor nits. Otherwise LGTM.

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

...ients/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java

frankyn · 2019-08-13T18:34:47Z

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+   *
+   * <pre>{@code
+   * ServiceAccount serviceAccount = ServiceAccount.of("[email protected]");
+   * HmacKey key = storage.createHmacKey(serviceAccount);


Perform a Get request instead of a create.

… hmacsa

frankyn

One more nit and I'll approve, e.g unused variable.

frankyn · 2019-08-13T20:42:12Z

google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+   * <p>Example of listing HMAC keys, specifying project id.
+   *
+   * <pre>{@code
+   * ServiceAccount serviceAccount = ServiceAccount.of("[email protected]");


serviceAccount isn't used.

… hmacsa

Storage: HMAC service account support

11ffccc

JesseLovelace requested a review from jkwlui June 3, 2019 20:21

JesseLovelace requested a review from a team as a code owner June 3, 2019 20:21

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 3, 2019

yoshi-automation added the 🚨 This issue needs some love. label Jun 10, 2019

JesseLovelace requested a review from frankyn June 13, 2019 21:14

JesseLovelace added 2 commits June 17, 2019 16:37

Storage: HMAC service account support

fe560aa

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

2375c85

… hmacsa

jkwlui reviewed Jun 18, 2019

View reviewed changes

jkwlui approved these changes Jun 18, 2019

View reviewed changes

frankyn suggested changes Jun 18, 2019

View reviewed changes

JesseLovelace added 2 commits June 21, 2019 13:42

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

42569aa

… hmacsa

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

a09334c

… hmacsa

frankyn suggested changes Jun 21, 2019

View reviewed changes

JesseLovelace added 2 commits June 21, 2019 14:35

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

89d165b

… hmacsa

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

7be7c2b

… hmacsa

frankyn requested a review from jkwlui June 21, 2019 21:46

JesseLovelace added 2 commits June 21, 2019 16:03

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

4bb6b44

… hmacsa

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

91287cd

… hmacsa

jkwlui reviewed Jun 21, 2019

View reviewed changes

JesseLovelace added 2 commits June 24, 2019 13:41

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

82259e4

… hmacsa

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

46f4e29

… hmacsa

JesseLovelace added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 26, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 26, 2019

yoshi-automation removed the 🚨 This issue needs some love. label Jul 3, 2019

frankyn suggested changes Jul 10, 2019

View reviewed changes

JesseLovelace added 3 commits July 11, 2019 16:17

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

df99b6d

… hmacsa

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

ab32675

… hmacsa

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

9e185bb

… hmacsa

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

6d6fd4b

… hmacsa

frankyn suggested changes Jul 29, 2019

View reviewed changes

JesseLovelace added 4 commits August 6, 2019 11:08

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

46fc073

… hmacsa

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

cefd5ea

… hmacsa

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

6dc9627

… hmacsa

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

6998a83

… hmacsa

frankyn suggested changes Aug 7, 2019

View reviewed changes

JesseLovelace added 2 commits August 13, 2019 10:46

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

0a8abee

… hmacsa

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

16a3c60

… hmacsa

frankyn suggested changes Aug 13, 2019

View reviewed changes

JesseLovelace added 2 commits August 13, 2019 12:54

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

af8bfd6

… hmacsa

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

584015f

… hmacsa

frankyn suggested changes Aug 13, 2019

View reviewed changes

JesseLovelace added 2 commits August 13, 2019 15:22

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

8760589

… hmacsa

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

55f1ef4

… hmacsa

frankyn approved these changes Aug 13, 2019

View reviewed changes

JesseLovelace added 2 commits August 13, 2019 16:48

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

5cfdea5

… hmacsa

Merge branch 'hmacsa' of github.com:googleapis/google-cloud-java into…

61187df

… hmacsa

JesseLovelace merged commit 2b0d5ce into master Aug 14, 2019

This was referenced Aug 22, 2019

Release google-cloud-java v0.106.0 #6127

Closed

Release google-cloud-java v0.106.0 #6140

Merged

Neenu1995 deleted the hmacsa branch July 6, 2022 17:49


		void deleteHmacKey(String accessId);

		HmacKeyMetadata updateHmacKeyState(

Storage: HMAC service account support #5284

Storage: HMAC service account support #5284

Uh oh!

Conversation

JesseLovelace commented Jun 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JesseLovelace Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkwlui left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frankyn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frankyn left a comment

Choose a reason for hiding this comment

Uh oh!

frankyn Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frankyn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JesseLovelace Jun 18, 2019 •

edited

Loading

codecov bot commented Jun 21, 2019 •

edited

Loading

frankyn Jul 10, 2019 •

edited

Loading