mattosaurus / PgpCore

.NET Core class library for using PGP
MIT License
245 stars 98 forks source link

Allow for subkeys to be used for encryption #159

Closed Mrvandalid closed 2 years ago

Mrvandalid commented 2 years ago

When I used a key ring containing a masterkey for signing and a subkey for encryption, the library always used the masterkey, even though it did not contain keyflags for encryption.

After some digging, I found this check in the ReadPublicKey method, where only masterkeys are checked. After cloning and removing the masterkey check, the library encrypts with the subkey as expected.

Mrvandalid commented 2 years ago

This is the same change proposed in #127

mattosaurus commented 2 years ago

Thanks for submitting this PR :)

mattosaurus commented 2 years ago

Hmm, looks like that's causing some of the verification tests to fail now.

Are you able to generate me a set of test keys and a password that I can use for testing so I can look into this a bit more?

Mrvandalid commented 2 years ago

Sure! Test_Keys.zip

The password is 1234

mattosaurus commented 2 years ago

So I think that the issue is that we're only selecting for keys with encrypt flags.

https://github.com/mattosaurus/PgpCore/blob/713538aa6fe6064dfeaff15fe98d6791dc5c9f20/PgpCore/Utilities.cs#L439

But in the case of the failing tests they're signing and verifying rather than encrypting so the wrong key gets pulled out.

I think the solution to this would be to make the ReadPublicKey method aware of the key type it needs to return as at the moment it's just defaulting to encryption.

I'll have a look and see how easy that is.

mattosaurus commented 2 years ago

Seems like we're getting the keys when creating the EncryptionKeys object which happens on PGP construction when we have no idea of the methods that will be used.

Ideally the relevant key would be pulled out when needed rather than doing it like this so a larger scale rewrite of the code would be required to get it working.

Mrvandalid commented 2 years ago

I see. It would seem my simple suggestion was a bit naïve in regards to the underlying code base :) When looking at the GetFirstSecretKey, which is used to get the secret key, the library checks if it is a signing key: https://github.com/mattosaurus/PgpCore/blob/713538aa6fe6064dfeaff15fe98d6791dc5c9f20/PgpCore/EncryptionKeys.cs#L524-L540 However, looking at the bouncycastle source here, we can see that it only considers the PublicKeyAlgorithmTag. We should do additional keyflag checks here as well (as suggested by the documentation above the function) when/if doing the rewrite.

In any case, what do you think would be the best way of solving this? Perhaps additional properties in the EncryptionKeys interface could work? I am thinking four in total:

  1. EncryptionPublicKey
  2. VerifyingPublicKey
  3. DecryptionSecretKey
  4. SigningSecretKey