feat: self signed jwt support #1294

arithmetic1728 · 2021-02-08T08:03:41Z

This PR adds self signed jwt support (https://google.aip.dev/auth/4111). Googlers see this doc for more details.

If user uses ServiceAccountCredentials, and doesn't provide custom endpoint, scopes, and audience, then a new ServiceAccountJwtAccessCredentials should be created and used for self signed jwt feature.

This PR doesn't break existing client libraries.

1. Details

endpoint

In StubSettings.java, currently because both user and client library set the endpoint property, so we cannot tell whether the endpoint property is a custom endpoint set by user. To solve this problem, this PR creates a new defaultApiEndpoint property for the client libraries to use. Then we can compare endpoint with defaultApiEndpoint to know if endpoint is a custom endpoint.

Client library example PR using java-cloudbuild

Note: once cl/356594013 is checked in and rolled out, then all the changes in this section is not needed anymore. Depending on the actual timeline, I can update this PR or create a subsequent PR.

scopes

There are two types of CredentialsProvider for the user to provide a credentials: GoogleCredentialsProvider and FixedCredentialsProvider. Whether scopes are provided by user depends on the provider type.

GoogleCredentialsProvider provides ADC credential. Currently both user and client library use setScopesToApply method to set scopes. To tell the difference, this PR creates a new setDefaultScopes method for the client library to use. Client library example PR using java-cloudbuild

FixedCredentialsProvider wraps a credential created by the user. If the credential is ServiceAccountCredentials, then we can look at the scopes property.

audience

Users cannot set audience for ServiceAccountCredentials regardless of the credential provider type, so this doesn't apply.

2. Client examples PRs

This PR works for both GAPIC and DIREGAPIC clients. Examples PRs for client libraries:

java-cloudbuild (GAPIC): PR
java-compute (DIREGAPIC): PR

codecov · 2021-02-08T08:05:42Z

Codecov Report

Merging #1294 (72b2544) into master (721617b) will decrease coverage by 0.09%.
The diff coverage is 70.49%.

@@             Coverage Diff              @@
##             master    #1294      +/-   ##
============================================
- Coverage     79.34%   79.24%   -0.10%     
- Complexity     1235     1245      +10     
============================================
  Files           209      209              
  Lines          5398     5455      +57     
  Branches        454      464      +10     
============================================
+ Hits           4283     4323      +40     
- Misses          936      945       +9     
- Partials        179      187       +8

Impacted Files	Coverage Δ	Complexity Δ
...main/java/com/google/api/gax/rpc/StubSettings.java	`73.88% <44.44%> (-1.80%)`	`13.00 <1.00> (+1.00)`	⬇️
...ain/java/com/google/api/gax/rpc/ClientContext.java	`85.24% <70.27%> (-6.62%)`	`22.00 <4.00> (+4.00)`	⬇️
...google/api/gax/core/GoogleCredentialsProvider.java	`87.80% <86.66%> (+1.59%)`	`11.00 <1.00> (+5.00)`

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 721617b...3fce95d. Read the comment docs.

elharo · 2021-02-10T13:06:08Z

gax/src/main/java/com/google/api/gax/core/GoogleCredentialsProvider.java

+  }
+
+  /**
+   * If user doesn't provide custom endpoint and scopes, and the credentials is service account


Thank you for all the comments. Will address them in the next commit.

elharo · 2021-02-10T13:06:20Z

gax/src/main/java/com/google/api/gax/core/GoogleCredentialsProvider.java

+  /**
+   * If user doesn't provide custom endpoint and scopes, and the credentials is service account
+   * credentials, then we need to create a new ServiceAccountJwtAccessCredentials to use self signed
+   * jwt. See https://google.aip.dev/auth/4111.


elharo · 2021-02-10T13:06:40Z

gax/src/main/java/com/google/api/gax/core/GoogleCredentialsProvider.java

+
+  /**
+   * If user doesn't provide custom endpoint and scopes, and the credentials is service account
+   * credentials, then we need to create a new ServiceAccountJwtAccessCredentials to use self signed


rewrite in third person
This doc comment only seems to describe one detail of the method's implementation, not the entire method.

elharo · 2021-02-10T13:08:20Z

gax/src/main/java/com/google/api/gax/core/GoogleCredentialsProvider.java

@@ -134,9 +161,20 @@ public static Builder newBuilder() {
    @BetaApi
    public abstract List<String> getJwtEnabledScopes();

+    /**
+     * Default scopes is for client libraries to use. Users should use setScopesToApply to set


elharo · 2021-02-10T13:08:57Z

gax/src/main/java/com/google/api/gax/core/GoogleCredentialsProvider.java

@@ -134,9 +161,20 @@ public static Builder newBuilder() {
    @BetaApi
    public abstract List<String> getJwtEnabledScopes();

+    /**
+     * Default scopes is for client libraries to use. Users should use setScopesToApply to set
+     * scopes, or setJwtEnabledScopes to force to use ServiceAccountJWTCredentials.


force what?

elharo · 2021-02-10T13:10:17Z

gax/src/main/java/com/google/api/gax/rpc/ClientContext.java

@@ -132,6 +140,73 @@ public static ClientContext create(ClientSettings settings) throws IOException {
    return create(settings.getStubSettings());
  }

+  /**
+   * If user doesn't provide custom endpoint and scopes, and the credentials is service account
+   * credentials, then we need to create a new ServiceAccountJwtAccessCredentials to use self signed


avoid first person in doc comments, per google style

elharo · 2021-02-10T13:12:13Z

gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java

@@ -597,4 +602,70 @@ public void testUserAgentConcat() throws Exception {
    assertThat(transportChannel.getHeaders())
        .containsEntry("user-agent", "user-supplied-agent internal-agent");
  }
+
+  @Test
+  public void testSelfSignedJwtEndpointNotDefault() throws Exception {


can exception be more specific?

elharo · 2021-02-10T13:12:36Z

gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java

+    // Test when endpoint is null.
+    Credentials returnedCredentials =
+        ClientContext.determineSelfSignedJWTCredentials(provider, null, "https://foo");
+    assertThat(returnedCredentials).isEqualTo(credentials);


unnecessary use of Truth for simple equality

elharo · 2021-02-10T13:12:40Z

gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java

+  }
+
+  @Test
+  public void testSelfSignedJwtWithGoogleCredentialsProvider() throws Exception {


can exception be more specific?

elharo · 2021-02-10T13:12:45Z

gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java

+  }
+
+  @Test
+  public void testSelfSignedJwtWithFixedCredentialsProvider() throws Exception {


can exception be more specific?

igorbernstein2

Hi, thanks for working on this. However I think there are a couple of flaws here:

this breaks existing functionality of restricting access by scope
it prevents usage of jwts custom endpoints: for example this will prevent bigtable dataflow connector from being able to access batch-bigtable.googleapis.com using JWTs (its the same OP service, with different routing rules).
2b. audiences arent necessarily tied to the endpoint, it just happens that it aligns most of the time. Forcing the audience to be a prefix of the endpoint will prevent that usecase
This prevents adding outgoing sidecar proxies in front of our clients.
Given that this a new api, anything exposed as public should be marked as internal api or beta api in case it turns out that we need to change this because it doesnt account for everything that client devs or customers need.

igorbernstein2 · 2021-02-10T15:35:17Z

gax/src/main/java/com/google/api/gax/core/GoogleCredentialsProvider.java

@@ -134,9 +161,20 @@ public static Builder newBuilder() {
    @BetaApi
    public abstract List<String> getJwtEnabledScopes();


Please deprecate the previous approach of setJwtEnabledScopes, it's still marked as BetaApi and we shouldnt have 2 ways to opt into jwt

igorbernstein2 · 2021-02-10T15:41:39Z

gax/src/main/java/com/google/api/gax/rpc/ClientContext.java

+    // provide scopes, then we create a ServiceAccountJwtAccessCredentials and use this credentials
+    // instead. This credential uses self signed jwt, and it is more efficient since the network
+    // call to token endpoint is avoided. See https://google.aip.dev/auth/4111.
+    if (provider instanceof GoogleCredentialsProvider) {


Can all of this move into a static helper in CredentialsProvider (or anything that is more related to credentials)? Having the credential logic split between CredentialsProviders and here makes it very hard to follow the logic unless you know what you are looking for

igorbernstein2 · 2021-02-10T15:43:51Z

gax/src/main/java/com/google/api/gax/core/GoogleCredentialsProvider.java

+   * credentials, then we need to create a new ServiceAccountJwtAccessCredentials to use self signed
+   * jwt. See https://google.aip.dev/auth/4111.
+   */
+  public Credentials getCredentials(boolean endpointIsDefault, URI audienceForSelfSignedJwt)


Please mark audience as @Nullable

igorbernstein2 · 2021-02-10T15:50:17Z

gax/src/main/java/com/google/api/gax/core/GoogleCredentialsProvider.java

+        if (!scopes.contains(scope)) {
+          scopes.add(scope);
+        }
+      }


This breaks an existing usecase of using a narrow scope to restrict functionality. For example if a customer wanted to restrict bigtable access in an application to just read only, they would change the scopes to https://www.googleapis.com/auth/cloud-bigtable.data.readonly.

And if this is an intentional breakage, this needs to be made a lot more apparent in a separate PR that explicitly changes that behavior

igorbernstein2 · 2021-02-10T15:54:32Z

gax/src/main/java/com/google/api/gax/rpc/StubSettings.java

@@ -120,6 +123,10 @@ public final String getEndpoint() {
    return endpoint;
  }

+  public final String getDefaultApiEndpoint() {


should this be public? it seems like this entire api is designed for client authors, I dont think this should be made public to end users

arithmetic1728 · 2021-02-11T10:57:47Z

Hi, thanks for working on this. However I think there are a couple of flaws here:

this breaks existing functionality of restricting access by scope

Yes, this is a mistake.

it prevents usage of jwts custom endpoints: for example this will prevent bigtable dataflow connector from being able to access batch-bigtable.googleapis.com using JWTs (its the same OP service, with different routing rules).

Yes, I was waiting for cl/356594013 to unblock custom endpoints. It seems cl will be submitted soon, I will change the design.

2b. audiences arent necessarily tied to the endpoint, it just happens that it aligns most of the time. Forcing the audience to be a prefix of the endpoint will prevent that usecase

I guess I can change the design to use the prefix of endpoint, only if prefix of endpoint == prefix of audiences. For http client like java-compute, only the prefix of endpoint is a valid audience.

This prevents adding outgoing sidecar proxies in front of our clients.

How does this design prevent it?

Given that this a new api, anything exposed as public should be marked as internal api or beta api in case it turns out that we need to change this because it doesnt account for everything that client devs or customers need.

Yep.

igorbernstein2 · 2021-02-11T16:04:42Z

This prevents adding outgoing sidecar proxies in front of our clients.

The problem is that you are tying the audience to the endpoint. Example scenario: bigtable client with a caching read through forward proxy. The proxy will forward the request to bigtable if the response is not cached. In this case, the endpoint is the proxy, while the audience of the creds is bigtable.googleapis.com.

arithmetic1728 · 2021-02-11T22:35:17Z

This prevents adding outgoing sidecar proxies in front of our clients.

The problem is that you are tying the audience to the endpoint. Example scenario: bigtable client with a caching read through forward proxy. The proxy will forward the request to bigtable if the response is not cached. In this case, the endpoint is the proxy, while the audience of the creds is bigtable.googleapis.com.

The audience being set is a default audience, which is not used if uri is provided when creating the token. Currently for ServiceAccountJWTCredentials, if uri and default audience are both not set, program will throw exception. Since the jwt credentials set by gax-java via jwt enabled scopes doesn't have default audience, so I think this scenario uri must be already set, so it won't be affected.

arithmetic1728 · 2021-02-16T23:22:55Z

I changed the design and now the major implementation is in auth library. On one hand, cl/356594013 is submitted so we no longer need to worry about if the endpoint is default or not, which makes it easy to implement the feature in auth lib; on the other hand, the changes to gax-java is now minimal and shouldn't affect any existing client libs.

Please check out this PR in auth lib and this PR in gax-java. Thanks! @elharo @igorbernstein2

arithmetic1728 added 3 commits February 7, 2021 21:52

feat: self signed jwt support

1149983

update

98d004a

update

2c82d0c

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Feb 8, 2021

arithmetic1728 added 3 commits February 8, 2021 00:28

update

c5284c8

update

af25ca6

update

3fce95d

arithmetic1728 marked this pull request as ready for review February 10, 2021 07:58

arithmetic1728 requested review from a team as code owners February 10, 2021 07:58

arithmetic1728 requested review from silvolu, bshaffer, chingor13 and vam-google February 10, 2021 07:58

elharo reviewed Feb 10, 2021

View reviewed changes

igorbernstein2 suggested changes Feb 10, 2021

View reviewed changes

arithmetic1728 marked this pull request as draft February 11, 2021 01:13

arithmetic1728 closed this Feb 16, 2021

chanseokoh deleted the jwt branch October 29, 2021 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: self signed jwt support #1294

feat: self signed jwt support #1294

arithmetic1728 commented Feb 8, 2021 •

edited

codecov bot commented Feb 8, 2021 •

edited

elharo Feb 10, 2021

arithmetic1728 Feb 11, 2021

elharo Feb 10, 2021

elharo Feb 10, 2021

elharo Feb 10, 2021

elharo Feb 10, 2021

elharo Feb 10, 2021

elharo Feb 10, 2021

elharo Feb 10, 2021

elharo Feb 10, 2021

elharo Feb 10, 2021

igorbernstein2 left a comment

igorbernstein2 Feb 10, 2021

igorbernstein2 Feb 10, 2021

igorbernstein2 Feb 10, 2021

igorbernstein2 Feb 10, 2021

igorbernstein2 Feb 10, 2021

arithmetic1728 commented Feb 11, 2021

igorbernstein2 commented Feb 11, 2021

arithmetic1728 commented Feb 11, 2021

arithmetic1728 commented Feb 16, 2021

		@@ -134,9 +161,20 @@ public static Builder newBuilder() {
		@BetaApi
		public abstract List<String> getJwtEnabledScopes();

feat: self signed jwt support #1294

feat: self signed jwt support #1294

Conversation

arithmetic1728 commented Feb 8, 2021 • edited

1. Details

endpoint

scopes

audience

2. Client examples PRs

codecov bot commented Feb 8, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorbernstein2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arithmetic1728 commented Feb 11, 2021

igorbernstein2 commented Feb 11, 2021

arithmetic1728 commented Feb 11, 2021

arithmetic1728 commented Feb 16, 2021

arithmetic1728 commented Feb 8, 2021 •

edited

codecov bot commented Feb 8, 2021 •

edited