bc-java icon indicating copy to clipboard operation
bc-java copied to clipboard

OpenPGP Stream-based PaddingPacket

Open mdusina opened this issue 1 year ago • 2 comments

The newly added PaddingPacket class offers a couple of ways to create the padding bytes when producing padding packets. You can give it the verbatim byte[] you want or give it a SecureRandom and a length and let it eagerly create the bytes for you. There doesn't appear to be a higher level class for configuring padding like you can configure compression and encryption options like integrity packet generation.

Consider tweaking the current design to support stream-based padding bytes so they don't have to be eagerly loaded into memory. It seems like there are two ways to go about this depending on the acceptable tradeoffs.

The more elegant way would likely involve creating something akin to the existing stream-based output classes (compression, encryption, etc). You could provide padding configuration options alongside the existing settings for compression algorithms. PGPDataEncryptorBuilder might be able to host padding options.

The hacky way is to just add some overloads to PaddingPacket that accept an InputStream of padding bytes and to avoid eagerly querying the SecureRandom (delaying it to a buffered-based generation when encoding the packet). The downsides to this approach is that reading from the input stream is one-shot, so you have to decide what to do about the method that returns the byte[] of padding bytes. Encoding will also be non-deterministic since you'd be getting new bytes from SecureRandom each time. The awkwardness comes from this class holding what was read from an existing message and being the only way for describing what will be written for a new message.

mdusina avatar Aug 29 '24 23:08 mdusina

There doesn't appear to be a higher level class for configuring padding like you can configure compression and encryption options like integrity packet generation.

PGPDataEncryptorBuilder might be able to host padding options.

Since the PGPDataEncryptorBuilder (which I would consider a "mid-level-API") can also be used to generate messages using legacy SymmetricallyEncryptedData packets and version 1 SymmetricallyEncryptedIntegrityProtectedData packets, I think care has to be given to the API design to reduce the chance of the user accidentally generating invalid messages with padding packets where they are not expected by other implementations.

I'm still thinking about the design of a higher-level API for message creation within BC, which would automatically take care of things like padding.

Regarding avoiding querying the SecureRandom eagerly, it might be possible to come up with a lazy evaluation setup for the byte array, which would basically be equivalent to a (more complicated) stream based solution, right?

vanitasvitae avatar Sep 02 '24 11:09 vanitasvitae

The problem isn't the eagerness of the allocation, it's the chunkiness of it (specifically, one chunk all at once). A stream based solution would allow clients to place caps on how much memory each PGP operation consumes.

The PGP spec leaves the amount of padding needed as unspecified. Even if clients standardize on relatively small amounts of padding, it would matter for clients processing many concurrent PGP streams. The system would be much less stable if there were a chance that every single stream might try to allocation large padding arrays at the same time (instead of streaming through a fixed buffer).

mdusina avatar Sep 03 '24 01:09 mdusina