Support for detached payload in jws.Sign()

[stevenvegt]

Hello! I'm not sure if this is a bug, or just lack of support for detached payloads in a JWS.

Describe the bug

When making a detached signature, the payload does not have to be encoded. As is specified here:
RFC7797 Section 5.1:

Appendix F of [JWS] describes how to represent JWSs with detached
content. A detached payload can contain any octet sequence
representable by the application. The payload value will not cause
problems parsing the JWS, since it is not represented as part of the
JWS. If an application uses a content encoding when representing the
payload, then it MUST specify whether the signature or MAC is
performed over the content-encoded representation or over the
unencoded content.

However, the Signing method checks for a dot (.) in the payload which makes it impossible to create a detached signature with a raw byte payload from a hash function.

jwx/jws/message.go

Lines 138 to 145 in 3774a6e

	if getB64Value(hdrs) {
	buf.WriteString(base64.EncodeToString(payload))
	} else {
	if bytes.ContainsRune(payload, '.') {
	return nil, nil, errors.New(`payload must not contain a "." when b64 = false`)
	}
	buf.Write(payload)
	}

Go Version:
go version go1.17.2 darwin/arm64

To Reproduce / Expected behavior

headers := jws.NewHeaders()
headers.Set("b64", false)
headers.Set("crit", []string{"b64"})

payload := "."
data, err := jws.Sign(payload, "ES256", privateKey, jws.WithHeaders(headers))

This gives the errors.New(`payload must not contain a "." when b64 = false`)

A possible change to make Sign compatible with detached signatures is a option like:

data, err := jws.Sign(payload, "ES256", privateKey, jws.WithHeaders(headers), jws.WithDetachedPayload())

[lestrrat]

@stevenvegt This seems like a bug. I have a feeling I messed up on one of the tests, and its effect kind of just rippled elsewhere.

Can you please try out https://github.com/lestrrat-go/jwx/tree/gh-541?

FYI: It's my bed time where I live, so my next response will take a while

[lestrrat]

@stevenvegt Please let me know if this worked? Thanks

[stevenvegt]

Thanks for the quick response! The suggested code works! However, I don't think your original code was wrong. It covered the Section 5.2:

When using the JWS Compact Serialization, unencoded non-detached
payloads using period ('.') characters would cause parsing errors;
such payloads MUST NOT be used with the JWS Compact Serialization.

So I think the solution might be providing an extra option to the sign function to indicate if the payload is detached or not.

[lestrrat]

My assumption is that the payload HAS to be detached in presence of b64: false header. Am I wrong?

[lestrrat]

hmm maybe you are right. I will revisit later

[lestrrat]

Yeah, okay, I get it https://datatracker.ietf.org/doc/html/rfc7797#section-5.2

[lestrrat]

Alright, so now it's coming down to the problem API design. It turns out that I've already used WithDetachedPayload() to work with jws.Verify().

Now, the problem is with the method signature for jws.Sign(). it is now

jws.Sign(payload, alg, key, options...)

We can't use jws.WithDetachedPayload(true) here, because it already exists as jws.WithDetachedPayload([]byte).
If we are to overload jws.WithDetachedPayload([]byte) to mean "Use this payload, unencoded", then we would have to force the user to say:

jws.Sign(nil, alg, key, jws.WithDetachedPayload(payload))

.... and at this point I'm not sure if this is kosher. Maybe it's okay if we document it in really large bold print pointing out this is weird, but you must follow it until we fix it in the next major version? WDYT?

[lestrrat]

@stevenvegt Okay, can you take a look at https://github.com/lestrrat-go/jwx/tree/gh-541 at commit 3fe6322?

[stevenvegt]

Yes. This will work and keeps everything backwards compatible 👍

[lestrrat]

Thanks for checking. Filed #543, and I'm going to sit on it at least overnight.