Page MenuHomePhabricator

[grpc_clients] fix gRPC client on native
ClosedPublic

Authored by varun on Dec 14 2023, 6:44 PM.
Tags
None
Referenced Files
F3376689: D10343.id34687.diff
Wed, Nov 27, 1:38 AM
F3376406: D10343.diff
Tue, Nov 26, 11:47 PM
Unknown Object (File)
Wed, Nov 6, 4:51 AM
Unknown Object (File)
Oct 18 2024, 2:24 AM
Unknown Object (File)
Oct 18 2024, 2:24 AM
Unknown Object (File)
Oct 18 2024, 2:24 AM
Unknown Object (File)
Oct 18 2024, 2:24 AM
Unknown Object (File)
Oct 18 2024, 2:23 AM
Subscribers

Details

Summary

The gRPC client needs a set of root certificates for the TLS handshake. When running the keyserver (both inside a Docker container and not) or simulator, the client is able to locate the root certificates at one of the paths we had enumerated. However, on physical iOS and Android devices, the certificates have to be bundled with the app.

The tls-webpki-roots feature adds Mozilla's root certificates to our rustls-based gRPC client, so we don't have to rely on platform certs.

Test Plan

This branch contains @kamil's code from D10327. I used that code (and verifyUserLoggedIn on keyserver) to test that the shared gRPC client could talk to staging AND local identity services (https and http) from:

  • local keyserver (non-Docker)
  • local keyserver (Docker)
  • iOS simulator
  • Android emulator
  • Physical iPhone
  • Physical Pixel 3

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 14 2023, 6:56 PM
Harbormaster failed remote builds in B25098: Diff 34683!
varun requested review of this revision.Dec 14 2023, 7:08 PM

A couple buildkite jobs failed but that's expected since they were failing in D10327. I'm requesting review but will rebase and remove D10327 from my branch after the diff is accepted and run arc diff again to make sure the jobs all pass before landing. Keeping D10327 in the branch for now will make it easier for reviewers to check out the code and validate my test plan with yarn arcpatch.

Seems reasonable, but I don't think I'm a good reviewer for this

Nice! So it turns out only tls-webpki-roots is needed for all platforms.

LGTM but I'll patch locally and check if it's also callable from other services (verify access token) before accepting

Tested:

  • It's accessible from other services locally
  • From local docker
  • From other AWS services
shared/grpc_clients/Cargo.toml
9 ↗(On Diff #34683)

Minor nit: My IDE formatter for Cargo.toml files suggests this.

This revision is now accepted and ready to land.Dec 14 2023, 11:58 PM

thanks for the quick review! will address the nit and rebase this branch to fix buildkite

shared/grpc_clients/Cargo.toml
9 ↗(On Diff #34683)

nice catch :)

This revision was landed with ongoing or failed builds.Dec 15 2023, 12:59 AM
This revision was automatically updated to reflect the committed changes.