x/crypto/openpgp: NewEntity creates malformed PGP key: unexpected EOF

[onetwopunch]

What I'm trying to do is programmatically generate a PGP keypair, store the private key somewhere only this program has access to write to, such as KMS or something, then use the public key. The problem I'm facing is that it seems my method of creating a key pair is not working such that it can be re-ingested by this same library.

I'm fully aware that I may be doing something wrong here, but on the off chance this is an actual error in the openpgp package I figured I might as well submit an issue.

What version of Go are you using (go version)?

go version go1.10.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

Mac OSX

What did you do?

I'm trying to autogenerate a PGP key pair and validate the public key armored using the openpgp package in the following way:

package main

import (
	"bytes"
	"encoding/base64"
	"fmt"
	"golang.org/x/crypto/openpgp"
	"golang.org/x/crypto/openpgp/armor"
	"golang.org/x/crypto/openpgp/packet"
	"log"
)

func main() {
	buf := createKeyPair()
	fmt.Println(buf)
	encoded, err := readPGP(buf)
	if err != nil {
		log.Fatal(err)
	}
	validate(encoded)
}

func createKeyPair() *bytes.Buffer {
	conf := &packet.Config{
		RSABits: 2048,
	}
	entity, err := openpgp.NewEntity("Rick Sanchez", "Autogenerated GPG Key", "test@example.com", conf)
	// TODO: Store private key somewhere safe
	if err != nil {
		log.Fatal(err)
	}
	serializedEntity := bytes.NewBuffer(nil)
	entity.Serialize(serializedEntity)

	buf := bytes.NewBuffer(nil)
	headers := map[string]string{"Version": "GnuPG v1"}
	w, err := armor.Encode(buf, openpgp.PublicKeyType, headers)
	if err != nil {
		log.Fatal(err)
	}
	_, err = w.Write(serializedEntity.Bytes())
	if err != nil {
		log.Fatalf("error armoring serializedEntity: %s")
	}
	w.Close()

	return buf
}

func validate(keystring string) {
	data, err := base64.StdEncoding.DecodeString(keystring)
	if err != nil {
		log.Fatalf("error decoding given PGP key: %s", err)
	}
	_, err = openpgp.ReadEntity(packet.NewReader(bytes.NewBuffer(data)))
	if err != nil {
		log.Fatalf("error parsing given PGP key: %s", err)
	}
	fmt.Println("PASSED")
}

func readPGP(armoredKey *bytes.Buffer) (string, error) {
	keyReader := bytes.NewReader(armoredKey.Bytes())
	entityList, err := openpgp.ReadArmoredKeyRing(keyReader)
	if err != nil {
		log.Fatalf("error reading armored key %s", err)
	}
	serializedEntity := bytes.NewBuffer(nil)
	err = entityList[0].Serialize(serializedEntity)
	if err != nil {
		return "", fmt.Errorf("error serializing entity for file %s", err)
	}

	return base64.StdEncoding.EncodeToString(serializedEntity.Bytes()), nil
}

What did you expect to see?

<public key>
PASSED

What did you see instead?

-----BEGIN PGP PUBLIC KEY BLOCK-----
Version: GnuPG v1

xsBNBFr/vScBCADL8q0l5c/H1H5cvIRRvmX0Ulw3CuVodJ8JpMUM288xlkrYRgyE
mJzGyi2puZYXz1MAeutOZQTVjCRz4ReJ0aXOposVZek5wN3Mg/KGh9P8OAZuMFVP
F/KgzWpYxxxVeIglEyka1/5b0k2nv9ypkwsmRlQ1n78hgYzNT+bFi3ElWhVcLV7s
yNV+YHh0YzRLQsR73Ip9Mi5/NbZv4YegdAVJKPAV2ab+oDYiVnatYIxePNuOeGYu
oTGh41M+luqikgryYO61Z8VkQNFf2BuFTUQdMpbH3FIVBqWFDZAJ9kzFD+mNBYeA
fgk9AvnC/fAFCJw732VjcRaeX2D5O6E+I68/ABEBAAHNN1JpY2sgU2FuY2hleiAo
QXV0b2dlbmVyYXRlZCBHUEcgS2V5KSA8dGVzdEBleGFtcGxlLmNvbT4=
=vSR7
-----END PGP PUBLIC KEY BLOCK-----
2018/05/18 22:59:03 error reading armored key unexpected EOF
exit status 1

Also, using this same createKeyPair method but just outputting it to stdout then attempting to import it to gpg (instead of this validation code) also did not work but with a different error:

$ go run gpg_test.go | gpg --import
gpg: key 962282668E9C926F: no valid user IDs
gpg: this may be caused by a missing self-signature
gpg: Total number processed: 1
gpg:           w/o user IDs: 1

[james-lawrence]

I believe you need to use e.SerializePrivate to get the gpg import to work. but if you want the full key exported you need to use both Serialize and SerializePrivate.

[onetwopunch]

@james-lawrence You should be able to export/armor just the public portion for distribution. I can import the key into my gpg key ring, but I have to add the option: --allow-non-selfsigned-uid. I just can't seem to decode it with openpgp.

$ gpg --import --allow-non-selfsigned-uid /tmp/test.asc
gpg: key 962282668E9C926F: accepted non self-signed user ID "Rick Sanchez (Autogenerated GPG Key) <test@example.com>"
gpg: key 962282668E9C926F: public key "Rick Sanchez (Autogenerated GPG Key) <test@example.com>" imported
gpg: Total number processed: 1
gpg:               imported: 1

[bcmills]

(CC: @FiloSottile)

[james-lawrence]

@onetwopunch try using SerializePrivate to ioutil.Discard then just the Serialize to your buffer. it may not work, but iirc you need to invoke SerializePrivate to trigger the self signing aspect (could be completely wrong, going off memory atm)

[onetwopunch]

@james-lawrence You're right: https://github.com/golang/crypto/blob/master/openpgp/keys.go#L550

Is this desired though? I'd think that we should have the Serialize func sign the identities with the private key (if one exists as part of the entity) as well right?

When I change to use the SerializePrivate function with the config I get the public-private key pair armored together. I verified this by importing the key into GPG and the secret key was also imported.

I was able to update the Serialize method to give me what I need. I'm happy to create a pull request if a contributer deems it appropriate:

diff --git a/openpgp/keys.go b/openpgp/keys.go
index fd582a8..5ff4218 100644
--- a/openpgp/keys.go
+++ b/openpgp/keys.go
@@ -29,6 +29,7 @@ type Entity struct {
        Identities  map[string]*Identity // indexed by Identity.Name
        Revocations []*packet.Signature
        Subkeys     []Subkey
+       config      *packet.Config
 }

 // An Identity represents an identity claimed by an Entity and zero or more
@@ -483,6 +484,7 @@ func NewEntity(name, comment, email string, config *packet.Config) (*Entity, err
                PrimaryKey: packet.NewRSAPublicKey(currentTime, &signingPriv.PublicKey),
                PrivateKey: packet.NewRSAPrivateKey(currentTime, signingPriv),
                Identities: make(map[string]*Identity),
+               config:     config,
        }
        isPrimaryId := true
        e.Identities[uid.Id] = &Identity{
@@ -585,6 +587,12 @@ func (e *Entity) Serialize(w io.Writer) error {
                if err != nil {
                        return err
                }
+               if e.PrivateKey != nil {
+                       err = ident.SelfSignature.SignUserId(ident.UserId.Id, e.PrimaryKey, e.PrivateKey, e.config)
+                       if err != nil {
+                               return err
+                       }
+               }
                err = ident.SelfSignature.Serialize(w)
                if err != nil {
                        return err
@@ -601,6 +609,12 @@ func (e *Entity) Serialize(w io.Writer) error {
                if err != nil {
                        return err
                }
+               if e.PrivateKey != nil {
+                       err = subkey.Sig.SignKey(subkey.PublicKey, e.PrivateKey, e.config)
+                       if err != nil {
+                               return err
+                       }
+               }
                err = subkey.Sig.Serialize(w)
                if err != nil {

[james-lawrence]

@onetwopunch I would say its unnessarily confusing. just pointing you in the direction to unblock you, not making claims about the usability of the API. =)

[onetwopunch]

For now and for future users who have this problem, it can also be solved using your method like so:

// Signs the identities of the entity reference
entity.SerializePrivate(bytes.NewBuffer(nil), conf)

// Serialize only the public key.
serializedEntity := bytes.NewBuffer(nil)
entity.Serialize(serializedEntity)

I'm still happy to make a PR to clean this up a bit if someone wants. :) Thanks so much for the help @james-lawrence

[FiloSottile]

Please feel free to submit a fix as a PR. Thanks!

You might want to look a bit more into what the most appropriate time to fill in the signature is. I would not expect Serialize to cause a sign op every time it's called. (But I haven't looked at the code in depth so you might be right.)

[onetwopunch]

@FiloSottile Thanks, I'll do that. I think I might just add a separate method with the same signature as SerializePrivate called SerializeWithSignature so that we don't have to add the config to the entity and then we can allow users to serialize with the signature only when they want to, not affecting Serialize

[FiloSottile]

We should fix Serialize rather than grow a new API. Generating invalid output based on the order things are called in is not good. Just suggesting maybe the result should be cached.

[onetwopunch]

Yeah that makes sense. I'll get started tonight