Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add retrieve details #50

Merged
merged 6 commits into from
Aug 1, 2022
Merged

Add retrieve details #50

merged 6 commits into from
Aug 1, 2022

Conversation

indomitableSwan
Copy link
Contributor

@indomitableSwan indomitableSwan commented Jul 29, 2022

This PR closes #12.

Things it doesn't address, but came up as questions during drafting:

@marsella
Copy link
Contributor

Second bullet is a good point. In phase 0, we included a "get all key IDs" function that didn't make it into this version yet. Would "a list of the user's associated data" include anything other than key IDs?

@indomitableSwan
Copy link
Contributor Author

Second bullet is a good point. In phase 0, we included a "get all key IDs" function that didn't make it into this version yet. Would "a list of the user's associated data" include anything other than key IDs?

See #53

@indomitableSwan
Copy link
Contributor Author

There should probably be implementation guidance around the handlings of master_key added in...

Copy link
Contributor

@marsella marsella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a couple of notes about when to delete master_key from memory. Looks good!

1. Runs the [generate protocol](cryptographic_flows.md#generate-a-secret) to get a symmetric key `storage_key` for [`Enc`](#external-dependencies) of length 32 bytes.
- The `storage key` MUST NOT be saved, stored, or used in any context outside this protocol. It must not be passed to the calling application.
1. Computes a ciphertext `encrypted_storage_key = Enc(opaque_encryption_key, storage_key, user_id||"storage key")`.
1. Computes a ciphertext `encrypted_storage_key = Enc(master_key, storage_key, user_id||"storage key"|context)`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open question: Is there additional context here beyond "storage key"? We temporarily had "context" set in a previous step, but that has been removed now. The only thing I can think of is details of the encryption scheme or maybe a note that it was encrypted with the master key, but neither of those seem particularly useful to me since you have to know them to decrypt the thing anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had simplified this for now, based on your previous comments and after thinking about how we plan to use this key.

In general, though, it's much safer to view a key as both the key material and the context (which primitive, which parameters) and to be careful to enforce that the context is correct. This helps prevent downstream misuse like where you use the key in a different primitive or with different parameters, for example as part of an attack. Does this make sense? @marsella

@marsella marsella merged commit 073de1b into main Aug 1, 2022
@marsella marsella deleted the retrieve branch September 21, 2022 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write cryptographic flow for retrieving a secret
2 participants