secp256k1: Bumping into private keys of 31 bytes in length

[alexkravets]

While testing secp256k1 I'm randomly bumping into an issue with private key size:

Error: Expected private key to be an Uint8Array with length 32

Size of the key in my case is 31. Looks to be related to this issue: https://stackoverflow.com/questions/62938091/why-are-secp256k1-privatekeys-not-always-32-bytes-in-nodejs

secp256k1 library throws the exception here: https://github.com/cryptocoinjs/secp256k1-node/blob/master/lib/index.js#L254

Seems like solution to this would add 00 to the beginning of the array: https://github.com/transmute-industries/did-key.js/blob/master/packages/secp256k1/src/keyUtils.ts#L175

Or should that be addressed in secp256k1-node?

[OR13]

hmm, I hate this curve... I tend to try to align with:

https://tools.ietf.org/html/rfc8812#section-3.1

   As a compressed point encoding
   representation is not defined for JWK elliptic curve points, the
   uncompressed point encoding defined there MUST be used.  The "x" and
   "y" values represented MUST both be exactly 256 bits, with any
   leading zeros preserved.  Other optional values such as "alg" MAY
   also be present.

If you are saying our library is generating private keys that are not 32 bytes its a bug in our library...

Our library should throw if you try to import a key that does not match rfc8812.... regardless of jwk, hex or base58 encoding.

See https://github.com/transmute-industries/did-key.js/blob/master/packages/secp256k1/src/Secp256k1KeyPair.ts#L9

Are you saying we should be checking array length here?

[alexkravets]

Hmm, it looks like we already doing this size check in the loop while generation. I'm bumping into this error for signDetached method:

  const jws = await signDetached(buffer, privateKeyJwk, {
    b64:  false,
    crit: [ 'b64' ],
    alg
  })

— with keys that have been generated by the same library. So it looks like something goes wrong here with privateKeyJwk to privateKeyBuffer conversion: https://github.com/transmute-industries/did-key.js/blob/master/packages/secp256k1/src/ES256K.ts#L43

Should we prepend leading zero here for 31 bytes buffers: https://github.com/transmute-industries/did-key.js/blob/master/packages/secp256k1/src/keyUtils.ts#L177 ?

[OR13]

@alexkravets thanks, can you provide example JSON / hex?

[OR13]

nvm :) was able to reproduce:

import * as keyUtils from '../keyUtils';
it('private keys are always 32 bytes', async () => {
    // this JWK generatetes 31 byte length private keys
    const example = {
        "kty": "EC",
        "crv": "secp256k1",
        "d": "H8IPdO0ZRDrma0Oc1ASGp4R-7ioP3HKC2o3dBYLHUg",
        "x": "F0UpAkmilL3GafMgs_NMLqwGpUYPEnFphet8wS21jMg",
        "y": "vTGyefjnlCj2-T7gYw3Det6m1UtDOfbB4CTzROlT6QA",
        "kid": "y3hMPnp_oK9BM_V9vXu0aErpbzz0mDKe7xjEG44doUg"
    }
    const privateKeyBuffer = keyUtils.privateKeyUInt8ArrayFromJwk(example);
    expect(privateKeyBuffer.length).toBe(32)
});

[alexkravets]

@OR13 yes, here is an example: 00cc29ad08a78a97a68f0164cece3d82d48f42c089937074a977eba21f4d94c6

Seed is generated via:

crypto.randomBytes(32).toString('hex')

This give a key pair that fails to sign:

const keyPair = await KeyPair.generate({
  secureRandom: () => Buffer.from('00cc29ad08a78a97a68f0164cece3d82d48f42c089937074a977eba21f4d94c6', 'hex')
})

[OR13]

@alexkravets this should be fixed in the latest unstable, but I will try and add your exact test case before closing this issue as resolved.

[alexkravets]

K, I’ll test it out tomorrow. Thank you for addressing these issues! - A

…

On 9 Feb 2021, at 00:04, Orie Steele ***@***.***> wrote:  @alexkravets this should be fixed in the latest unstable, but I will try and add your exact test case before closing this issue as resolved. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[alexkravets]

@OR13 is looks like we've changed the interface for verify method now:

const payload = await verify(token, publicKeyJwk)

Code above doesn't work anymore, as payload is true.

So in order to pull payload from JWT I have to do the following:

const { header: { kid }, payload: { iss: issuer } } = await decode(token, { complete: true })

// NOTE: use kid and issuer to get publicKeyJwk ...

const isValid = await verify(token, publicKeyJwk)

if (isValid) {
  return payload
}

throw new Error('Invalid token')

Is this intentional? Doesn't look very convenient for the cases when you have key on hands and need to verify and get payload. Looking at this as a reference for verify method: https://www.npmjs.com/package/jsonwebtoken#jwtverifytoken-secretorpublickey-options-callback

[OR13]

@alexkravets yes, my interpretation was that you wanted a consistent interface for both verify and verifyDetached.

Are you saying you want:

verify(jws, publicKey) => Promise<decodedPayload> | thrown error.
verifyDetached(jws, message, publicKey) => Promise<boolean>

It's easy for us to fix.

[alexkravets]

K, so in my project I was switching from ed25519 to secp256k1 — and everything was fine, except verifyDetached method, which was working differently for these two methods.

I'm not sure which implementation should be considered as primary.

You've made a change that makes it work as in ed25519, but as a result of this fix, now verify works differently which was returning payload for both ed25519 and secp256k1 before the fix.

In general, I think throwing exception is better, cause this way you can throw different reasons of verification failure. Otherwise, if function returns false — it's a guess work why it's failed.

[OR13]

@alexkravets I would generally advise against secp256k1 if you can avoid it...

I think for verifyDetached a boolean makes sense, and for consistency with other JWT libraries, maybe verify(jws) should return the decoded payload.

IMO, exceptions are less functional, and I would prefer to return a boolean for simple crypto operations... but verify(jws) is not really a simple crypto operation as it is implemented in most libraries...

Would this work for you:

verify(jws, publicKey) => Promise<decodedPayload> | thrown error.
verifyDetached(jws, message, publicKey) => Promise<boolean>

I can make sure its handled like this everywhere.

[alexkravets]

Yes, this would work. The reason why I've switched to secp256k1 is to use this library https://github.com/pedrouid/eccrypto-js which supports encrypt / decrypt on this key pair.

Btw, there is something going with dependencies, as having "@transmute/did-key-ed25519": "^0.2.1-unstable.34", — works fine, but when having just "@transmute/did-key-secp256k1": "^0.2.1-unstable.34", — throws some internal error. So I have to include both libraries in dependencies. LMK if I should create another issue for this.

[OR13]

hmm, probably an issue with peerDependencies... please open a separate issue.

[alexkravets]

Created here: #70