[native] remove dependency on identity service package for gRPC client
ClosedPublic
Actions

Authored by varun on Sep 6 2022, 9:21 AM.

Details

Reviewers

tomek
atul
ashoat
• jon

Commits

rCOMMa7259c860c29: [native] remove dependency on identity service package for gRPC client

Summary

The location of the protos directory is still in flux. That means that it's difficult to take a dependency on the Identity service crate, even though it would be ideal to not repeat OPAQUE config code on client and server. This issue tracks restoring the dependency on the Identity crate: https://linear.app/comm/issue/ENG-1746/remove-opaquers-from-grpc-client

Added ashoat since there are new dependencies (these are all in the Identity service as well)

Test Plan

cargo build

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

varun created this revision.Sep 6 2022, 9:21 AM

Herald added a reviewer: • jon. · View Herald TranscriptSep 6 2022, 9:21 AM

Herald added a subscriber: • abosh. · View Herald Transcript

varun added inline comments.Sep 6 2022, 9:22 AM

native/cpp/CommonCpp/grpc/grpc_client/Cargo.toml
20–22 ↗	(On Diff #16381)	we don't use the latest version of `sha2` or `digest` because the API has changed and I want to keep things consistent with the Identity service. these dependencies will all be removed anyway, once we can take a dependency on the Identity service crate again

Harbormaster completed remote builds in B11932: Diff 16381.Sep 6 2022, 9:31 AM

varun requested review of this revision.Sep 6 2022, 9:31 AM

varun added a child revision: D4953: [native] add Rust gRPC client to XCode project.Sep 6 2022, 11:22 AM

wrt Test Plan I can figure out where to run cargo build, but it's helpful for this sort of thing to give reviewers something they can copy and paste. Something like:

cd native/cpp/CommonCpp/grpc/grpc_client && cargo clean && cargo build

native/cpp/CommonCpp/grpc/grpc_client/src/opaque.rs
1–30 ↗	(On Diff #16381)	nit: was able to get an idea of what was going on from the Linear link, but would've been good to at least minimally mention what's going on here in the Summary section

In D5075#148029, @atul wrote:
wrt Test Plan I can figure out where to run cargo build, but it's helpful for this sort of thing to give reviewers something they can copy and paste. Something like:
cd native/cpp/CommonCpp/grpc/grpc_client && cargo clean && cargo build

Fair enough

varun added inline comments.Sep 8 2022, 9:56 AM

native/cpp/CommonCpp/grpc/grpc_client/src/opaque.rs
1–30 ↗	(On Diff #16381)	additional context: this file currently exists in the identity service cargo project. we don't want to depend directly on that crate until the .proto file location issue is resolved. once resolved, we can remove this file and the new dependencies in Cargo.toml and just use the identity crate directly

ashoat accepted this revision.Sep 9 2022, 1:33 PM

tomek accepted this revision.Sep 12 2022, 5:36 AM

tomek added inline comments.

native/cpp/CommonCpp/grpc/grpc_client/src/opaque.rs
26 ↗	(On Diff #16381)	Is it a good idea to use hardcoded salt?

This revision is now accepted and ready to land.Sep 12 2022, 5:36 AM

varun added inline comments.Sep 12 2022, 3:24 PM

native/cpp/CommonCpp/grpc/grpc_client/src/opaque.rs
26 ↗	(On Diff #16381)	`opaque-ke` already takes care of the salting but `argon2` requires a salt of minimum 8 bytes so we just zero out an 8-byte array