[identity] load OPAQUE server setup from environment variable
ClosedPublic
Actions

Authored by varun on Jul 26 2023, 2:14 PM.

Details

Reviewers

• jon
bartek

Commits

rCOMM7ba6965cc648: [identity] load OPAQUE server setup from environment variable
rCOMM8690489b1f29: [identity] load OPAQUE server setup from environment variable

Summary

The Identity service now expects the OPAQUE_SERVER_SETUP environment variable to be set at runtime.

When the keygen command is run, the serialized server setup bytes are now base64 encoded before they're written to the file system. This base64 encoded string must then be made available with the above environment variable when the server is run.

Test Plan

ran the keygen program, retrieved the base64 encoded string, ran the server program with OPAQUE_SERVER_SETUP=<encoded-string> and the program succeeded.

Also tried supplying an invalid string via env var and the server program crashed, as expected.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

varun created this revision.Jul 26 2023, 2:14 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 26 2023, 2:14 PM

varun added inline comments.Jul 26 2023, 2:18 PM

services/identity/src/main.rs
7 ↗	(On Diff #29078)	threw in this small deletion, unrelated to the rest of the diff

Harbormaster completed remote builds in B21234: Diff 29078.Jul 26 2023, 2:34 PM

varun requested review of this revision.Jul 26 2023, 2:34 PM

LGTM. Can you push a docker image that contains this change with some custom tag, so I can deploy it tomorrow?

This revision is now accepted and ready to land.Jul 26 2023, 2:55 PM

• jon requested changes to this revision.Jul 26 2023, 5:00 PM

• jon added inline comments.

services/identity/src/config.rs
69–76 ↗	(On Diff #29078)	since we are still writing the contents to a file, we should fall back to it. Also allows for the existing workflow to continue to work: cargo run -- keygen cargo run -- server

This revision now requires changes to proceed.Jul 26 2023, 5:00 PM

• jon added inline comments.Jul 26 2023, 5:11 PM

services/identity/src/config.rs
71 ↗	(On Diff #29078)	Also might be nice to have some insight what is going on:

• jon added inline comments.Jul 26 2023, 5:13 PM

services/identity/src/config.rs
71 ↗	(On Diff #29078)	`ErrorKind::NotFound` was what I meant to put there

bartek added inline comments.Jul 27 2023, 1:24 AM

services/identity/src/config.rs
69–76 ↗	(On Diff #29078)	Yeah, good idea, this makes sense

bartek added inline comments.Jul 27 2023, 2:20 AM

services/identity/src/config.rs
69–76 ↗	(On Diff #29078)	But rather do `.or_else(\|_\| fs::read_to_string(path))?`

This revision was not accepted when it landed; it landed in state Needs Revision.Jul 27 2023, 6:28 AM

Closed by commit rCOMM8690489b1f29: [identity] load OPAQUE server setup from environment variable (authored by varun). · Explain Why

This revision was automatically updated to reflect the committed changes.

varun added a commit: rCOMM8690489b1f29: [identity] load OPAQUE server setup from environment variable.

varun added a reverting change: rCOMMf04146c374ed: Revert "[identity] load OPAQUE server setup from environment variable".

varun reopened this revision.Jul 27 2023, 6:32 AM

varun planned changes to this revision.Jul 27 2023, 7:50 AM

address feedback

varun added inline comments.Jul 27 2023, 1:15 PM

services/identity/src/config.rs
76 ↗	(On Diff #29144)	Removed the generic because `PathBuf` doesn't implement `Display` and it's more important IMO that we are able to log the path
87 ↗	(On Diff #29144)	Should actually be this. Will fix before landing