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

crypto-refresh: add support for new Ed25519/X25519 keys, signatures and messages #1620

Merged
merged 6 commits into from Jul 26, 2023

Conversation

larabr
Copy link
Collaborator

@larabr larabr commented Mar 24, 2023

This addition is backwards compatible. We offer no way to generate v4 keys in the new format, since existing implementations might not support them.

TODO

  • update kdf to include public key
  • test with messages (to decrypt and verify) from gopenpgp
  • merge after introducing support for intended recipient (no longer needed after spec's kdf change)
  • enforce using AES (https://gitlab.com/openpgp-wg/rfc4880bis/-/merge_requests/276) (done in x448 PR, so it's only needed if we don't merge that on in v5)
  • merge lukas changes
  • change base branch (to be merged into v6) we merge to v5 as there are no breaking changes

@larabr larabr force-pushed the new-25519 branch 5 times, most recently from ed0b0e4 to c7003f9 Compare March 29, 2023 16:32
@larabr larabr requested a review from twiss March 29, 2023 17:02
@larabr larabr force-pushed the new-25519 branch 3 times, most recently from dd814b5 to 785b47b Compare April 25, 2023 09:08
src/type/mont_symkey.js Outdated Show resolved Hide resolved
src/type/ecdh_symkey.js Outdated Show resolved Hide resolved
This addition is backwards compatible. We offer no way to generate v4 keys in the new format.
Comment on lines +348 to +355
await Promise.all(encryptionKeys.map(key => key.getEncryptionKey()
.catch(() => null) // ignore key strength requirements
.then(maybeKey => {
if (maybeKey && (maybeKey.keyPacket.algorithm === enums.publicKey.x25519) && !util.isAES(algo)) {
throw new Error('Could not generate a session key compatible with the given `encryptionKeys`: X22519 keys can only be used to encrypt AES session keys; change `config.preferredSymmetricAlgorithm` accordingly.');
}
})
));
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be safer to just use AES128 in this scenario. After all, we're always allowed to use that, and the config is called preferredSymmetricAlgorithm, it's not a guarantee it will be used (and in this case we can just say that the key doesn't support it - although tbh it would be a bit strange if the key's algorithm preferences claim otherwise).

Copy link
Collaborator Author

@larabr larabr Jul 13, 2023

Choose a reason for hiding this comment

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

This is an edge case which requires passing config.preferredSymmetricAlgorithm with a non-AES algo as well as (all) encryptionKeys with that algo appearing in the prefs.
So I think a specific error can make sense in this case, also considering that AES-128 is not the best fallback for x488, so users might want to go with AES-256 at that point? 🤞

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, fair enough 👍

src/enums.js Outdated Show resolved Hide resolved
src/enums.js Outdated Show resolved Hide resolved
src/packet/public_key_encrypted_session_key.js Outdated Show resolved Hide resolved
src/packet/public_key_encrypted_session_key.js Outdated Show resolved Hide resolved
src/packet/public_key_encrypted_session_key.js Outdated Show resolved Hide resolved
src/type/mont_symkey.js Outdated Show resolved Hide resolved
src/type/mont_symkey.js Outdated Show resolved Hide resolved
src/type/mont_symkey.js Outdated Show resolved Hide resolved
test/karma.conf.js Outdated Show resolved Hide resolved
src/crypto/public_key/elliptic/ecdh_x.js Outdated Show resolved Hide resolved
Comment on lines +1 to +17
// OpenPGP.js - An OpenPGP implementation in javascript
// Copyright (C) 2018 Proton Technologies AG
//
// This library is free software; you can redistribute it and/or
// modify it under the terms of the GNU Lesser General Public
// License as published by the Free Software Foundation; either
// version 3.0 of the License, or (at your option) any later version.
//
// This library is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
// Lesser General Public License for more details.
//
// You should have received a copy of the GNU Lesser General Public
// License along with this library; if not, write to the Free Software
// Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// OpenPGP.js - An OpenPGP implementation in javascript
// Copyright (C) 2018 Proton Technologies AG
//
// This library is free software; you can redistribute it and/or
// modify it under the terms of the GNU Lesser General Public
// License as published by the Free Software Foundation; either
// version 3.0 of the License, or (at your option) any later version.
//
// This library is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
// Lesser General Public License for more details.
//
// You should have received a copy of the GNU Lesser General Public
// License along with this library; if not, write to the Free Software
// Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was already there, I just renamed the file. Maybe we should review these copyright notes in a separate PR?

src/crypto/hkdf.js Outdated Show resolved Hide resolved

const webCrypto = util.getWebCrypto();
const nodeCrypto = util.getNodeCrypto();
const nodeSubtleCrypto = nodeCrypto && nodeCrypto.webcrypto && nodeCrypto.webcrypto.subtle;
Copy link
Member

Choose a reason for hiding this comment

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

(For now this is fine but maybe long-term we can just make getWebCrypto return Node's Web Crypto too? And then the code can decide by the ordering which they prefer between Node Crypto and Node Web Crypto.)

larabr and others added 5 commits July 25, 2023 10:17
As specified in openpgp-crypto-refresh-09.

Instead of encoding the symmetric key algorithm in the PKESK ciphertext (requiring padding),
the symmetric key algorithm is left unencrypted.

Co-authored-by: Lukas Burkhalter <lukas.burkhalter@proton.ch>
Fail on PKESK parsing as well as session key generation and encryption
Following the addition of the new format for Montgomery curves,
which do not rely on OIDs.
SubtleCrypto not available in the latter due to stricter secure context checks
@larabr larabr merged commit 8d4dd34 into openpgpjs:main Jul 26, 2023
12 checks passed
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.

None yet

2 participants