openpgp: read keys with revoked user ids

The existing code was wrongly assuming that UserID packets must be
immediately followed by a Signature packet. However, this is not true.

See RFC4880 11.1:
> Immediately following each User ID packet, there are zero or more
> Signature packets.

This change will ensure that Entities that are not immediately followed
by a Signature packet are read without raising a StructuralError.
Instead, UserID packets that are not immediately followed by a self
signature will be ignored.

Maximum backwards compatibility is retained because revoked UserIDs are
not added to the Entity's identities.

In a follow-up patch, we should probably add these UserIDs to the
Entity's identities too, but not without making sure that the revocation
is also available in the Entity's (or the Identity's) Revocations slice.
This would require adding support for a new Signature Type,
"Certification revocation signature", as defined in RFC 48880 5.2.1.

Fixes golang/go#25850

Change-Id: Idde34b97429998f28e0c687171024e51ed959bf0
Reviewed-on: https://go-review.googlesource.com/118376
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

openpgp/keys.go

@@ -346,22 +346,25 @@ EachPacket:
 
 		switch pkt := p.(type) {
 		case *packet.UserId:
+			// Make a new Identity object, that we might wind up throwing away.
+			// We'll only add it if we get a valid self-signature over this
+			// userID.
 			current = new(Identity)
 			current.Name = pkt.Id
 			current.UserId = pkt
-			e.Identities[pkt.Id] = current
 
 			for {
 				p, err = packets.Next()
 				if err == io.EOF {
-					return nil, io.ErrUnexpectedEOF
+					break EachPacket
 				} else if err != nil {
 					return nil, err
 				}
 
 				sig, ok := p.(*packet.Signature)
 				if !ok {
-					return nil, errors.StructuralError("user ID packet not followed by self-signature")
+					packets.Unread(p)
+					continue EachPacket
 				}
 
 				if (sig.SigType == packet.SigTypePositiveCert || sig.SigType == packet.SigTypeGenericCert) && sig.IssuerKeyId != nil && *sig.IssuerKeyId == e.PrimaryKey.KeyId {
@@ -369,9 +372,10 @@ EachPacket:
 						return nil, errors.StructuralError("user ID self-signature invalid: " + err.Error())
 					}
 					current.SelfSignature = sig
-					break
+					e.Identities[pkt.Id] = current
+				} else {
+					current.Signatures = append(current.Signatures, sig)
 				}
-				current.Signatures = append(current.Signatures, sig)
 			}
 		case *packet.Signature:
 			if pkt.SigType == packet.SigTypeKeyRevocation {

openpgp/keys_test.go

@@ -105,6 +105,33 @@ func TestGoodCrossSignature(t *testing.T) {
 	}
 }
 
+func TestRevokedUserID(t *testing.T) {
+	// This key contains 2 UIDs, one of which is revoked:
+	// [ultimate] (1)  Golang Gopher <no-reply@golang.com>
+	// [ revoked] (2)  Golang Gopher <revoked@golang.com>
+	keys, err := ReadArmoredKeyRing(bytes.NewBufferString(revokedUserIDKey))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if len(keys) != 1 {
+		t.Fatal("Failed to read key with a revoked user id")
+	}
+
+	var identities []*Identity
+	for _, identity := range keys[0].Identities {
+		identities = append(identities, identity)
+	}
+
+	if numIdentities, numExpected := len(identities), 1; numIdentities != numExpected {
+		t.Errorf("obtained %d identities, expected %d", numIdentities, numExpected)
+	}
+
+	if identityName, expectedName := identities[0].Name, "Golang Gopher <no-reply@golang.com>"; identityName != expectedName {
+		t.Errorf("obtained identity %s expected %s", identityName, expectedName)
+	}
+}
+
 // TestExternallyRevokableKey attempts to load and parse a key with a third party revocation permission.
 func TestExternallyRevocableKey(t *testing.T) {
 	kring, err := ReadKeyRing(readerFromHex(subkeyUsageHex))
@@ -481,3 +508,42 @@ SqLHvbKh2dL/RXymC3+rjPvQf5cup9bPxNMa6WagdYBNAfzWGtkVISeaQW+cTEp/
 MtgVijRGXR/lGLGETPg2X3Afwn9N9bLMBkBprKgbBqU7lpaoPupxT61bL70=
 =vtbN
 -----END PGP PUBLIC KEY BLOCK-----`
+
+const revokedUserIDKey = `-----BEGIN PGP PUBLIC KEY BLOCK-----
+
+mQENBFsgO5EBCADhREPmcjsPkXe1z7ctvyWL0S7oa9JaoGZ9oPDHFDlQxd0qlX2e
+DZJZDg0qYvVixmaULIulApq1puEsaJCn3lHUbHlb4PYKwLEywYXM28JN91KtLsz/
+uaEX2KC5WqeP40utmzkNLq+oRX/xnRMgwbO7yUNVG2UlEa6eI+xOXO3YtLdmJMBW
+ClQ066ZnOIzEo1JxnIwha1CDBMWLLfOLrg6l8InUqaXbtEBbnaIYO6fXVXELUjkx
+nmk7t/QOk0tXCy8muH9UDqJkwDUESY2l79XwBAcx9riX8vY7vwC34pm22fAUVLCJ
+x1SJx0J8bkeNp38jKM2Zd9SUQqSbfBopQ4pPABEBAAG0I0dvbGFuZyBHb3BoZXIg
+PG5vLXJlcGx5QGdvbGFuZy5jb20+iQFUBBMBCgA+FiEE5Ik5JLcNx6l6rZfw1oFy
+9I6cUoMFAlsgO5ECGwMFCQPCZwAFCwkIBwMFFQoJCAsFFgIDAQACHgECF4AACgkQ
+1oFy9I6cUoMIkwf8DNPeD23i4jRwd/pylbvxwZintZl1fSwTJW1xcOa1emXaEtX2
+depuqhP04fjlRQGfsYAQh7X9jOJxAHjTmhqFBi5sD7QvKU00cPFYbJ/JTx0B41bl
+aXnSbGhRPh63QtEZL7ACAs+shwvvojJqysx7kyVRu0EW2wqjXdHwR/SJO6nhNBa2
+DXzSiOU/SUA42mmG+5kjF8Aabq9wPwT9wjraHShEweNerNMmOqJExBOy3yFeyDpa
+XwEZFzBfOKoxFNkIaVf5GSdIUGhFECkGvBMB935khftmgR8APxdU4BE7XrXexFJU
+8RCuPXonm4WQOwTWR0vQg64pb2WKAzZ8HhwTGbQiR29sYW5nIEdvcGhlciA8cmV2
+b2tlZEBnb2xhbmcuY29tPokBNgQwAQoAIBYhBOSJOSS3Dcepeq2X8NaBcvSOnFKD
+BQJbIDv3Ah0AAAoJENaBcvSOnFKDfWMIAKhI/Tvu3h8fSUxp/gSAcduT6bC1JttG
+0lYQ5ilKB/58lBUA5CO3ZrKDKlzW3M8VEcvohVaqeTMKeoQd5rCZq8KxHn/KvN6N
+s85REfXfniCKfAbnGgVXX3kDmZ1g63pkxrFu0fDZjVDXC6vy+I0sGyI/Inro0Pzb
+tvn0QCsxjapKK15BtmSrpgHgzVqVg0cUp8vqZeKFxarYbYB2idtGRci4b9tObOK0
+BSTVFy26+I/mrFGaPrySYiy2Kz5NMEcRhjmTxJ8jSwEr2O2sUR0yjbgUAXbTxDVE
+/jg5fQZ1ACvBRQnB7LvMHcInbzjyeTM3FazkkSYQD6b97+dkWwb1iWG5AQ0EWyA7
+kQEIALkg04REDZo1JgdYV4x8HJKFS4xAYWbIva1ZPqvDNmZRUbQZR2+gpJGEwn7z
+VofGvnOYiGW56AS5j31SFf5kro1+1bZQ5iOONBng08OOo58/l1hRseIIVGB5TGSa
+PCdChKKHreJI6hS3mShxH6hdfFtiZuB45rwoaArMMsYcjaezLwKeLc396cpUwwcZ
+snLUNd1Xu5EWEF2OdFkZ2a1qYdxBvAYdQf4+1Nr+NRIx1u1NS9c8jp3PuMOkrQEi
+bNtc1v6v0Jy52mKLG4y7mC/erIkvkQBYJdxPaP7LZVaPYc3/xskcyijrJ/5ufoD8
+K71/ShtsZUXSQn9jlRaYR0EbojMAEQEAAYkBPAQYAQoAJhYhBOSJOSS3Dcepeq2X
+8NaBcvSOnFKDBQJbIDuRAhsMBQkDwmcAAAoJENaBcvSOnFKDkFMIAIt64bVZ8x7+
+TitH1bR4pgcNkaKmgKoZz6FXu80+SnbuEt2NnDyf1cLOSimSTILpwLIuv9Uft5Pb
+OraQbYt3xi9yrqdKqGLv80bxqK0NuryNkvh9yyx5WoG1iKqMj9/FjGghuPrRaT4l
+QinNAghGVkEy1+aXGFrG2DsOC1FFI51CC2WVTzZ5RwR2GpiNRfESsU1rZAUqf/2V
+yJl9bD5R4SUNy8oQmhOxi+gbhD4Ao34e4W0ilibslI/uawvCiOwlu5NGd8zv5n+U
+heiQvzkApQup5c+BhH5zFDFdKJ2CBByxw9+7QjMFI/wgLixKuE0Ob2kAokXf7RlB
+7qTZOahrETw=
+=IKnw
+-----END PGP PUBLIC KEY BLOCK-----`