Details

Reviewers

• jon
bartek
tomek

Commits

rCOMMbe572df58390: [services] Tunnelbroker - Modify server handler to use Rust signature…

Summary

This diff modifies the Tunnelbroker new session gRPC server handler to drop using the C++ cryptopp function for the signed string verification in a favor of using the Rust verify_signed_string function from D5945.

Removing deprecated code from the C++ codebase is in the following D5948.

Linear task: ENG-2492

Test Plan

Testing process:

Patch to the D5947
Run the integration tests in D5931 using the cargo t --test tunnelbroker_integration_test command.

The expected result is passing the integration test (signing and verifying the string are successful and the new session creation test will pass).

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• max created this revision.Dec 19 2022, 12:37 PM

• max held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptDec 19 2022, 12:37 PM

Herald added subscribers: atul, tomek, ashoat. · View Herald Transcript

• max added a parent revision: D5946: [services] Tunnelbroker - Expose string to be signed to the Rust.Dec 19 2022, 12:37 PM

• max added a child revision: D5948: [services] Tunnelbroker - Remove `cryptopp` usage from the C++ codebase.

Harbormaster failed remote builds in B14468: Diff 19723!Dec 19 2022, 1:04 PM

Rebasing on parent changes.

Harbormaster failed remote builds in B14528: Diff 19785!Dec 20 2022, 9:39 AM

• max mentioned this in D5944: [services] Tunnelbroker - Adding of the `base64` crate to dependencies.Dec 21 2022, 5:41 AM

• max mentioned this in D5945: [services] Tunnelbroker - Adding signed string verification function.Dec 21 2022, 5:45 AM

• max mentioned this in D5946: [services] Tunnelbroker - Expose string to be signed to the Rust.Dec 21 2022, 5:51 AM

• max edited the summary of this revision. (Show Details)Dec 21 2022, 5:57 AM

• max edited the test plan for this revision. (Show Details)

• max added a reviewer: bartek. • max added 1 blocking reviewer(s): • jon.

• max mentioned this in D5948: [services] Tunnelbroker - Remove `cryptopp` usage from the C++ codebase.Dec 21 2022, 6:01 AM

• max mentioned this in D5949: [services] Tunnelbroker - Remove `cryptopp` from dependencies.

As we have trouble using Nix in the CI gate please ignore the Nix build fail for now.

There's likely a more idiomatic rust way to do this. But it escapes me (and c++ interop makes some conventions awkward).

This revision is now accepted and ready to land.Dec 21 2022, 2:06 PM

• max added a reviewer: tomek.Dec 23 2022, 7:50 AM

This revision now requires review to proceed.Dec 23 2022, 7:50 AM

Adding @tomek for a final pass.

Rebasing on master.

Harbormaster completed remote builds in B14714: Diff 20067.Dec 23 2022, 8:09 AM

Shouldn't we use nonce name also here? D5999

services/tunnelbroker/src/server/mod.rs
64–73 ↗	(On Diff #20067)	This code can be simplified
79–92 ↗	(On Diff #20067)	We can make our match more precise
86 ↗	(On Diff #20067)	Are you sure that you don't need a comma here?

This revision now requires changes to proceed.Dec 27 2022, 3:53 AM

• max mentioned this in rCOMM3c34a2e014ba: [services] Tunnelbroker - Adding of the `base64` crate to dependencies.Dec 27 2022, 6:15 AM

Renaming a function and rebasing on parent changes.

In D5947#181356, @tomek wrote:

Shouldn't we use nonce name also here? D5999

That makes sense to make a change.
I've renamed the function to the getSavedNonceToSign (from D5946) and renamed variables. Looks better now, thanks!

services/tunnelbroker/src/server/mod.rs
79–92 ↗	(On Diff #20067)	We can make our match more precise I think it will look more complex than it is, and we are adding an empty default match here. I would prefer to leave it as is, but thanks anyway!
86 ↗	(On Diff #20067)	Are you sure that you don't need a comma here? Yes, it's removed by the rust-analyzer if we put it here.