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

PGPainless produces broken messages for some signature / data format combinations. #264

Closed
6 tasks done
vanitasvitae opened this issue Mar 24, 2022 · 4 comments
Closed
6 tasks done
Labels
bug Something isn't working interop Issue affects interoperability with other implementations module: core Issue affects the core module

Comments

@vanitasvitae
Copy link
Member

vanitasvitae commented Mar 24, 2022

A signature over data can either be of SignatureType.BINARY_DOCUMENT or SignatureType.CANONICAL_TEXT_DOCUMENT.
The body of a literal data packet can either be ´StreamEncoding.BINARY´, ´StreamEncoding.TEXT´ or ´StreamEncoding.UTF8´.

It turns out, that PGPainless only produces valid output for (BINARY,BINARY) combination.
Others result in errors, or lead to interoperability problems.

This is being investigated in https://github.com/pgpainless/pgpainless/tree/canonicalizedDatza

UPDATE

Some of the error cases have been fixed:

  • Binary Data, Binary Signature
  • Binary Data, Text Signature
  • Text Data, Binary Signature
  • Text Data, Text Signature
  • UTF8 Data, Binary Signature
  • UTF8 Data, Text Signature
@vanitasvitae vanitasvitae added bug Something isn't working module: core Issue affects the core module interop Issue affects interoperability with other implementations labels Mar 24, 2022
@vanitasvitae
Copy link
Member Author

For now users are advised to avoid using StreamEncoding.TEXT/StreamEncoding.UTF8 in combination with DocumentSignatureType.BINARY_DOCUMENT.

All other combinations should be fine now.

vanitasvitae added a commit that referenced this issue Mar 27, 2022
The reason is that values other than BINARY oftentimes cause issues
(see #264), and further
experts recommended to ignore the metadata of the LiteralData packet
and only produce with ('b'/0/) as metadata values.
@vanitasvitae
Copy link
Member Author

vanitasvitae commented Mar 30, 2022

After checking back with some other OpenPGP developers it was decided that the best way forward is to deprecate ProducerOptions.setEncoding(StreamEncoding).

This would mean that all messages would use StreamEncoding.BINARY by default, which produces valid signatures for all combinations with DocumentSignatureTypes.

PGPainless release 1.2.X will remove ProducerOptions.setEncoding(StreamEncoding) completely.

@vanitasvitae
Copy link
Member Author

vanitasvitae commented Mar 30, 2022

@teythoon suggested that the Literal Data packets format option (StreamEncoding) is merely intended as a hint for the consumer.

As a consequence I decided to stop automatically CRLF-encoding messages based on the StreamEncoding.
That means that applications that rely on messages to be CRLF-encoded (e.g. to match gpg --textmode output) would need to do the CRLF-encoding themselves before feeding the output to PGPainless.

For that purpose I copied a class from Bouncy Castle that gets the job done.
An application that needs textmode output can apply the CRLFGeneratorStream as follows:

InputStream plaintext = ... // message input
StreamEncoding encoding = StreamEncoding.UTF8: // or StreamEncoding.TEXT. THIS IS NOW MERELY CONSIDERED METADATA!
ProducerOptions options = ...; // configure how the message shall be created
options.setEncoding(encoding); // set encoding metadata 

EncryptionStream signerOrEncryptor = PGPainless.encryptAndOrSign()
    .onOutputStream(...)
    .withOptions(options);

// Wrap encryptionStream in CRLF generator
CRLFGeneratorStream crlfOut = new CRLFGeneratorStream(signerOrEncryptor, encoding);

// Feed the message to the CRLF stream instead of the encryptor
Streams.pipeAll(plaintext, crlfOut);

crlfOut.close; // closes both crlf and encryption stream
// continue as usual
EncryptionResult result = signerOrEncryptor.getResult();

UPDATE: This is no longer necessary.

@vanitasvitae
Copy link
Member Author

Update:
I added ProducerOptions.applyCRLFEncoding() which will handle CRLF encoding for the user.
So applying CRLF encoding is now a matter of calling said method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working interop Issue affects interoperability with other implementations module: core Issue affects the core module
Projects
None yet
Development

No branches or pull requests

1 participant