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

adding detached tag mode to stream #1189

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

laudiacay
Copy link

Hiiii

I really wanted detached tags on the streaming encryption mode because I was having to do some really messed up things with buffers that were slowing me down.

I wrote it, it compiles and I'm not sure what tests you'd want... I didn't quite get it working with y'all's macros either but I don't think that made sense bc they were meant to deduplicate code between Encryptor and Decryptor, but it only really makes sense for Encryptors to detach tags... Hope this is cleaned up okay enough, lmk what I should change :)

@tarcieri
Copy link
Member

This is OK but it should probably also have the corresponding detached methods on Decryptor too

@laudiacay
Copy link
Author

added- let's see if tests pass now...

@tarcieri
Copy link
Member

This looks good now, however it's a breaking change, so it will have to be part of the next release cycle (which we'll likely be starting soon)

@laudiacay
Copy link
Author

laudiacay commented Jan 15, 2023 via email

@laudiacay
Copy link
Author

I also wrote a silly little Writer impl for Encryptor:

can obviously be improved in multiple places (generalizing over ciphers, for one, haha), but it works

here's how i used it to make my code a little nicer...

    let mut gzencoder = GzEncoder::new(encrypted_writer, Compression::default());
...
        gzencoder.write_all(&buf_for_input[..n])?;
        gzencoder.flush()?;
...
    let encryptor = gzencoder.finish()?;
    let bytes_written = encryptor.finish()?;

and here's the code:

pub struct EncryptionWriter<W: Write> {
    buf: Vec<u8>,
    writer: W,
    encryptor: Encryptor<Aes256Gcm, StreamBE32<Aes256Gcm>>,
    bytes_written: usize,
}

impl<W: Write> EncryptionWriter<W> {
    pub fn new(writer: W, key: &[u8]) -> Self {
        let mut nonce = [0u8; 12];
        OsRng.fill_bytes(&mut nonce);
        let cipher = Aes256Gcm::new(key.as_ref().into());
        let encryptor = StreamBE32::from_aead(cipher, nonce.as_ref().into()).encryptor();
        Self {
            buf: Vec::new(),
            writer,
            encryptor,
            bytes_written: 0,
        }
    }

    fn finish(mut self) -> Result<usize> {
        self.flush()?;
        self.encryptor
            .encrypt_last_in_place(b"".as_ref(), &mut self.buf)
            .unwrap();
        self.writer.write_all(&self.buf)?;
        self.bytes_written += self.buf.len();
        Ok(self.bytes_written)
    }
}

impl<W: Write> Write for EncryptionWriter<W> {
    // TODO better buffering? bufwriter?
    fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> {
        self.buf.extend_from_slice(buf);
        Ok(buf.len())
    }

    fn flush(&mut self) -> std::io::Result<()> {
        self.encryptor
            .encrypt_next_in_place(b"", &mut self.buf)
            .unwrap();
        self.writer.write_all(&self.buf)?;
        self.writer.flush()?;
        self.bytes_written += self.buf.len();
        self.buf.clear();
        Ok(())
    }
}

@laudiacay
Copy link
Author

sorta like #641 but not for hashes

@tarcieri
Copy link
Member

@laudiacay yeah, the buffering is what makes that tricky, but also without either fixed size chunks or some kind of framing protocol you can't write a corresponding decryptor because you don't know the boundaries of the AEAD messages (I personally prefer the fixed-sized chunks approach because it has no size overhead beyond the tags)

If you're looking for other small things to work on, we could use some help here: RustCrypto/hashes#87

@kbknapp
Copy link

kbknapp commented Jul 25, 2023

What is the status of this PR? I have a use case where I need to use a &mut [u8] instead of &mut dyn Buffer (like the standard AeadInPlace::*_detached methods). Would changing this PR to match that signature be a blocker?

Also re:

This looks good now, however it's a breaking change, so it will have to be part of the next release cycle (which we'll likely be starting soon)

Not sure if that has already come and gone, or was delayed since that was 6 months ago. So this may be a consideration as well.

@tarcieri
Copy link
Member

Still waiting on the next breaking release cycle, which will come after #1174

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

Successfully merging this pull request may close these issues.

None yet

3 participants