Add support for explicit java.security.Provider to ServiceAccountCred… #411

stian-svedenborg-gul-katt · 2020-03-02T09:08:15Z

…entials

This enables the storage of service account credentials in external key
storage such as remote KeyVaults or HSMs.

Fixes #410

googlebot · 2020-03-02T09:08:19Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

stian-svedenborg-gul-katt · 2020-03-02T09:16:05Z

@googlebot I signed it!

googlebot · 2020-03-02T09:16:09Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

oauth2_http/java/com/google/auth/oauth2/ServiceAccountCredentials.java

oauth2_http/javatests/com/google/auth/oauth2/ServiceAccountCredentialsTest.java

elharo · 2020-03-02T11:06:18Z

oauth2_http/java/com/google/auth/oauth2/ServiceAccountCredentials.java

@@ -667,14 +691,8 @@ String createAssertion(JsonFactory jsonFactory, long currentTime, String audienc
      payload.setAudience(audience);
    }

-    String assertion;
-    try {
-      assertion = JsonWebSignature.signUsingRsaSha256(privateKey, jsonFactory, header, payload);


Critical: I'm not sure the new code is using this algorithm. I might need to patch this into an IDE and check.

The signing algorithm is dictated by OAuth2Utils.SIGNATURE_ALGORITHM. Which is "SHA256withRSA"

OK. Since this is security critical code I'm going to need go over this carefully and make sure of that. It might take a little time to review, or perhaps someone else can get to this before I do.

codecov · 2020-03-02T13:08:39Z

Codecov Report

Merging #411 into master will increase coverage by 0.13%.
The diff coverage is 85%.

@@             Coverage Diff              @@
##             master     #411      +/-   ##
============================================
+ Coverage     79.53%   79.66%   +0.13%     
- Complexity      397      399       +2     
============================================
  Files            27       27              
  Lines          1803     1810       +7     
  Branches        188      188              
============================================
+ Hits           1434     1442       +8     
+ Misses          269      268       -1     
  Partials        100      100

Impacted Files	Coverage Δ	Complexity Δ
.../google/auth/oauth2/ServiceAccountCredentials.java	`83.53% <85%> (+0.88%)`	`50 <6> (+2)`	⬆️

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 0a57cd5...256ffef. Read the comment docs.

stian-svedenborg-gul-katt · 2020-03-02T13:10:48Z

@elharo Comments fixed or answered.

elharo · 2020-03-02T13:22:45Z

oauth2_http/java/com/google/auth/oauth2/ServiceAccountCredentials.java

+      byte[] signature = this.sign(signedContentBytes);
+      return signedContentString + "." + Base64.encodeBase64URLSafeString(signature);
+
+    } catch (SigningException e) {


Is there a reason to convert this to an IOException instead of declaring this method to throw SigningException?

No, not really, I just kept the pattern that was there before.

SigningException is unchecked. Whilst the api for refreshAccessToken declares that it throws IOException. (I'm not sure I agree that SigningException should be unchecked, but that is outside the scope of this change).

In any case I argue that a failing signature should be handled by the calling code, and as such it should be changed to an IOException before exiting refreshAccessToken.

As to whether I should keep the conversion inside signJsonWebSignature or move it to createAssertion (and duplicate it in createAssertionForIdToken) I'll follow ruling.

@elharo I'll await your answer on this before I reupload. (I'm I doing this correctly with the --amends by the way?)

I honestly do not have enough git-fu to tell. Whatever you're doing seems to work fine on my end. Just commit and upload as you need to. We'll squash the commits when we merge.

elharo · 2020-03-02T13:23:42Z

oauth2_http/java/com/google/auth/oauth2/ServiceAccountCredentials.java

@@ -667,14 +691,8 @@ String createAssertion(JsonFactory jsonFactory, long currentTime, String audienc
      payload.setAudience(audience);
    }

-    String assertion;
-    try {
-      assertion = JsonWebSignature.signUsingRsaSha256(privateKey, jsonFactory, header, payload);


OK. Since this is security critical code I'm going to need go over this carefully and make sure of that. It might take a little time to review, or perhaps someone else can get to this before I do.

oauth2_http/javatests/com/google/auth/oauth2/ServiceAccountCredentialsTest.java

oauth2_http/java/com/google/auth/oauth2/ServiceAccountCredentials.java

…ntCredentials This enables the storage of service account credentials in external key storage such as remote KeyVaults or HSMs.

stian-svedenborg-gul-katt · 2020-03-19T13:58:14Z

@elharo Hi any chance of this one getting some love soon? :)

stian-svedenborg-gul-katt requested a review from a team as a code owner March 2, 2020 09:08

googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Mar 2, 2020

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 Mar 2, 2020

stian-svedenborg-gul-katt force-pushed the explicit-provider branch from 7054a1a to 2eb46f9 Compare March 2, 2020 09:18

elharo suggested changes Mar 2, 2020

View reviewed changes

elharo added kokoro:run Add this label to force Kokoro to re-run the tests. priority: p2 Moderately-important priority. Fix may not be included in next release. semver: minor A new feature was added. No breaking changes. labels Mar 2, 2020

kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Mar 2, 2020

stian-svedenborg-gul-katt force-pushed the explicit-provider branch from 2eb46f9 to d75519b Compare March 2, 2020 13:08

elharo reviewed Mar 2, 2020

View reviewed changes

stian-svedenborg-gul-katt force-pushed the explicit-provider branch from d75519b to 153f5e5 Compare March 3, 2020 05:59

stian-svedenborg-gul-katt requested a review from elharo March 3, 2020 05:59

elharo added the kokoro:run Add this label to force Kokoro to re-run the tests. label Mar 3, 2020

kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Mar 3, 2020

elharo reviewed Mar 4, 2020

View reviewed changes

oauth2_http/java/com/google/auth/oauth2/ServiceAccountCredentials.java Outdated Show resolved Hide resolved

feat: Add support for explicit java.security.Provider to ServiceAccou…

256ffef

…ntCredentials This enables the storage of service account credentials in external key storage such as remote KeyVaults or HSMs.

stian-svedenborg-gul-katt force-pushed the explicit-provider branch from 153f5e5 to 256ffef Compare March 7, 2020 07:52

stian-svedenborg-gul-katt requested a review from elharo March 7, 2020 07:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for explicit java.security.Provider to ServiceAccountCred… #411

Add support for explicit java.security.Provider to ServiceAccountCred… #411

stian-svedenborg-gul-katt commented Mar 2, 2020

googlebot commented Mar 2, 2020

stian-svedenborg-gul-katt commented Mar 2, 2020

googlebot commented Mar 2, 2020

elharo Mar 2, 2020

stian-svedenborg-gul-katt Mar 2, 2020

elharo Mar 2, 2020

codecov bot commented Mar 2, 2020 •

edited

stian-svedenborg-gul-katt commented Mar 2, 2020

elharo Mar 2, 2020

stian-svedenborg-gul-katt Mar 2, 2020 •

edited

stian-svedenborg-gul-katt Mar 2, 2020

elharo Mar 2, 2020

elharo Mar 2, 2020

stian-svedenborg-gul-katt commented Mar 19, 2020

Add support for explicit java.security.Provider to ServiceAccountCred… #411

Are you sure you want to change the base?

Add support for explicit java.security.Provider to ServiceAccountCred… #411

Conversation

stian-svedenborg-gul-katt commented Mar 2, 2020

googlebot commented Mar 2, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

stian-svedenborg-gul-katt commented Mar 2, 2020

googlebot commented Mar 2, 2020

elharo Mar 2, 2020

Choose a reason for hiding this comment

stian-svedenborg-gul-katt Mar 2, 2020

Choose a reason for hiding this comment

elharo Mar 2, 2020

Choose a reason for hiding this comment

codecov bot commented Mar 2, 2020 • edited

Codecov Report

stian-svedenborg-gul-katt commented Mar 2, 2020

elharo Mar 2, 2020

Choose a reason for hiding this comment

stian-svedenborg-gul-katt Mar 2, 2020 • edited

Choose a reason for hiding this comment

stian-svedenborg-gul-katt Mar 2, 2020

Choose a reason for hiding this comment

elharo Mar 2, 2020

Choose a reason for hiding this comment

elharo Mar 2, 2020

Choose a reason for hiding this comment

stian-svedenborg-gul-katt commented Mar 19, 2020

codecov bot commented Mar 2, 2020 •

edited

stian-svedenborg-gul-katt Mar 2, 2020 •

edited