Verify base64-encoded detached payload

[ferdypruis]

Describe the bug
Using v1.2.21, when singing a detached payload the payload is base64 encoded before calculating the signature. But when verifying detached payload it is not encoded, causing verification errors for proper signatures.

To Reproduce / Expected behavior
This simple implementation fails, due to the signature being based on base64-encoded payload and the verification on the plain payload;

package main

import (
	"crypto/rand"
	"crypto/rsa"
	"log"

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

func main() {
	privkey, err := rsa.GenerateKey(rand.Reader, 2048)
	if err != nil {
		log.Printf("failed to create private key: %s", err)
		return
	}

	buf, err := jws.Sign(nil, jwa.RS256, privkey, jws.WithDetachedPayload([]byte("Lorem ipsum")))
	if err != nil {
		log.Printf("failed to sign payload: %s", err)
		return
	}

	_, err = jws.Verify(buf, jwa.RS256, &privkey.PublicKey, jws.WithDetachedPayload([]byte("Lorem ipsum")))
	if err != nil {
		log.Printf("failed to verify JWS message: %s", err)
		return
	}

	log.Println("ok")
}

Prints

2009/11/10 23:00:00 failed to verify JWS message: failed to verify message: crypto/rsa: verification error

Program exited.

I expect my own signature to verify.

The issue can be worked around by encoding the detached signature before providing it to jws.Verify;

package main

import (
	"crypto/rand"
	"crypto/rsa"
	"encoding/base64"
	"log"

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

func main() {
	privkey, err := rsa.GenerateKey(rand.Reader, 2048)
	if err != nil {
		log.Printf("failed to create private key: %s", err)
		return
	}

	buf, err := jws.Sign(nil, jwa.RS256, privkey, jws.WithDetachedPayload([]byte("Lorem ipsum")))
	if err != nil {
		log.Printf("failed to sign payload: %s", err)
		return
	}

	// base64-encode the payload
	encoded := func(src []byte) []byte {
		enc := base64.RawURLEncoding
		dst := make([]byte, enc.EncodedLen(len(src)))
		enc.Encode(dst, src)
		return dst
	}([]byte("Lorem ipsum"))

	_, err = jws.Verify(buf, jwa.RS256, &privkey.PublicKey, jws.WithDetachedPayload(encoded))
	if err != nil {
		log.Printf("failed to verify JWS message: %s", err)
		return
	}

	log.Println("ok")
}

Prints

2009/11/10 23:00:00 ok

Program exited.

While by providing the b64-header the encoding of the payload can be disabled and making Verify pass, I believe encoding is actually the correct behavior.
Therefor the fix should be with Verify also encoding the payload, for example like this;

diff --git a/jws/jws.go b/jws/jws.go
--- a/jws/jws.go	(revision b66a2cb442d2a9523f4338fd156f99cd3dec1c81)
+++ b/jws/jws.go	(date 1649413906853)
@@ -476,16 +476,6 @@
 		return nil, errors.Wrap(err, `failed extract from compact serialization format`)
 	}
 
-	verifyBuf := pool.GetBytesBuffer()
-	defer pool.ReleaseBytesBuffer(verifyBuf)
-
-	verifyBuf.Write(protected)
-	verifyBuf.WriteByte('.')
-	if len(payload) == 0 && ctx.detachedPayload != nil {
-		payload = ctx.detachedPayload
-	}
-	verifyBuf.Write(payload)
-
 	decodedSignature, err := base64.Decode(signature)
 	if err != nil {
 		return nil, errors.Wrap(err, `failed to decode signature`)
@@ -501,6 +491,20 @@
 		return nil, errors.Wrap(err, `failed to decode headers`)
 	}
 
+	verifyBuf := pool.GetBytesBuffer()
+	defer pool.ReleaseBytesBuffer(verifyBuf)
+
+	verifyBuf.Write(protected)
+	verifyBuf.WriteByte('.')
+	if len(payload) == 0 && ctx.detachedPayload != nil {
+		if getB64Value(hdr) {
+			payload = base64.Encode(ctx.detachedPayload)
+		} else {
+			payload = ctx.detachedPayload
+		}
+	}
+	verifyBuf.Write(payload)
+
 	if !ctx.useJKU {
 		if hdr.KeyID() != "" {
 			if jwkKey, ok := ctx.key.(jwk.Key); ok {

ps. The issue template refers a non-existing CONTRIBUTING.md

[ferdypruis]

Related to #541

[lestrrat]

Thanks, looking at it...

[lestrrat]

Everything checks out. I can't believe I didn't test this case :/

Thanks for reporting! Will be merging soon, and will fix that link

[lestrrat]

Oh, you have a PR? Should I wait?

[ferdypruis]

No PR, go ahead.

[lestrrat]

For the record I ported the equivalent test to develop/v2 branch, and it didn't cause any problems. I guess this was taken care of in v2 branches, as it did receive a fair bit of refactoring.

[lestrrat]

v1.2.22 has been released and my silly typo in the issue templates have been fixed. Thanks @ferdypruis

[ferdypruis]

That's quick! Much appreciated.