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

feat: add IDTokenCredential support #303

Merged
merged 25 commits into from Aug 14, 2019
Merged

Conversation

salrashid123
Copy link
Contributor

@salrashid123 salrashid123 commented Jul 24, 2019

Fixes #99

Adds support for IDTokens into google-auth-java for ServiceAccountCredentials, ComuteEngineCredentials and ImpersonatedCredentials

GCP users currently have to do these steps manually and outside of google-auth-java:
ref: https://github.com/salrashid123/salrashid123.github.io/tree/master/google_id_token#java
this change wraps the various flows into the library

I do not have any testcases here. I'm submitting this first for review of the api surface. Usage for this change is shown here:
#99 (comment)

@salrashid123 salrashid123 requested a review from as a code owner Jul 24, 2019
@googlebot googlebot added the cla: yes label Jul 24, 2019
@salrashid123
Copy link
Contributor Author

@salrashid123 salrashid123 commented Jul 25, 2019

ok, i've addressed some of the request and left a bunch unsolved with this commit as they are relaed to the bit mentioned here:

The biggest thing is if

public class IdToken extends AccessToken

is the best way or not

The IdToken a bit different than AccessToken...the former is a readable jwt token that customers can readily use as-is in an authorized http transport as shown below or with any third party http transport library by coaxing out the token credential.getAccessToken() and then attaching it manually to an Authorization: Bearer header.

However, customers will also want to evaluate and inspect claims embedded within the token (expiration, the aud: field, etc)

  public JsonWebSignature getJsonWebSignature() {
    return jws;
  }

for example

System.out.println(tokenCredential.getIdToken().getTokenValue());
System.out.println(tokenCredential.getIdToken().getJsonWebSignature().getPayload().getAudienceAsList());
System.out.println(tokenCredential.getIdToken().getJsonWebSignature().getPayload().getExpirationTimeSeconds());

in that way, the AccessToken and IdToken diverge and thats why i set them up different.

FWIW, he only reason i extended AccessToken is so that i can plumb it inot a Credential object for use like this

// Invoke the API
GenericUrl genericUrl = new GenericUrl("https://myapp-6w42z6vi3q-uc.a.run.app");
HttpCredentialsAdapter adapter = new HttpCredentialsAdapter(tokenCredential);
HttpTransport transport = new NetHttpTransport();
HttpRequest request = transport.createRequestFactory(adapter).buildGetRequest(genericUrl);
request.setThrowExceptionOnExecuteError(false);
HttpResponse response = request.execute();

note, an IdToken cannot currently be used against a GCP service like GCS, Pub/Sub (a self-issued JWTAccessToken can be, however...(some conjecture on my part: i suspect someday a googleissued ID token can be used in a similar way to JWTAccessToken to interact with a googleAPI...)

  • I've removed IdTokenProvider.IdTokenProviderException. LMK if it shoudl remain (i bubble everything via IOException)

@broady

@chingor13
Copy link
Contributor

@chingor13 chingor13 commented Jul 25, 2019

FWIW, he only reason i extended AccessToken is so that i can plumb it into a Credential object for use like this

The IdTokenCredential can be used in the HttpCredentialsAdapter and should be setting the "Authorization: Bearer [tokenValue]" header if installed as a HttpRequestInitializer.

@chingor13
Copy link
Contributor

@chingor13 chingor13 commented Jul 26, 2019

As for whether IdToken should extend AccessToken, OpenID Connect extends OAuth 2.0 so an IdToken is probably find extending AccessToken.

Perhaps IdTokenCredentials should extend OAuth2Credentials instead of GoogleCredentials given that it's not currently able to be used against Google services and you cannot call createScoped or createDelegated.

@chingor13 chingor13 changed the title Add IDTokenCredential support feat: add IDTokenCredential support Jul 26, 2019
@salrashid123
Copy link
Contributor Author

@salrashid123 salrashid123 commented Jul 26, 2019

ok, i changed to

public class IdTokenCredentials extends OAuth2Credentials {

and verified the credentials are usable in the same way. I also see the raw token accessible in a couple ways:

System.out.println(tokenCredential.getAccessToken().getTokenValue());               
System.out.println(tokenCredential.getIdToken().getTokenValue());
System.out.println(tokenCredential.getIdToken().getJsonWebSignature().getPayload().getAudienceAsList());
System.out.println(tokenCredential.getIdToken().getJsonWebSignature().getPayload().getExpirationTimeSeconds());

Copy link
Contributor

@elharo elharo left a comment

Lots of style nits (not all called out) and one substantive comment

return getMetadataServerUrl(DefaultCredentialsProvider.DEFAULT)
+ "/computeMetadata/v1/instance/service-accounts/default/identity";
}

@Override
public int hashCode() {
return Objects.hash(transportFactoryClassName);
Copy link
Contributor

@elharo elharo Jul 27, 2019

Choose a reason for hiding this comment

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

not for this PR, but this looks really wonky. Note to myself: can this possibly be correct?

oauth2_http/java/com/google/auth/oauth2/IamUtils.java Outdated Show resolved Hide resolved
oauth2_http/java/com/google/auth/oauth2/IamUtils.java Outdated Show resolved Hide resolved
oauth2_http/java/com/google/auth/oauth2/IamUtils.java Outdated Show resolved Hide resolved
String errorMessage = OAuth2Utils.validateString(error, "message", PARSE_ERROR_MESSAGE);
throw new IOException(String.format("Error code %s trying to getIDToken: %s", statusCode, errorMessage));
}
if (statusCode != HttpStatusCodes.STATUS_CODE_OK) {
Copy link
Contributor

@elharo elharo Jul 27, 2019

Choose a reason for hiding this comment

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

should this be any 200 level response?

Copy link
Contributor Author

@salrashid123 salrashid123 Jul 27, 2019

Choose a reason for hiding this comment

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

i'm not absolutely sure but i've never seen responses range 2xx from this api layer:
ref: https://cloud.google.com/apis/design/errors#handling_errors
but storage mentions a bit of that here: https://cloud.google.com/storage/docs/json_api/v1/status-codes

(the google apis endpoint iamcredentials uses is older one (similar to drive api, calendar api, etc)

@elharo elharo added the semver: minor label Jul 31, 2019
Copy link
Contributor

@elharo elharo left a comment

Since we're unlikely to get the API perfect on the first try, we should mark this @beta to give ourselves flexibility to change it in the next couple of releases.

@chingor13 chingor13 added the kokoro:force-run label Aug 9, 2019
@kokoro-team kokoro-team removed the kokoro:force-run label Aug 9, 2019
Copy link
Contributor

@chingor13 chingor13 left a comment

Looking good! Mostly nits, however, can we add some tests on the IAMUtils error handling

@chingor13 chingor13 added the kokoro:force-run label Aug 12, 2019
@kokoro-team kokoro-team removed the kokoro:force-run label Aug 12, 2019
@chingor13 chingor13 added the kokoro:force-run label Aug 13, 2019
@kokoro-team kokoro-team removed the kokoro:force-run label Aug 13, 2019
@chingor13 chingor13 added the kokoro:force-run label Aug 13, 2019
@kokoro-team kokoro-team removed the kokoro:force-run label Aug 13, 2019
@chingor13 chingor13 requested a review from elharo Aug 13, 2019
Copy link
Contributor

@elharo elharo left a comment

I don't have any major concerns or objections to this if you're both happy with it. Main thing is that everything is marked @beta so we can and will change it if we find we didn't get it quite right the first time, which we almost never do of course. :-)

oauth2_http/java/com/google/auth/oauth2/IdToken.java Outdated Show resolved Hide resolved
oauth2_http/java/com/google/auth/oauth2/IdToken.java Outdated Show resolved Hide resolved
oauth2_http/java/com/google/auth/oauth2/IdToken.java Outdated Show resolved Hide resolved
elharo
elharo approved these changes Aug 13, 2019
@salrashid123
Copy link
Contributor Author

@salrashid123 salrashid123 commented Aug 14, 2019

ok,i reverified all the credential types work as intended;

  • checked refresh works (ie., init one credential, use it, sleep for 1 hr then resuse the credential)

Note, the UserCredentials (ones based on an enduser three legged auth), does not implement the required interface so the cast won't work

GoogleCredentials adcCreds = GoogleCredentials.getApplicationDefault();              
IdTokenCredentials tokenCredential = IdTokenCredentials.newBuilder()
                         .setIdTokenProvider((IdTokenProvider) adcCreds)
                         .setTargetAudience(targetAudience).build();

will throw

Caused by: java.lang.ClassCastException: class com.google.auth.oauth2.UserCredentials cannot be cast to class com.google.auth.oauth2.IdTokenProvider (com.google.auth.oauth2.UserCredentials and com.google.auth.oauth2.IdTokenProvider are in unnamed module of loader java.net.URLClassLoader @6345dc06)

(unless ofcourse the GoogleCredential is internally a ServiceAccountCredential. Thid'd be working as intended though)

@chingor13 chingor13 added the kokoro:force-run label Aug 14, 2019
@kokoro-team kokoro-team removed the kokoro:force-run label Aug 14, 2019
@chingor13 chingor13 merged commit a87e3fd into googleapis:master Aug 14, 2019
10 checks passed
@salrashid123 salrashid123 deleted the add-id-token branch Aug 14, 2019
@salrashid123
Copy link
Contributor Author

@salrashid123 salrashid123 commented Aug 14, 2019

thanks; final note, you can use these id_tokens now directly inside gRPC channel credentials

https://github.com/salrashid123/grpc_google_id_tokens/blob/master/java/src/main/java/com/test/TestApp.java#L62

(i'll update my sample there once this change is live and shipped in a release)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes semver: minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants