General ergonomics

[dantee-e]

This crate is very complicated to use, documentation is so basic that any complex functions are virtually unusable.
No support for serialization, and I can't figure out the most basic of things like verifying the signature of an encrypted message. This here does not work, and I don't understand why the EncryptedMessage would not be able to be used as the message ibn the verify function (it does not implement Hashable, and neither does &[u8;384]. Honest question, how was I supposed to use this?)

pub fn verify_payload_signature(
    public_key: &PublicSigningKey,
    signature: &Ed25519Signature,
    payload: EncryptedMessage,
) -> bool {
    public_key.verify(&payload.bytes(), signature)
}

Another thing that has been really annoying is declaring EncryptedOnceValue as a enum variant of EncryptedValue. That makes it so that I can't do anything with the EncryptedValue enum unless I "if let" the contents out from it:

if let EncryptedValue::EncryptedOnceValue {
        ephemeral_public_key,
        encrypted_message,
        auth_hash,
        public_signing_key,
        signature,
    } = encrypted_value
    {
}

This makes it look terrible and everything becomes a big hassle. Serializing this for storage or to send it somewhere else is so inefficient. I had to create a helper struct just to do this:

use recrypt::api::*;
use serde::{Deserialize, Serialize};

use crate::error::ServerError;

#[derive(Serialize, Deserialize)]
pub struct EncryptedValueBytes {
    pub ephemeral_public_key: Vec<u8>,
    pub encrypted_message: Vec<u8>,
    pub auth_hash: Vec<u8>,
    pub public_signing_key: Vec<u8>,
    pub signature: Vec<u8>,
}

impl From<EncryptedValue> for EncryptedValueBytes {
    fn from(value: EncryptedValue) -> Self {
        let EncryptedValue::EncryptedOnceValue {
            ephemeral_public_key,
            encrypted_message,
            auth_hash,
            public_signing_key,
            signature,
        } = value
        else {
            panic!("Expected EncryptedOnceValue");
        };
        Self {
            ephemeral_public_key: ephemeral_public_key.to_bytes().to_vec(),
            encrypted_message: encrypted_message.bytes().to_vec(),
            auth_hash: auth_hash.bytes().to_vec(),
            public_signing_key: public_signing_key.bytes().to_vec(),
            signature: signature.bytes().to_vec(),
        }
    }
}

impl EncryptedValueBytes {
    pub fn to_encrypted_value(&self) -> Result<EncryptedValue, RecryptErr> {
        Ok(EncryptedValue::EncryptedOnceValue {
            ephemeral_public_key: PublicKey::new_from_slice((
                &self.ephemeral_public_key[0..32],
                &self.ephemeral_public_key[32..64],
            ))?,
            encrypted_message: EncryptedMessage::new_from_slice(
                &self.encrypted_message,
            )?,
            auth_hash: AuthHash::new_from_slice(&self.auth_hash)?,
            public_signing_key: PublicSigningKey::new_from_slice(
                &self.public_signing_key,
            )?,
            signature: Ed25519Signature::new_from_slice(&self.signature)?,
        })
    }

    pub fn to_sql(&self) -> Result<Vec<u8>, ServerError> {
        Ok(serde_json::to_vec(self)?)
    }
}

There should also be more integration with the ed25519_dalek crate, which is already one of your dependencies. There are no utility functions to convert between them, and the only way I found of constructing a SingingKeypair from my already existent keypair was this big mess here:

use std::io::Read;

use ed25519_dalek::{SigningKey, VerifyingKey};
use recrypt::api::{PrivateKey, PublicKey, SigningKeypair};
use serde::{Deserialize, Serialize};

#[derive(Serialize, Deserialize)]
pub struct KeysRaw {
    pub private_key_encoding: [u8; 32],
    pub public_key_encoding: Vec<u8>, // serde does not support [u8;64] natively, and I don't want a
    // new crate just for this
    pub signing_key: [u8; 32],
    pub verifying_key: [u8; 32],
}

enum EcryptedV {
    EncryptedOV {
        auth_hash: String,
        signature: String,
    },
}

pub struct Keys {
    pub private_key_encoding: PrivateKey,
    pub public_key_encoding: PublicKey,
    pub signing_key: SigningKey,
    pub verifying_key: VerifyingKey,
    pub signing_key_pair: SigningKeypair,
}

impl From<KeysRaw> for Keys {
    fn from(value: KeysRaw) -> Self {
        let pub_1 = &value.public_key_encoding[0..32];
        let pub_2 = &value.public_key_encoding[32..64];

        let mut signing_keypair_bytes = value.signing_key.to_vec();
        signing_keypair_bytes.append(&mut value.verifying_key.to_vec());
        Keys {
            private_key_encoding: PrivateKey::new_from_slice(&value.private_key_encoding).unwrap(),
            public_key_encoding: PublicKey::new_from_slice((pub_1, pub_2)).unwrap(),
            signing_key: SigningKey::from_bytes(&value.signing_key),
            verifying_key: VerifyingKey::from_bytes(&value.verifying_key).unwrap(),
            signing_key_pair: SigningKeypair::from_bytes(
                &signing_keypair_bytes.try_into().unwrap(),
            )
            .unwrap(),
        }
    }
}

I don't want to just complain, this is a good project, and it's really nice that you had it audited so that we can trust the soundness of the cryptography, but in it's current state, I think the crate is barely usable for anyone without internal access to the development.
What I think would really benefit the project:

• More documentation
• Serialization support would be greatly appreciated
• Separating the EncryptedOnceValue and the other one into their own separate structs that implement the type EncryptedValue
• Separating the functions of singing the encryption from the encryption itself. (Maybe I'm talking bullshit, I don't know enough about cryptography to argue about that, but I already had my signing keys from the ed25519_dalek crate, and I was already going to sign them independently, but instead I had to reconstruct them to fit the internal ed25519 keypair), and the simplest way to convert between them was the code shown above)

[coltfred]

Thanks for taking the time to write this up, this is genuinely useful feedback. You're right that the ergonomics could be better in several areas. I'd like to address each of your problem areas and propose things that would be good additions to the library.

Signature verification

The signature on an EncryptedValue covers the entire encrypted payload (ephemeral public key + encrypted message + auth hash), not just the EncryptedMessage field alone. This is why
EncryptedMessage doesn't implement Hashable on its own, verifying a signature over just that one field would always fail.

To verify the signature embedded in an EncryptedValue, you'd need to reconstruct the same bytes that were signed. Today there isn't a convenient public helper for this. We'd welcome a PR that either:

• Documents this more clearly
• Adds a convenience method on EncryptedValue to verify its own signature

Enum ergonomics for EncryptedValue

Agreed this is painful. The enum design reflects that EncryptedOnceValue and TransformedValue have different fields (transform_blocks only exists on the latter for instance), but that the encrypted values can be transformed more than once. Accessor methods for the shared fields (ephemeral_public_key, encrypted_message, auth_hash, public_signing_key, signature) would be a good addition.

Serialization

We intentionally kept serde out of the core crate to minimize the dependency surface for a crypto library, but every type exposes bytes() / new_from_slice() for round-tripping. Some options I have in my mind:

• An optional serde feature flag that derives Serialize/Deserialize on the public types
• Better documentation of the serialization pattern (the doc example only covers keys, not EncryptedValue)

Ed25519 / signing keypair interop

SigningKeypair::from_bytes(&[u8; 64]) accepts exactly the ed25519-dalek format. It's 32 bytes of secret key followed by 32 bytes of compressed public key. So from an existing
ed25519_dalek::SigningKey, you can do:

let signing_keypair = SigningKeypair::from_bytes(&dalek_signing_key.to_keypair_bytes()).unwrap();

This should be documented.

Signing coupled with encryption

The signing is intentionally coupled with encryption. The signature proves that the encryptor authorized the ciphertext, which is a security property of the scheme (it was part of the audit scope). Allowing unsigned ciphertexts would weaken the guarantees.

Note that the above solutions are just what's bouncing around in my head. If you have other thoughts on solving them or other questions, let me know. The library design represented here was an attempt by me to make things well typed, generic and obviously correct. It represents my understanding of Rust as of a decade ago and is working for us and as such doesn't get improvements to the API surface or documentation. We would welcome these improvements!

[dantee-e]

Thanks a lot for the extensive answer! I apologize for taking so long to reply, I wanted to properly sit down and read with the attention you dispended on writing it.

About the signature verification: I understand, that makes sense. I think the serialization would be a great help here.

Enum ergonomics for EncryptedValue: Yes, accessor methods would be a great solution, and the transform_blocks accessor could be a "try" function returning an Option enum.

Serialization: The optional feature is a good idea, and as you said documentation could be a solution on its own.

Ed25519 / signing keypair interop: That's my bad. I didn't know about the to_keypair_bytes() function, thanks for letting me know. Although I still feel a SigningKeypair::from_signing_key() function would not hurt, even if it may be over the scope of the crate by having too direct of a coupling with another crate.

Signing coupled with encryption: If you have the time, could you please elaborate on how unsigned texts could weaken the guarantees? Or maybe direct me to a resource that explains it.

Again, I really appreciate you taking the time to answer and propose solutions. I'll try providing some help in the future on implementing these changes you proposed when I have some time.