Make Sign1 and Verify1 difficult to misuse

[qmuntal]

Problem statement

NCC Group found a (known) footgun in our API:

The library does not prevent a user from manually populating the signature field or modifying the protected header after calling Sign() to potentially violate this requirement.

Example:

msg, _ := cose.Sign1(rand.Reader, signer, protected, []byte("hello world"), nil)
msg.Signature = []byte("footgun!") // <-------
sig, _ := msg.MarshalCBOR()

The same applies to Verify():

var msg cose.Sign1Message
msg.UnmarshalCBOR(sig)
msg.Signature = nil  // <------- footgun
msg.Verify(nil, verifier)

We initially added Sign1 and Verify1 to make it easier to sign and verify messages, but with the current signature they don't prevent user to misuse go-cose.

Proposal

Me proposal is to change these two functions so they don't work with Sign1Message but just deal with cbor-marshaled messages:

func Sign1(rand io.Reader, signer Signer, header Headers, payload []byte, external []byte) (cbor_msg []byte, err error)

~func Verify1(verifier Verifier, cbor_msg []byte, external []byte) error~

With this change, users that don't need the flexibility of Sign1Message won't need to even know it exist, as the signature would be represented an opaque, difficult-to-misuse, []byte.

Worth nothing that users can still misuse Sign1Message.Sign() and Sign1Message.Verify(), but keeping them as-is allow advanced users to do their custom marshalling or validations.

Edited: Update Sign1 proposed API. Edit2: Notice that the proposed signature is very similar to .Net's static CoseSign1Message.Sign(...). Edit3: Verify1 is proposed to be removed as per thread discussion.

[shizhMSFT]

For Sign1, we are not able to add timestamp signatures with marshalled Sign1Message since the timestamp signature is generated after the payload signing.

For Verify1, it is strange to not have the Sign1Message returned after verification. In other words, I am not able to get the payload easily after verification. If I unmarshal it again, how do I make sure the previous verification is valid for the unmarshalled content as different unmarshaller may be used?

Overall, I think we should just remove those two methods :D

[qmuntal]

For Sign1, we are not able to add timestamp signatures with marshalled Sign1Message since the timestamp signature is generated after the payload signing.

How common are timestamp signatures? Sign1 would be targeted for basic use cases. If someone wants to do anything fancy, they can still create their own Sign1Message. I see value on having Sign1 as a one-shot function, as it would mitigate NCC Group feedback about go-cose not having a misuse-free API.

For Verify1, it is strange to not have the Sign1Message returned after verification. In other words, I am not able to get the payload easily after verification. If I unmarshal it again, how do I make sure the previous verification is valid for the unmarshalled content as different unmarshaller may be used?

Agree that Verify1 might not be really useful, neither in the current form nor with the proposed signature. Should we remove it then? As additional data point, .Net does not provide a one-shot Verify1 function neither.

[shizhMSFT]

At least, we are sure to drop Verify1().

@thomas-fossati Could we have more inputs on Sign1()?

[thomas-fossati]

At least, we are sure to drop Verify1().

@thomas-fossati Could we have more inputs on Sign1()?

Sign1 as put forward by Quim looks good to me. It seems like a useful interface to have. I agree on the limited value of Verify1; the only argument for having it would be interface symmetry, which is not a very compelling argument, I reckon :-)

[qmuntal]

As there seems to be quorum on updating cose.Sign1 and removing cose.Verify1, I've submitted a PR doing so.