Skip to content

Employ user verification mechanisms from OpenPGP v5 #4946

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rrrooommmaaa merged 9 commits into from
Feb 13, 2023
Merged

Employ user verification mechanisms from OpenPGP v5 #4946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4b7d800
reuse getPrimeryUser method
Feb 9, 2023
74a2090
verifying key users
Feb 9, 2023
872cef5
Merge remote-tracking branch 'origin/master' into issue-4588-verify-uids
Feb 10, 2023
54fe83d
allow localhost domain for email address
Feb 10, 2023
93e2b64
fix
Feb 10, 2023
421c159
fix and test
Feb 11, 2023
94271d3
use verified users in key-import-ui
Feb 11, 2023
13cda8c
Merge remote-tracking branch 'origin/master' into issue-4588-verify-uids
Feb 13, 2023
72138e6
PR review fixes
Feb 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions extension/js/common/core/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,9 @@ 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(
email = email.replace(/\:8001$/, ''); // for MOCK tests, todo: remove from production
// `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(
Copy link
Contributor Author

@rrrooommmaaa rrrooommmaaa Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm re-using Str.parseEmail method to re-extract email from userids verified by OpenPGP.js, as those verification methods only return userid string (and all valid userids are supposed to have email address embedded).
OpenPGP.js is using email-addresses NPM package to extract email address from the userid.
So several questions/issues arise:

  • Should we use email-addresses package or our own version for the re-extraction?
  • If using own version, should we validate the email address? email-addresses validation is more lenient, as it allows a top level domain only, e.g. address@domain. As I understand, this type of validation isn't convenient for compose boxes, should we have a separate validation option when extracting emails from userids, like `VALIDATE-LENIENT"?

I had to explicitly add localhost as a valid domain into the regex

Copy link
Contributor Author

@rrrooommmaaa rrrooommmaaa Feb 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomholub ?

Copy link
Collaborator

@tomholub tomholub Feb 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good as you've done it. It would indeed be odd to allow hello@yup as a valid email when composing. Similarly, having two ways to verify based on context is unnecessary complication. I think it doesn't necessarily need 100% alignment with OpenPGP.js. If there is an edge case when one check passes and the other doesn't, the result is still a failed action. Actually there's currently three places that validate email: this, OpenPGP.js, Gmail API. So it will never be unified. As long as nobody reports a problem, I'd leave it.

Copy link
Collaborator

@tomholub tomholub Feb 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per above comment I'd remove the todo since I think it's ok as is

Copy link
Contributor Author

@rrrooommmaaa rrrooommmaaa Feb 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

email
);
};
Expand Down
81 changes: 35 additions & 46 deletions extension/js/common/core/crypto/pgp/openpgp-key.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,9 +305,10 @@ export class OpenPGPKey {
const key = await OpenPGPKey.extractExternalLibraryObjFromKey(pubkey);
const result = new Map<string, string>();
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 ? '' : '* REVOKED, INVALID OR MISSING SIGNATURE * ') + users[i].userID);
}
const user = await key.getPrimaryUser();
result.set(`Primary User`, user?.user?.userID?.userID || 'No primary user');
Expand Down Expand Up @@ -470,17 +471,6 @@ export class OpenPGPKey {
return keyPacket.isDecrypted() === true;
};

public static getPrimaryUserId = async (pubs: OpenPGP.Key[], keyid: OpenPGP.KeyID): Promise<string | undefined> => {
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<VerifyRes> => {
// todo: double-check if S/MIME ever gets here
const validKeys = pubs.filter(x => !x.revoked && x.pubkey.family === 'openpgp').map(x => x.pubkey);
Expand Down Expand Up @@ -546,39 +536,35 @@ 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;
};

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()?
};
// 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) => {
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 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
return { emails, identities };
}
}
return { emails: [], identities: [] };
};

// mimicks OpenPGP.helper.getLatestValidSignature
Expand Down Expand Up @@ -669,7 +655,10 @@ export class OpenPGPKey {
};

private static getPrimaryKeyFlags = async (key: OpenPGP.Key): Promise<OpenPGP.enums.keyFlags> => {
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;
}
Expand Down
16 changes: 6 additions & 10 deletions extension/js/common/ui/key-import-ui.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down
36 changes: 36 additions & 0 deletions test/source/tests/unit-node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 * <user@example.com>');
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 */
Expand Down