Page MenuHomePhabricator

[Identity] Support keygen and server setup for opaque 2.0
ClosedPublic

Authored by jon on Apr 4 2023, 10:17 AM.
Tags
None
Referenced Files
F3566434: D7299.diff
Fri, Dec 27, 7:16 PM
Unknown Object (File)
Sun, Dec 15, 7:08 PM
Unknown Object (File)
Sun, Dec 15, 7:08 PM
Unknown Object (File)
Sun, Dec 15, 7:08 PM
Unknown Object (File)
Sun, Dec 15, 7:08 PM
Unknown Object (File)
Sun, Dec 15, 7:08 PM
Unknown Object (File)
Sun, Dec 15, 7:02 PM
Unknown Object (File)
Thu, Dec 5, 8:43 PM
Subscribers

Details

Summary

Opaque 2.0 requires different setup for it to run.
This adds support generating the secrets and adding the
ServerSetup object to the configuration.

Test Plan
cd services/identity
cargo run -- keygen
cargo run -- keygen # should skip writting files
cargo run -- server

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

varun requested changes to this revision.Apr 5 2023, 11:48 AM
varun added inline comments.
services/identity/src/config.rs
4 ↗(On Diff #24600)

should we just remove path::Path here and inline path::Path where we currently write Path?

40 ↗(On Diff #24600)

should we do this above on line 31 as well instead of calling env::current_dir? figure we should be consistent

64 ↗(On Diff #24600)

why are we removing derive_more::Error?

88 ↗(On Diff #24600)
services/identity/src/constants.rs
6 ↗(On Diff #24600)

i know that PathBuf will infer that txt is the extension but i think it's better to be explicit like we are above. what do you think?

services/identity/src/keygen.rs
13 ↗(On Diff #24600)

same q as above, can we use PathBuf::new here?

30 ↗(On Diff #24600)

i think we can call path.pop() instead of shadowing path with a new value here

32–35 ↗(On Diff #24600)

haven't we already checked this above on line 15?

37–42 ↗(On Diff #24600)

we should invert the if condition so we can return early if the path already exists

This revision now requires changes to proceed.Apr 5 2023, 11:48 AM
jon marked 9 inline comments as done.

Address feedback

services/identity/src/config.rs
4 ↗(On Diff #24600)

sure

40 ↗(On Diff #24600)

we shouldn't call current_dir, as the directory could be an absolute path.

64 ↗(On Diff #24600)
services/identity/src/constants.rs
6 ↗(On Diff #24600)

I've never seen a service configure an extension, they aren't required in unix.

services/identity/src/keygen.rs
13 ↗(On Diff #24600)

yea

30 ↗(On Diff #24600)

If the secrets directory is something like foo/bar, then pop() will just go to the parent.

32–35 ↗(On Diff #24600)

Since the original code assumes a relative path, these could be different if the path were changed to an absolute value.

We should probably just remove the env_dir logic from the existing code.

varun requested changes to this revision.Apr 7 2023, 10:52 AM
varun added inline comments.
services/identity/src/keygen.rs
13 ↗(On Diff #24600)

cool, can you make that change?

30 ↗(On Diff #24600)

makes sense

32–35 ↗(On Diff #24600)

cool, can we just switch to PathBuf::new above and then avoid checking this twice?

This revision now requires changes to proceed.Apr 7 2023, 10:52 AM
jon marked 3 inline comments as done.

Address feedback: move secrets dir creation to it's own block

services/identity/src/keygen.rs
13 ↗(On Diff #24600)

weird, thought I pushed the change.

32–35 ↗(On Diff #24600)

sure

This revision is now accepted and ready to land.Apr 11 2023, 7:24 AM