ES256K (secp256k1) signature #278
Conversation
|
|
go.mod
Outdated
| @@ -3,6 +3,7 @@ module github.com/square/go-jose/v3 | |||
| go 1.12 | |||
|
|
|||
| require ( | |||
| github.com/btcsuite/btcd v0.20.1-beta | |||
There was a problem hiding this comment.
Is there another package implementing secp256k1 we could use here? I'm a bit wary of having this library depend on the entirety of btcd, it'd be nice if there was just a package for the curve split out somewhere.
There was a problem hiding this comment.
What about https://github.com/ethereum/go-ethereum/tree/master/crypto/secp256k1? I used this to extend x509 to support der/pem encoding for secp256k1.
There was a problem hiding this comment.
I think that'd be a better package to use. Requires CGO, so just need to make sure we use build tags to ensure we can still compile with CGO disabled, but I think this is much better than the btcd package.
There was a problem hiding this comment.
Though also again like the btcd version we'd want to pull in only the secp256k1 implementation as a dep, not the whole of go-ethereum.
There was a problem hiding this comment.
I agree. You'd need only the secp256k1 part, not the whole go-ethereum lib.
There was a problem hiding this comment.
Since this is (at least) a 2nd case where the secp256k1 is needed separately, we could put it in a separate repo -- it would require to follow the new updates on go-ethereum.
There was a problem hiding this comment.
@csstaub @alenhorvat https://github.com/decred/dcrd has only crypto stuff, not the whole btcd project, this PR has been updated with it.
It's also written 100% in Go, so no C dependencies needed to build it. This is the main reason why we chose this package in this PR. We don't want any external build dependency to ease the building process of go-jose and our projects.
There was a problem hiding this comment.
or a more proper one would be: https://github.com/btcsuite/btcd
|
Hi @kdimak, thanks for the pull request! I have some concerns about depending on btcd for this, because it's quite a large dependency to force on downstream users. Would prefer if we could use a purpose-built package just for secp256k1. |
|
Hi @csstaub , https://github.com/decred/dcrd/tree/master/dcrec was recommended (btcsuite/btcd#1495 (comment)) as it's "maintained by the original btcsuite authors and it provides a module". I've updated the PR to have a dependency on that library solely, please take a look. |
cryptosigner/cryptosigner_test.go
Outdated
| @@ -26,12 +26,13 @@ import ( | |||
| "fmt" | |||
| "testing" | |||
|
|
|||
| btcec "github.com/decred/dcrd/dcrec/secp256k1/v2" | |||
There was a problem hiding this comment.
Rename this import? Since it's not btcec anymore, upstream package name is secp256k1.
jose-util/generate.go
Outdated
| @@ -26,19 +26,21 @@ import ( | |||
| "errors" | |||
| "fmt" | |||
|
|
|||
| btcec "github.com/decred/dcrd/dcrec/secp256k1/v2" | |||
There was a problem hiding this comment.
Same comment as above applies here, let's rename the import to secp256k1.
jwk.go
Outdated
| @@ -33,6 +33,7 @@ import ( | |||
| "reflect" | |||
| "strings" | |||
|
|
|||
| btcec "github.com/decred/dcrd/dcrec/secp256k1/v2" | |||
There was a problem hiding this comment.
Again let's rename this import to secp256k1.
jwk.go
Outdated
| @@ -454,6 +455,8 @@ func (key rawJSONWebKey) ecPublicKey() (*ecdsa.PublicKey, error) { | |||
| switch key.Crv { | |||
| case "P-256": | |||
| curve = elliptic.P256() | |||
| case "secp256k1": | |||
There was a problem hiding this comment.
Huh, is this definitely the right string? Maybe add a comment here and quote the appropriate RFC to make sure people reading this code in the future won't be confused about the dissonance.
There was a problem hiding this comment.
Yeah, I will add a reference to https://tools.ietf.org/html/draft-ietf-cose-webauthn-algorithms-03
shared.go
Outdated
| @@ -23,6 +23,8 @@ import ( | |||
| "errors" | |||
| "fmt" | |||
|
|
|||
| btcec "github.com/decred/dcrd/dcrec/secp256k1/v2" | |||
signing_test.go
Outdated
| @@ -26,6 +26,8 @@ import ( | |||
| "reflect" | |||
| "testing" | |||
|
|
|||
| btcec "github.com/decred/dcrd/dcrec/secp256k1/v2" | |||
shared.go
Outdated
| @@ -495,6 +498,8 @@ func curveName(crv elliptic.Curve) (string, error) { | |||
| return "P-384", nil | |||
| case elliptic.P521(): | |||
| return "P-521", nil | |||
| case btcec.S256(): | |||
| return "secp256k1", nil | |||
There was a problem hiding this comment.
Add comment here, like // sic! maybe. Similar to what we do for the 512/521 bits.
…the SHA-256 cryptographic hash function.
|
Thank you @kdimak for this contribution. I've taken out a PR with your commit on new repo here go-jose/go-jose#14. I intend to use this in my project whether or not it gets merged. |
Added ES256K algorithm which uses ECDSA with the secp256k1 curve and the SHA-256 cryptographic hash function.
Fixes #263 in accordance with https://tools.ietf.org/html/draft-ietf-cose-webauthn-algorithms-03 (see comment).