Fix verification of User Attributes

This change corrects verification of certifications over User Attributes
(such as photos).

Before this change the code did not differentiate between User IDs and
User Attributes as both of them were stored in `data.userid` [0] and
incorrectly used the User ID constant (0xB4) for both cases.

This change fixes the bug by storing User IDs in `userId` property and
User Attributes in `userAttribute` property. The check for property
existence has been modified to avoid comparisons with `undefined` as the
`User` class sets `null` for not assigned packets instead of
`undefined`.

Only data structures for signing and verification were modified and not
the properties used in the `User` class.

[0]: 11b2d2de3c/src/key.js (L872)

src/key.js

@@ -536,7 +536,7 @@ Key.prototype.getPrimaryUser = async function(date=new Date(), userId={}) {
   }).pop();
   const { user, selfCertification: cert } = primaryUser;
   const primaryKey = this.keyPacket;
-  const dataToVerify = { userid: user.userId , key: primaryKey };
+  const dataToVerify = { userId: user.userId, key: primaryKey };
   // skip if certificates is invalid, revoked, or expired
   // eslint-disable-next-line no-await-in-loop
   if (!(cert.verified || await cert.verify(primaryKey, dataToVerify))) {
@@ -828,8 +828,12 @@ User.prototype.toPacketlist = function() {
  * @async
  */
 User.prototype.sign = async function(primaryKey, privateKeys) {
-  const dataToSign = { userid: this.userId || this.userAttribute, key: primaryKey };
+  const dataToSign = {
-  const user = new User(dataToSign.userid);
+    userId: this.userId,
+    userAttribute: this.userAttribute,
+    key: primaryKey
+  };
+  const user = new User(dataToSign.userId || dataToSign.userAttribute);
   user.otherCertifications = await Promise.all(privateKeys.map(async function(privateKey) {
     if (privateKey.isPublic()) {
       throw new Error('Need private key for signing');
@@ -869,7 +873,8 @@ User.prototype.isRevoked = async function(primaryKey, certificate, key, date=new
   return isDataRevoked(
     primaryKey, {
       key: primaryKey,
-      userid: this.userId || this.userAttribute
+      userId: this.userId,
+      userAttribute: this.userAttribute
     }, this.revocationSignatures, certificate, key, date
   );
 };
@@ -909,7 +914,11 @@ export async function createSignaturePacket(dataToSign, privateKey, signingKeyPa
 User.prototype.verifyCertificate = async function(primaryKey, certificate, keys, date=new Date()) {
   const that = this;
   const keyid = certificate.issuerKeyId;
-  const dataToVerify = { userid: this.userId || this.userAttribute, key: primaryKey };
+  const dataToVerify = {
+    userId: this.userId,
+    userAttribute: this.userAttribute,
+    key: primaryKey
+  };
   const results = await Promise.all(keys.map(async function(key) {
     if (!key.getKeyIds().some(id => id.equals(keyid))) { return; }
     const signingKey = await key.getSigningKey(keyid, date);
@@ -961,7 +970,11 @@ User.prototype.verify = async function(primaryKey) {
     return enums.keyStatus.no_self_cert;
   }
   const that = this;
-  const dataToVerify = { userid: this.userId || this.userAttribute, key: primaryKey };
+  const dataToVerify = {
+    userId: this.userId,
+    userAttribute: this.userAttribute,
+    key: primaryKey
+  };
   // TODO replace when Promise.some or Promise.any are implemented
   const results = [enums.keyStatus.invalid].concat(
     await Promise.all(this.selfCertifications.map(async function(selfCertification) {
@@ -987,7 +1000,11 @@ User.prototype.verify = async function(primaryKey) {
  *          module:packet.SecretSubkey} primaryKey primary key used for validation
  */
 User.prototype.update = async function(user, primaryKey) {
-  const dataToVerify = { userid: this.userId || this.userAttribute, key: primaryKey };
+  const dataToVerify = {
+    userId: this.userId,
+    userAttribute: this.userAttribute,
+    key: primaryKey
+  };
   // self signatures
   await mergeSignatures(user, this, 'selfCertifications', async function(srcSelfSig) {
     return srcSelfSig.verified || srcSelfSig.verify(primaryKey, dataToVerify);
@@ -1391,7 +1408,7 @@ async function wrapKeyObject(secretKeyPacket, secretSubkeyPackets, options) {
     userIdPacket.format(userId);
 
     const dataToSign = {};
-    dataToSign.userid = userIdPacket;
+    dataToSign.userId = userIdPacket;
     dataToSign.key = secretKeyPacket;
     const signaturePacket = new packet.Signature(options.date);
     signaturePacket.signatureType = enums.signature.cert_generic;

src/packet/signature.js

@@ -594,14 +594,14 @@ Signature.prototype.toSign = function (type, data) {
       let packet;
       let tag;
 
-      if (data.userid !== undefined) {
+      if (data.userId) {
         tag = 0xB4;
-        packet = data.userid;
+        packet = data.userId;
-      } else if (data.userattribute !== undefined) {
+      } else if (data.userAttribute) {
         tag = 0xD1;
-        packet = data.userattribute;
+        packet = data.userAttribute;
       } else {
-        throw new Error('Either a userid or userattribute packet needs to be ' +
+        throw new Error('Either a userId or userAttribute packet needs to be ' +
           'supplied for certification.');
       }
 

test/general/packet.js

@@ -662,7 +662,7 @@ describe("Packet", function() {
     return Promise.all([
       expect(key[2].verify(key[0],
         {
-            userid: key[1],
+            userId: key[1],
             key: key[0]
         })).to.eventually.be.true,
       expect(key[4].verify(key[0],

test/general/signature.js

@@ -876,7 +876,7 @@ yYDnCgA=
 
   it('Verify V3 certification signature', function(done) {
     const pubKey = openpgp.key.readArmored(pub_v3).keys[0];
-    expect(pubKey.users[0].selfCertifications[0].verify(pubKey.primaryKey, {key: pubKey.primaryKey, userid: pubKey.users[0].userId})).to.eventually.be.true.notify(done);
+    expect(pubKey.users[0].selfCertifications[0].verify(pubKey.primaryKey, {key: pubKey.primaryKey, userId: pubKey.users[0].userId})).to.eventually.be.true.notify(done);
   });
 
   it('Write unhashed subpackets', function() {
@@ -1015,4 +1015,35 @@ yYDnCgA=
     });
   });
 
+  it('Verify signed UserIDs and User Attributes', async function() {
+    const armoredKeyWithPhoto = `-----BEGIN PGP PUBLIC KEY BLOCK-----
+
+mI0EW1CJGAEEAM+BzuFzcYk9HttmDbjGexQ8dfme074Q5PuHas3PBISPm0AwmnDM
+tzjlcrrg2VGuLqHvNF600w2ZgOo2gElNYCOas1q/fVFuIgJ4SUduNOEe/JnIW4uP
+iEGU9l6zOVVgTc/nGVpZdvHgvOL8nl9BKHtWEnMD3Du7UYAm+Avshu9jABEBAAG0
+AViI1AQTAQoAPhYhBKcH118Rrg0wLBrTk5IyMikCym+4BQJbUIkYAhsDBQkDwmcA
+BQsJCAcDBRUKCQgLBRYDAgEAAh4BAheAAAoJEJIyMikCym+4K8oEAJc7YFiNau6V
+HTVK4cTvWU5MuYiejejFZai4ELUJy+WF6cZYrLuF/z/kRt8B7hpumXChPCUlT0q7
+FWypQtA3leu83DGMXqhfS80h2S1+VLmDVVWKQXOwgOb44jT9F08bDU5QK08SkjF8
+/EirIy8ANzdwCA4rHytIS2yx6tLlthvX0cBwwG4BEAABAQAAAAAAAAAAAAAAAP/Y
+/+AAEEpGSUYAAQEAAAEAAQAA/9sAQwABAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEB
+AQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEB/9sAQwEBAQEB
+AQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEB
+AQEBAQEBAQEBAQEB/8AAEQgAAQABAwEiAAIRAQMRAf/EABUAAQEAAAAAAAAAAAAA
+AAAAAAAK/8QAFBABAAAAAAAAAAAAAAAAAAAAAP/EABQBAQAAAAAAAAAAAAAAAAAA
+AAD/xAAUEQEAAAAAAAAAAAAAAAAAAAAA/9oADAMBAAIRAxEAPwC/gAH/2YjUBBMB
+CgA+FiEEpwfXXxGuDTAsGtOTkjIyKQLKb7gFAltQimUCGwMFCQPCZwAFCwkIBwMF
+FQoJCAsFFgMCAQACHgECF4AACgkQkjIyKQLKb7gm/wQAiyZF89qr8hf3XQNJ6Ir/
+QtaniPcesjrYCIE47ZfeDYpBTPeiMm295o2dZXVJS4ItllYsplASw5DJiIMnQKlJ
+mbXakYFzzclTa/JrKzFYCy/DPT95xK+633omgrIUgJodizoKJE7XeB2U6aRUJJ4O
+iTuGu4fEU1UligAXSrZmCdE=
+=VK6I
+-----END PGP PUBLIC KEY BLOCK-----`;
+
+    const key = openpgp.key.readArmored(armoredKeyWithPhoto).keys[0];
+    for (const user of key.users) {
+      expect(await user.verify(key.primaryKey)).to.equal(openpgp.enums.keyStatus.valid);
+    }
+  });
+
 });

test/general/x25519.js

@@ -240,7 +240,7 @@ describe('X25519 Cryptography', function () {
         // Self Certificate is valid
         const user = hi.users[0];
         await expect(user.selfCertifications[0].verify(
-          primaryKey, { userid: user.userId, key: primaryKey }
+          primaryKey, { userId: user.userId, key: primaryKey }
         )).to.eventually.be.true;
         await expect(user.verifyCertificate(
           primaryKey, user.selfCertifications[0], [hi.toPublic()]
@@ -260,7 +260,7 @@ describe('X25519 Cryptography', function () {
           // Self Certificate is valid
           const user = bye.users[0];
           await expect(user.selfCertifications[0].verify(
-            bye.primaryKey, { userid: user.userId, key: bye.primaryKey }
+            bye.primaryKey, { userId: user.userId, key: bye.primaryKey }
           )).to.eventually.be.true;
           await expect(user.verifyCertificate(
             bye.primaryKey, user.selfCertifications[0], [bye.toPublic()]
@@ -270,7 +270,7 @@ describe('X25519 Cryptography', function () {
             // Hi trusts Bye!
             bye.toPublic().signPrimaryUser([hi]).then(trustedBye => {
               expect(trustedBye.users[0].otherCertifications[0].verify(
-                primaryKey, { userid: user.userId, key: bye.toPublic().primaryKey }
+                primaryKey, { userId: user.userId, key: bye.toPublic().primaryKey }
               )).to.eventually.be.true;
             }),
             // Signing message
@@ -539,7 +539,7 @@ describe('X25519 Cryptography', function () {
     expect(results.user).to.exist;
     const user = results.user;
     expect(user.selfCertifications[0].verify(
-      hi.primaryKey, {userid: user.userId, key: hi.primaryKey}
+      hi.primaryKey, {userId: user.userId, key: hi.primaryKey}
     )).to.eventually.be.true;
     expect(user.verifyCertificate(
       hi.primaryKey, user.selfCertifications[0], [hi]