Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

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

Merged
merged 14 commits into from May 26, 2021

Conversation

arithmetic1728
Copy link
Contributor

@arithmetic1728 arithmetic1728 commented Nov 12, 2020

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

Short summary:

Background

mtls has two parts: client certificate, endpoint

  1. 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
  2. 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:

  1. mtls client cert loading and the two mtls env var are wrapped in MtlsProvider class.
  2. 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)
  3. 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

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

codecov bot commented Nov 12, 2020

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
Impacted file tree graph

@@             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.

@arithmetic1728 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 arithmetic1728 force-pushed the sijun_mtls branch 2 times, most recently from ff0e7c9 to 18c26b5 Compare November 20, 2020 01:48
@arithmetic1728 arithmetic1728 marked this pull request as ready for review November 20, 2020 02:11
@arithmetic1728 arithmetic1728 requested review from a team as code owners November 20, 2020 02:11
dependencies.properties Outdated Show resolved Hide resolved
Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@miraleung miraleung left a comment

Choose a reason for hiding this comment

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

Took a first pass.

elharo
elharo previously requested changes Nov 30, 2020
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

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
Copy link

ejona86 commented Nov 30, 2020

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

@arithmetic1728
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@arithmetic1728 arithmetic1728 marked this pull request as draft December 3, 2020 23:45
@google-cla
Copy link

google-cla bot commented Dec 4, 2020

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 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
@google-cla
Copy link

google-cla bot commented Dec 4, 2020

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.

1 similar comment
@google-cla
Copy link

google-cla bot commented Dec 4, 2020

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 arithmetic1728 removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 6, 2021
@arithmetic1728
Copy link
Contributor Author

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

Copy link
Contributor

@miraleung miraleung left a comment

Choose a reason for hiding this comment

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

Took a first pass.

Copy link
Contributor

@miraleung miraleung left a comment

Choose a reason for hiding this comment

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

LGTM, please wait for approval by one other reviewer.

@arithmetic1728 arithmetic1728 merged commit b863041 into master May 26, 2021
@arithmetic1728 arithmetic1728 deleted the sijun_mtls branch May 26, 2021 20:46
gcf-merge-on-green bot pushed a commit that referenced this pull request Jun 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants