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
Changes from 4 commits
ec8d191
c8e0264
dbe557f
26079a1
b19fc94
009f6f5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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")); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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' | ||
|
@@ -150,11 +151,13 @@ subprojects { | |
cronet_embedded: 'org.chromium.net:cronet-embedded:66.3359.158', | ||
gson: "com.google.code.gson:gson:2.8.6", | ||
guava: "com.google.guava:guava:${guavaVersion}", | ||
httpcore: "org.apache.httpcomponents:httpcore:4.4.14", | ||
javax_annotation: 'org.apache.tomcat:annotations-api:6.0.53', | ||
jsr305: 'com.google.code.findbugs:jsr305:3.0.2', | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. This is because grpc-xds uses a class from the artifact:
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}", | ||
|
@@ -250,10 +253,12 @@ subprojects { | |
exclude group: 'com.google.guava', module: 'guava' | ||
exclude group: 'io.grpc', module: 'grpc-context' | ||
exclude group: 'io.opencensus', module: 'opencensus-api' | ||
exclude group: 'org.apache.httpcomponents', module: 'httpcore' | ||
} | ||
dependencies.runtimeOnly project(':grpc-context') | ||
censusApiDependency 'runtimeOnly' | ||
guavaDependency 'runtimeOnly' | ||
dependencies.runtimeOnly libraries.httpcore | ||
} | ||
|
||
// A util function to config perfmark dependency with transitive | ||
|
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.
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. YetAccessTokenCredentials
, 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.
@ejona86
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.