feat: add mtls feature to http and grpc transport provider #1249

arithmetic1728 · 2020-11-12T23:40:01Z

Internal doc: go/java-gapic-client-mtls.
AIP: https://google.aip.dev/auth/4114

Short summary:

Background

mtls has two parts: client certificate, endpoint

client certificate
(1) client certificate is loaded from device by running the command specified in ~/.secureConnect/context_aware_metadata.json
(2) whether to load/use client certificate is controlled by GOOGLE_API_USE_CLIENT_CERTIFICATE env variable. To load/use it, set the env var to "true".
(3) if client cert exists and used, it will be used to create a HttpTransport for http, and ChannelCredentials for grpc
endpoint
(1) there are regular endpoint and mtls endpoint. Regular endpoint is used by default.
(2) if user provides endpoint, it should always be used as it is
(3) if endpoint is not provided by the user (i.e. it is set by client lib), then endpoint behavior can be controlled by GOOGLE_API_USE_MTLS_ENDPOINT env var
i) if client cert exists and is used, auto switch to use mtls endpoint
ii) if env var is "never", always use regular endpoint
iii) if env var is "always", always use mtls endpoint

This PR

This PR is organized as follows:

mtls client cert loading and the two mtls env var are wrapped in MtlsProvider class.
client cert is converted to key store by MtlsProvider, and used to create HttpTransport for http, and ChannelCredentials for grpc (key store is converted to key manager for grpc)
in stub setting, adds allowSwitchToMtlsEndpoint property. This is used to tell who sets the endpoint. Client lib needs to set it to true. If the value is false, we just use the provided endpoint and cannot switch to mtls endpoint.

Testing

This PR has been tested with kms (for grpc) and compute (for http).
arithmetic1728/java-kms#1
https://github.com/arithmetic1728/java-compute/pull/4

codecov · 2020-11-12T23:42:03Z

Codecov Report

Merging #1249 (71d7e35) into master (52e39f8) will decrease coverage by 1.88%.
The diff coverage is 74.80%.

❗ Current head 71d7e35 differs from pull request most recent head 7d7612a. Consider uploading reports for the commit 7d7612a to get more accurate results

@@             Coverage Diff              @@
##             master    #1249      +/-   ##
============================================
- Coverage     81.46%   79.58%   -1.89%     
+ Complexity     1347     1275      -72     
============================================
  Files           211      211              
  Lines          5730     5588     -142     
  Branches        525      477      -48     
============================================
- Hits           4668     4447     -221     
- Misses          850      952     +102     
+ Partials        212      189      -23

Impacted Files	Coverage Δ
...main/java/com/google/api/gax/rpc/StubSettings.java	`74.11% <59.09%> (-1.56%)`	⬇️
...java/com/google/api/gax/rpc/mtls/MtlsProvider.java	`75.92% <75.92%> (ø)`
...ain/java/com/google/api/gax/rpc/ClientContext.java	`91.83% <78.57%> (-0.03%)`	⬇️
...httpjson/InstantiatingHttpJsonChannelProvider.java	`71.60% <78.94%> (+1.76%)`	⬆️
...api/gax/grpc/InstantiatingGrpcChannelProvider.java	`77.58% <80.00%> (+0.12%)`	⬆️
...gle/api/gax/rpc/mtls/ContextAwareMetadataJson.java	`100.00% <100.00%> (ø)`
...m/google/api/gax/retrying/NoopRetryingContext.java	`0.00% <0.00%> (-100.00%)`	⬇️
...ogle/api/gax/retrying/StreamingRetryAlgorithm.java	`0.00% <0.00%> (-82.61%)`	⬇️
...e/api/gax/rpc/RetryingServerStreamingCallable.java	`0.00% <0.00%> (-70.00%)`	⬇️
...ava/com/google/api/gax/retrying/RetrySettings.java	`31.25% <0.00%> (-24.52%)`	⬇️
... and 23 more

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 3b1859e...7d7612a. Read the comment docs.

dependencies.properties

vam-google

I'm a bit concerned by usage of system tools to read files: is it necessary? if yes, then why? It seems like the same result can be achieved in pure system-independent java-native way. Which will be more efficient and most importantly more portable and reliable.

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

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

gax/src/main/java/com/google/api/gax/rpc/mtls/MtlsUtils.java

gax/src/test/java/com/google/api/gax/rpc/mtls/MtlsUtilsTest.java

gax/src/test/resources/com/google/api/gax/rpc/mtls/mtls_context_aware_metadata.json

gax/src/main/java/com/google/api/gax/rpc/mtls/MtlsUtils.java

gax/src/main/java/com/google/api/gax/rpc/mtls/ContextAwareMetadataJson.java

gax/src/main/java/com/google/api/gax/rpc/mtls/MtlsProvider.java

gax/src/main/java/com/google/api/gax/rpc/mtls/MtlsUtils.java

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

miraleung

Took a first pass.

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

gax/src/main/java/com/google/api/gax/rpc/mtls/MtlsProvider.java

gax/src/main/java/com/google/api/gax/rpc/mtls/MtlsUtils.java

elharo

grpc team has expressed strong misgivings about the use of the shaded class. I'm not sure I fully grok everything in this PR, but I think their concerns need to be given serious weight. At this time, I am unwilling to proceed further without their signoff.

This work is complex because it crosses multiple repos and teams. I suggest adding someone from GRPC to the reviewers on go/java-gapic-client-mtls and make sure they're OK with the approach. The ultimate solution might need to be implemented in a different way, or in a different order. It's possible we should roll back some of the changes that have already been made. I'm not sure.

FYI, you don't require my approval, but I do think you should get the gRPC team's approval.

grpc/grpc-java#7667

ejona86 · 2020-11-30T19:33:35Z

As I mentioned in grpc/grpc-java#7667, even non-shaded Netty is a no-no for gax, as it is unstable API. Gax should really be using https://github.com/grpc/grpc-java-api-checker, to make that sort of bug a compile error. Discussion about which API to use (there isn't one currently) will continue on grpc/grpc-java#7667

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

arithmetic1728 · 2020-12-03T23:41:02Z

I'm a bit concerned by usage of system tools to read files: is it necessary? if yes, then why? It seems like the same result can be achieved in pure system-independent java-native way. Which will be more efficient and most importantly more portable and reliable.

Because we need to run a system-dependent native helper binary to get the certificate. This is mentioned in Obtaining the Default Device Certificate section in the AIP. Quoted from the AIP:

The default device certicate should be procured using the EndpointVerification workflow, 
which fetches the certificate from a platform-specific credential store (ex. KeyChain in 
macOS) via a native helper.

Exporting the certificate via the native helper involves executing a "cert provider command"
specified in a well-known gcloud metadata file of the following format:

{
   "version": 1
   "min_cloud_sdk_version": "240.0.0"
   "has_client_cert": true
   "endpoint_verification_error": ""
   "cert_provider_command": "[absolute_path_to_provider_command] --fetch_client_cert"
}

For Linux and macOS platforms, the above metadata file is located at "~/.secureConnect/context_aware_metadata.json".

The cert provider command will print the certificate to stdout

arithmetic1728 · 2020-12-03T23:45:00Z

I will revisit this PR and re-implement the grpc part after a new stable api is available in grpc-java. Currently putting this PR on hold.

google-cla · 2020-12-04T06:55:25Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla · 2020-12-04T07:44:52Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla · 2020-12-04T09:18:16Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

arithmetic1728 · 2021-05-06T00:38:57Z

This PR is ready now. Please take another look, thanks! @elharo @miraleung @vam-google @ejona86

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

gax/src/main/java/com/google/api/gax/rpc/mtls/ContextAwareMetadataJson.java

done

miraleung

Took a first pass.

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

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

gax/src/main/java/com/google/api/gax/rpc/mtls/MtlsProvider.java

miraleung

LGTM, please wait for approval by one other reviewer.

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

gax/src/main/java/com/google/api/gax/rpc/mtls/MtlsProvider.java

dependencies.properties

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

...httpjson/src/main/java/com/google/api/gax/httpjson/InstantiatingHttpJsonChannelProvider.java

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

gax/src/main/java/com/google/api/gax/rpc/mtls/MtlsProvider.java

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

gax/src/test/java/com/google/api/gax/rpc/mtls/AbstractMtlsTransportChannelTest.java

...httpjson/src/main/java/com/google/api/gax/httpjson/InstantiatingHttpJsonChannelProvider.java

gax/src/main/java/com/google/api/gax/rpc/mtls/MtlsProvider.java

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

...json/src/test/java/com/google/api/gax/httpjson/InstantiatingHttpJsonChannelProviderTest.java

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

gax/src/main/java/com/google/api/gax/rpc/mtls/MtlsProvider.java

🤖 I have created a release \*beep\* \*boop\* --- ## [1.65.0](https://www.github.com/googleapis/gax-java/compare/v1.64.0...v1.65.0) (2021-06-02) ### Features * add mtls feature to http and grpc transport provider ([#1249](https://www.github.com/googleapis/gax-java/issues/1249)) ([b863041](https://www.github.com/googleapis/gax-java/commit/b863041bc4c03c8766e0feca8cb10f531373dc44)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 12, 2020

arithmetic1728 changed the title ~~[WIP] feat: add mtls feature to http and grpc transports~~ feat: add mtls feature to http and grpc transport provider Nov 20, 2020

arithmetic1728 force-pushed the sijun_mtls branch 2 times, most recently from ff0e7c9 to 18c26b5 Compare November 20, 2020 01:48

arithmetic1728 marked this pull request as ready for review November 20, 2020 02:11

arithmetic1728 requested review from a team as code owners November 20, 2020 02:11

arithmetic1728 force-pushed the sijun_mtls branch from 18c26b5 to 3586d5a Compare November 21, 2020 01:59

arithmetic1728 requested review from miraleung, chingor13, silvolu and elharo November 23, 2020 21:49

elharo reviewed Nov 24, 2020

View reviewed changes

dependencies.properties Outdated Show resolved Hide resolved

vam-google reviewed Nov 24, 2020

View reviewed changes

elharo reviewed Nov 24, 2020

View reviewed changes

miraleung reviewed Nov 25, 2020

View reviewed changes

gax/src/main/java/com/google/api/gax/rpc/StubSettings.java Outdated Show resolved Hide resolved

gax/src/main/java/com/google/api/gax/rpc/mtls/MtlsProvider.java Outdated Show resolved Hide resolved

gax/src/main/java/com/google/api/gax/rpc/mtls/MtlsUtils.java Outdated Show resolved Hide resolved

elharo previously requested changes Nov 30, 2020

View reviewed changes

ejona86 reviewed Nov 30, 2020

View reviewed changes

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java Outdated Show resolved Hide resolved

ejona86 mentioned this pull request Nov 30, 2020

Support custom KeyManager with TlsChannelCredentials grpc/grpc-java#7667

Closed

arithmetic1728 force-pushed the sijun_mtls branch from 3586d5a to 210ed08 Compare December 3, 2020 23:14

arithmetic1728 marked this pull request as draft December 3, 2020 23:45

google-cla bot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Dec 4, 2020

Merge branch 'master' into sijun_mtls

c23acbd

arithmetic1728 removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 6, 2021

elharo reviewed May 6, 2021

View reviewed changes

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java Outdated Show resolved Hide resolved

gax/src/main/java/com/google/api/gax/rpc/mtls/ContextAwareMetadataJson.java Show resolved Hide resolved

arithmetic1728 force-pushed the sijun_mtls branch from 36adb04 to c23acbd Compare May 6, 2021 22:23

Merge branch 'master' into sijun_mtls

e85ce2c

miraleung reviewed May 13, 2021

View reviewed changes

arithmetic1728 added 2 commits May 13, 2021 13:03

resolve comments

d8d6e98

update

90d4d9c

miraleung approved these changes May 17, 2021

View reviewed changes

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java Outdated Show resolved Hide resolved

gax/src/main/java/com/google/api/gax/rpc/mtls/MtlsProvider.java Outdated Show resolved Hide resolved

arithmetic1728 and others added 2 commits May 17, 2021 17:22

update

926d31a

Merge branch 'master' into sijun_mtls

8b1a615

elharo reviewed May 18, 2021

View reviewed changes

arithmetic1728 and others added 2 commits May 19, 2021 00:14

update

a6da341

Merge branch 'master' into sijun_mtls

93d4a0a

elharo reviewed May 19, 2021

View reviewed changes

arithmetic1728 and others added 2 commits May 19, 2021 11:27

update

c7263dd

Merge branch 'master' into sijun_mtls

db1b3b3

elharo reviewed May 21, 2021

View reviewed changes

update

b3a5439

elharo reviewed May 25, 2021

View reviewed changes

arithmetic1728 and others added 2 commits May 25, 2021 13:55

update

716bce1

Merge branch 'master' into sijun_mtls

7d7612a

silvolu approved these changes May 26, 2021

View reviewed changes

arithmetic1728 merged commit b863041 into master May 26, 2021

arithmetic1728 deleted the sijun_mtls branch May 26, 2021 20:46

arithmetic1728 mentioned this pull request Jun 2, 2021

feat: add mtls feature googleapis/sdk-platform-java#672

Merged

zhumin8 mentioned this pull request Nov 7, 2024

add documentation about mTLS support and usage googleapis/sdk-platform-java#3354

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add mtls feature to http and grpc transport provider #1249

feat: add mtls feature to http and grpc transport provider #1249

arithmetic1728 commented Nov 12, 2020 •

edited

Loading

codecov bot commented Nov 12, 2020 •

edited

Loading

vam-google left a comment

miraleung left a comment

elharo left a comment

ejona86 commented Nov 30, 2020

arithmetic1728 commented Dec 3, 2020

arithmetic1728 commented Dec 3, 2020

google-cla bot commented Dec 4, 2020

google-cla bot commented Dec 4, 2020

google-cla bot commented Dec 4, 2020

arithmetic1728 commented May 6, 2021

miraleung left a comment

miraleung left a comment

feat: add mtls feature to http and grpc transport provider #1249

feat: add mtls feature to http and grpc transport provider #1249

Conversation

arithmetic1728 commented Nov 12, 2020 • edited Loading

Background

This PR

Testing

codecov bot commented Nov 12, 2020 • edited Loading

Codecov Report

vam-google left a comment

Choose a reason for hiding this comment

miraleung left a comment

Choose a reason for hiding this comment

elharo left a comment

Choose a reason for hiding this comment

ejona86 commented Nov 30, 2020

arithmetic1728 commented Dec 3, 2020

arithmetic1728 commented Dec 3, 2020

google-cla bot commented Dec 4, 2020

google-cla bot commented Dec 4, 2020

google-cla bot commented Dec 4, 2020

arithmetic1728 commented May 6, 2021

miraleung left a comment

Choose a reason for hiding this comment

miraleung left a comment

Choose a reason for hiding this comment

arithmetic1728 commented Nov 12, 2020 •

edited

Loading

codecov bot commented Nov 12, 2020 •

edited

Loading