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

add secp256k1 support #31

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

Conversation

lemoustachiste
Copy link

Hey guys,

due to key management in the broader system I integrate with, I need to have secp256k1 support for multibase format.

I have started some work and this far I've managed to get a JWK serialized to multibase (both public and private key. I was able to confirm the public key multibase decodes back to the expected value in JWK with this library (https://github.com/public-square/jwk-multibase-key-converter-js), I couldn't test with the private key as it does not handle the d value (https://github.com/public-square/jwk-multibase-key-converter-js/blob/main/src/converters/ES256K.ts#L67).

However now as I'm trying to sign with ECDSA-SD-2023 and my secp256k1 multibase key, I am facing some expected hurdles, first of which is the lack of prefix (https://github.com/digitalbazaar/ecdsa-multikey/blob/main/lib/serialize.js#L51):

const spki = new Uint8Array(header.length + publicKey.length);
                                     ^

TypeError: Cannot read properties of undefined (reading 'length')
    at _rawToSpki (file:///Users/julien/work/ecdsa-multikey/lib/serialize.js:327:38)
    at _multikeyToSpki (file:///Users/julien/work/ecdsa-multikey/lib/serialize.js:278:10)
    at importKeyPair (file:///Users/julien/work/ecdsa-multikey/lib/serialize.js:152:16)
    at _createKeyPairInterface (file:///Users/julien/work/ecdsa-multikey/lib/index.js:131:21)
    at from (file:///Users/julien/work/ecdsa-multikey/lib/index.js:80:10)
    at fromJwk (file:///Users/julien/work/ecdsa-multikey/lib/index.js:95:10)
    at Module.from (file:///Users/julien/work/ecdsa-multikey/lib/index.js:62:14)
    at getKeyPair (/Users/julien/work/poc-ecdsa-sd-blockcerts/src/suite/ecdsa-sd-2023.ts:37:39)
    at EcdsaSd2023.sign (/Users/julien/work/poc-ecdsa-sd-blockcerts/src/suite/ecdsa-sd-2023.ts:65:27)
    at sign (/Users/julien/work/poc-ecdsa-sd-blockcerts/src/sign.ts:8:40)

I tried researching but I couldn't find something that resembles this information online, so that I could add it and move on to the next hurdle.

Could you explain to me what is the purpose of using SPKI here? And where I could find the expected value?

I'm not a trained cryptographer, so I'm mostly going forward with trial and error, but would be happy to contribute my work back into this library, and I'm hoping you could guide me towards a satisfactory point for both of us.

@lemoustachiste
Copy link
Author

ok I've found that SPKI does refer to Simple PKI but to https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/importKey#subjectpublickeyinfo

@lemoustachiste
Copy link
Author

Ok so reading through this: https://www.rfc-editor.org/rfc/rfc5480 there does not seem to be room for secp256k1.

I thought I could potentially bypass the SPKI code by first importing the key with the node:crypto implementation, but again this one does not support out of the box secp256k1 (and would require some polyfill?). The supported curves are listed https://developer.mozilla.org/en-US/docs/Web/API/EcKeyImportParams#namedcurve, however NIST allows the use of secp256k1 within a blockchain context, so there could be room there?

I'm not an expert at the nodejs internals, but it seems that the C code allows for secp256k1 export to JWK: https://github.com/nodejs/node/blame/9f6c12413cde5074893ffb378b8c3310275aa016/src/crypto/crypto_ec.cc#L809, but importing a secp256k1 key yields the following error:

[NotSupportedError]: Unrecognized namedCurve
    at Object.ecImportKey (node:internal/crypto/ec:162:11)
    at SubtleCrypto.importKey (node:internal/crypto/webcrypto:621:10)

Highlighting that lack of support.

What was @kezike's idea on how to add support for secp256k1? @noble/secp256k1 library does not seem to cover the initial interpretation of that key, and I couldn't find again the accepted prefix in this case which I could add to this library.

@dlongley
Copy link
Member

Hi, I don't have a lot of cycles to respond here at the moment, but the use of SPKI is just an internal implementation detail (IIRC) -- and the public interfaces work with raw (compressed) keys, multibase-encoded multikeys, and JWKs. To my knowledge supporting P256K/secp256k1 in this library shouldn't be a problem, we just haven't gotten around to it. I would expect some library like @noble/secp256k1 would be needed (minimally for the browser case since it's not available in WebCrypto).

It may be the case that we don't want to expose any of the secp256k1 keypairs using the CryptoKey interface (or that we'll have to polyfill it) -- so there may have to be a difference there for practicality purposes. Sorry for not having more time to look into this at the moment! A proposal / PR with something that seems workable enough, even if slightly different for secp256k1 might allow others to iterate and help get support in sooner.

@lemoustachiste
Copy link
Author

Hi @dlongley,

thanks for your answer. Would you still be able to help me figure out where this piece of information: https://github.com/digitalbazaar/ecdsa-multikey/blob/main/lib/serialize.js#L51 comes from? That would help piece things together.

@aljones15
Copy link
Contributor

aljones15 commented Jun 3, 2024

Hi @dlongley,

thanks for your answer. Would you still be able to help me figure out where this piece of information: https://github.com/digitalbazaar/ecdsa-multikey/blob/main/lib/serialize.js#L51 comes from? That would help piece things together.

I believe those are DER/ASN.1 prefixes for those keyTypes. They should match the values produced when exporting a key of that type in node using the key export interface https://nodejs.org/api/crypto.html#keyobjectexportoptions and the DER encoding not the PEM encoding (we usually prefer DER to PEM). The easiest way to figure out the prefix is generate a few keys, export them as DER and see what common prefix they have.

p.s. there is a comment a few lines down that suggests I might be partially correct:

/* Format:
SPKI DER PUBLIC KEY HEADER (w/algorithm OID for specific key type)
COMPRESSED / UNCOMPRESSED PUBLIC KEY BYTES */
const header = SPKI_PREFIXES.get(curve);

@dlongley
Copy link
Member

dlongley commented Jun 3, 2024

@lemoustachiste,

Yes, the SPKI_PREFIXES refer to data that is constant in the SubjectPublicKeyInfo (SPKI) formatted public key, which is expressed using DER (Distinguished Encoding Rules, i.e., a canonical form of the Basic Encoding Rules (BER)) for ASN.1 (Abstract Syntax Notation 1).

For more information on ECC SubjectPublicKeyInfo you can see:

https://www.rfc-editor.org/rfc/rfc5480.html#section-2

Notably, ECDSA has two different raw formats, an uncompressed format and a compressed one, each starting with a header identifying which form is used (and if the compressed form is used, whether the y coordinate of the public key is even or odd).

The 'raw' format output by WebCrypto (today at least) is the uncompressed form, so it needs to be converted to the compressed form for expression as a multikey. This isn't a complicated operation, it just means detecting whether y is even/odd by checking its last byte value and then setting the appropriate header. Here's a gist of how it works:

ECDSA raw => multikey:
https://gist.github.com/dlongley/3ca06233504a35e2fbd81ca1e9ea88de

The Ed25519 public key is much simpler, there is just one raw format, so you just export to raw and then add the multikey header:
https://gist.github.com/dlongley/6a34fa5315adf9c59e7de7cfbe6212e3

Note that for ECDSA, the SPKI format includes the compressed form of the key, making it easy to import a compressed public key into WebCrypto without having to perform any ECC math to uncompress the key. This is why our libraries import ECDSA keys from SPKI. To do so, you just need to inject the compressed key into what is essentially a SPKI DER template, as constructed by SPKI_PREFIXES. Hopefully this helps.

@lemoustachiste
Copy link
Author

lemoustachiste commented Jun 4, 2024

I was able to move forward with the public key (SPKI formatting and public key decompression) and I think I've managed it as expected, thanks to your comment as well as this one: https://stackoverflow.com/questions/67428299/how-to-do-ec-compression-on-a-public-key-in-python/67433909?noredirect=1#comment138519430_67433909. I've pushed the changes so you can take a look, at the end of it all I'll remove the trailing console.logs.

Now I'm facing the issue with the private key, trying to convert to PKCS 8 (https://github.com/digitalbazaar/ecdsa-multikey/blob/main/lib/serialize.js#L303). I'm not sure I should be using the same tool (https://lapo.it/asn1js/#) to get the same information. It looks like at least that the public key header is not consistent with the other headers defined already (https://github.com/digitalbazaar/ecdsa-multikey/blob/main/lib/serialize.js#L31), and I'm getting something for the private key but with no certainty that it is the expected value.

Do you know how I could easily gather the expected information, or if not easily it does not matter I can also achieve it if I have some guidelines.

Thanks

@lemoustachiste
Copy link
Author

Ok I'm getting closer but I need an external and experienced eye.

Using this private key which I believe I converted to PKCS#8 using openssl:

MIGEAgEAMBAGByqGSM49AgEGBSuBBAAKBG0wawIBAQQgTECmkTCUcLx6ps6peq4jRYSx2kwzF1LlTmTD
IqXS1CahRANCAAQTBU1DyMMTbUkwlulXLS1AvynKZvFpbCtpaF2/aEV0LZhcTgyOxSKWK8H2/wIx9X9l
/pFtcqLHSXazFNleNhM9

And opening it in ans1js parser: https://asn1js.eu/#MIGEAgEAMBAGByqGSM49AgEGBSuBBAAKBG0wawIBAQQgTECmkTCUcLx6ps6peq4jRYSx2kwzF1LlTmTDIqXS1CahRANCAAQTBU1DyMMTbUkwlulXLS1AvynKZvFpbCtpaF2_aEV0LZhcTgyOxSKWK8H2_wIx9X9l_pFtcqLHSXazFNleNhM9

I get the following ASN1 object:

PrivateKeyInfo SEQUENCE (3 elem)

    version Version INTEGER 0
    privateKeyAlgorithm AlgorithmIdentifier SEQUENCE (2 elem)
        algorithm OBJECT IDENTIFIER 1.2.840.10045.2.1 ecPublicKey (ANSI X9.62 public key type)
        parameters ANY OBJECT IDENTIFIER 1.3.132.0.10 secp256k1 (SECG (Certicom) named elliptic curve)
    privateKey PrivateKey OCTET STRING (109 byte) 306B02010104204C40A691309470BC7AA6CEA97AAE234584B1DA4C331752E54E64C32…
        SEQUENCE (3 elem)
            INTEGER 1
            OCTET STRING (32 byte) 4C40A691309470BC7AA6CEA97AAE234584B1DA4C331752E54E64C322A5D2D426
            [1] (1 elem)
                BIT STRING (520 bit) 0000010000010011000001010100110101000011110010001100001100010011011011…

Which translates to the following HEX value (in bold are the private key/public key info):

30 81 84 02 01 00 30 10 06 07 2A 86 48 CE 3D 02
01 06 05 2B 81 04 00 0A 04 6D 30 6B 02 01 01 04
20 4C 40 A6 91 30 94 70 BC 7A A6 CE A9 7A AE 23
45 84 B1 DA 4C 33 17 52 E5 4E 64 C3 22 A5 D2 D4
26
A1 44 03 42 00 04 13 05 4D 43 C8 C3 13 6D 49
30 96 E9 57 2D 2D 40 BF 29 CA 66 F1 69 6C 2B 69
68 5D BF 68 45 74 2D 98 5C 4E 0C 8E C5 22 96 2B
C1 F6 FF 02 31 F5 7F 65 FE 91 6D 72 A2 C7 49 76
B3 14 D9 5E 36 13 3D

Now, as the site handily allows to read the different blocks of data, I took the following sequence as the private key prefix:

30 81 84 02 01 00 30 10 06 07 2A 86 48 CE 3D 02 01 06 05 2B 81 04 00 0A 04 6D 30 6B 02 01 01 04 20

And for the public key prefix:
A1 44 03 42 00

which gives me the following uint8array data:

[ECDSA_CURVE.secp256k1, {
    secret: new Uint8Array([
      48, 129, 132, 2, 1, 0, 48, 16, 6,
      7, 42, 134, 72, 206, 61, 2, 1, 6,
      5, 43, 129, 4, 0, 10, 4, 109, 48,
      107, 2, 1, 1, 4, 32
    ]), // length == 33
    public: new Uint8Array([161, 68, 3, 66, 0]) // length == 5
  }]

Which altogether looks acceptable from my novice eye.

But when I try to execute the signature of ECDSA-SD-2023 with this key, I get the following error:

/Users/julien/work/ecdsa-multikey/node_modules/@peculiar/asn1-schema/build/cjs/parser.js:14
            throw new Error(asn1Parsed.result.error);
                  ^

Error: End of input reached before message was fully decoded (inconsistent offset and length values)
    at AsnParser.parse (/Users/julien/work/ecdsa-multikey/node_modules/@peculiar/asn1-schema/build/cjs/parser.js:14:19)
    at EcCrypto.importKey (/Users/julien/work/ecdsa-multikey/node_modules/@peculiar/webcrypto/build/webcrypto.js:1651:54)
    at EcdsaProvider.onImportKey (/Users/julien/work/ecdsa-multikey/node_modules/@peculiar/webcrypto/build/webcrypto.js:1748:36)
    at EcdsaProvider.importKey (/Users/julien/work/ecdsa-multikey/node_modules/webcrypto-core/build/webcrypto-core.js:232:33)
    at SubtleCrypto.importKey (/Users/julien/work/ecdsa-multikey/node_modules/webcrypto-core/build/webcrypto-core.js:1521:25)
    at importKeyPair (/Users/julien/work/ecdsa-multikey/lib/serialize.js:216:55)
    at async _createKeyPairInterface (file:///Users/julien/work/ecdsa-multikey/lib/index.js:132:15)
    at getKeyPair (/Users/julien/work/poc-ecdsa-sd-blockcerts/src/suite/ecdsa-sd-2023.ts:38:19)
    at EcdsaSd2023.sign (/Users/julien/work/poc-ecdsa-sd-blockcerts/src/suite/ecdsa-sd-2023.ts:88:21)
    at sign (/Users/julien/work/poc-ecdsa-sd-blockcerts/src/sign.ts:8:28)

I've tried investigating into the parser but I'm not exactly sure what's happening, here is the debug output I'm getting

L967 LocalLengthBlock {
  blockLength: 2,
  error: '',
  warnings: [],
  valueBeforeDecodeView: Uint8Array(0) [],
  isIndefiniteForm: false,
  longFormUsed: true,
  length: 132
}
Uint8Array(103) [
  // redacted to protect private key
  ... 3 more items
] 103 3 132 -32 

That's coming from this file https://github.com/PeculiarVentures/ASN1.js/blob/master/src/Constructed.ts#L25 (as published into the npm package, so build.js line 967).

It's taking this.lenBlock.length and the subtraction of all these value yields a negative number instead of 0, hence the error. I don't know nor understand where the 132 length comes from, but I'm obviously wrong somewhere.

Anything obvious to you guys?

@lemoustachiste lemoustachiste changed the title WIP: add secp256k1 support add secp256k1 support Jun 12, 2024
@lemoustachiste
Copy link
Author

@dlongley I was finally able to get to sign and verify with secp256k1.

what I was stuck on was the length of the global key, apparently the webcrypto polyfill only accepts uncompressed public keys so I needed to adjust the header accordingly.

At this time I am dynamically importing the webcrypto polyfill wherever it is required. I think it is generally cleaner as it is easily identifiable, rather than requiring on the fly somewhere else. From a bundling perspective it also means the polyfill is loaded side by side so not polluting a bundle when not needed.

I have added tests based on the ones already in place, they all pass.

I am also using this in the context of signing/verifying ECDSA-SD-2023, for which I had to allow specifying a "requiredAlgorithm" to match secp256k1 (I have settled on using K-256 as it is the one understood by the webcrypto polyfill).

All in all this is ready for review and publishing.

lib/constants.js Outdated Show resolved Hide resolved
lib/constants.js Outdated Show resolved Hide resolved
lib/constants.js Show resolved Hide resolved
Comment on lines +47 to +48
// ensure compatibility with https://github.com/PeculiarVentures/webcrypto
secp256k1: 'K-256'
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I'm not sure whether 'K-256' or 'secp256k1' is what is commonly used (as the string value), e.g., in JWKs. This is fine if it's the commonly used value, but we don't want to pick something just for compatibility with a particular library. We might need some additional translation code, I don't know. Just need a bit more research / link to RFCs, common libs (like panva's node.js jose lib) or some other docs in support of this approach. It's fine (but unfortunate) if we have to support two different expressions of this curve.

Copy link
Author

Choose a reason for hiding this comment

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

I agree, I haven't seen K-256 before and I initially named it secp256k1.
That naming convention will be important for future usage, I'll see what I can find about it.

Copy link
Author

Choose a reason for hiding this comment

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

Ok so I couldn't find a clear nomenclature definition about this.

However, section 2.1 of this file https://www.secg.org/sec2-v2.pdf does name k for Koblitz curve, and this paragraph has quite a sensible explanation about why it could be named K-256: https://lib.rs/crates/k256#readme-about-secp256k1-k-256.

But it seems the more common way is to call it secp256k1, but it's also not consistent with calling secp256r1 P-256.

I'll let you be the judge as this is ultimately your package, my inclination is to keep things simple with K-256 support only (and document it) as it is a valid and consistent naming, although a bit uncommon at this point in time.

Copy link
Author

Choose a reason for hiding this comment

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

In the NIST document about sanctioned algorithm, the nomenclature is indeed as detailed in the rust package's paragraph: https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-186.pdf (P, K, B).

Appendix H.2 mentions secp256k1 without explicitly naming it K-256.

return new Uint8Array(await webcrypto.subtle.sign(
algorithm, secretKey, data));
if(curve === ECDSA_CURVE.secp256k1) {
const {Crypto} = await import('@peculiar/webcrypto');
Copy link
Member

Choose a reason for hiding this comment

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

We will have to figure something else out here, I don't expect us to pull in this whole library nor list it as a dependency. I'm not sure what static analysis will do here either, even though this is "just in time" / dynamic imported. We have to think of this multikey library being included in places where the additional @peculiar/webcrypto dependency isn't needed and the security/auditing overhead/issues it creates. cc: @mattcollier @davidlehn

Instead, we could possibly allow for some API (interface) injection via a param where the key is imported/generated or an additional optional registration (aka use()) API as setup. Either that or just require the caller to have polyfilled this and provide some docs on how to do it. A proper polyfill installation would also remove all of the extra optional and duplicated code below (I'd hope).

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I agree,

However it has been stated that there is no intent to support secp256k1 in the webcrypto node implementation (not sure if I can find that conversation again).

DI would be good as a per customer basis, but the thing is it's not just about importing the library, it's also consuming it instead of the native webcrypto. I think there would be a bigger vulnerability risk by allowing injection of a webcrypto polyfill (which could overwrite any curve) than by fully controlling where, when and how it's used by a dynamic require.

Dynamic import in my experience is quite smart about being there but only used as needed, so not polluting other cases.

@dlongley
Copy link
Member

Still trying to get back to this, too many other items on the plate at the moment to look closely and get this integrated.

I do want to note that even once this is incorporated, its use with data integrity cryptosuites will be non-standard, since those focus on providing cryptosuites that are always NIST-approved for ECDSA (I will note that there is now a somewhat strange NIST carve-out for secp256k1 "but for blockchain applications only" that is not easy to deal with).

Regardless, non-standard use is still acceptable use for a lot of people, we just need to make sure software can be configured to tell the difference.

@lemoustachiste
Copy link
Author

FWIW, with a multikey representation made by this package (and PR), I was able to create, sign and verify a did:tdw using @noble/secp256k1, so it looks like the computations are correct.

@davidlehn
Copy link
Member

  • Please fix the npm run lint and npm test issues. We don't have actions setup to run those checks for external repo PRs at the moment. Merging this would trigger those issues.
  • I'd rebase on main too. There is some unreleased testing work there that hopefully will continue to work.

@lemoustachiste
Copy link
Author

@davidlehn done

@lemoustachiste
Copy link
Author

I have had some issues with the @peculiar/webcrypto polyfill in the browser, after pulling the thread I also found that the library was not recommended for production use at this stage, confirming your reluctance to use it.

Thus, I am currently trying to replace the secp256k1 logic with @noble/secp256k1 as mentioned in the early comments of the library (https://github.com/digitalbazaar/ecdsa-multikey/blob/main/lib/index.js#L21).

However the API is not the same, especially on the signature output, so I'm trying to reconcile it with the crypto signature output.

Would you mind taking a look at the problem stated there: paulmillr/noble-secp256k1#125 to see if:

  • A) we can just accommodate with the new output format and work with that (but it's completely different from the current output)
  • B) maybe you know the answer to the question and you can unstuck me?

Thanks

@dlongley
Copy link
Member

+1 for the answer to B) that was given in the linked issue.

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.

4 participants