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.

Sign up for GitHub

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

Jump to bottom

Upgrading Google auth library to 0.25.2 with dependency exclusion #8078

Closed
wants to merge 6 commits into from
Closed

Upgrading Google auth library to 0.25.2 with dependency exclusion #8078

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ec8d191
Upgrading google-auth-library to 0.25.2
suztomo Apr 2, 2021
c8e0264
Dependency override
suztomo Apr 12, 2021
dbe557f
google-http-java-client-jackson2 with dependency exclusions
suztomo Apr 12, 2021
26079a1
Exclusion for httpcore rather than overwriting it
suztomo Apr 16, 2021
b19fc94
Allow both old and new behavior from google-auth-library-java
ejona86 Apr 16, 2021
009f6f5
Excluding junit from google-http-client-jackson2
suztomo Apr 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion auth/src/test/java/io/grpc/auth/GoogleAuthLibraryCallCredentialsTest.java
View file
Open in desktop
Expand Up @@ -394,7 +394,7 @@ public void jwtAccessCredentialsInRequestMetadata() throws Exception {
Map<?, ?> header = (Map<?, ?>) JsonParser.parse(jsonHeader);
assertEquals("test-private-key-id", header.get("kid"));
Map<?, ?> payload = (Map<?, ?>) JsonParser.parse(jsonPayload);
assertEquals("https://example.com:123/a.service", payload.get("aud"));
assertEquals("https://example.com/", payload.get("aud"));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#8037 is about this change.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is go/simpler-JWT-token-audience. I'm surprised to see the port removed since the port was not 443. @jiangtaoli2016, is this okay?

This does have the potential to break users. Technically the JWT logic was gRPC-specific, not googleapis.com-specific. I do wonder if there will be any blow-back from that. This right here is a service account which I don't believe is a standard format, so it may be fine. But I worry what other paths there are to create JWTs that aren't Google-specific that may be broken (Java for one thing; but other languages is also a concern).

Copy link
Contributor

Choose a reason for hiding this comment

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

It is the new standard of setting audience in JWT token
https://cloud.google.com/api-gateway/docs/authenticating-users-jwt

We also need to change some of the C++ grpc implementation.

Copy link
Member

Choose a reason for hiding this comment

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

@jiangtaoli2016, specifically, is stripping the port when it is not 443 "okay"? I see nothing regarding it in the doc you linked to.

To be pedantic, JWT is a standard, but this authority change appears to be Google-specific. With this change it would be easy to break the audience for anyone using JWT for their own services. For these specific Java APIs, the obvious ones to me seem to say "Google" prominently in the class name or javadoc. I don't know if that is true in other languages (e.g., C++).

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, this auth library is only used for talking to Google APIs.

If a customer wants to use JWT to talk to its own services, he/she should use his/her own library rather than Google auth library.

Copy link
Member

Choose a reason for hiding this comment

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

@jiangtaoli2016, is stripping the port okay? I'd like an explicit answer. If the answer is "it is okay for Google properties" then that is still a useful answer.

IIUC, this auth library is only used for talking to Google APIs.

That was not the case for google-auth-library-java, at least for the Credentials and OAuth2Credentials classes. I expect most gRPC users doing authentication to use the google-auth-library-java Credentials class. The Google-specific credentials are primarily derived from GoogleCredentials. ServiceAccountJwtAccessCredentials does not extend GoogleCredentials, but the JSON service account stuff is fairly Google-specific.

The C++ API is nowhere near as clear. ServiceAccountJWTAccessCredentials doesn't have "Google" anywhere to be found. Yet AccessTokenCredentials, which should not be specific to Google, says "only send tokens to Google" (paraphrase), without actually saying they are Google tokens. I'm not trying to call out C++ saying it is "doing it wrong" or whatever; I'm trying to call out "we have to be careful" and "don't go around assuming things."

I found the PR with the change: googleapis/google-auth-library-java#572 . Interestingly, it is now auto-promoting ServiceAccountAccessCredentials to ServiceAccountJWTAccessCredentials. So that means we can delete all of JwtHelper that does that promotion.

I don't know if it is just me, but the release notes of google-auth-library-java seem really bad and useless. It seems I'm expected to read every PR to have any hope of understanding what changed. Deleting methods is considered bug fixes, even if only a single method is changed there's no mention of the class/method in the description, and completely-internal lint-level fixes are mixed in with everything else.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ejona86

is stripping the port okay?

It is OK for google APIs. "API Gateway accepts all JWTs with the backend service name in the form of https://SERVICE_NAME in the aud claim."

Copy link
Member

Choose a reason for hiding this comment

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

https://somename:123/ is a different backend service name than https://somename/

Copy link
Member

Choose a reason for hiding this comment

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

The way that could be fine for Google APIs is if we said, "Google will never use a non-443 port, or if it does it will be considered equivalent."

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked with Shin Fan who confirmed that we should stripe port. Don't know why we have port in the first place.
Also I think all Google APIs use 443 port.

Copy link
Member

Choose a reason for hiding this comment

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

This will require an atomic upgrade of the auth library inside Google as well. We don't want to require that. Change the logic here to allow either the old or the new audience, with a comment.

assertEquals("test-email@example.com", payload.get("iss"));
assertEquals("test-email@example.com", payload.get("sub"));
}
Expand Down
14 changes: 13 additions & 1 deletion build.gradle
View file
Open in desktop
Expand Up @@ -56,7 +56,8 @@ subprojects {

nettyVersion = '4.1.52.Final'
guavaVersion = '30.0-android'
googleauthVersion = '0.22.2'
googleauthVersion = '0.25.2'
googlehttpVersion = '1.39.1'
protobufVersion = '3.12.0'
protocVersion = protobufVersion
opencensusVersion = '0.28.0'
Expand Down Expand Up @@ -155,6 +156,7 @@ subprojects {
google_api_protos: 'com.google.api.grpc:proto-google-common-protos:2.0.1',
google_auth_credentials: "com.google.auth:google-auth-library-credentials:${googleauthVersion}",
google_auth_oauth2_http: "com.google.auth:google-auth-library-oauth2-http:${googleauthVersion}",
google_http_client_jackson2: "com.google.http-client:google-http-client-jackson2:${googlehttpVersion}",
Copy link
Contributor Author

@suztomo suztomo Apr 12, 2021 •
edited

Choose a reason for hiding this comment

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

This is because grpc-xds uses a class from the artifact:

> Task :grpc-xds:compileJava FAILED
/Users/suztomo/grpc-java/xds/src/main/java/io/grpc/xds/internal/sts/StsCredentials.java:29: error: package com.google.api.client.json.jackson2 does not exist
import com.google.api.client.json.jackson2.JacksonFactory;

It was a used-but-undeclared dependency.

okhttp: 'com.squareup.okhttp:okhttp:2.7.4',
okio: 'com.squareup.okio:okio:1.17.5',
opencensus_api: "io.opencensus:opencensus-api:${opencensusVersion}",
Expand Down Expand Up @@ -289,6 +291,16 @@ subprojects {
])) {
runtimeClasspath {
resolutionStrategy.failOnVersionConflict()

// We can remove this dependency override once org.apache.httpcomponents:httpclient:4.5.13
// is released with org.apache.httpcomponents:httpcore:4.4.14 dependency, and
// com.google.http-client:google-http-client:1.39.1 is upgraded with them.
// https://github.com/grpc/grpc-java/issues/8037
configurations.all {
resolutionStrategy {
force 'org.apache.httpcomponents:httpcore:4.4.14'
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this does what we need it to do. It may satisfy failOnVersionConflict(), but we have failOnVersionConflict() only to detect cases where Maven would interpret the dependencies incorrectly. Generally we have to exclude+redefine the dependency to force a specific transitive dependency version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good. I'll update so.

}
}
}
}
}
Expand Down
7 changes: 7 additions & 0 deletions xds/build.gradle
View file
Open in desktop
Expand Up @@ -37,6 +37,7 @@ dependencies {
libraries.re2j,
libraries.bouncycastle,
libraries.autovalue_annotation

def nettyDependency = implementation project(':grpc-netty')

implementation (libraries.opencensus_proto) {
Expand All @@ -51,6 +52,12 @@ dependencies {
exclude group: 'com.google.errorprone', module: 'error_prone_annotations'
}

implementation (libraries.google_http_client_jackson2) {
exclude group: 'com.google.guava', module: 'guava'
exclude group: 'com.google.errorprone', module: 'error_prone_annotations'
exclude group: 'io.grpc', module: 'grpc-context'
}

testImplementation project(':grpc-core').sourceSets.test.output

annotationProcessor libraries.autovalue
Expand Down