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

ECDSA Support #925

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

ECDSA Support #925

wants to merge 5 commits into from

Conversation

hamano
Copy link

@hamano hamano commented Nov 22, 2021

I just added ECDSA support with elliptic.

Supported algo:

  • secp192r1, prime192v1
  • secp256r1, prime256v1
  • secp224r1
  • secp384r1
  • secp521r1
  • secp256k1

Features:

  • key generation
  • DER/PEM encoding/decoding
  • sign and verify
  • ECDSA certifiate validation

TODO:

  • derivation public key from private key

@msalado
Copy link

msalado commented Jan 12, 2022

Hi @hamano

I've tried this code:

`var keys = forge.pki.ecdsa.generateKeyPair({name:"p256"});

let privateKeyToPem = keys.privateKey.toPem();
let publicKeyToPem = keys.publicKey.toPem();
console.log('Key-pair created. privateKey' + privateKeyToPem);
console.log('Key-pair created. publicKey' + publicKeyToPem);
`

But the key length p256 seems to be smaller as compared to the ones generated with openssl

Keys generated with the forge:
`
Key-pair created. privateKey-----BEGIN EC PRIVATE KEY-----
MDECAQEEINQYnDIgVwCD881LL5NYArpFs9oZQZfWedn/X6ZfAL5loAoGCCqGSM49
AwEH
-----END EC PRIVATE KEY-----

Key-pair created. publicKey-----BEGIN PUBLIC KEY-----
MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEZtBpf6rIVFsTWb9ss1uph5ECNowo
paijMJ6Ki/NrIeSQ38EwIfu7125b00H40EkyKrTEkIUFOscJppX0BG6/2g==
-----END PUBLIC KEY-----
`

openssl output:
-----BEGIN EC PARAMETERS----- BggqhkjOPQMBBw== -----END EC PARAMETERS----- -----BEGIN EC PRIVATE KEY----- MHcCAQEEIAHtTd1ScmU2mX3l/JayWj03hoOw97wE8XB0RAJ3HQAgoAoGCCqGSM49 AwEHoUQDQgAEMlXaf/9rgoOhqyUzYwKYE3ca5u4S7O8JsHT9Wrrvzex+L2Rn3NFs ZkLT688ySc9MiKaIVcpwcadbp2vAPxmrFg== -----END EC PRIVATE KEY-----


Also the CSR generation is failing, cause the CSR generation seems to be not be implemented

@cipherboy
Copy link

cipherboy commented Jan 12, 2022

Hi @msalado,

That appears to be because per RFC 5915 Section 3, the code in this PR appears to not encode the public key in the private key (marked optional per spec link above). Which, from my reading of the RFC, is within spec.

See e.g., https://github.com/digitalbazaar/forge/pull/925/files#diff-f515e671436f66ff2118cc1d6c71dd3978438e8a27094fffd057568ed129f64fR87 and https://github.com/digitalbazaar/forge/pull/925/files#diff-f515e671436f66ff2118cc1d6c71dd3978438e8a27094fffd057568ed129f64fR374 -- the latter in particular doesn't include the optional public key field.

Also, when using a named curve, my understanding is that the EC PARAMETERS block is optional (see e.g., RFC 5840) when the curve is a known, named curve. Indeed, the blob you include above merely decodes to the OID of P-256.

@msalado
Copy link

msalado commented Jan 13, 2022

Hi @cipherboy,

Thank you that makes sense, in fact we used the key to generate a CSR using openssl and it seems to work:

`$ openssl req -new -key test.key -nodes -out test.csr -subj "/CN=test/O=testing/C=MX"

root@ubuntu:/home/san# openssl req -in test.csr -text
Certificate Request:
Data:
Version: 1 (0x0)
Subject: CN = test, O = testing, C = MX
Subject Public Key Info:
Public Key Algorithm: id-ecPublicKey
Public-Key: (256 bit)
pub:
04:66:d0:69:7f:aa:c8:54:5b:13:59:bf:6c:b3:5b:
a9:87:91:02:36:8c:28:a5:a8:a3:30:9e:8a:8b:f3:
6b:21:e4:90:df:c1:30:21:fb:bb:d7:6e:5b:d3:41:
f8:d0:49:32:2a:b4:c4:90:85:05:3a:c7:09:a6:95:
f4:04:6e:bf:da
ASN1 OID: prime256v1
NIST CURVE: P-256
Attributes:
a0:00
Signature Algorithm: ecdsa-with-SHA256
30:44:02:20:6a:87:fc:37:0c:db:8d:85:f0:f6:9f:ce:c2:a5:
71:d4:17:9b:c4:d2:fa:55:cf:5f:58:ce:7f:53:26:df:e5:bd:
02:20:40:a6:7a:0c:64:db:6d:7e:3a:d5:96:b0:1c:e1:3f:7a:
5c:10:3b:cf:28:00:e6:47:b8:58:ce:59:3f:40:30:94
-----BEGIN CERTIFICATE REQUEST-----
MIHoMIGQAgEAMC4xDTALBgNVBAMMBHRlc3QxEDAOBgNVBAoMB3Rlc3RpbmcxCzAJ
BgNVBAYTAk1YMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEZtBpf6rIVFsTWb9s
s1uph5ECNowopaijMJ6Ki/NrIeSQ38EwIfu7125b00H40EkyKrTEkIUFOscJppX0
BG6/2qAAMAoGCCqGSM49BAMCA0cAMEQCIGqH/DcM242F8PafzsKlcdQXm8TS+lXP
X1jOf1Mm3+W9AiBApnoMZNttfjrVlrAc4T96XBA7zygA5ke4WM5ZP0AwlA==
-----END CERTIFICATE REQUEST-----
`

Do you know how we may do the same with the forge library (using private key generated with "ecdsa" implemented by @hamano )

I modified the create-csr.js file like this:

`
var forge = require('..');

console.log('Generating 1024-bit key-pair...');
var keys = forge.pki.ecdsa.generateKeyPair();

let privateKeyToPem = keys.privateKey.toPem();
let publicKeyToPem = keys.publicKey.toPem();
console.log('Key-pair created. privateKey' + privateKeyToPem);
console.log('Key-pair created. publicKey' + publicKeyToPem);

console.log('Creating certification request (CSR) ...');
var csr = forge.pki.createCertificationRequest();
csr.publicKey = keys.publicKey;
csr.setSubject([{
name: 'commonName',
value: 'example.org'
}, {
name: 'countryName',
value: 'US'
}, {
shortName: 'ST',
value: 'Virginia'
}, {
name: 'localityName',
value: 'Blacksburg'
}, {
name: 'organizationName',
value: 'Test'
}, {
shortName: 'OU',
value: 'Test'
}]);
// add optional attributes
csr.setAttributes([{
name: 'challengePassword',
value: 'password'
}, {
name: 'unstructuredName',
value: 'My company'
}]);

// sign certification request
csr.sign(keys.privateKey/, forge.md.sha256.create()/);
console.log('Certification request (CSR) created.');

// PEM-format keys and csr
var pem = {
privateKey: privateKeyToPem,
publicKey: publicKeyToPem,
csr: forge.pki.certificationRequestToPem(csr)
};

console.log('\nKey-Pair:');
console.log(pem.privateKey);
console.log(pem.publicKey);

console.log('\nCertification Request (CSR):');
console.log(pem.csr);

// verify certification request
try {
if(csr.verify()) {
console.log('Certification request (CSR) verified.');
} else {
throw new Error('Signature not verified.');
}
} catch(err) {
console.log('Certification request (CSR) verification failure: ' +
JSON.stringify(err, null, 2));
}

`

But I get the following error
`
Key-pair created. privateKey-----BEGIN EC PRIVATE KEY-----
MDECAQEEIJvjyKawPBDQSFCRPxr21ADIdQVjHUL2oW8mc9koiXLWoAoGCCqGSM49
AwEH
-----END EC PRIVATE KEY-----

Key-pair created. publicKey-----BEGIN PUBLIC KEY-----
MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE7gwPsSiXdVy6remOaS59vqWUEpBe
j+foESrU7mtZ6BqjCgRruG8qcnREilkFs3ObYipTmf3HPPfjyZdzoVxMpA==
-----END PUBLIC KEY-----

Creating certification request (CSR) ...
/Users/msalado/git/forge-hamano/lib/rsa.js:1721
var hex = b.toString(16);
^

TypeError: Cannot read properties of undefined (reading 'toString')
at _bnToBytes (/Users/msalado/git/forge-hamano/lib/rsa.js:1721:15)
at Object.pki.publicKeyToRSAPublicKey (/Users/msalado/git/forge-hamano/lib/rsa.js:1430:7)
at Object.pki.publicKeyToAsn1.pki.publicKeyToSubjectPublicKeyInfo (/Users/msalado/git/forge-hamano/lib/rsa.js:1413:11)
at Object.pki.getCertificationRequestInfo (/Users/msalado/git/forge-hamano/lib/x509.js:2603:9)
at Object.csr.sign (/Users/msalado/git/forge-hamano/lib/x509.js:1828:40)
at Object. (/Users/msalado/git/forge-hamano/examples/create-csr.js:43:5)
at Module._compile (node:internal/modules/cjs/loader:1101:14)
at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
at Module.load (node:internal/modules/cjs/loader:981:32)
at Function.Module._load (node:internal/modules/cjs/loader:822:12)

`

@hamano
Copy link
Author

hamano commented Jan 14, 2022

@msalado
The OpenSSL EC private key format includes the public key, but it is optional in the specification.
I have listed the EC private key structure in the comments.

/*
 * RCF5915: Elliptic Curve Private Key Format
 * https://datatracker.ietf.org/doc/html/rfc5915
 *
 * ECPrivateKey ::= SEQUENCE {
 *   version        INTEGER { ecPrivkeyVer1(1) } (ecPrivkeyVer1),
 *   privateKey     OCTET STRING,
 *   parameters [0] ECParameters {{ NamedCurve }} OPTIONAL,
 *   publicKey  [1] BIT STRING OPTIONAL
 * }
 */

You can modify to include the public key, however some people prefer a compact format.
We could add a switch flag to include the public key.
Thanks.

@hamano
Copy link
Author

hamano commented Jan 14, 2022

Many code still assume RSA keys and need to be modified for EC keys.
I think we need an abstraction of the key object.

ex:

  • ECublicKey.prototype.toAsn1(options)
  • RSAPublicKey.prototype.toAsn1(options)

@sanaullah82
Copy link

Thanks @hamano for the response.
Could you please let us know, how to generate the CSR ? as the code shared by @msalado is failing.

Creating certification request (CSR) ...
/Users/msalado/git/forge-hamano/lib/rsa.js:1721
var hex = b.toString(16);
^

TypeError: Cannot read properties of undefined (reading 'toString')
at _bnToBytes (/Users/msalado/git/forge-hamano/lib/rsa.js:1721:15)
at Object.pki.publicKeyToRSAPublicKey (/Users/msalado/git/forge-hamano/lib/rsa.js:1430:7)
at Object.pki.publicKeyToAsn1.pki.publicKeyToSubjectPublicKeyInfo (/Users/msalado/git/forge-hamano/lib/rsa.js:1413:11)
at Object.pki.getCertificationRequestInfo (/Users/msalado/git/forge-hamano/lib/x509.js:2603:9)
at Object.csr.sign (/Users/msalado/git/forge-hamano/lib/x509.js:1828:40)
at Object. (/Users/msalado/git/forge-hamano/examples/create-csr.js:43:5)
at Module._compile (node:internal/modules/cjs/loader:1101:14)
at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
at Module.load (node:internal/modules/cjs/loader:981:32)
at Function.Module._load (node:internal/modules/cjs/loader:822:12)

@tanmac2905
Copy link

hi @hamano Could you please let us know, how to using ECDSA in type script angular 12, i need read .csr file

@ghost
Copy link

ghost commented Apr 16, 2022

@hamano Could you rebase this PR?

@hamano
Copy link
Author

hamano commented Jul 8, 2022

@ayanamidev rebased

@stoprocent
Copy link

Hi what is a status on this one ? Can I help somehow to speed it up?

@dany74q
Copy link

dany74q commented Nov 7, 2022

+1, any updates on this ?

/cc @davidlehn, @dlongley

@dany74q
Copy link

dany74q commented Nov 22, 2022

Ping @davidlehn , @dlongley ❤️

@dany74q
Copy link

dany74q commented Feb 23, 2023

Ping @davidlehn, @dlongley (:

@dlongley
Copy link
Member

So I think the problem here is the use of elliptic which has been criticized over the years for not doing its best to use constant timing where possible. The ASN.1 / related work is great and useful though. The core crypto implementation here should be swapped out, however, with either WebCrypto or https://github.com/paulmillr/noble-curves. The former being the best option and the latter being a secure and audited JS implementation that can provide backup when WebCrypto isn't available.

@LunaSquee
Copy link

How far are we from having this merged?

@otimoshc
Copy link

otimoshc commented Mar 1, 2024

Thanks @hamano for the response. Could you please let us know, how to generate the CSR ? as the code shared by @msalado is failing.

Creating certification request (CSR) ...
/Users/msalado/git/forge-hamano/lib/rsa.js:1721
var hex = b.toString(16);
^

TypeError: Cannot read properties of undefined (reading 'toString')
at _bnToBytes (/Users/msalado/git/forge-hamano/lib/rsa.js:1721:15)
at Object.pki.publicKeyToRSAPublicKey (/Users/msalado/git/forge-hamano/lib/rsa.js:1430:7)
at Object.pki.publicKeyToAsn1.pki.publicKeyToSubjectPublicKeyInfo (/Users/msalado/git/forge-hamano/lib/rsa.js:1413:11)
at Object.pki.getCertificationRequestInfo (/Users/msalado/git/forge-hamano/lib/x509.js:2603:9)
at Object.csr.sign (/Users/msalado/git/forge-hamano/lib/x509.js:1828:40)
at Object. (/Users/msalado/git/forge-hamano/examples/create-csr.js:43:5)
at Module._compile (node:internal/modules/cjs/loader:1101:14)
at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
at Module.load (node:internal/modules/cjs/loader:981:32)
at Function.Module._load (node:internal/modules/cjs/loader:822:12)

@sanaullah82 I'm getting the same error, have you found a workaround?

@cottiar
Copy link

cottiar commented May 15, 2024

@hamano thanks for the initiative. My attempt to clone the repo for generating dist-minified files failed owing to npm dependency. I am trying to resolve the conflict, but, I was wondering if you have a latest min files that you could share as I use refer these via script tag directly? Please advise. TIA.

@01101sam
Copy link

01101sam commented Jun 6, 2024

Thanks @hamano for the response. Could you please let us know, how to generate the CSR ? as the code shared by @msalado is failing.

Creating certification request (CSR) ...
/Users/msalado/git/forge-hamano/lib/rsa.js:1721
var hex = b.toString(16);
^

TypeError: Cannot read properties of undefined (reading 'toString')
at _bnToBytes (/Users/msalado/git/forge-hamano/lib/rsa.js:1721:15)
at Object.pki.publicKeyToRSAPublicKey (/Users/msalado/git/forge-hamano/lib/rsa.js:1430:7)
at Object.pki.publicKeyToAsn1.pki.publicKeyToSubjectPublicKeyInfo (/Users/msalado/git/forge-hamano/lib/rsa.js:1413:11)
at Object.pki.getCertificationRequestInfo (/Users/msalado/git/forge-hamano/lib/x509.js:2603:9)
at Object.csr.sign (/Users/msalado/git/forge-hamano/lib/x509.js:1828:40)
at Object. (/Users/msalado/git/forge-hamano/examples/create-csr.js:43:5)
at Module._compile (node:internal/modules/cjs/loader:1101:14)
at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
at Module.load (node:internal/modules/cjs/loader:981:32)
at Function.Module._load (node:internal/modules/cjs/loader:822:12)

@sanaullah82 I'm getting the same error, have you found a workaround?

The whole code base is so old and need someone to pay the time to reconstruct it.
Therefore, only partial code is implemented for own use and willing to share (Like this one)
Thanks to @hamano , I can add more s*it code to make those function working.
Please see my code of this PR, and add your own solution.

For example, based on your traceback I suggest you to add this patch:

Location: https://github.com/01101sam/node-forge/blob/2bb97afb5058285ef09bcf1d04d6bd6b87cffd58/lib/x509.js#L2501-L2516

Diff Change
pki.getCertificationRequestInfo = function(csr) {
  // CertificationRequestInfo
  var cri = asn1.create(asn1.Class.UNIVERSAL, asn1.Type.SEQUENCE, true, [
    // version
    asn1.create(asn1.Class.UNIVERSAL, asn1.Type.INTEGER, false,
      asn1.integerToDer(csr.version).getBytes()),
    // subject
    _dnToAsn1(csr.subject),
    // SubjectPublicKeyInfo
-   csr.publicKey.toAsn1(),
    // attributes
    _CRIAttributesToAsn1(csr)
  ]);

  return cri;
};
pki.getCertificationRequestInfo = function(csr) {
  // CertificationRequestInfo
  var cri = asn1.create(asn1.Class.UNIVERSAL, asn1.Type.SEQUENCE, true, [
    // version
    asn1.create(asn1.Class.UNIVERSAL, asn1.Type.INTEGER, false,
      asn1.integerToDer(csr.version).getBytes()),
    // subject
    _dnToAsn1(csr.subject),
    // SubjectPublicKeyInfo
+   csr.publicKey instanceof ECPublicKey ? csr.publicKey.toAsn1() : pki.publicKeyToAsn1(csr.publicKey),
    // attributes
    _CRIAttributesToAsn1(csr)
  ]);

  return cri;
};

Remember, this is only a temporary solution, and I havn't test this patch.
If you get a new error please fix it yourself by looking at stack traceback.

Thank you 😄

Edit: My usage is mainly on x509, so you will see some code not suitable on that location, that's normal. My target isn't create a new PR for this PR.

@sibelius
Copy link

what plans to get this merged in?

@jkrajinovic
Copy link

+1

@defacto64
Copy link

Please merge this PR, there is a lot of people needing ECC support since a few years now....

@serdarde
Copy link

serdarde commented Nov 7, 2024

Many companies are switching to ECC because it offers both smaller key sizes and increased difficulty to break. For example, in Germany, the use of ECC will be mandatory starting in 2026. Adding ECC curves appears to be highly beneficial.

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.