Page MenuHomePhabricator
Differential D7022

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

Authored by jon on Thu, Mar 9, 10:10 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 16, 9:37 AM
Unknown Object (File)
Thu, Mar 16, 9:37 AM
Unknown Object (File)
Sat, Mar 11, 7:53 AM
Unknown Object (File)
Sat, Mar 11, 7:53 AM
Unknown Object (File)
Sat, Mar 11, 7:53 AM
Unknown Object (File)
Fri, Mar 10, 10:24 PM
Subscribers
ashoat
atul
tomek

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
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.Fri, Mar 10, 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.Fri, Mar 10, 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.Tue, Mar 14, 9:34 AM
jon updated this revision to Diff 23741.Tue, Mar 14, 2:40 PM
jon added a parent revision: D7020: [comm-opaque] Add dependencies for client and server logic.

Expose facade as OOP

jon marked 2 inline comments as done.Tue, Mar 14, 2:46 PM

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.Tue, Mar 14, 2:55 PM
jon added a comment.Tue, Mar 14, 8:12 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)Tue, Mar 14, 8:15 PM
varun added a comment.Wed, Mar 15, 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. 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.Wed, Mar 15, 12:29 PM
This revision now requires changes to proceed.Wed, Mar 15, 12:29 PM
jon planned changes to this revision.Wed, Mar 15, 9:04 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.

jon updated this revision to Diff 23786.Thu, Mar 16, 9:12 AM

Use opaque 2.0, remove server setup

jon updated this revision to Diff 23787.Thu, Mar 16, 9:15 AM

Remove sever_setup from constructors

jon planned changes to this revision.Thu, Mar 16, 9:16 AM
jon marked an inline comment as done.
jon requested review of this revision.Thu, Mar 16, 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.Thu, Mar 16, 9:28 AM
Harbormaster completed remote builds in B17361: Diff 23787.
jon marked an inline comment as done.Thu, Mar 16, 9:31 AM
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.

jon updated this revision to Diff 23789.Thu, Mar 16, 9:37 AM

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.Thu, Mar 16, 9:41 AM
jon edited the summary of this revision. (Show Details)
Harbormaster failed remote builds in B17363: Diff 23789!Thu, Mar 16, 9:44 AM
Harbormaster completed remote builds in B17363: Diff 23789.Thu, Mar 16, 10:07 AM
jon updated this revision to Diff 23790.Thu, Mar 16, 10:30 AM

Prune stale dependencies

Owners added a reviewer: Restricted Owners Package.Thu, Mar 16, 10:30 AM
Harbormaster completed remote builds in B17364: Diff 23790.Thu, Mar 16, 10:46 AM
jon added inline comments.Thu, Mar 16, 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.Thu, Mar 16, 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.Thu, Mar 16, 12:41 PM
varun added inline comments.Thu, Mar 16, 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.Thu, Mar 16, 4:30 PM
jon updated this revision to Diff 23804.Thu, Mar 16, 4:56 PM
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

jon updated this revision to Diff 23806.Thu, Mar 16, 4:59 PM

Move wasm-pack to wasm diff

Harbormaster completed remote builds in B17371: Diff 23804.Thu, Mar 16, 5:12 PM
Harbormaster completed remote builds in B17373: Diff 23806.Thu, Mar 16, 5:14 PM
varun accepted this revision.Thu, Mar 16, 5:23 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.Thu, Mar 16, 5:23 PM
jon updated this revision to Diff 23829.Fri, Mar 17, 2:38 PM

rlib -> lib

Harbormaster completed remote builds in B17393: Diff 23829.Fri, Mar 17, 2:54 PM
jon marked an inline comment as done.Fri, Mar 17, 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 WhyFri, Mar 17, 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.

Revision Contents
Changeset List

PathSize
shared/
comm-opaque2/
1 line
18 lines
src/
client/
60 lines
5 lines
52 lines
28 lines
48 lines
11 lines
server/
64 lines
5 lines
40 lines

Diff 23786

View Options

shared/comm-opaque2/.gitignore

Loading...
View Options

shared/comm-opaque2/Cargo.lock

Loading...
View Options

shared/comm-opaque2/Cargo.toml

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

shared/comm-opaque2/src/grpc.rs

Loading...
View Options

shared/comm-opaque2/src/lib.rs

Loading...
View Options

shared/comm-opaque2/src/opaque.rs

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...