Page MenuHomePhabricator
Differential D6925

[Identity] Refactor opaque_ke usage to dedicated file
ClosedPublic
Actions

Authored by • jon on Mar 1 2023, 3:10 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 5, 7:32 AM
Unknown Object (File)
Tue, Oct 29, 5:51 AM
Unknown Object (File)
Sat, Oct 26, 9:10 AM
Unknown Object (File)
Mon, Oct 21, 3:53 PM
Unknown Object (File)
Mon, Oct 21, 3:52 PM
Unknown Object (File)
Fri, Oct 18, 9:47 AM
Unknown Object (File)
Oct 12 2024, 5:12 AM
Unknown Object (File)
Oct 9 2024, 4:11 AM
View All Files
Subscribers
atul
tomek

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

Related https://linear.app/comm/issue/ENG-2733

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
• jon updated this revision to Diff 23351.Mar 1 2023, 5:44 PM

Include tag in commit name

Harbormaster completed remote builds in B17052: Diff 23351.Mar 1 2023, 5:59 PM
ashoat resigned from this revision.Mar 1 2023, 8:27 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
• jon updated this revision to Diff 23388.Mar 2 2023, 7:26 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

can this function take rng: &mut (impl Rng + CryptoRng) ? i'd like the flexibility to change the rng if needed

29

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

aborted?

also same feedback as above about keeping status message vague and logging the detailed error

50

invalid_argument?

62

let's take rng as a param here

63

i don't have a preference on Vec vs slice but we should be consistent

93

invalid_argument?

services/identity/src/service.rs
497

let's keep the rng param here

511

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

That's probably fair.

62

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

I decided to just receive a slice, as it's similar to &str

services/identity/src/service.rs
497

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

probably, but I would like to tackle that in https://linear.app/comm/issue/ENG-3147. there's a lot of technical debt here.

• jon updated this revision to Diff 23426.Mar 3 2023, 12:42 PM
• jon marked 9 inline comments as done.

Make logging and errors more consistent

services/identity/src/pake_grpc.rs
23

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
• jon updated this revision to Diff 23433.Mar 3 2023, 2:11 PM

Rebase on master

varun accepted this revision.Mar 3 2023, 2:13 PM

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

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

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
• jon updated this revision to Diff 23437.Mar 3 2023, 2:22 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.

Revision Contents
Changeset List

PathSize
services/
identity/
src/
1 line
102 lines
136 lines
service/
5 lines

Diff 23388

View Options

services/identity/src/main.rs

Loading...
View Options

services/identity/src/pake_grpc.rs

Loading...
View Options

services/identity/src/service.rs

Loading...
View Options

services/identity/src/service/registration.rs

Loading...