Skip to content

Comments

Show warnings on unusable keys#3198

Merged
tomholub merged 12 commits intomasterfrom
issue-2887-unusable-key-warning
Dec 1, 2020
Merged

Show warnings on unusable keys#3198
tomholub merged 12 commits intomasterfrom
issue-2887-unusable-key-warning

Conversation

@rrrooommmaaa
Copy link
Contributor

@rrrooommmaaa rrrooommmaaa commented Nov 27, 2020

Fixes #2887

Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

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

Looks good so far

Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

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

Looks much better 👍

@rrrooommmaaa
Copy link
Contributor Author

@tomholub I made some change that seemed logical to me.
That is, set usableButExpired to false for an expired encryption key missing private key parameters.
(It can be used only for decryption of older messages only, right? -- so no point having it without private key).
As a consequence, some tests broke -- for example, 'decrypt - load key - expired key' test. Why does this test load public expired key? Is there any sense in that?

@tomholub
Copy link
Collaborator

tomholub commented Dec 1, 2020

@tomholub I made some change that seemed logical to me.
That is, set usableButExpired to false for an expired encryption key missing private key parameters.
(It can be used only for decryption of older messages only, right? -- so no point having it without private key).

Sounds ok, but only for private key. If it's public key, and it contains the necessary information to encrypt for it, but it's just expired, then it should say true.

You could still -encrypt- for such -public- key if you pretend to encrypt at an older date. We have such mechanism where user can choose to do it, after they confirm a warning that it's not recommended.

As a consequence, some tests broke -- for example, 'decrypt - load key - expired key' test. Why does this test load public expired key? Is there any sense in that?

Sure, maybe you want to verify old signatures made by corresponding private key.

@rrrooommmaaa
Copy link
Contributor Author

You could still -encrypt- for such -public- key if you pretend to encrypt at an older date. We have such mechanism where user can choose to do it, after they confirm a warning that it's not recommended.

Ok then, I'll restore usableButExpired calculation. I just derived that logic from key-import-ui.

Sure, maybe you want to verify old signatures made by corresponding private key.

usableButExpired is deduced based on encryption key only (signing key isn't taken into consideration). Maybe it's time to introduce 2 properties: usableForEncryptionButExpired and usableForSigningButExpired?

@tomholub
Copy link
Collaborator

tomholub commented Dec 1, 2020

usableButExpired is deduced based on encryption key only (signing key isn't taken into consideration). Maybe it's time to introduce 2 properties: usableForEncryptionButExpired and usableForSigningButExpired?

good idea

@rrrooommmaaa rrrooommmaaa marked this pull request as ready for review December 1, 2020 17:27
@rrrooommmaaa rrrooommmaaa changed the title Show warnings on unusable keys (wip) Show warnings on unusable keys Dec 1, 2020
Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

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

Excellent. Thank you.

const key = keyToUpdate || {} as Key; // if no key to update, use empty object, will get props assigned below
const encryptionKey = await Catch.undefinedOnException(opgpKey.getEncryptionKey());
const getEncryptionKey = (keyid?: OpenPGP.Keyid | null, date?: Date, userId?: OpenPGP.UserId | null) =>
opgpKey.getEncryptionKey(keyid, date, userId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Much better! (for me)

const encryptionKey = await Catch.undefinedOnException(opgpKey.getEncryptionKey());
const getEncryptionKey = (keyid?: OpenPGP.Keyid | null, date?: Date, userId?: OpenPGP.UserId | null) =>
opgpKey.getEncryptionKey(keyid, date, userId);
const encryptionKeyIgnoringExpiration = encryptionKey ? encryptionKey : await OpenPGPKey.getKeyIgnoringExpiration(getEncryptionKey, exp, expired);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use a ?? operator (or was it ?: ?) which typescript will transpile as what you wrote.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just forgot about this operator 😊

@tomholub tomholub merged commit f6c5e33 into master Dec 1, 2020
@tomholub tomholub deleted the issue-2887-unusable-key-warning branch December 1, 2020 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

warn during import when key unusable for signing or encryption (Missing private key parameters)

2 participants