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

Add Signer for impersonatied credentials #279

Merged

Conversation

salrashid123
Copy link
Contributor

Fixes googleapis/google-cloud-java#5043

Adds Signer capability to ImpersonatedCredentials. This will allow impersonated credentials to make signedURLs and basically, just sign bytes as the target_credential

sample usage

          
ServiceAccountCredentials sourceCredentials = ServiceAccountCredentials
          .fromStream(new FileInputStream("svc_account.json"));
sourceCredentials = (ServiceAccountCredentials) sourceCredentials
 .createScoped(Arrays.asList("https://www.googleapis.com/auth/iam"));

ImpersonatedCredentials targetCredentials = ImpersonatedCredentials.create(sourceCredentials,
          "impersonated-account@project.iam.gserviceaccount.com", null,
          Arrays.asList("https://www.googleapis.com/auth/devstorage.read_only"), 300);

Storage storage_service = StorageOptions.newBuilder()
.setCredentials(targetCredentials)
.build()
.getService();

String BUCKET_NAME1= "bucket-name";
String BLOB_NAME1 = "object-name.txt";

BlobInfo BLOB_INFO1 = BlobInfo.newBuilder(BUCKET_NAME1, BLOB_NAME1).build();

URL url =
storage_service.signUrl(
  BLOB_INFO1,
  140,
  TimeUnit.MINUTES,
     Storage.SignUrlOption.httpMethod(HttpMethod.GET), 
     Storage.SignUrlOption.withV4Signature());
System.out.println(url);

@salrashid123 salrashid123 requested a review from a team as a code owner June 24, 2019 02:14
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 24, 2019
@chingor13 chingor13 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 26, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 26, 2019
@codecov
Copy link

codecov bot commented Jun 26, 2019

Codecov Report

Merging #279 into master will increase coverage by 0.5%.
The diff coverage is 95.34%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #279     +/-   ##
===========================================
+ Coverage     78.21%   78.72%   +0.5%     
- Complexity      330      337      +7     
===========================================
  Files            21       21             
  Lines          1446     1490     +44     
  Branches        157      162      +5     
===========================================
+ Hits           1131     1173     +42     
- Misses          237      238      +1     
- Partials         78       79      +1
Impacted Files Coverage Δ Complexity Δ
...om/google/auth/oauth2/ImpersonatedCredentials.java 87.5% <95.34%> (+2.95%) 18 <7> (+7) ⬆️
...m/google/auth/oauth2/ComputeEngineCredentials.java 86.33% <0%> (+0.09%) 31% <0%> (ø) ⬇️
...tp/java/com/google/auth/oauth2/UserAuthorizer.java 76.57% <0%> (+0.13%) 20% <0%> (ø) ⬇️

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 0a5443a...b679be4. Read the comment docs.

Copy link
Contributor

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this!

We have something similar in the ComputeEngineCredentials class to implement the signing via the IAM credentials endpoints. Do you think it's possible to extract the shared implementation into a shared class?

@salrashid123
Copy link
Contributor Author

Sure, i can do that (infact, i pretty much copied the code for ComuteEngineCredentials here

however, if GCE someday has a native capability to sign (eg, metadata server), then this shared class will only be used by ImpersonatedCredentials. (i'd keep it separate for now but can create the share class if you want; up to you).

@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Jul 1, 2019
@salrashid123
Copy link
Contributor Author

@chingor13 Does leaving it as independent implementations make sense? I also would like to keep it separate since my take is ComputeCredentials shoudn't have the Signer interface anyway: the ability to sign arbitrary blobs is not a capability of compute engine metadata server which is what the credentials reflects. Having arbitrary blob signing with compute engine is also a form of privlege escalation if wall you wanted is an id_token

Copy link
Contributor

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

We can leave them as separate implementations for now. I may refactor it and the tests across the board to ensure the behavior is consistent across the board for the credentials signing across the implementations.

As for this PR, only a couple small nits, then I can merge it in.

…s.java

Co-Authored-By: Jeff Ching <chingor@google.com>
@chingor13 chingor13 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 9, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 9, 2019
Copy link
Contributor

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

Thank you!

@chingor13 chingor13 merged commit 70767e3 into googleapis:master Jul 9, 2019
@salrashid123 salrashid123 deleted the impersonated-signer-idtoken branch July 9, 2019 23:14
@chingor13 chingor13 added the semver: minor A new feature was added. No breaking changes. label Jul 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. feature semver: minor A new feature was added. No breaking changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

google-cloud-storage: Cannot create signed url with ImpersonatedCredentials
5 participants