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

@salrashid123 salrashid123 commented Jun 24, 2019

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 as a code owner Jun 24, 2019
@googlebot googlebot added the cla: yes label Jun 24, 2019
@chingor13 chingor13 added the kokoro:force-run label Jun 26, 2019
@kokoro-team kokoro-team removed the kokoro:force-run label Jun 26, 2019
@codecov
Copy link

@codecov 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

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

@salrashid123 salrashid123 commented Jun 26, 2019

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 🚨 and removed 🚨 labels Jul 1, 2019
@salrashid123
Copy link
Contributor Author

@salrashid123 salrashid123 commented Jul 8, 2019

@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

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 label Jul 9, 2019
@kokoro-team kokoro-team removed the kokoro:force-run label Jul 9, 2019
Copy link
Contributor

@chingor13 chingor13 left a comment

Thank you!

@chingor13 chingor13 merged commit 70767e3 into googleapis:master Jul 9, 2019
8 checks passed
@salrashid123 salrashid123 deleted the impersonated-signer-idtoken branch Jul 9, 2019
@chingor13 chingor13 added the semver: minor 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 feature semver: minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants