Issue/427 #484

Closed
wants to merge 14 commits into
from

Conversation

Projects
None yet
4 participants
@ismaelbej
Contributor

ismaelbej commented Aug 18, 2016

This is an implementation of RFC 6637 to partially fix issue #427.

A few remarks of the code:

  • Uses Elliptic for curve cryptography, but should be possible to add others backends like webcrypto or node's crypto.
  • It uses AES library/ to implement key wrapping. I've failed to find a suitable AES decrypt function for RFC 3394 in the current code. But should be easy to replace.

It was tested against GnuPG version v2.1.8 + libgcrypt v1.7.0-beta262, some other details are in the pull request README-ECC.md.

extra/bitcoin_interop.js
@@ -0,0 +1,66 @@
+var openpgp = require('openpgp');

This comment has been minimized.

@evilaliv3

evilaliv3 Aug 20, 2016

Contributor

This whole bitcoin example is nice but i would keep it out of the patch. We will document it in a different way.

Let's evaluate the removal before finishing the integration of the patch

@evilaliv3

evilaliv3 Aug 20, 2016

Contributor

This whole bitcoin example is nice but i would keep it out of the patch. We will document it in a different way.

Let's evaluate the removal before finishing the integration of the patch

@ismaelbej

This comment has been minimized.

Show comment
Hide comment
@ismaelbej

ismaelbej Aug 20, 2016

Contributor

Removed 'const' from unit test and dropped bitcoin example.

Contributor

ismaelbej commented Aug 20, 2016

Removed 'const' from unit test and dropped bitcoin example.

@@ -100,8 +104,13 @@ PublicKey.prototype.read = function (bytes) {
var p = 0;
for (var i = 0; i < mpicount && p < bmpi.length; i++) {
-
- this.mpi[i] = new type_mpi();
+ if ((this.algorithm === 'ecdsa' || this.algorithm === 'ecdh') && i === 0) {

This comment has been minimized.

@ismaelbej

ismaelbej Aug 20, 2016

Contributor

Here is the issue related to the assumption that key packets only contain MPI. In order to support Ed25519 also a similar change have to be made.

@ismaelbej

ismaelbej Aug 20, 2016

Contributor

Here is the issue related to the assumption that key packets only contain MPI. In order to support Ed25519 also a similar change have to be made.

@@ -374,6 +374,12 @@ function checkCleartextMessage(message) {
* Format user ids for internal use.
*/
function formatUserIds(options) {
+ if (!options.curve) {

This comment has been minimized.

@ismaelbej

ismaelbej Aug 20, 2016

Contributor

This is a work around because some tests do not expect extra parameters like 'curve' and 'material'.

@ismaelbej

ismaelbej Aug 20, 2016

Contributor

This is a work around because some tests do not expect extra parameters like 'curve' and 'material'.

+ material = options.material.subkey;
+ }
+ secretSubkeyPacket.algorithm = enums.read(enums.publicKey, subkeyType);
+ return secretSubkeyPacket.generate(options.numBits, options.curve, material);
}
function wrapKeyObject() {

This comment has been minimized.

@ismaelbej

ismaelbej Aug 20, 2016

Contributor

Here wrapKeyObject has to be modified to remove hash/encryption algorithms that should not be used with a given curve. For example a p521 key should not be used with sha1, but with sha256. Also prefer_hash_algorithm need to reflect that also.

@ismaelbej

ismaelbej Aug 20, 2016

Contributor

Here wrapKeyObject has to be modified to remove hash/encryption algorithms that should not be used with a given curve. For example a p521 key should not be used with sha1, but with sha256. Also prefer_hash_algorithm need to reflect that also.

@ismaelbej

This comment has been minimized.

Show comment
Hide comment
@ismaelbej

ismaelbej Jul 26, 2017

Contributor

Replying to @sanjanarajan who asked why did I choose an external AES library. In order to support ECDH We've to implement key wrapping algorithm from RFC 3394 which uses AES encrypt and AES decrypt functions.

At the time the repository didn't have AES decrypt only AES encrypt was present (https://github.com/Jaxx-io/openpgpjs/blob/issue/427/src/crypto/cipher/aes.js), so we use the fast and light implementation from https://github.com/cryptocoinjs/aes. Before sending our pull request v2.0 was released with the new addition of asmcrypt-lite I did try to use AES decrypt from there but was unsuccessful. So we send our pull request with the annotation that the additional dependency can be removed if AES decrypt was made available to use.

Contributor

ismaelbej commented Jul 26, 2017

Replying to @sanjanarajan who asked why did I choose an external AES library. In order to support ECDH We've to implement key wrapping algorithm from RFC 3394 which uses AES encrypt and AES decrypt functions.

At the time the repository didn't have AES decrypt only AES encrypt was present (https://github.com/Jaxx-io/openpgpjs/blob/issue/427/src/crypto/cipher/aes.js), so we use the fast and light implementation from https://github.com/cryptocoinjs/aes. Before sending our pull request v2.0 was released with the new addition of asmcrypt-lite I did try to use AES decrypt from there but was unsuccessful. So we send our pull request with the annotation that the additional dependency can be removed if AES decrypt was made available to use.

@evilaliv3

This comment has been minimized.

Show comment
Hide comment
@evilaliv3

evilaliv3 Jul 30, 2017

Contributor

@sanjanarajan i'm noticing that you are try to move forward the ECC implementation.

that is great! i'm interested to help retesting this and eventually study with you the possible adoption of WebCrypto as backend. are you already evaluating this possible backend alternative in your current implementation?

Contributor

evilaliv3 commented Jul 30, 2017

@sanjanarajan i'm noticing that you are try to move forward the ECC implementation.

that is great! i'm interested to help retesting this and eventually study with you the possible adoption of WebCrypto as backend. are you already evaluating this possible backend alternative in your current implementation?

@sanjanarajan

This comment has been minimized.

Show comment
Hide comment
@sanjanarajan

sanjanarajan Aug 1, 2017

Member

@evilaliv3 Yes, my current plan is to use a WebCrypto backend for the curves for which it is available, fall back to a Node Crypto backend when available, and use the elliptic backend as the last option. I have also done some refactoring to address the structure/style issues detailed in the previous comments. I am hoping to make a new pull request in the next week, looking forward to your feedback!

Member

sanjanarajan commented Aug 1, 2017

@evilaliv3 Yes, my current plan is to use a WebCrypto backend for the curves for which it is available, fall back to a Node Crypto backend when available, and use the elliptic backend as the last option. I have also done some refactoring to address the structure/style issues detailed in the previous comments. I am hoping to make a new pull request in the next week, looking forward to your feedback!

@bartbutler

This comment has been minimized.

Show comment
Hide comment
@bartbutler

bartbutler Feb 8, 2018

Member

Closing as another version of this PR has just been merged!

Member

bartbutler commented Feb 8, 2018

Closing as another version of this PR has just been merged!

@bartbutler bartbutler closed this Feb 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment