aead v0.6.0 release tracking issue

[tarcieri]

This is an issue for tracking the next stable release of the aead crate, v0.6.0

[newpavlov]

One open question is whether we want to support nonce sizes variable at runtime. Some algorithms (e.g. GCM) support variable nonce sizes and currently we expose it at compile time using generic parameters. We could add methods which accept nonces as byte slices, but it's unclear whether it's worth to do.

[kkoyung]

One possible downstream benefit of runtime-sized nonces is standards compatibility for browser engines.

Servo uses the aes-gcm crate for its WebCrypto AES-GCM implementation, and the WebCrypto specification allows callers to provide nonces of varying lengths. Under the current aead trait design, Servo only supports a few fixed, commonly used nonce lengths.

Disclaimer: I'm one of the maintainers of Servo. Just wanted to share this as a downstream use case.

[rozbb]

Sounds like a valuable feature, though maybe hazmat-gated since short nonces can easily cause a catastrophic failure

[tarcieri]

It seems like there are more or less two ways to implement dynamically-sized nonces: a generic parameter that can abstract over Array<u8, _> vs [u8], or a parallel trait with [u8] in the place of the current Array.

We can debate the virtues of one over the other, however I'll note the latter unblocks this feature from preventing a release, since it can be done in a purely additive manner, and I think it's a bit late for major breaking changes at this point.

I say we try the parallel trait approach, and if it seems like a generic approach would work out better, we can revisit it in the next breaking release. Like @rozbb said perhaps it should be hazmat, and I think it can present a fairly low-level API aimed for "power user" use cases like Servo, as opposed to having to duplicate every single method of e.g. AeadInOut.

--

I'll also say I otherwise don't see any other release blockers. The other features I'm interested in pursuing can also be done in an additive manner.

[newpavlov]

I'll also say I otherwise don't see any other release blockers.]

I am not sure about the Aead trait in it's current form, e.g. currently it's not dyn-compatible because of the TAG_POSITION constant and impl Payload. The Buffer trait may also require changes (see #1713). Right now we have to rely on copy_within which is somewhat suboptimal when we have the inout-based API. Same for reliance on dynamic dispatch in encrypt/decrypt_in_place. Finally, I don't think we need the AeadInOut/AeadCore split.

If you want to ship a minimal release ASAP, then how about doing something like this:

pub trait AeadCore {
    type NonceSize: ArraySize;
    type TagSize: ArraySize;
    const TAG_POSITION: TagPosition;

    fn encrypt_inout_detached(
        &self,
        nonce: &Nonce<Self>,
        associated_data: &[u8],
        buffer: InOutBuf<'_, '_, u8>,
    ) -> Result<Tag<Self>>;

    fn decrypt_inout_detached(
        &self,
        nonce: &Nonce<Self>,
        associated_data: &[u8],
        buffer: InOutBuf<'_, '_, u8>,
        tag: &Tag<Self>,
    ) -> Result<()>;
}

#[cfg(feature = "alloc")]
pub trait Aead {
    fn encrypt(
        &self,
        nonce: &[u8],
        plaintext: &[u8],
    ) -> Result<Vec<u8>>;

    fn encrypt_with_aad(
        &self,
        nonce: &[u8],
        payload: Payload,
    ) -> Result<Vec<u8>>;

    fn decrypt(
        &self,
        nonce: &[u8],
        ciphertext: &[u8],
    ) -> Result<Vec<u8>>;


    fn decrypt_with_aad(
        &self,
        nonce: &[u8],
        payload: Payload,
    ) -> Result<Vec<u8>>;
}

impl<C: AeadCore> Aead for C { ... }

This also should allow to implement the Aead trait independently from AeadCore, e.g. for encryption based on HW tokens.

Instead of passing nonce as a separate argument we also could make it part of Payload.

If you want to keep impl Payload and array-based nonces, then we should officially declare Aead non-dyn-compatible and keep Aead: AeadCore.

[tarcieri]

I'm not really a fan of trying to make major changes like that at the last minute, but if you're willing to do all of the work and get it done promptly I guess I'm okay with it in the interest of getting a release out and ending the seemingly interminable API design debate.

If a month has passed and the changes to implement this both in the traits repo and the AEADs repo aren't merged, I would suggest shipping aead as-is and exploring other features as additive changes from there.

[kkoyung]

That sounds like a better plan to me as well.

Supporting variable nonce sizes seems to require quite a few changes. Given that aead already seems close to being ready for release, I think it makes sense to ship a release first and explore this as an additive change afterward.

I'm also not in a hurry to have this feature immediately, so I’m fine with deferring it until after the release.

[tarcieri]

@kkoyung are you okay with the API proposed in #2427 requiring an allocation to use variable-sized nonces?

[newpavlov]

#2427 does not add API for variable size nonces since it aims to minimize the existing API. It should be easy enough to add variable nonce methods with blanket impls to AeadCore, but it's probably better to do in a separate PR. There is also the question of whether Aead should use &Nonce<Self> or &[u8] for nonces.

[kkoyung]

Requiring an allocation is not a problem in Servo, so the proposed API in #2427 looks good to me.

There is also the question of whether Aead should use &Nonce<Self> or &[u8] for nonces.

Is it for the additional API in a separate PR? Would &Nonce<Self> restrict the ability to support variable nonce sizes, since the nonce size is specified by the type?

[newpavlov]

Would &Nonce restrict the ability to support variable nonce sizes, since the nonce size is specified by the type?

Yes. It also makes the Aead trait non-dyn-compatible.

I will add variable nonce size support to the PR shortly.

[kkoyung]

Understood. I saw your new commit for variable size nonces. I have a much better picture of it now.