Details

Reviewers

varun
bartek

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Commits

rCOMM8751e77ed757: [comm-opaque] Create opaque 2.0 facade for usage in Comm

Summary

This exposes our usage of opaque 2.0 in a way that
many of the details are obscured from the calling code.

This should eventually allow for significant reduction
in code re-usage in current code base, as well as
open the door for exporting the facade as a wasm api.

https://linear.app/comm/issue/ENG-3277
https://linear.app/comm/issue/ENG-3148

Depends on D7020

Test Plan

Full end-to-end testing would need to be done in another diff.

However, there is a unit test in lib.rs which goes through a register + login workflow in rust.

cd shared/comm-opaque
cargo test

Diff Detail

Repository

rCOMM Comm

Branch

jonringer/refactor-opaque (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

varun added inline comments.Mar 10 2023, 12:21 PM

shared/comm-opaque/src/constants.rs
1–5 ↗	(On Diff #23581)	i don't think this is a good idea. we should only be doing the opaque server setup once ever in production, which is why it's a separate subcommand and not part of `cargo run server`

• jon added inline comments.Mar 10 2023, 12:56 PM

shared/comm-opaque/src/config.rs
49 ↗	(On Diff #23581)	this function is used in the production path, though. imagine a scenario where we fail to load the secrets file and then start registering users with this random keypair that we're not persisting Yep, you're right, I'll change to a `#[cfg(test)]` so that we can still run the tests without requiring a file

• jon removed a parent revision: D7020: [comm-opaque] Add dependencies for client and server logic.Mar 14 2023, 9:34 AM

Expose facade as OOP

Not going tackle updating to 2.0 in this diff, as all downstream usages of comm-opaque::Cipher (which is currently the entire code base) will need to be updated as well.

Harbormaster completed remote builds in B17334: Diff 23741.Mar 14 2023, 2:55 PM

I might move this code into shared/comm-opaque2, so that we can retain the opaque 1.2 Cipher definition as it is now and avoid having to tackle the migration immediately. Once we trigger the migration, we can just replace the 1.2 code once we are ready to migrate over to opaque 2.0.

• jon edited the test plan for this revision. (Show Details)Mar 14 2023, 8:15 PM

i'm a little confused why we're moving config.rs and constants.rs into this cargo project. they seem pretty specific to the identity service. if we decide we want to store the secret key in a different location, it feels awkward that we'd have to modify a library that the identity service consumes rather than the service's own config

shared/comm-opaque/src/client/login.rs
9 ↗	(On Diff #23741)	these are all `pub`, so we shouldn't need the `#[allow(dead_code)]` attribute on any of these
12 ↗	(On Diff #23741)
19 ↗	(On Diff #23741)	can we have the constructor take a `&mut (impl Rng + CryptoRng)` param and use that for the `rng` field?
shared/comm-opaque/src/config.rs
18 ↗	(On Diff #23741)	this should probably be `pub` not `pub(super)`
29 ↗	(On Diff #23741)	why are we panicking here? can we copy what we're doing in identity's `config.rs` today and have an `Error` enum that we return here? #[derive( Debug, derive_more::Display, derive_more::From, derive_more::Error, )] pub enum Error { #[display(...)] Pake(PakeError), #[display(...)] IO(io::Error), #[display(...)] Env(env::VarError), }
shared/comm-opaque/src/server/register.rs
11 ↗	(On Diff #23741)

varun requested changes to this revision.Mar 15 2023, 12:29 PM

This revision now requires changes to proceed.Mar 15 2023, 12:29 PM

i'm a little confused why we're moving config.rs and constants.rs into this cargo project. they seem pretty specific to the identity service.

Because I expose the server side of this logic as well... And was just relocating associated logic originally.

However, I guess I could just take the server secret in as a parameter when constructing the server.

shared/comm-opaque/src/client/login.rs
9 ↗	(On Diff #23741)	I can add a crate wide `#![allow(dead_code)]`, but visibility doesn't silence the warning. https://stackoverflow.com/questions/25877285/how-to-disable-unused-code-warnings-in-rust
19 ↗	(On Diff #23741)	Hmm, I guess, but for the wasm bindings I wanted this to be the constructor, similar to `wasm-opaque`. I will look into the best way to do this, might just be able to construction your own `Login { ... }` as well.
shared/comm-opaque/src/config.rs
18 ↗	(On Diff #23741)	I will be removing it
29 ↗	(On Diff #23741)	because loading of the secrets file is critical to it working. Wanted to fail immediately, and made the error handling "cleaner" by just doing expect. However, I'll just be removing this altogether and delegating it to the service.

Use opaque 2.0, remove server setup

Remove sever_setup from constructors

• jon planned changes to this revision.Mar 16 2023, 9:16 AM

• jon marked an inline comment as done.

• jon requested review of this revision.Mar 16 2023, 9:22 AM

• jon marked 5 inline comments as done.

• jon added inline comments.

shared/comm-opaque/src/client/login.rs
12 ↗	(On Diff #23741)	Structs can't hold a reference unless you start doing some intense lifetime decoration everywhere. Furthermore, `impl Trait only allowed in function and inherent method types`. Can't have a loosely defined thing in a struct. If this is about testing, I have a test in `lib.rs`
19 ↗	(On Diff #23741)	the constructor function can take a `&mut (impl Rng + CryptoRng)`, but a struct can not store just an `impl Trait`. It needs to be a concrete type.

Harbormaster completed remote builds in B17360: Diff 23786.Mar 16 2023, 9:28 AM

Harbormaster completed remote builds in B17361: Diff 23787.

• jon marked an inline comment as done.Mar 16 2023, 9:31 AM

• jon added inline comments.

shared/comm-opaque2/src/server/register.rs
9–11	In opaque 2.0, they removed the need to track state between the `start` and `finish` call. So this could just be implemented as two method calls.

Also assert that session_key was set in test

• jon retitled this revision from [comm-opaque] Create opaque facade for usage in Comm to [comm-opaque] Create opaque 2.0 facade for usage in Comm.Mar 16 2023, 9:41 AM

• jon edited the summary of this revision. (Show Details)

Harbormaster failed remote builds in B17363: Diff 23789!Mar 16 2023, 9:44 AM

Harbormaster completed remote builds in B17363: Diff 23789.Mar 16 2023, 10:07 AM

Prune stale dependencies

Owners added a reviewer: Restricted Owners Package.Mar 16 2023, 10:30 AM

Harbormaster completed remote builds in B17364: Diff 23790.Mar 16 2023, 10:46 AM

• jon added inline comments.Mar 16 2023, 11:34 AM

nix/dev-shell.nix
66 ↗	(On Diff #23790)	oops, this wasn't meant to get into this diff

varun requested changes to this revision.Mar 16 2023, 12:41 PM

varun added inline comments.

nix/dev-shell.nix
43 ↗	(On Diff #23790)	wondering if this is potentially overkill? why not just use `wasm-bindgen` directly? seems out of scope for this diff anyway but figured i'd ask
shared/comm-opaque/src/client/login.rs
9 ↗	(On Diff #23741)	you just need to make the modules in `lib.rs` pub, and then you can remove the crate-wide `#![allow(dead_code)]` attribute
12 ↗	(On Diff #23741)	makes sense
shared/comm-opaque2/Cargo.toml
2 ↗	(On Diff #23790)	should we give this a different name? or alternatively we could just version comm-opaque, git tag it, and not create a new cargo project? i actually think the latter makes more sense... `git tag -a comm-opaque-v0.1.0 -m "comm-opaque version 0.1.0"` then dependencies would consume this library like this `comm-opaque = { git = "https://github.com/CommE2E/comm.git", tag = "comm-opaque-v0.1.0" }`
7 ↗	(On Diff #23790)	can you explain why we are building these targets? `cdylib` make sense, i'm guessing that's for `wasm` stuff. however `rlib` is used to produce statically linked executables as well as `staticlib` outputs, so not sure why we need it. also don't we need just normal `lib` here?
shared/comm-opaque2/src/client/login.rs
50 ↗	(On Diff #23790)
shared/comm-opaque2/src/server/login.rs
12 ↗	(On Diff #23790)	do we ever use this?
17 ↗	(On Diff #23790)	we don't need this if we just add a `#[derive(Default)]` attribute to the struct
53 ↗	(On Diff #23790)
shared/comm-opaque2/src/server/register.rs
9–11		awkward that they only did this for server registration. i think we should just keep the same format as the other structs for consistency
21 ↗	(On Diff #23790)

This revision now requires changes to proceed.Mar 16 2023, 12:41 PM

varun added inline comments.Mar 16 2023, 1:42 PM

shared/comm-opaque2/src/grpc.rs
5 ↗	(On Diff #23790)	this should probably be pub

• jon added a child revision: D7087: [Comm-opaque2] Add wasm build to client.Mar 16 2023, 4:30 PM

Address feedback

nix/dev-shell.nix
43 ↗	(On Diff #23790)	need to use both, moving this to later diff which targets wasm
shared/comm-opaque2/Cargo.toml
2 ↗	(On Diff #23790)	since we reference it by path currently, I don't think it makes too much of a difference. I would like to avoid tagging in case there is a use case for touching that code again, then it we will have an awkward time referencing it.
7 ↗	(On Diff #23790)	"cdylib" is meant for wasm_bindgen. `rlib` is the compiled intermediate of a crate, so we should be retain this because we will still be using this from other places in the code as a crate. https://users.rust-lang.org/t/what-is-the-difference-between-dylib-and-cdylib/28847/3
shared/comm-opaque2/src/grpc.rs
5 ↗	(On Diff #23790)	Agreed
shared/comm-opaque2/src/server/login.rs
12 ↗	(On Diff #23790)	Yea, just forgot to use `self.rng` instead of `OsRng` directly
17 ↗	(On Diff #23790)	I need it on the client side to decorate a constructor. And I would prefer to keep server and client as similar as possible. Also, I think new is a bit more clear in usage. comm-opaque::server::Login::new() # vs comm-opaque::server::Login::default()
53 ↗	(On Diff #23790)	thanks
shared/comm-opaque2/src/server/register.rs
21 ↗	(On Diff #23790)	thanks

Move wasm-pack to wasm diff

Harbormaster completed remote builds in B17371: Diff 23804.Mar 16 2023, 5:12 PM

Harbormaster completed remote builds in B17373: Diff 23806.Mar 16 2023, 5:14 PM

just one nit inline

shared/comm-opaque2/Cargo.toml
7 ↗	(On Diff #23790)	i think we should use `lib`, not `rlib` For current compiler versions there is no difference in what they produce, because lib is an alias for rlib. The difference is in what changes future compiler versions are allowed to make. The rlib type will always produce the same format as it does in the current version, but the lib type may be changed to a different format (but it will always be an alias of one of the other types). For example, maybe dynamic libraries will become the recommended style, and the alias will change to one of those. In general, use lib unless you specifically need the output to be rlib (but I don't know why you would need to do that).

This revision is now accepted and ready to land.Mar 16 2023, 5:23 PM

rlib -> lib

Harbormaster completed remote builds in B17393: Diff 23829.Mar 17 2023, 2:54 PM

• jon marked an inline comment as done.Mar 17 2023, 3:38 PM

• jon added inline comments.

shared/comm-opaque2/Cargo.toml
7 ↗	(On Diff #23790)	Sounds good to me.

Closed by commit rCOMM8751e77ed757: [comm-opaque] Create opaque 2.0 facade for usage in Comm (authored by • jon). · Explain WhyMar 17 2023, 3:41 PM

This revision was automatically updated to reflect the committed changes.

• jon marked an inline comment as done.

• jon added a commit: rCOMM8751e77ed757: [comm-opaque] Create opaque 2.0 facade for usage in Comm.

[comm-opaque] Create opaque 2.0 facade for usage in Comm
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 23787

shared/comm-opaque2/.gitignore

shared/comm-opaque2/Cargo.lock

shared/comm-opaque2/Cargo.toml

shared/comm-opaque2/src/client/login.rs

shared/comm-opaque2/src/client/mod.rs

shared/comm-opaque2/src/client/register.rs

shared/comm-opaque2/src/grpc.rs

shared/comm-opaque2/src/lib.rs

shared/comm-opaque2/src/opaque.rs

shared/comm-opaque2/src/server/login.rs

shared/comm-opaque2/src/server/mod.rs

shared/comm-opaque2/src/server/register.rs

[comm-opaque] Create opaque 2.0 facade for usage in CommClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 23787

shared/comm-opaque2/.gitignore

shared/comm-opaque2/Cargo.lock

shared/comm-opaque2/Cargo.toml

shared/comm-opaque2/src/client/login.rs

shared/comm-opaque2/src/client/mod.rs

shared/comm-opaque2/src/client/register.rs

shared/comm-opaque2/src/grpc.rs

shared/comm-opaque2/src/lib.rs

shared/comm-opaque2/src/opaque.rs

shared/comm-opaque2/src/server/login.rs

shared/comm-opaque2/src/server/mod.rs

shared/comm-opaque2/src/server/register.rs

[comm-opaque] Create opaque 2.0 facade for usage in Comm
ClosedPublic
Actions

Revision Contents
Changeset List