In-place encoding and decoding

[roshanrags]

The read_message and write_message functions take a & source and &mut destination to perform encoding and decoding. Currently this necessitates different buffers which seems wasteful. Most crypto libraries (e.g. libsodium) offer in-place encoding and decoding support which is quite convenient. Is this something desirable for snow?

[Cognoscan]

Seconded, I would also find this quite useful, as in-place is what I need in order to plug snow into quinn-proto's cipher traits for a project I'm working on. Copying the data into a temporary buffer to meet snow's interface would work, but that would mean I'm actually copying data into a temporary buffer, then snow copies it into another buffer, and then performs the encrypt/decrypt step. Workable, but not my favorite approach.

Given that all the existing resolvers actually do in-place encrypt/decrypt internally, it seems like this should be pretty easy to switch to, while maintaining the existing read_message and write_message functions. The new in_place functions could be just like those functions, but have only a single buf: &mut [u8] buffer instead, which would be sized to the plaintext+TAGLEN / the ciphertext length.

I know this would change the Cipher trait's interface, but I think that's already in the cards for the next update anyway? It looks like there's a plan to switch to a fixed-size reference for set, which would also be a breaking change.

I'd be happy to implement this and make a pull request.

[Cognoscan]

After taking an initial shot at implementing and using this, it looks like a better addition would be to have a mutable buffer for encrypt/decrypt, but to make the authentication tag detached. That way the existing read_message functions can call the in-place versions under the hood without needing an output buffer big enough to contain the tag as well as the plaintext. This also matches up with what all the snow-supported crypto crates do.

[roshanrags]

Taking inspiration from libsodium (https://doc.libsodium.org/secret-key_cryptography/aead/aes-256-gcm), it features combined and detached modes with the auth tag attached (the usual case) and detached respectively. Ofc it is C, so there's no aliasing concerns or bounds checks or slice arguments, would have to prototype and see what's best for Rust.

[Cognoscan]

Exposing a detached mode seems like the most important - any other interface can be built on top of that.

That said, my motivating use case is writing a crate that uses snow to provide the cryptography for quinn-proto, a QUIC protocol implementation. In that case, I already have to do the somewhat uncomfortable step of using the snow/risky-raw-split feature to extract the send & receive keys into new boxed Cipher instances. I really just need these cipher-in-place interfaces for that trait. As such, I don't have any particular need for these to be exposed in the transport structs, and am not sure what would be the most helpful.

If I were to guess, though - exposing write_message and read_message detached variants since they're the most fundamental, and exposing ones that work with BytesMut and Bytes from the bytes crate. These would match the "combined" mode you see in libsodium, ring, and the ciphers that use the aead traits. It would also make it fairly easy to upgrade libp2p-noise to stop copying bytes around, which would be good since that seems to be where a significant number of snow downloads off crates.io are coming from.

I don't think it's nearly as important to bother changing this stuff in the handshake struct, since total handshake data will be vastly smaller than total transport data.

@roshanr95 do you have a particular use case in mind? Would this fill that need?

[roshanrags]

For me, it's mainly just not having to create two separate buffers and throw away one every time. Given a detached version, the combined version seems to be just a split_at(_mut) away if I'm not wrong.

[Cognoscan]

Yep, it'd be exactly that if your interface works with buffers that leave space for the tag. This is a mockup of what could be done for TransportState:

    pub fn write_message_in_place_detached(
        &mut self,
        authtext: &[u8],
        buffer: &mut [u8],
    ) -> Result<[u8; TAGLEN], Error> {
        // ...
    }

    pub fn write_message_in_place(
        &mut self,
        authtext: &[u8],
        buffer: &mut [u8],
    ) -> Result<(), Error> {
        // Verify the buffer has enough space before splitting it
        if buffer.len() < TAGLEN { return Err(Error::Input); }
        let (buffer, tag_buffer) = buffer.split_at_mut(buffer.len()-TAGLEN);

        // Write the message and append the tag
        let tag = self.write_message_in_place_detached(authtext, buffer)?;
        copy_slices!(tag, tag_buffer);
        Ok(())
    }

    pub fn read_message_in_place_detached(
        &mut self,
        authtext: &[u8],
        buffer: &mut [u8],
        tag: &[u8; TAGLEN],
    ) -> Result<(), Error> {
        // ...
    }

    pub fn read_message_in_place<'a>(
        &mut self,
        authtext: &[u8],
        buffer: &'a mut [u8],
    ) -> Result<&'a [u8], Error> {
        // Verify the buffer has enough space before splitting it
        if buffer.len() < TAGLEN { return Err(Error::Decrypt); }
        let (buffer, tag_buffer) = buffer.split_at_mut(buffer.len()-TAGLEN);
        let tag: &[u8; 16] = (&*tag_buffer).try_into().unwrap();

        // Read the message and return the plaintext slice on success
        self.read_message_in_place_detached(authtext, buffer, tag)?;
        Ok(buffer)
    }

I've got a working branch that implements the detached versions here: https://github.com/Cognoscan/snow/tree/cipher_in_place

The interface into Cipher has been augmented with two more functions, which any trait implementation has to use instead of the original read and write traits. Blanket implementations are in the trait for those. The various pieces that use Cipher thus don't need to change at all, and the detached functions can just call into the trait functions pretty transparently.

[Cognoscan]

@mcginty At this point I've got what I need in a fork, so I can keep developing my own snow-dependent crate. I'd love to see some variation on this extension in snow at some point if possible though! I know it's not part of the core Noise protocol documentation, but sure seems like it'd be helpful for users of this crate.

[mcginty]

@Cognoscan thanks for the inspiration! snow has been in maintenance mode for the last little while due to other engagements that took my time away, but I'm catching up now and excited to rework some of the API to fit a wider range of uses - I'll check out your fork and see what we can do!