lestrrat-go / jwx

Implementation of various JWx (Javascript Object Signing and Encryption/JOSE) technologies
MIT License
1.96k stars 165 forks source link

jws.Sign() allows jwa.RS256 alg when using ECDSA key #996

Closed eest closed 1 year ago

eest commented 1 year ago

Describe the bug

I noticed that jws.Sign() does not appear to get upset if passed a jws.WithKey(jwa.RS256, jwsSigningKey) where jwsSigningKey contains a *ecdsa.PrivateKey

$ go version
go version go1.21.3 darwin/arm64

To Reproduce / Expected behavior

package main

import (
    "crypto/ecdsa"
    "crypto/elliptic"
    "crypto/rand"
    "fmt"
    "log"

    "github.com/lestrrat-go/jwx/v2/jwa"
    "github.com/lestrrat-go/jwx/v2/jws"
)

func main() {
    jwsSigningKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
    if err != nil {
        log.Fatal(err)
    }

    signedMessage, err := jws.Sign([]byte("test message"), jws.WithKey(jwa.RS256, jwsSigningKey))
    if err != nil {
        log.Fatal(err)
    }

    fmt.Println(string(signedMessage))

    verifiedContent, err := jws.Verify(signedMessage, jws.WithKey(jwa.ES256, &jwsSigningKey.PublicKey))
    if err != nil {
        log.Fatalf("failed to verify message: %s", err)
    }

    fmt.Printf("verified content: %s\n", string(verifiedContent))
}

Running it:

go run .
eyJhbGciOiJSUzI1NiJ9.dGVzdCBtZXNzYWdl.MEYCIQDGAqlqYaAvfoxgzfvazNqcVhy-1EmKpTtAYrWNdSkWUwIhAOKi3NADH2_hNccbq8PaIE4YdoAUyanoPbe3d3tSmgpe
2023/10/17 23:08:45 failed to verify message: could not verify message using any of the signatures or keys
exit status 1

While Verify() fails it surprises me that Sign() did not return an error when mixing an RSA-based alg with an ECDSA key. Is this expected?

If I change jwa.RS256 to jwa.ES256 like so:

signedMessage, err := jws.Sign([]byte(message), jws.WithKey(jwa.ES256, jwsSigningKey))

... then Verify() is also happy as expected:

go run .
eyJhbGciOiJFUzI1NiJ9.dGVzdCBtZXNzYWdl.0_PZhDXilhhVHp1FKZTPDrLry3dAXuP_DYlt9N0TOhJSeIp6j2yUgCbkN_yiK7-gs1zsPyT1v_sHHGX-E84LrQ
verified content: test message
lestrrat commented 1 year ago

Oh, hmmmmmmmm. This one's a bit tricky... See, we allow crypto.Signer to be passed as key, so that hardware based keys and KMSes can do their thing. The problem here is that jwk.ECDSA(Private|Public)Key is also a crypto.Signer.

let me think about it

lestrrat commented 1 year ago

After some digging around, I've concluded that I can protect users from misusing KNOWN key types (i.e. those that are defined in Go std lib and within this package, but not for other key types that implement crypto.Signer. I added some notes in the jws.WithKey documentation as well.

Please check #997 and let me know

lestrrat commented 1 year ago

Forgot to mention: @eest (just making sure...)

eest commented 1 year ago

That was some impressive response time :)

I updated my go.mod in order to test your fix

require github.com/lestrrat-go/jwx/v2 gh-996

And now:

go run .
2023/10/18 09:04:00 failed to generate signature for signer #0 (alg=RS256): failed to sign payload: cannot use key of type *ecdsa.PrivateKey to generate RSA based signatures
exit status 1

So it looks good to me, thanks!

lestrrat commented 1 year ago

@eest merged. Please note that I'm going to wait releasing it for a bit. Please point our go.mod to the commit SHA for the time being, until we make a release.

eest commented 1 year ago

Great! Even running on a currently released version is good enough for me right now, this was more of an unexpected behaviour when testing what happens when doing things wrong. I'll attempt to do things right instead :)