Page MenuHomePhabricator

[Identity] Refactor opaque_ke usage to dedicated file
ClosedPublic

Authored by jon on Mar 1 2023, 3:10 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Apr 2, 3:50 AM
Unknown Object (File)
Thu, Mar 28, 7:25 PM
Unknown Object (File)
Sat, Mar 16, 6:14 PM
Unknown Object (File)
Feb 27 2024, 11:49 PM
Unknown Object (File)
Feb 27 2024, 11:49 PM
Unknown Object (File)
Feb 27 2024, 11:49 PM
Unknown Object (File)
Feb 27 2024, 11:49 PM
Unknown Object (File)
Feb 27 2024, 11:49 PM
Subscribers

Details

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

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.

Include tag in commit name

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
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 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 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.

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