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

Associated data API for encryption is not very ergonomic #4742

Closed
tiziano88 opened this issue Feb 1, 2024 · 2 comments
Closed

Associated data API for encryption is not very ergonomic #4742

tiziano88 opened this issue Feb 1, 2024 · 2 comments

Comments

@tiziano88
Copy link
Collaborator

/// Decrypts a [`EncryptedResponse`] proto message using AEAD.
/// Returns a response message plaintext and associated data.
/// <https://datatracker.ietf.org/doc/html/rfc5116>
pub fn decrypt(
&self,
encrypted_response: &EncryptedResponse,
) -> anyhow::Result<(Vec<u8>, Vec<u8>)> {

Currently, additional authenticated data is passed in by the caller when encrypting, then stored alongside the encrypted message, and then returned to the caller when decrypting. This is not how it is commonly done in most other crypto libraries (e.g. https://docs.rs/ring/latest/ring/aead/struct.OpeningKey.html ); instead, the associated data is explicitly passed in when decrypting, since it usually comes from somewhere else entirely (e.g. a user id from an auth token, or a digest of some other piece of data known to the caller). In my experience, it is highly unusal for the additional data to be an actual self-contained byte string that needs to be stored alongside the ciphertext.

My suggestion is to:

  1. change the Rust API to require the caller to pass in the additional data when decrypting
  2. remove the associated_data field from the proto entirely (no need to shift proto field numbers around, we can just mark as reserved)

I don't believe anyone is actively using it anyways (but let me know if you know of any use case).

@ipetr0v @jul-sh @fernandolobato @conradgrobler WDYT?

@ipetr0v
Copy link
Contributor

ipetr0v commented Feb 2, 2024

As per Rust SDK we were planning to return only the decrypted message from the decrypt function (i.e. a vec). And since the EncryptedMessage already contains an associated data, we won't need to pass it separately to the decrypt function.

Though in order to extract the associated data, prod teams would need to parse the Proto message.

The plan was also to use https://docs.rs/aead/latest/aead/struct.Payload.html for encryption.

@tiziano88
Copy link
Collaborator Author

My (unverified) assumption is that clients would not normally expect the AAD to be stored alongside the ciphertext -- perhaps it would come from somewhere else entirely (like an auth token or some other part of the message). So they would not have to parse the protobuf message either.

Using Payload for encryption still makes sense to me (in fact I suggest doing so both for encryption and decryption). The question is whether to store the AAD in the output proto.

@tiziano88 tiziano88 closed this as not planned Won't fix, can't repro, duplicate, stale Jul 31, 2024
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

No branches or pull requests

2 participants