-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Upgrading Google auth library to 0.25.2 with dependency exclusion #8078
Conversation
build.gradle
Outdated
@@ -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_htt_client_jackson2: "com.google.http-client:google-http-client-jackson2:${googlehttpVersion}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xds/src/main/java/io/grpc/xds/internal/sts/StsCredentials
uses com.google.api.client.json.jackson2.JacksonFactory
from this artifact.
64e37f6
to
b2aac49
Compare
assertEquals("https://example.com:123/a.service", payload.get("aud")); | ||
assertEquals("https://example.com/", payload.get("aud")); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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++).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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."
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
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.
"Linux artifacts" and "Macos" didn't trigger job. (I guess some Kokoro issue) |
@@ -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}", |
There was a problem hiding this comment.
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.
b2aac49
to
dbe557f
Compare
Now all checks are green. @dapengzhang0 Would you review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
build.gradle
Outdated
// https://github.com/grpc/grpc-java/issues/8037 | ||
configurations.all { | ||
resolutionStrategy { | ||
force 'org.apache.httpcomponents:httpcore:4.4.14' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Thank you for the review. I'll wait for @jiangtaoli2016 for the input. |
assertEquals("https://example.com:123/a.service", payload.get("aud")); | ||
assertEquals("https://example.com/", payload.get("aud")); |
There was a problem hiding this comment.
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.
It's preferred to setup dependency exclusions and declare the problematic dependency as a direct dependency, because overwriting the dependency (in this case org.apache.httpcomponents:httpcore), while it may pass the checks in the build, does not have any effect in the produced pom.xml after the library is published to Maven Central.
@@ -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")); |
There was a problem hiding this comment.
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.
Problem in ':grpc-all:dependencies' task over JUnit dependencies.
|
Jackson-base declares junit (with test scope appropriately)
I'm not sure why the Gradle tool is showing Having said that, I think this is a bug in Gradle. JUnit declared as test-scope at jackson-core (see the effective pom at jackson-core 2.12.2) should not affect any configuration (i.e. class path) of grpc-java project. |
Google-http-client-jackson2 depends on jackson-core. Jackson-core depends on junit (with test scope). If we don't exclude the junit dependency, :grpc-all:dependencies (unexpectedly) fails to resolve dependencies. Details: grpc#8078 (comment)
jackson-parent has
And this is if I disable
|
Android failed:
This PR does not touch Guava. I assume this is irrelevant? |
google-auth-library-java (specifically google-auth-library-oauth2-http) bumped the Guava version. https://github.com/googleapis/google-auth-library-java/releases/tag/v0.23.0 We didn't notice earlier because we exclude the version of guava (to upgrade it): Lines 248 to 257 in b4fe07d
But now it is a downgrade. |
I see. The error is from the android-interop-testing, which uses
The error message suggests that guava-30.1-android is not compatible with Android. It's strange. Digging further. |
For reference: google/guava#5358 Let's upgrade Guava as a separate PR before this one goes in. That's probably something better for grpc-team to manage, as it could potentially devolve. |
google-auth-library-java:0.25.0 strips port and path parts in the audience claim ("aud"). Updating the test to pass in both old and new version of google-auth-library-java. This commit does not upgrade google-auth-library-java because it turned out that the upgrade involves the newer Guava version (google-auth-library-java's dependency) failing with DexingNoClasspathTransform. Details: grpc#8078 (comment) It's technically possible to exclude the newer Guava, but it's a good practice avoid excluding the newer version of a library.
google-auth-library-java:0.25.0 strips port and path parts in the audience claim ("aud"). Updating the test to pass in both old and new version of google-auth-library-java. This commit does not upgrade google-auth-library-java because it turned out that the upgrade involves the newer Guava version (google-auth-library-java's dependency) failing with DexingNoClasspathTransform. Details: grpc#8078 (comment) It's technically possible to exclude the newer Guava, but it's a good practice avoid excluding the newer version of a library.
We're splitting out two parts of this PR for the short-term. #8099 will update the test which allows upgrading google-auth-library-java anywhere the grpc tests are also run. That addresses the main issue @suztomo is suffering from. Additionally, #8100 upgrades to the newer Guava. Since upgrading to the newer Guava is risky, and I don't get excited about the idea of flip-flopping on the JWT behavior of google-auth-library-java (if we have to revert the upgrade due to Guava), I think I'd like to hold off on the upgrade of google-auth-library-java for a release. Users are free to use a newer version, and the googleapis libraries can use a newer release. |
google-auth-library-java:0.25.0 strips port and path parts in the audience claim ("aud"). Updating the test to pass in both old and new version of google-auth-library-java. This commit does not upgrade google-auth-library-java because it turned out that the upgrade involves the newer Guava version (google-auth-library-java's dependency) failing with DexingNoClasspathTransform. Details: #8078 (comment) It's technically possible to exclude the newer Guava, but it's a good practice avoid excluding the newer version of a library.
Fixes #8037
Why dependency exclusion of httpcore?
Previous attempt failed (#8044) due to
failOnVersionConflict
, and there's no planned upcoming release fororg.apache.httpcomponents:httpclient
4.5.X, which would fix the discrepancy.Initially this PR tried to overwrite the httpcore dependency, but it does not have effect in the pom.xml consumed by gRPC users. Therefore this PR now excludes httpcore dependency and redeclare it as direct dependencies.