@x1ddos Any reason for this? The crypto.Signer interface should always be expected to behave like the corresponding PrivateKey implementation. If we need a different format, we need to take the output apart and reserialize.

Unless I'm missing something, we should fix this soon so that code doesn't start relying on it.

The bug was introduced by golang/crypto@bfa7d42.

Yeah, pretty sure this is a bug. Sorry I missed it in review.

That comment would be misleading if you read it from a public API point of view. What it actually does is it formats the signature according to JWS if it's ECDSA or uses whatever crypt.Signer.Sign outputs as is, which is the case for RSA too.

Any reason for this?

Yes, see commit message golang/crypto@bfa7d42 (you reviewed it at the time, it seems)

Unless I'm missing something, we should fix this soon so that code doesn't start relying on it.

The jwsSign code isn't public API so it shouldn't be relied upon by anyone. But it is already in use by Google internally to sign using RPCs. It's backed by ECDSA but they can't return (r, s) tuple directly IIRC.

What is public API is https://godoc.org/golang.org/x/crypto/acme#Client:

type Client struct {
    // Key is the account key used to register with a CA and sign requests.
    // Key.Public() must return a *rsa.PublicKey or *ecdsa.PublicKey.
    //
    // The following algorithms are supported:
    // RS256, ES256, ES384 and ES512.
    // See RFC7518 for more details about the algorithms.
    Key crypto.Signer

If a crypto.Signer with a Sign method that behaves correctly like ecdsa.PrivateKey.Sign is used as a Client.Key, the library will not behave correctly, right?

From the crypto.Signer docs:

For an (EC)DSA key, [the resulting signature] should be a DER-serialised, ASN.1 signature structure.

Ok. I'll take a look. Can't remember at the moment what the issue with using ASN1 for ECDSA was exactly.

That comment would be misleading if you read it from a public API point of view. What it actually does is it formats the signature according to JWS if it's ECDSA or uses whatever crypt.Signer.Sign outputs as is, which is the case for RSA too.

No, signatures are never reformatted from the ASN.1 format to the JWS format, not even for ECDSA.
For *ecdsa.PrivateKey, it uses ecdsa.Sign to gather the raw (r, s) tuple and then formats that for a JWS. This distinct treatment of *ecdsa.PrivateKey is perfectly visible in public interface.

This is fairly easy to demonstrate:

package main

import (
	"context"
	"crypto"
	"crypto/ecdsa"
	"crypto/elliptic"
	"crypto/rand"
	"fmt"
	"golang.org/x/crypto/acme"
)

func main() {
	var key crypto.Signer
	key, _ = ecdsa.GenerateKey(elliptic.P256(), rand.Reader)

	// comment this line out to generate valid signatures
	key = struct{ crypto.Signer }{key}

	ac := &acme.Client{Key: key, DirectoryURL: "https://acme-staging-v02.api.letsencrypt.org/directory"}
	fmt.Println(ac.Register(context.Background(), &acme.Account{}, acme.AcceptTOS))
}

As of golang/crypto@ac88ee7:

$ go run broken.go
<nil> 400 urn:ietf:params:acme:error:malformed: JWS verification error
$ go run fixed.go
&{https://acme-staging-v02.api.letsencrypt.org/acme/acct/11650481 [] valid      } <nil>

Found where it's used internally. I believe I'll be able to make it work with ASN1 in all cases.

FWIW, I've got a patch ready to roll, but the tests would need a bit of rework to actually pass valid signatures around rather than "testsig".

@edef1c then please take this bug.

@edef1c If you want to send a CL with tests, that's great, otherwise we introduced the issue so we'll fix it, don't worry. Just let us know.

@x1ddos I am on vacation for the rest of the week, if you don't have time to fix this this week feel free to reassign it to me and I'll send a CL next Monday.

Change https://golang.org/cl/209537 mentions this issue: acme: process ASN.1 ECDSA signatures correctly