justinludwig / jpgpj

Java Pretty Good Privacy Jig
MIT License
74 stars 20 forks source link

Signature verification when signature is present #40

Closed smirandamedallia closed 2 years ago

smirandamedallia commented 2 years ago

Is there a way to set the Decryptor to verifiy signatures only if the signature exists. I am trying to have parity with the way GnuPG operates. GnuPG only verifies a signature if it is present but however with JPGPJ you need to control that with setVerificationRequired and in the event when a signature is not present it throws an error saying

Verification failed : 
org.c02e.jpgpj.VerificationException: content not signed with a required key 
justinludwig commented 2 years ago

Currently it doesn't allow for that. JPGPJ was originally designed for the use-case of machine-to-machine (or human-to-machine) encrypting & signing files, where you generally know ahead of time which keys should be used for encryption & signing, and whether or not files should be signed.

But I can see how for other use-cases you might want to decrypt a file or message first, and then later make some further decisions about what to do with it based on whether or not it was signed by a known key.

To allow for that, what if we add a verificationOptional property to the Decryptor class that would effectively work as a tri-state setting in conjunction with the current verificationRequired property? I'm envisioning with that setting, the Decryptor class would:

  1. Verify signatures and require messages be signed by a known key when verificationRequired == true && verificationOptional == false
  2. Ignore signatures completely when verificationRequired == false && verificationOptional == false
  3. Verify signatures but not require messages be signed when verificationRequired == false && verificationOptional == true
  4. Disallow the state of verificationRequired == true && verificationOptional == true (ie setting verificationRequired = true would set verificationOptional = false and vice-versa).

States 1) and 2) would work like the single verificationRequired setting works today; and default setting for the Decryptor class would still be 1); but if you set a Decryptor to state 3), it would attempt to verify each message's signatures, and attach the verified-signature keys to the FileMetadata result (the way it does with 1), but without raising an exception if not signed by a known key.

smirandamedallia commented 2 years ago

I am not sure if you are saying the same thing for state 3 but this is what I thought of:

if signed: 
   // follow logic for `verificationRequired = true`,  which means that 
        // if matching verification key in ring then decrypt and verify
        // if no matching verification key then decrypt but throw verification exception
else not signed
   // follow logic for `verificationRequired = false` which should be just decrypt

This is the output I get from the command line when I try to decrypt when only the decryption key is present

gpg: encrypted with 2048-bit RSA key, ID B9A7200E, created 2021-10-21
      "real company <real@company.com>"
Hello World hey.      <------- DECRYPTED OUTPUT
gpg: Signature made Thu Oct 28 16:37:27 2021 PDT using RSA key ID 1C873979
gpg: Can't check signature: public key not found
justinludwig commented 2 years ago

I'm envisioning that state 3 would follow the same logic as state 1, but just not raise any verification exceptions. I can't imagine a scenario where someone would want to reject messages that were signed by unknown keys, but accept messages that weren't signed at all.

State 1 would reject all messages that weren't signed by a known key, whereas states 2 and 3 would accept all messages regardless of signing. But unlike state 2, state 3 would go through the same verification process as state 1, and like state 1, record when valid signatures were found.

So with state 3, messages would be decrypted regardless of signing; but if a message was signed, you'd be able to access the signing keys via the verified property of the returned FileMetadata.

smirandamedallia commented 2 years ago

This is actually the case we have since we are migrating away from gnupg command line, we are trying to keep parity with the command line which does just that:

  1. If encrypted + unsigned then just decrypt
  2. If encrypted + signed with unknown key throw exception
  3. if encrypted + signed with known key then decrypt and verify
justinludwig commented 2 years ago

Well in that case, I'm still in favor of modifying the Decryptor to have the three verification-processing states I suggested previously:

  1. Same as today with verificationRequired == true (raise exception if not signed by known key)
  2. Same as today with verificationRequired == false (ignore signatures)
  3. New: check signatures, record results in metadata, but don't raise any exceptions

In cases like yours, where neither 1 nor 2 do what you need, you would at least be able to use state 3, and raise exceptions in your application logic as appropriate based on the metadata results from the Decryptor.


But to satisfy your use case, we'd also have to record some additional information in the FileMetadata object about signatures that were not verified. I'm thinking something along the lines of adding an inner class like the following to the FileMetadata object:

public class Signature {
  boolean verified;
  long keyId;
  org.c02e.jpgpj.Key key;
}

(Plus with friendly accessors as appropriate like isVerified(), setVerified() etc.)

And then adding a signatures property to the FileMetadata object (again with appropriate accessors) that would hold a list of Signature objects, similar to the FileMetadata object's verified property:

final List<Signature> signatures = new ArrayList<Signatures>();

And then the Decryptor class would need to be modified to:

  1. (in the logic around the Decryptor.buildVerifiers() method) add an entry to the signatures list of the FileMetadata object for each of the message's signatures (recording the PGP key ID of each signature)
  2. (in the logic of the Decryptor.verify() method) update the signature entry in the FileMetadata object corresponding to each signature that was verified, to set the entry's verified property to true and key property to the signed-by key

That way, your application can run the Decryptor in state 3, and check the signatures list on the resulting FileMetadata object to determine whether to accept or reject the results:

  1. if the signatures list is empty, accept the results
  2. otherwise, if the signatures list contains a signature entry with verified == false, raise an exception (or if the signatures list doesn't contain an entry with verified == true, raise an exception, if that's how your existing application's logic works)
  3. otherwise, accept the results (as the signatures list contains a signature with verified == true)

This would also allow for different uses cases with other signature-verification requirements, like say a requirement that all messages be signed by at least 2 known keys; or a requirement that if the decrypted message turned out to contain a purchase order for more than $1000, the message must be signed by a specific key; etc.

smirandamedallia commented 2 years ago

I think that would work perfectly with our application logic. Just one follow up on this How do we capture the verification exception. So let's say we received a payload which is signed by a key that is not in the Ring The List<Signature> would contain one entry where verified=false but would it be possible to capture an error/stacktrace so that we can debug why it failed ?

justinludwig commented 2 years ago

As long as the Decryptor records the PGP key ID of each signature it encounters (including those it can't verify), the Signature entry for an unverified key would look like [verified:false, keyId:1234, key:null], where 1234 would be the (full 64-bit) key ID of the unknown key it couldn't verify. So you'd have that information for your application's own logging and error messages.

And currently if you turn on JPGPJ's logging, it will log the following message at an INFO level if it encounters a signature with an unknown key:

not found verification key 0x72A423A0013826C3

And the following message if it encounters a signature for a known key that isn't configured for verification:

not using verification key pub    013826C3 Test Key 1 <test-key-1@c02e.org> for ID=0x72A423A0013826C3

Today (in state 1) if it encounters a bad signature with a known key, it will raise a JPGPJ VerificationException with the following message:

bad signature for key pub v  013826C3 Test Key 1 <test-key-1@c02e.org>

So if for state 3 we log a similar message (instead of raising an exception), that would give you the 3 reasons via logging why JPGPJ wouldn't verify a signature: 1) unknown key, 2) known key but not configured for verification, 3) known key but bad signature. (And if a signature packet itself was malformed, you'd still get a PGPException from the underlying Bouncy Castle code during the message-unpacking process.)

justinludwig commented 2 years ago

I implemented what I was envisioning for this issue via PR #41 -- please take a look at it when you get a chance and let me know what you think.

Instead of adding a new verificationOptional boolean property to the decryptor, I ended up adding a verificationType enum property instead, where you would set it to VerificationType.Optional to put the decryptor into this new 3rd verification state. You might use it like this:

FileMetadata meta =
    new Decryptor(
        new Key(new File("path/to/my/keys/alice-pub.gpg")),
        new Key(new File("path/to/my/keys/bob-sec.gpg"), "b0bru1z!")
    )
    .withVerificationType(Decryptor.VerificationType.Optional)
    .decrypt(
        new File("path/to/ciphertext.txt.gpg"),
        new File("path/back-to/plaintext.txt")
    );

// raise an exception if ciphertext.txt.gpg had any unverified signatures
for (FileMetadata.Signature signature : meta.getSignatures())
    if (!signature.isVerified())
        throw new VerificationException("unverified signature for key "
            + signature.getKeyId());
justinludwig commented 2 years ago

I merged PR #41, and released as version 1.3.