Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/crypto/acme: crypto.Signer interface mismatch #35829

Closed
edef1c opened this issue Nov 25, 2019 · 12 comments
Closed

x/crypto/acme: crypto.Signer interface mismatch #35829

edef1c opened this issue Nov 25, 2019 · 12 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Soon This needs to be done soon. (regressions, serious bugs, outages)
Milestone

Comments

@edef1c
Copy link
Contributor

edef1c commented Nov 25, 2019

Currently, acme.Client.Key is allowed to be a crypto.Signer, but has rather unusual expectations of that interface:

// Note: non-stdlib crypto.Signer implementations are expected to return
// the signature in the format as specified in RFC7518.
// See https://tools.ietf.org/html/rfc7518 for more details.

It is the understanding of me and @FiloSottile that a crypto.Signer should return the "standard" signature format matching the key type returned by Public(), which is the ASN.1 (r, s) tuple for *ecdsa.PublicKey, and certainly not a JWS-specific format.

@gopherbot gopherbot added this to the Unreleased milestone Nov 25, 2019
@FiloSottile FiloSottile added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 25, 2019
@FiloSottile
Copy link
Contributor

@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.

@edef1c
Copy link
Contributor Author

edef1c commented Nov 25, 2019

The bug was introduced by golang/crypto@bfa7d42.

@FiloSottile
Copy link
Contributor

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

@FiloSottile FiloSottile added NeedsFix The path to resolution is known, but the work has not been done. Soon This needs to be done soon. (regressions, serious bugs, outages) and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Nov 25, 2019
@x1ddos
Copy link

x1ddos commented Nov 25, 2019

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

@FiloSottile
Copy link
Contributor

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.

@x1ddos
Copy link

x1ddos commented Nov 25, 2019

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

@edef1c
Copy link
Contributor Author

edef1c commented Nov 25, 2019

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>

@x1ddos
Copy link

x1ddos commented Nov 25, 2019

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

@edef1c
Copy link
Contributor Author

edef1c commented Nov 25, 2019

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".

@x1ddos
Copy link

x1ddos commented Nov 25, 2019

@edef1c then please take this bug.

@FiloSottile
Copy link
Contributor

@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.

@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Dec 4, 2020
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
Previously, an ECDSA crypto.Signer would have been expected to return a
signature in RFC7518 format, which violates crypto.Signer's interface
contract.

Fixes golang/go#35829

Change-Id: Id0cc2d9296cfb9f89925ab9ac02e12d68eec734b
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/209537
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
Previously, an ECDSA crypto.Signer would have been expected to return a
signature in RFC7518 format, which violates crypto.Signer's interface
contract.

Fixes golang/go#35829

Change-Id: Id0cc2d9296cfb9f89925ab9ac02e12d68eec734b
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/209537
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
Previously, an ECDSA crypto.Signer would have been expected to return a
signature in RFC7518 format, which violates crypto.Signer's interface
contract.

Fixes golang/go#35829

Change-Id: Id0cc2d9296cfb9f89925ab9ac02e12d68eec734b
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/209537
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
@rsc rsc unassigned x1ddos Jun 23, 2022
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
Previously, an ECDSA crypto.Signer would have been expected to return a
signature in RFC7518 format, which violates crypto.Signer's interface
contract.

Fixes golang/go#35829

Change-Id: Id0cc2d9296cfb9f89925ab9ac02e12d68eec734b
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/209537
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
Previously, an ECDSA crypto.Signer would have been expected to return a
signature in RFC7518 format, which violates crypto.Signer's interface
contract.

Fixes golang/go#35829

Change-Id: Id0cc2d9296cfb9f89925ab9ac02e12d68eec734b
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/209537
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Soon This needs to be done soon. (regressions, serious bugs, outages)
Projects
None yet
Development

No branches or pull requests

4 participants