Page MenuHomePhabricator

[Identity] Refactor opaque_ke usage to dedicated file

Authored by jon on Wed, Mar 1, 3:10 PM.
Referenced Files
Unknown Object (File)
Sat, Mar 18, 5:38 AM
Unknown Object (File)
Fri, Mar 17, 9:58 AM
Unknown Object (File)
Fri, Mar 17, 2:05 AM
Unknown Object (File)
Thu, Mar 16, 5:41 AM
Unknown Object (File)
Thu, Mar 16, 5:21 AM
Unknown Object (File)
Thu, Mar 16, 3:43 AM
Unknown Object (File)
Thu, Mar 16, 2:45 AM
Unknown Object (File)
Wed, Mar 15, 10:06 PM



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

Part of


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

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

13 ↗(On Diff #23349)

opaque_grpc might be more accurate. But pake was the term already used in the code base.

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.

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

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.Wed, Mar 1, 8:54 PM
varun requested changes to this revision.Thu, Mar 2, 7:47 PM
varun added inline comments.
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)


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

50 ↗(On Diff #23388)


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)


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.Thu, Mar 2, 7:47 PM
jon planned changes to this revision.Thu, Mar 2, 10:46 PM
jon marked an inline comment as done.
jon added inline comments.
37 ↗(On Diff #23388)

That's probably fair.

62 ↗(On Diff #23388)

Eventually I would like to move to something like 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

497 ↗(On Diff #23388)

Next week or two, I would like to do 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 there's a lot of technical debt here.

jon marked 9 inline comments as done.

Make logging and errors more consistent

23 ↗(On Diff #23388)

In, 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

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?

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.Fri, Mar 3, 2:13 PM
jon marked 2 inline comments as done.Fri, Mar 3, 2:28 PM