Page MenuHomePhabricator

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

Authored by jon on Mar 9 2023, 10:10 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Feb 26, 4:58 PM
Unknown Object (File)
Fri, Feb 23, 10:22 PM
Unknown Object (File)
Fri, Feb 23, 11:01 AM
Unknown Object (File)
Fri, Feb 23, 10:59 AM
Unknown Object (File)
Fri, Feb 23, 10:31 AM
Unknown Object (File)
Fri, Feb 23, 10:13 AM
Unknown Object (File)
Fri, Feb 23, 10:06 AM
Unknown Object (File)
Fri, Feb 23, 10:06 AM
Subscribers

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

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

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

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

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.

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.

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 marked an inline comment as done.
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.

jon added inline comments.
shared/comm-opaque2/src/server/register.rs
9–11 ↗(On Diff #23787)

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)
Owners added a reviewer: Restricted Owners Package.Mar 16 2023, 10:30 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 ↗(On Diff #23787)

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
shared/comm-opaque2/src/grpc.rs
5 ↗(On Diff #23790)

this should probably be pub

jon marked 10 inline comments as done.

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

This revision is now accepted and ready to land.Mar 16 2023, 5:23 PM
jon added inline comments.
shared/comm-opaque2/Cargo.toml
7 ↗(On Diff #23790)

Sounds good to me.

This revision was automatically updated to reflect the committed changes.
jon marked an inline comment as done.