Details

Reviewers

varun
bartek
• max
ashoat

Commits

rCOMM592b0649dfb9: [Identity] Refactor opaque_ke usage to dedicated file

Summary

ServerRegistration operations need to be used by both register_user
and update_user. ServerLogin operations need to be used by register_user,
update_user, and login_user. May as well refactor this shared logic to
provide similar terminology in potential errors, and improve readability.

These map the potential opaque protocol errors to a corresponding gRPC status
error.

Part of https://linear.app/comm/issue/ENG-2763

Test Plan

This is a refactor, and shouldn't affect the observable behavior of the
identity service. However, it will be fully tested in a later diff.

cd services/identity
cargo build

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• jon created this revision.Mar 1 2023, 3:10 PM

Herald added subscribers: atul, tomek. · View Herald TranscriptMar 1 2023, 3:10 PM

• jon added inline comments.Mar 1 2023, 3:23 PM

services/identity/src/main.rs
13 ↗	(On Diff #23349)	`opaque_grpc` might be more accurate. But `pake` was the term already used in the code base.
services/identity/src/service.rs
427 ↗	(On Diff #23349)	`Vec<u8>` is what protobuf gives us, `Vec` just has a `AsRef<slice>` implementation, so it can implicitly transform when passed to a function. Don't feel strongly about this change, I can return it to `&[u8]`, similar to how `&str` is preferred when applicable.

Harbormaster completed remote builds in B17050: Diff 23349.Mar 1 2023, 3:25 PM

• jon requested review of this revision.Mar 1 2023, 3:25 PM

Include tag in commit name

Harbormaster completed remote builds in B17052: Diff 23351.Mar 1 2023, 5:59 PM

I don't know Rust :(

• jon retitled this revision from Refactor opaque_ke usage to dedicated file to [Identity] Refactor opaque_ke usage to dedicated file.Mar 1 2023, 8:54 PM

• jon edited the summary of this revision. (Show Details)Mar 1 2023, 8:57 PM

• jon added a child revision: D6939: [Identity] Implment update_user endpoint on server.Mar 2 2023, 2:10 PM

Rebase on master

Harbormaster completed remote builds in B17083: Diff 23388.Mar 2 2023, 7:42 PM

varun requested changes to this revision.Mar 2 2023, 7:47 PM

varun added inline comments.

services/identity/src/pake_grpc.rs
23 ↗	(On Diff #23388)	can this function take `rng: &mut (impl Rng + CryptoRng)` ? i'd like the flexibility to change the rng if needed
29 ↗	(On Diff #23388)	we should keep the status message more opaque like we had before and just use tracing::error to log errors in detail. this feedback is applicable in other places in this diff
37 ↗	(On Diff #23388)	aborted? also same feedback as above about keeping status message vague and logging the detailed error
50 ↗	(On Diff #23388)	invalid_argument?
62 ↗	(On Diff #23388)	let's take rng as a param here
63 ↗	(On Diff #23388)	i don't have a preference on Vec vs slice but we should be consistent
93 ↗	(On Diff #23388)	invalid_argument?
services/identity/src/service.rs
497 ↗	(On Diff #23388)	let's keep the rng param here
511 ↗	(On Diff #23388)	should this be renamed too for consistency? how about the other function names?

This revision now requires changes to proceed.Mar 2 2023, 7:47 PM

• jon planned changes to this revision.Mar 2 2023, 10:46 PM

• jon marked an inline comment as done.

• jon added inline comments.

services/identity/src/pake_grpc.rs
37 ↗	(On Diff #23388)	That's probably fair.
62 ↗	(On Diff #23388)	Eventually I would like to move to something like https://linear.app/comm/issue/ENG-3147. And just have the RNG managed similar to Cipher, that can be shared across both client and server
63 ↗	(On Diff #23388)	I decided to just receive a slice, as it's similar to `&str`
services/identity/src/service.rs
497 ↗	(On Diff #23388)	Next week or two, I would like to do https://linear.app/comm/issue/ENG-3147. This was more or less to reduce the noise around doing PAKE related operations
511 ↗	(On Diff #23388)	probably, but I would like to tackle that in https://linear.app/comm/issue/ENG-3147. there's a lot of technical debt here.

Make logging and errors more consistent

services/identity/src/pake_grpc.rs
23 ↗	(On Diff #23388)	In https://linear.app/comm/issue/ENG-3147, I was hoping to refactor this code completely. There's not a good reason for grpc endpoints to know about what Rng implementation the oqaque protocol is using.

Harbormaster completed remote builds in B17103: Diff 23426.Mar 3 2023, 12:57 PM

• jon added a parent revision: D6912: [Protos] Add UpdateUser definitions.Mar 3 2023, 1:00 PM

Rebase on master

I'm good with backlogging my suggestions, just want to make sure we call them all out specifically in the linear task so we don't lose track of small things

See comments inline

services/identity/src/pake_grpc.rs
63 ↗	(On Diff #23388)	I meant, in `server_login_finish` and other functions we pass a `Vec<u8>`, but here we're passing a `[u8]`. Let's stick with one or the other. Can you call this out specifically in ENG-3147?
services/identity/src/service.rs
511 ↗	(On Diff #23388)	Can you call this out specifically in ENG-3147 so we don't forget about it?

This revision is now accepted and ready to land.Mar 3 2023, 2:13 PM

Restore rng parameter

Harbormaster completed remote builds in B17107: Diff 23433.Mar 3 2023, 2:26 PM

• jon marked 2 inline comments as done.Mar 3 2023, 2:28 PM

Harbormaster completed remote builds in B17111: Diff 23437.Mar 3 2023, 2:38 PM

Closed by commit rCOMM592b0649dfb9: [Identity] Refactor opaque_ke usage to dedicated file (authored by • jon). · Explain WhyMar 3 2023, 4:24 PM

This revision was automatically updated to reflect the committed changes.

• jon added a commit: rCOMM592b0649dfb9: [Identity] Refactor opaque_ke usage to dedicated file.

[Identity] Refactor opaque_ke usage to dedicated file
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 23351

services/identity/src/main.rs

services/identity/src/pake_grpc.rs

services/identity/src/service.rs

services/identity/src/service/registration.rs

[Identity] Refactor opaque_ke usage to dedicated fileClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 23351

services/identity/src/main.rs

services/identity/src/pake_grpc.rs

services/identity/src/service.rs

services/identity/src/service/registration.rs

[Identity] Refactor opaque_ke usage to dedicated file
ClosedPublic
Actions

Revision Contents
Changeset List