Open alexkravets opened 3 years ago
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.
Are you saying we should be checking array length here?
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 ?
@alexkravets thanks, can you provide example JSON / hex?
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)
});
@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')
})
@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.
K, I’ll test it out tomorrow. Thank you for addressing these issues!
On 9 Feb 2021, at 00:04, Orie Steele notifications@github.com 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.
@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
@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.
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.
@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.
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.
hmm, probably an issue with peerDependencies... please open a separate issue.
While testing secp256k1 I'm randomly bumping into an issue with private key size:
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#L254Seems 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
?