API Gap: KeyWrapper, KeyUwrapper

[dghgit]

These are also on the OpenSSL list with the driving specifications being SP 800-38F "Recommendation for Block Cipher Modes of Operation: Methods for Key Wrapping" and RFC 5649.

Primarily the algorithms described are for AES and TDES, however Aria and Camellia (and I think SM4, not 100% sure....) support the same algorithms.

The algorithms require the full material being wrapped to be passed in.

Typing all this into Opus resulted in a suggestion looking like this:

pub trait KeyWrapper<const KEK_LEN: usize>: Algorithm {
    /// Half the underlying block cipher's block size, in bytes -- "semiblock", Sec. 4.1.
    /// 8 for KW and KWP (128-bit block); 4 for TKW (TDEA, 64-bit block).
    const SEMIBLOCK_LEN: usize;

    /// Wraps `key` under `kek`. `CT_LEN` is determined by `KEY_LEN` and checked at compile time.
    ///
    /// # Errors
    /// Rejects a KEK whose [`KeyType`] is not [`KeyType::SymmetricCipherKey`], and one whose
    /// security strength is below that of the key being wrapped (Appendix A.2), both as
    /// [`KeyWrapError::KeyMaterialError`].
    fn wrap_key<const KEY_LEN: usize, const CT_LEN: usize>(
        kek: &KeyMaterial<KEK_LEN>,
        key: &KeyMaterial<KEY_LEN>,
    ) -> Result<[u8; CT_LEN], KeyWrapError>;

    /// As [`wrap_key`](Self::wrap_key), writing into a caller-provided buffer. Returns the number
    /// of bytes written, which is always `CT_LEN`.
    fn wrap_key_out<const KEY_LEN: usize, const CT_LEN: usize>(
        kek: &KeyMaterial<KEK_LEN>,
        key: &KeyMaterial<KEY_LEN>,
        ciphertext: &mut [u8; CT_LEN],
    ) -> Result<usize, KeyWrapError> {
        *ciphertext = Self::wrap_key(kek, key)?;
        Ok(CT_LEN)
    }
}

With an equivalent trait for KeyUnwrapper. I note KeyWrapErrror is also new.

Also of note is that these algorithms (especially the KWP (padded) format), get used for things other than symmetric and private keys. So while the suggestion above shows KeyMaterial going in (mostly correct) input as [u8] might be better.

[dghgit]

Actually, I just realise the answer, it should be:

pub trait KeyWrapper<const KEK_LEN: usize>: Algorithm {
    /// Half the underlying block cipher's block size, in bytes -- "semiblock", Sec. 4.1.
    /// 8 for KW and KWP (128-bit block); 4 for TKW (TDEA, 64-bit block).
    const SEMIBLOCK_LEN: usize;

    /// Wraps `key` under `kek`. `CT_LEN` is determined by `KEY_LEN` and checked at compile time.
    ///
    /// # Errors
    /// Rejects a KEK whose [`KeyType`] is not [`KeyType::SymmetricCipherKey`], and one whose
    /// security strength is below that of the key being wrapped (Appendix A.2), both as
    /// [`KeyWrapError::KeyMaterialError`].
    fn wrap_key<const KEY_LEN: usize, const CT_LEN: usize>(
        kek: &KeyMaterial<KEK_LEN>,
        key: &[u8; KEY_LEN],
    ) -> Result<[u8; CT_LEN], KeyWrapError>;

    /// As [`wrap_key`](Self::wrap_key), writing into a caller-provided buffer. Returns the number
    /// of bytes written, which is always `CT_LEN`.
    fn wrap_key_out<const KEY_LEN: usize, const CT_LEN: usize>(
        kek: &KeyMaterial<KEK_LEN>,
        key: &[u8; KEY_LEN],
        ciphertext: &mut [u8; CT_LEN],
    ) -> Result<usize, KeyWrapError> {
        *ciphertext = Self::wrap_key(kek, key)?;
        Ok(CT_LEN)
    }
}

We should be forcing people to wrap and unwrap byte arrays - the act of wrapping removes the additional information that makes KeyMaterial what it is, likewise, unwrapping says nothing about the provenance of the data - the user needs to explicitly make that decision.

[ounsworth]

I don't have big comments on this, mainly because I have only passing familiarity with key wraps.

But a couple of thoughts:

1. We should convince ourselves that compile-time constants for KEK_LEN, KEY_LEN and CT_LEN are appropriate. Are you sure that those are things that will always be known at compile time?
2. I understand why it doesn't make sense for key to be KeyMaterial<>, but could it at least be Secret<> (defined in the bouncycastle-utils` crate)? That way it still benefits from the forced zeroize-on-drop and stack trace logging protections.
3. The pattern:

    fn wrap_key_out(..) -> Result<usize, KeyWrapError> {
        *ciphertext = Self::wrap_key(kek, key)?;
        Ok(CT_LEN)
    }

I suspect that this will end up compiling such that Self::wrap_key() allocates a buffer, and then this memcpy's the contents into ciphertext, which is (to a small degree) wasteful on both memory usage and performance.

This is also going to blow up when Jason comes along to apply #[cfg(alloc)] feature-guards all over the place and turns off wrap_key() in a no-alloc build, leaving only wrap_key_out().

The established pattern is to do the function chaining the other way where _out() carries the implementation, and the other one is a convenience wrapper:

  fn thing() -> [u8; LEN] {
    let mut out = [0u8; LEN];
    let bytes_written = thing_out(&mut out)?;
    debug_assert_eq!(LEN, bytes_written);
    Ok(out)
  }

Claude should be able to both check the assembly to see if your method is wasteful, and it should be able to check for the existing pattern of _out() functions within existing crates.

---

As a meta-point: whether thing() wraps thing_out() or the other way round should be a pretty well-established pattern through the existing code. An LLM is a pattern-matching engine, so I'm rather surprised that it invented a new pattern instead of using the existing one. This is reinforcing my belief that for parts of the library where consistency and performance matter, we need to be reviewing AI-generated code quite carefully.