Skip to content
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.

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

Switch to OpenPGP v5 #4725

Merged
merged 105 commits into from
Feb 7, 2023
Merged

Switch to OpenPGP v5 #4725

merged 105 commits into from
Feb 7, 2023

Conversation

rrrooommmaaa
Copy link
Contributor

@rrrooommmaaa rrrooommmaaa commented Sep 29, 2022

This PR switches to OpenPGP v5

close #3324
close #4904

Tests (delete all except exactly one):

  • Tests added or updated

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

@tomholub
Copy link
Collaborator

tomholub commented Oct 6, 2022

looking great, finally :-)

are we able to drop any functionality related to dropping weak algorithms? There should be settings on v5 to do this natively.

@tomholub
Copy link
Collaborator

@rrrooommmaaa how is this looking at this moment?

@rrrooommmaaa
Copy link
Contributor Author

rrrooommmaaa commented Dec 21, 2022

@rrrooommmaaa how is this looking at this moment?

I need to find a way to neatly wire the new library to the browser page, most of incompatibilities were resolved 2 months ago and unit tests are looking good.
I had to suspend the development in favor of reviews, now I'm able to finish it.

@rrrooommmaaa
Copy link
Contributor Author

While waiting for openpgpjs/openpgpjs#1583 to be merged and published, looking at other issues

const possibleExpirations: number[] = [];
const primaryKeyExpiration = OpenPGPKey.getExpirationAsDateOrUndefined(await key.getExpirationTime())?.getTime();
if (!encryptionKey || !signingKey) {
possibleExpirations.push(
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 reworked this method to make use of getSigningKey and getEncryptionKey calls from OpenPGP.js instead of trying to replicate OpenPGP.js behaviour when selecting keys, but it turned out to be not so simple. Supplying date: null (meaning: ignore date) parameter instead of date: undefined (meaning: take current date) it is possible to find a (sub)key for singing/encryption, but it may not be the subkey we're looking for ( 1) the subkey may have created property after primary key's expiration is in the key was never usable unit test, 2) the subkey may tell us an incorrect expiration date, as there may be another subkey with later expiration that didn't show up because OpenPGP.js sorts subkeys by created property)
So, for already expired keys the following algo is implemented:

  1. create a list of all the subkey's expiration dates (prior to primary key's expiration)
    (todo: we can make it faster by manually collecting expirations from signatures?)
  2. call getEncryptionKey/getSigningKey with dates from the list in descending order until we get a usable key.

If a not-expired encryption key is returned for the current date, then we'll call getEncryptionKey with the expiration date of the found key to find a next valid key to figure out the final expiration date over all the subkeys.
What do you think, @tomholub ? Is this approach good?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's a reasonable compromise to do it they way you did. It still relies on original functionality in OpenPGP.js, and the guess-try approach is understandable in this situation. You should comment it as such, eg by adding a code comment with a link to our conversation here.

In the long term, we should decide if we really want to support this use case or not, and maybe we'll drop it. These days I'm leaning towards dropping it, but not just yet - first we'll ensure that all possible avenues of getting updated keys are working properly (eg we didn't connect keys.openpgp.org yet).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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.

delta review - looks good

@rrrooommmaaa rrrooommmaaa marked this pull request as ready for review February 7, 2023 08:44
@rrrooommmaaa
Copy link
Contributor Author

Please also do rudimentary manual test on Firefox, to double check nothing is horribly broken, like the content script not loading or not working properly, and setup/send/receive emails with attachments.

I tested the extension in Firefox and it seems to setup and send/receive messages with attachments ok

@tomholub tomholub merged commit 2874d7f into master Feb 7, 2023
@tomholub tomholub deleted the issue-3324-openpgp-v5 branch February 7, 2023 10:11
@sosnovsky
Copy link
Collaborator

@rrrooommmaaa I tried to build extension with npm run build from master branch, but it fails with error after this merge

+ mkdir -p ./build/generic-extension-wip/lib/streams
+ cp node_modules/@openpgp/web-stream-tools/lib/node-conversions.js node_modules/@openpgp/web-stream-tools/lib/reader.js node_modules/@openpgp/web-stream-tools/lib/streams.js node_modules/@openpgp/web-stream-tools/lib/util.js node_modules/@openpgp/web-stream-tools/lib/writer.js ./build/generic-extension-wip/lib/streams
+ sed -i -E 's/'\''\.\/(streams|util|writer|reader|node-conversions)'\''/'\''\.\/\1\.js'\''/g' ./build/generic-extension-wip/lib/streams/node-conversions.js ./build/generic-extension-wip/lib/streams/reader.js ./build/generic-extension-wip/lib/streams/streams.js ./build/generic-extension-wip/lib/streams/util.js ./build/generic-extension-wip/lib/streams/writer.js
sed: 1: "s/'\.\/(streams|util|wr ...": \1 not defined in the RE

Should I change something to make it work?

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.

could key be sent to background page binary / unarmored? switch browser extension to OpenPGP v5
4 participants