From 4b7d800ea026f1bb2d9b813e5fbc5ebac372cd1e Mon Sep 17 00:00:00 2001 From: Roman Shevchenko Date: Thu, 9 Feb 2023 10:12:14 +0000 Subject: [PATCH 1/7] reuse getPrimeryUser method --- extension/js/common/core/crypto/pgp/openpgp-key.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/extension/js/common/core/crypto/pgp/openpgp-key.ts b/extension/js/common/core/crypto/pgp/openpgp-key.ts index b4c2b1560dc..6a9b616206c 100644 --- a/extension/js/common/core/crypto/pgp/openpgp-key.ts +++ b/extension/js/common/core/crypto/pgp/openpgp-key.ts @@ -669,7 +669,10 @@ export class OpenPGPKey { }; private static getPrimaryKeyFlags = async (key: OpenPGP.Key): Promise => { - const selfCertification = (await OpenPGPKey.getUsersAndSelfCertifications(key)).map(x => x.selfCertification).find(Boolean); + // Note: The selected selfCertification (and hence the flags) will differ based on the current date + const primaryUser = await Catch.undefinedOnException(key.getPrimaryUser()); + // a type patch until https://github.com/openpgpjs/openpgpjs/pull/1594 is resolved + const selfCertification = (primaryUser as { selfCertification: OpenPGP.SignaturePacket } | undefined)?.selfCertification; if (!selfCertification) { return 0; } From 74a209013182540732272552cff590fabe5816bf Mon Sep 17 00:00:00 2001 From: Roman Shevchenko Date: Thu, 9 Feb 2023 15:45:46 +0000 Subject: [PATCH 2/7] verifying key users --- .../js/common/core/crypto/pgp/openpgp-key.ts | 75 ++++++++----------- 1 file changed, 31 insertions(+), 44 deletions(-) diff --git a/extension/js/common/core/crypto/pgp/openpgp-key.ts b/extension/js/common/core/crypto/pgp/openpgp-key.ts index 6a9b616206c..30fca358cff 100644 --- a/extension/js/common/core/crypto/pgp/openpgp-key.ts +++ b/extension/js/common/core/crypto/pgp/openpgp-key.ts @@ -305,9 +305,10 @@ export class OpenPGPKey { const key = await OpenPGPKey.extractExternalLibraryObjFromKey(pubkey); const result = new Map(); result.set(`Is Private?`, KeyUtil.formatResult(key.isPrivate())); - for (let i = 0; i < key.users.length; i++) { + const users = await OpenPGPKey.verifyAllUsers(key); + for (let i = 0; i < users.length; i++) { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - result.set(`User id ${i}`, key.users[i].userID!.userID); + result.set(`User id ${i}`, users[i].valid ? users[i].userID : 'Invalid'); // todo: clarify "REVOKED"? } const user = await key.getPrimaryUser(); result.set(`Primary User`, user?.user?.userID?.userID || 'No primary user'); @@ -470,17 +471,6 @@ export class OpenPGPKey { return keyPacket.isDecrypted() === true; }; - public static getPrimaryUserId = async (pubs: OpenPGP.Key[], keyid: OpenPGP.KeyID): Promise => { - for (const opgpkey of pubs) { - const matchingKeys = opgpkey.getKeys(keyid); - if (matchingKeys.length > 0) { - const primaryUser = await opgpkey.getPrimaryUser(); - return primaryUser?.user?.userID?.userID; - } - } - return undefined; - }; - public static verify = async (msg: OpenpgpMsgOrCleartext, pubs: PubkeyInfo[]): Promise => { // todo: double-check if S/MIME ever gets here const validKeys = pubs.filter(x => !x.revoked && x.pubkey.family === 'openpgp').map(x => x.pubkey); @@ -546,39 +536,36 @@ export class OpenPGPKey { private static getExpirationAsDateOrUndefined = (expirationTime: Date | typeof Infinity | null) => { return expirationTime instanceof Date ? expirationTime : undefined; // we don't differ between Infinity and null }; - private static getUsersAndSelfCertifications = async (key: OpenPGP.Key) => { - const data = ( - await Promise.all( - key.users - .filter(user => user?.userID) - .map(async user => { - const dataToVerify = { userID: user.userID, key: key.keyPacket }; - const selfCertification = await OpenPGPKey.getLatestValidSignature( - user.selfCertifications, - key.keyPacket, - opgp.enums.signature.certGeneric, - dataToVerify - ); - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - return { userid: user.userID!.userID, email: user.userID!.email, selfCertification }; - }) - ) - ).filter(x => x.selfCertification); - // sort the same way as OpenPGP.js does - data.sort((a, b) => { - const A = a.selfCertification!; // eslint-disable-line @typescript-eslint/no-non-null-assertion - const B = b.selfCertification!; // eslint-disable-line @typescript-eslint/no-non-null-assertion - return Number(A.revoked) - Number(B.revoked) || Number(B.isPrimaryUserID) - Number(A.isPrimaryUserID) || Number(B.created) - Number(A.created); - }); - return data; + + // returns all the `key.users` preserving order with `valid` property + private static verifyAllUsers = async (key: OpenPGP.Key) => { + return await ( + key as unknown as { + // a type patch until https://github.com/openpgpjs/openpgpjs/pull/1594 is resolved + // eslint-disable-next-line no-null/no-null + verifyAllUsers(): Promise<{ userID: string; keyID: OpenPGP.KeyID; valid: boolean | null }[]>; + } + ).verifyAllUsers(); }; - private static getSortedUserids = async (key: OpenPGP.Key): Promise<{ identities: string[]; emails: string[] }> => { - const data = await OpenPGPKey.getUsersAndSelfCertifications(key); - return { - identities: data.map(x => x.userid).filter(Boolean), - emails: data.map(x => x.email).filter(Boolean), // todo: toLowerCase()? - }; + private static getSortedUserids = async (key: OpenPGP.Key) => { + const primaryUser = await Catch.undefinedOnException(key.getPrimaryUser()); + // if there is no good enough user id to serve as primary identity, we assume other user ids are even worse + if (primaryUser?.user?.userID?.userID) { + const identities = [primaryUser.user.userID.userID]; // put the "primary" identity first + const verifiedUsers = await OpenPGPKey.verifyAllUsers(key); + for (let i = 0; i < verifiedUsers.length; i++) { + if (i !== primaryUser.index && verifiedUsers[i].valid) { + identities.push(verifiedUsers[i].userID); + } + } + const emails = identities.map(userid => Str.parseEmail(userid).email).filter(Boolean); + if (emails.length === identities.length) { + // OpenPGP.js uses RFC 5322 `email-addresses` parser, so we expect all identities to contain a valid e-mail address + return { emails, identities }; + } + } + return { emails: [], identities: [] }; }; // mimicks OpenPGP.helper.getLatestValidSignature From 54fe83d7b5138640a861781440d8897f8d5a3bdb Mon Sep 17 00:00:00 2001 From: Roman Shevchenko Date: Fri, 10 Feb 2023 09:50:17 +0000 Subject: [PATCH 3/7] allow localhost domain for email address --- extension/js/common/core/common.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/extension/js/common/core/common.ts b/extension/js/common/core/common.ts index ef98bcc5aa2..ac37f44d9b9 100644 --- a/extension/js/common/core/common.ts +++ b/extension/js/common/core/common.ts @@ -87,8 +87,10 @@ export class Str { if (email.indexOf(' ') !== -1) { return false; } - email = email.replace(/\:8001$/, ''); // for MOCK tests until https://github.com/FlowCrypt/flowcrypt-browser/issues/4631 - return /^(([^<>()\[\]\\.,;:\s@"]+(\.[^<>()\[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$/i.test( + // todo: `email-addresses` parser used by OpenPGP.js consider top-level domains to be valid for emails e.g. address@domain + // should we allow it too (or use `email-addresses` package when manipulating user ids in keys? + // For now, I'm explicitly `localhost` domain, which is perfectly legal for testing + return /^(([^<>()\[\]\\.,;:\s@"]+(\.[^<>()\[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}])|localhost|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$/i.test( email ); }; From 93e2b644ca46fff4d5ba7e64f93726dce1bb938b Mon Sep 17 00:00:00 2001 From: Roman Shevchenko Date: Fri, 10 Feb 2023 15:41:41 +0000 Subject: [PATCH 4/7] fix --- extension/js/common/core/common.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/extension/js/common/core/common.ts b/extension/js/common/core/common.ts index ac37f44d9b9..9dc274ccd98 100644 --- a/extension/js/common/core/common.ts +++ b/extension/js/common/core/common.ts @@ -87,6 +87,7 @@ export class Str { if (email.indexOf(' ') !== -1) { return false; } + email = email.replace(/\:8001$/, ''); // for MOCK tests, todo: remove from production // todo: `email-addresses` parser used by OpenPGP.js consider top-level domains to be valid for emails e.g. address@domain // should we allow it too (or use `email-addresses` package when manipulating user ids in keys? // For now, I'm explicitly `localhost` domain, which is perfectly legal for testing From 421c159ef59220fe4ad06cab0d4fbee7dcfee993 Mon Sep 17 00:00:00 2001 From: Roman Shevchenko Date: Sat, 11 Feb 2023 08:49:45 +0000 Subject: [PATCH 5/7] fix and test --- .../js/common/core/crypto/pgp/openpgp-key.ts | 15 ++++---- test/source/tests/unit-node.ts | 36 +++++++++++++++++++ 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/extension/js/common/core/crypto/pgp/openpgp-key.ts b/extension/js/common/core/crypto/pgp/openpgp-key.ts index 30fca358cff..9d4837e4178 100644 --- a/extension/js/common/core/crypto/pgp/openpgp-key.ts +++ b/extension/js/common/core/crypto/pgp/openpgp-key.ts @@ -308,7 +308,7 @@ export class OpenPGPKey { const users = await OpenPGPKey.verifyAllUsers(key); for (let i = 0; i < users.length; i++) { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - result.set(`User id ${i}`, users[i].valid ? users[i].userID : 'Invalid'); // todo: clarify "REVOKED"? + result.set(`User id ${i}`, users[i].valid ? users[i].userID : '* REVOKED, INVALID OR MISSING SIGNATURE *'); } const user = await key.getPrimaryUser(); result.set(`Primary User`, user?.user?.userID?.userID || 'No primary user'); @@ -552,13 +552,12 @@ export class OpenPGPKey { const primaryUser = await Catch.undefinedOnException(key.getPrimaryUser()); // if there is no good enough user id to serve as primary identity, we assume other user ids are even worse if (primaryUser?.user?.userID?.userID) { - const identities = [primaryUser.user.userID.userID]; // put the "primary" identity first - const verifiedUsers = await OpenPGPKey.verifyAllUsers(key); - for (let i = 0; i < verifiedUsers.length; i++) { - if (i !== primaryUser.index && verifiedUsers[i].valid) { - identities.push(verifiedUsers[i].userID); - } - } + const primaryUserId = primaryUser.user.userID.userID; + const identities = [ + primaryUserId, // put the "primary" identity first + // other identities go in indeterministic order + ...Value.arr.unique((await OpenPGPKey.verifyAllUsers(key)).filter(x => x.valid && x.userID !== primaryUserId).map(x => x.userID)), + ]; const emails = identities.map(userid => Str.parseEmail(userid).email).filter(Boolean); if (emails.length === identities.length) { // OpenPGP.js uses RFC 5322 `email-addresses` parser, so we expect all identities to contain a valid e-mail address diff --git a/test/source/tests/unit-node.ts b/test/source/tests/unit-node.ts index da4d1cb6be9..07f7a3d946a 100644 --- a/test/source/tests/unit-node.ts +++ b/test/source/tests/unit-node.ts @@ -1391,6 +1391,42 @@ jSB6A93JmnQGIkAem/kzGkKclmfAdGfc4FS+3Cn+6Q==Xmrz t.pass(); }); + test('[KeyUtil.diagnose] correctly displays revoked userid', async t => { + const key = await KeyUtil.parse(`-----BEGIN PGP PRIVATE KEY BLOCK----- +Version: FlowCrypt Testing Only unspecified + +lFgEX6UIExYJKwYBBAHaRw8BAQdAMfHf64wPQ2LC9In5AKYU/KT1qWvI7e7aXr+L +WeQGUKIAAQCcB3zZlHfepQT26LIwbTDn4lvQ9LuD1fk2hK6i9FXFxxO7tBI8dXNl +ckBleGFtcGxlLmNvbT6IjwQQFgoAIAUCX6UIEwYLCQcIAwIEFQgKAgQWAgEAAhkB +AhsDAh4BACEJEEoCtcZ3snFuFiEENY1GQZqrKQqgUAXASgK1xneycW6P6AEA5iXF +K+fWpj0vn3xpKEuFRqvytPKFzhwd4wEvL+IGSPEBALE/pZdMzsDoKPENiLFpboDV +NVJScwFXIleKmtNaRycFiIwEExYIAD4FAmLqt7IJEEoCtcZ3snFuFiEENY1GQZqr +KQqgUAXASgK1xneycW4CngECmwMEFgIBAAYLCQcIAwIEFQgKAgAA7VwA/3x+J0i5 +DPaKtiosXHEV3LnOjaDGJgQlj7bR1BD4P62RAP0To1EcOvYk3qdgwda00oDkvYon +aAtVAK9dqadkbOI4D4h7BDAWCAAtBQJi6reyCRBKArXGd7JxbhYhBDWNRkGaqykK +oFAFwEoCtcZ3snFuAocAAh0gAABfXQEAvxCRqQz9r7iyrPyo4R/xF1BajPxoHd0Q +y4GYx/aIq5UA/19k0C/X7tH+fPJEd3Z2QjlrvyTbymUa+z4YGK1rh/YHtA9maXJz +dEBtb2NrLnRlc3SIjwQTFggAQQUCYuq3sgkQSgK1xneycW4WIQQ1jUZBmqspCqBQ +BcBKArXGd7JxbgKeAQKbAwQWAgEABgsJBwgDAgQVCAoCApkBAACNnQEA8tTL+tGS +wC9u4ECmo2Y8AUa0nvvv9+JmiMQphqldxD0A/jkDmtuj+KX8zxArkwC4IKCAFd2G +cdgj1z2/dAKVWmICnF0EX6UIExIKKwYBBAGXVQEFAQEHQBDdeawWVNqYkP8c/ihL +EUlVpn8cQw7rmRc/sIhdAXhfAwEIBwAA/0Jy7IelcHDjxE3OzagEzSxNrCVw8uPH +NRl8s6iP+CQYEfGIeAQYFggACQUCX6UIEwIbDAAhCRBKArXGd7JxbhYhBDWNRkGa +qykKoFAFwEoCtcZ3snFuWp8BAIzRBYJSfZzlvlyyPhrbXJoYSICGNy/5x7noXjp/ +ByeOAQDnTbQi4XwXJrU4A8Nl9eyz16ZWUzEPwfWgahIG1eQDDA== +=eyAR +-----END PGP PRIVATE KEY BLOCK-----`); + expect(key.identities).to.have.length(1); + expect(key.identities).to.eql(['first@mock.test']); + expect(key.emails).to.have.length(1); + expect(key.emails).to.eql(['first@mock.test']); + const result = await KeyUtil.diagnose(key, ''); + expect(result.get('Primary User')).to.equal('first@mock.test'); + expect(result.get('User id 0')).to.equal('* REVOKED, INVALID OR MISSING SIGNATURE *'); + expect(result.get('User id 1')).to.equal('first@mock.test'); + t.pass(); + }); + test('[KeyUtil.diagnose] displays PK and SK usage', async t => { const usageRegex = /\[\-\] \[(.*)\]/; /* eslint-disable @typescript-eslint/no-non-null-assertion */ From 94271d3d5ba4827133d0ec284275b869ba68a036 Mon Sep 17 00:00:00 2001 From: Roman Shevchenko Date: Sat, 11 Feb 2023 09:11:37 +0000 Subject: [PATCH 6/7] use verified users in key-import-ui --- extension/js/common/ui/key-import-ui.ts | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/extension/js/common/ui/key-import-ui.ts b/extension/js/common/ui/key-import-ui.ts index 9f8f4c1b39f..df473db28e3 100644 --- a/extension/js/common/ui/key-import-ui.ts +++ b/extension/js/common/ui/key-import-ui.ts @@ -112,19 +112,15 @@ export class KeyImportUi { Ui.event.handle(async target => { $('.action_add_private_key').addClass('btn_disabled').attr('disabled'); $('.input_email_alias').prop('checked', false); - const prv = await Catch.undefinedOnException(opgp.readKey({ armoredKey: String($(target).val()) })); + const prv = await Catch.undefinedOnException(KeyUtil.parse(String($(target).val()))); if (prv !== undefined) { $('.action_add_private_key').removeClass('btn_disabled').removeAttr('disabled'); if (submitKeyForAddrs !== undefined) { - const users = prv.users; - for (const user of users) { - const userId = user.userID; - if (userId) { - for (const inputCheckboxesWithEmail of $('.input_email_alias')) { - if (String($(inputCheckboxesWithEmail).data('email')) === userId.email) { - KeyImportUi.addAliasForSubmission(userId.email, submitKeyForAddrs); - $(inputCheckboxesWithEmail).prop('checked', true); - } + for (const email of prv.emails) { + for (const inputCheckboxesWithEmail of $('.input_email_alias')) { + if (String($(inputCheckboxesWithEmail).data('email')) === email) { + KeyImportUi.addAliasForSubmission(email, submitKeyForAddrs); + $(inputCheckboxesWithEmail).prop('checked', true); } } } From 72138e6b50f1ca897e8eaabd9ce56cd0c4b05dea Mon Sep 17 00:00:00 2001 From: Roman Shevchenko Date: Mon, 13 Feb 2023 15:03:53 +0000 Subject: [PATCH 7/7] PR review fixes --- extension/js/common/core/common.ts | 4 +--- extension/js/common/core/crypto/pgp/openpgp-key.ts | 2 +- test/source/tests/unit-node.ts | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/extension/js/common/core/common.ts b/extension/js/common/core/common.ts index 9dc274ccd98..7724e604e45 100644 --- a/extension/js/common/core/common.ts +++ b/extension/js/common/core/common.ts @@ -88,9 +88,7 @@ export class Str { return false; } email = email.replace(/\:8001$/, ''); // for MOCK tests, todo: remove from production - // todo: `email-addresses` parser used by OpenPGP.js consider top-level domains to be valid for emails e.g. address@domain - // should we allow it too (or use `email-addresses` package when manipulating user ids in keys? - // For now, I'm explicitly `localhost` domain, which is perfectly legal for testing + // `localhost` is a valid top-level domain for an email address, otherwise we require a second-level domain to be present return /^(([^<>()\[\]\\.,;:\s@"]+(\.[^<>()\[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}])|localhost|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$/i.test( email ); diff --git a/extension/js/common/core/crypto/pgp/openpgp-key.ts b/extension/js/common/core/crypto/pgp/openpgp-key.ts index 9d4837e4178..a95ae1b5702 100644 --- a/extension/js/common/core/crypto/pgp/openpgp-key.ts +++ b/extension/js/common/core/crypto/pgp/openpgp-key.ts @@ -308,7 +308,7 @@ export class OpenPGPKey { const users = await OpenPGPKey.verifyAllUsers(key); for (let i = 0; i < users.length; i++) { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - result.set(`User id ${i}`, users[i].valid ? users[i].userID : '* REVOKED, INVALID OR MISSING SIGNATURE *'); + result.set(`User id ${i}`, (users[i].valid ? '' : '* REVOKED, INVALID OR MISSING SIGNATURE * ') + users[i].userID); } const user = await key.getPrimaryUser(); result.set(`Primary User`, user?.user?.userID?.userID || 'No primary user'); diff --git a/test/source/tests/unit-node.ts b/test/source/tests/unit-node.ts index 07f7a3d946a..70c8abbd019 100644 --- a/test/source/tests/unit-node.ts +++ b/test/source/tests/unit-node.ts @@ -1422,7 +1422,7 @@ ByeOAQDnTbQi4XwXJrU4A8Nl9eyz16ZWUzEPwfWgahIG1eQDDA== expect(key.emails).to.eql(['first@mock.test']); const result = await KeyUtil.diagnose(key, ''); expect(result.get('Primary User')).to.equal('first@mock.test'); - expect(result.get('User id 0')).to.equal('* REVOKED, INVALID OR MISSING SIGNATURE *'); + expect(result.get('User id 0')).to.equal('* REVOKED, INVALID OR MISSING SIGNATURE * '); expect(result.get('User id 1')).to.equal('first@mock.test'); t.pass(); });