feat(client): minimal singular import_secret / export_secret wrappers - #253
feat(client): minimal singular import_secret / export_secret wrappers#253emostov wants to merge 3 commits into
Conversation
369da54 to
97c53d4
Compare
ccef5ca to
17182c5
Compare
54d5620 to
c84006f
Compare
…s built on the existing enclave-encrypt clients, with a runnable secrets example
c84006f to
fed5248
Compare
figitaki
left a comment
figitaki
left a comment
There was a problem hiding this comment.
Reviewed with tests run locally (secrets tests pass, clippy clean). One question about the hardcoded empty user ID, plus a few non-blocking nits inline. The tests are strong: real HPKE round-trips, per-secret one-shot target keys, and enclave-side decrypt of the import payload.
| // always empty: the flow is scoped to the organization, not to a user. | ||
| let target_data = target_data(&target)?; | ||
| let secret_payload = ImportClient::new(signer_quorum_public_key) | ||
| .encrypt_wallet_with_bundle(&plaintext, &target, &organization_id, "")?; |
There was a problem hiding this comment.
This bakes in the assumption that secrets ingress targets always carry an empty user_id. encrypt_wallet_with_bundle verifies the signed bundle's user_id against this value, so if the signer ever stamps the initiating user into these targets, every import_secret call fails with InvalidUser.
For comparison, mono's e2e test (src/go/e2e/public/secrets_test.go) passes targetData.UserId from the bundle through instead of asserting it is empty. Since target_data() already parses the bundle here, passing target_data.user_id would be forward-compatible. Keeping "" is a stricter contract assertion, which is also defensible — but it should be a deliberate choice. Is "always empty" guaranteed by the signer, or just current behavior?
There was a problem hiding this comment.
It will always be empty. The user ID is a vestige of how wallets import/export was designed but is not at all related to secret storage other then the fact it re-uses some of the primitives
|
|
||
| /// Exports secrets and returns their decrypted UTF-8 plaintexts, in the | ||
| /// order of `secret_ids`. | ||
| pub async fn export_secret( |
There was a problem hiding this comment.
Nit: import_secret is truly singular but export_secret takes a slice and returns Vec<String>. I assume the generated export_secrets occupies the plural name — fine, but the asymmetry reads oddly. Worth a doc sentence explaining why, if a rename is off the table.
(One property I checked: trusting response order is safe here, because each payload is encrypted to its own one-shot target key — a reordered response fails HPKE decryption rather than silently mis-associating plaintexts.)
There was a problem hiding this comment.
Doc comment should now reflect it is batch
| .map(|(payload, recipient)| { | ||
| // Secret payloads are enclave-signed bundles over UTF-8 bytes, | ||
| // the same shape a wallet export bundle carries. | ||
| recipient.decrypt_wallet_mnemonic_phrase(payload, &organization_id) |
There was a problem hiding this comment.
Nit: reusing decrypt_wallet_mnemonic_phrase / encrypt_wallet_with_bundle for secrets is functionally correct (both are just UTF-8 bytes) and the comments explain it, but a neutral-named alias in turnkey_enclave_encrypt (e.g. decrypt_utf8 / encrypt_utf8_with_bundle) would keep call sites honest long-term. Fine as a follow-up.
There was a problem hiding this comment.
Originally I had something like this, but was just trying to minimize diff
…ecrypt_secret wrappers and use them in the secrets flow
… and document why export_secret is singular
2a55462 to
6e30622
Compare
3 participants
Summary
Adds two client methods for single-secret import and export. Each folds the whole flow into one call; callers pass only business inputs
Tested demo locally - requires secrets enabled for org since it is still in private beta.