Add a packet.PrivateKey.Encrypt() method

[dvusboy]

This is the counterpart to the packet.PrivateKey.Decrypt() method. The intention is to produce a password-protected PrivateKey that can be serialized.

• S2K Usage is always 254 (RFC-4880 5.5.3)
• S2K Specifier Type is always Iterated and Salted (RFC-4880 3.7.1.3)

[dvusboy]

I'll work on fixing the failed tests.

[dvusboy]

There's a silly mistake, so I revised the commit. The point of the PR is so that I can encrypt the private/secret keys and eventually export the entities all from within this library.

[maxtaco]

@zapu can you do the code review?

[zapu]

On it! Thank you for the PR.

[zapu]

I've checked against gpg2 if I could import a key generated and encrypted by this PR, and it seemed to work. Thank you again for this PR! I will do another read through the code tomorrow.

[dvusboy]

Added a new test case in openpgp/read_test.go to do the passphrase change and then decrypt the message from a previous test case.

[dvusboy]

BTW, I ran into an unrelated problem. If one calls openpgp.NewEntity() and create a new Entity, you cannot immediately call entity.Serialize() because none of the signatures are actually created. You'd have to call entity.SerializePrivate() first and then you can call entity.Serialize(). Why isn't the signing done during construction? Do you know? I think this is @agl legacy code.

[zapu]

But entity.SerializePrivate() returns serialized entity with private key and signatures, there's no reason to call Serialize() after.

I have this code for some keygen testing: https://gist.github.com/zapu/e58128dc1a691f3a1c18f61c6e6170e4

[dvusboy]

The reason is that I only need the public data to be sent to another service for data encryption. I just find it surprising that NewEntity() does not provide a object that can be readily serialized. It is not intuitive at all, that, in order to generate the serialized public data, you need the side-effect of SerializePrivate(). In that respect, I consider it a bug. It is tangential to the objective of this PR. Thank you for all your help though, @zapu.

[zapu]

Hm, it sounds like a valid workflow to be able to serialize not-signed keys or subkeys, even though they would not be usable in any pgp program. But in general we try not to change any go-crypto APIs, just improve upon existing code.

About this PR, I'm testing few more things, but Max will need to give a final :+1: to this, as he is the crypto expert :) Thank you for your patience.

my only issue right now is that Decrypt does not clear the Encrypted flag, but on the other hand I can understand this logic. In order to decrypt and serialize not-protected key, one needs to do

entity.PrivateKey.Decrypt([]byte("hello"))
entity.PrivateKey.Encrypted = false

So it's up to discussion how the API should work - for example if Encrypt should clear or zero out "clear-text" private key data or keep it, or if Decrypt should mutate the Encrypted field. Again, let's stick to what golang/crypto does.

[zapu]

Another thing is that once you import encrypted key, you Decrypt() it and do not manually clear Encrypted flag, it will fail to serialize. Also importing encrypted key and serializing it back is something that I would expect to work (and just serialize encrypted data back) as a consumer. So maybe parsing encrypted keys should fill the s2kHeader buffer properly, I don't know if that's possible. Or at least make exporting fail when s2kHeader buffer is but key says it's encrypted.

[dvusboy]

@zapu I've been exploring this some more. So, here's what I've found:

Decrypt() does clear both encryptedData and Encrypted flag, just not directly. It is done in all those parse*PrivateKey() methods that are specific to the key type (see example). This means after Decrypt() is called, the private key is no longer encrypted. I've added a test to validate the added field, s2kHeader, doesn't trip up serialization after decrypt.

As for the API, I'm not sure it makes sense that the private key cannot be used after Encrypt() is called. Since this is completely new, we are definitely in uncharted territory. Right now what we have is:

If the serialized form is of an encrypted private key, just calling Read() will not result in a usable PrivateKey, you must call Decrypt() first. This is similar to what happens when you do a gpg --import: you'll be prompted for the passphrase. But once Decrypt() is called, the PrivateKey is no longer encrypted. It can be used, but exporting it will result in an unencrypted payload. By adding Encrypt(), it is now possible to protect a bare PrivateKey. Serializing this PrivateKey will also produce an encrypted export.

The open question is: Should you be able to use an encrypted PrivateKey? I think the design of the original API is leaning towards no. Instead, to produce an encrypted export, Serialize() needs to ask for a passphrase. This is, BTW, a very different behavior than, say gpg. I'm wondering if I should change Decrypt() to keep the encrypted state around to make it all consistent with Encrypt() leaving the bare private key around. Changing the signature to Serialize() is not appetizing to me.

[zapu]

Oh, right! So I bumped into something else than!

    fmt.Printf("Encrypted 1: %v\n", entity.PrivateKey.Encrypted)
    entity.PrivateKey.Encrypt([]byte("hello"), nil)
    fmt.Printf("Encrypted 2: %v\n", entity.PrivateKey.Encrypted)
    entity.PrivateKey.Decrypt([]byte("hello"))
    fmt.Printf("Encrypted 3: %v\n", entity.PrivateKey.Encrypted)

will print

Encrypted 1: false
Encrypted 2: true
Encrypted 3: true

But that's because Decrypt will bail out here:

    if pk.s2k == nil {
        return nil
    }

Would it be possible to set s2k properly so this workflow can work? It's not necessary, this is just something I made up and not an actual use case we would have right now.

[dvusboy]

@zapu Thank you for the thorough review. I've addressed the issue you raised.

[zapu]

Thanks! Looks like my generate->encrypt->decrypt->serialize test program is now working correctly.

[maxtaco]

Great work here @zapu and @dvusboy!

[maxtaco]

👍

[dvusboy]

I'll rebase.

[zapu]

@maxtaco I think this looks ready

[dvusboy]

Much appreciated.

[maxtaco]

Thanks for your contributions!