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/autocert: support ECDSA on TLS1.3-only connections #50522

Open
mjl- opened this issue Jan 9, 2022 · 5 comments
Open

x/crypto/acme/autocert: support ECDSA on TLS1.3-only connections #50522

mjl- opened this issue Jan 9, 2022 · 5 comments
Assignees
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@mjl-
Copy link

mjl- commented Jan 9, 2022

Latest version of x/crypto/acme/autocert at the time of writing:

https://pkg.go.dev/golang.org/x/crypto@v0.0.0-20211215153901-e495a2d5b3d3/acme/autocert

What did you do?

Call Manager.GetCertificate to force an immediate retrieval of missing certificates (during application startup).

What did you expect to see?

I expected autocert to fetch an ECDSA certificate.

What did you see instead?

An RSA certificate was fetched instead.
Then, the first time I actually connect to my service (using standard tools like curl and openssl s_client), an ECDSA certificate was fetched. Because ECDSA is typically preferred over RSA by such tools.

Analysis

My initial tls.ClientHelloInfo had only ServerName set, no ciphersuites, versions, etc. This (correctly) fails the supportsECDSA check at: https://cs.opensource.google/go/x/crypto/+/e495a2d5:acme/autocert/autocert.go;drc=e495a2d5b3d3be43468d0ebb413f46eeaedf7eb3;l=322

That's fair, so I tried with an hello that supports only ECDSA:

tryGetCertificate := func(name string) {
	hello := &tls.ClientHelloInfo{
		ServerName: name,

		CipherSuites: []uint16{tls.TLS_AES_128_GCM_SHA256},
		SupportedCurves: []tls.CurveID{tls.CurveP256},
		SignatureSchemes: []tls.SignatureScheme{tls.ECDSAWithP256AndSHA256},
		SupportedVersions: []uint16{tls.VersionTLS13},
	}
	m.GetCertificate(hello)
}

However, that still results in an RSA certificate being fetched: supportsECDSA does not take TLS1.3 configs into account, requiring an ECDSA-supporting ciphersuite for TLS <= 1.2. See: https://cs.opensource.google/go/x/crypto/+/e495a2d5:acme/autocert/autocert.go;l=353;drc=e495a2d5b3d3be43468d0ebb413f46eeaedf7eb3
Background info: With TLS1.3, the signature schemes have been taken/separated out of CipherSuite definitions.

I believe supportsECDSA should be updated to recognize TLS1.3, with a check for ECDSA in SignatureSchemes.
Furthermore, I believe supportsECDSA should be modified to also check for RSA support in the ClientHelloInfo: There is no point in fetching an RSA certificate if the client cannot use it. I understand RSA used to be the default signature scheme. But it may not be in the future, and clients are already free to select the signature schemes they wish to use.

I can work around my immediate issue (forcing a fetch of an ECDSA certificate) by simply including tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 in the CipherSuites slice. Then it passes the supportsECDSA check. But actual TLS1.3-only connections will still fetch an RSA certificate, not an ECDSA certificate, even if RSA isn't supported by the client.

I can write a patch if this sounds reasonable.

Another idea: We could change Manager.GetCertificate to be satisfied with an existing RSA certificate in case of a non-existing ECDSA certificate. We would have to be careful that the supported RSA-using-cipher-suites aren't otherwise weaker than the ECDSA cipher suites. That's why I'm not sure it is worth it. But if we were to expand the signature-scheme-compatibility to check for RSA too, perhaps it is worth it.

@gopherbot gopherbot added this to the Unreleased milestone Jan 9, 2022
@dr2chase
Copy link
Contributor

@FiloSottile can you give this a look?

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 10, 2022
@cespare
Copy link
Contributor

cespare commented Jan 10, 2022

/cc @golang/security

@mjl-
Copy link
Author

mjl- commented Jan 11, 2022

I looked into this some more. I tried verifying that I would indeed get a error connecting with TLS1.3 with ECDSA only. I wanted to configure a tls.Config with only ECDSA as signature scheme. It looks like tls.Config does not allow setting signature schemes. I don't know if this is due to TLS1.3 the specification, or the Go implementation. I did notice this comment for CipherSuites: "Note that TLS 1.3 ciphersuites are not configurable.". I know modern crypto aims for fewer options/negotations. Perhaps RSA support is always required for TLS1.3. If so, the claim the title of this issue is incorrect (I extrapolated it from what I saw with autocert). At least it is clear that my knowledge about TLS1.3 is lacking.

But there is more. I wrote this program to connect to my autocert server. To see what the tls.ClientHelloInfo looks like, and if connection succeeds:

package main

import (
        "crypto/tls"
        "log"
        "os"
)

func main() {
        log.SetFlags(0)
        addr := os.Args[1]

        config := &tls.Config{
                MinVersion: tls.VersionTLS13,
        }
        if _, err := tls.Dial("tcp", addr, config); err != nil {
                log.Fatalf("dial: %s", err)
        }
        log.Printf("connected")
}

It resulted in this tls.ClientHelloInfo:

&tls.ClientHelloInfo{
	CipherSuites:[]uint16{0xc02b, 0xc02f, 0xc02c, 0xc030, 0xcca9, 0xcca8, 0xc009, 0xc013, 0xc00a, 0xc014, 0x9c, 0x9d, 0x2f, 0x35, 0xc012, 0xa, 0x1301, 0x1302, 0x1303},
	ServerName:"host.example",
	SupportedCurves:[]tls.CurveID{0x1d, 0x17, 0x18, 0x19},
	SupportedPoints:[]uint8{0x0},
	SignatureSchemes:[]tls.SignatureScheme{0x804, 0x403, 0x807, 0x805, 0x806, 0x401, 0x501, 0x601, 0x503, 0x603, 0x201, 0x203},
	SupportedProtos:[]string(nil),
	SupportedVersions:[]uint16{0x304},
...

I am surprised that cipher suites for TLS <=1.2 appear in the list for a TLS1.3-only connection. I tested with go1.17.6 and go1.18beta1, same result. I thought may be the crypto/tls server is inserting them.

To check that, I tried with a different ecosystem: curl --tlsv1.3 https://host.example.
That resulted in:

&tls.ClientHelloInfo{
	CipherSuites:[]uint16{0x1302, 0x1303, 0x1301, 0xff},
	ServerName:"host.example",
	SupportedCurves:[]tls.CurveID{0x1d, 0x17, 0x1e, 0x19, 0x18},
	SupportedPoints:[]uint8{0x0, 0x1, 0x2},
	SignatureSchemes:[]tls.SignatureScheme{0x403, 0x503, 0x603, 0x807, 0x808, 0x809, 0x80a, 0x80b, 0x804, 0x805, 0x806, 0x401, 0x501, 0x601},
	SupportedProtos:[]string{"h2", "http/1.1"},
	SupportedVersions:[]uint16{0x304},
...

No TLS<=1.2 cipher suites this time. So it looks like the crypto/tls client inserts the TLS<=1.2 cipher suites in a TLS1.3-only connection. And that causes the supportsECDSA check in autocert to succeed. The request made by curl results in supportECDSA returning false, so the (successful) connection uses RSA, not ECDSA.

In the ClientHelloInfo's above, the signature schemes are different. So I'm concluding that it signature schemes are configurable with TLS1.3. But RSA support may always be required, and "ECDSA-only TLS1.3 connections" may not be a thing.

For reference, here is the acmecert server I used for testing with https://github.com/letsencrypt/pebble:

package main

import (
	"context"
	"crypto/ecdsa"
	"crypto/elliptic"
	"crypto/rand"
	"crypto/tls"
	"log"
	"net/http"
	"os"
	"strings"
	"time"

	"golang.org/x/crypto/acme"
	"golang.org/x/crypto/acme/autocert"
)

func main() {
	log.SetFlags(0)

	hostname := os.Args[1]

	key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
	if err != nil {
		log.Fatal(err)
	}
	client := &acme.Client{
		DirectoryURL: "https://localhost:14000/dir",
		Key:          key,
	}

	m := &autocert.Manager{
		Cache:      dirCache("cert"),
		Prompt:     autocert.AcceptTOS,
		HostPolicy: autocert.HostWhitelist(hostname),
		Email:      "acme@host.example",
		Client:     client,
	}

	loggingGetCertificate := func(hello *tls.ClientHelloInfo) (*tls.Certificate, error) {
		log.Printf("hello:\n%#v", hello)
		cert, err := m.GetCertificate(hello)
		if err != nil {
			log.Printf("getting certificate for %q: %s", hello.ServerName, err)
		}
		return cert, err
	}

	tryGetCertificate := func(name string) {
		hello := &tls.ClientHelloInfo{
			ServerName: name,

			// Make us fetch an ECDSA P256 cert.
			// CipherSuites: []uint16{tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, tls.TLS_AES_128_GCM_SHA256},
			CipherSuites:      []uint16{tls.TLS_AES_128_GCM_SHA256},
			SupportedCurves:   []tls.CurveID{tls.CurveP256},
			SignatureSchemes:  []tls.SignatureScheme{tls.ECDSAWithP256AndSHA256},
			SupportedVersions: []uint16{tls.VersionTLS13},
		}
		loggingGetCertificate(hello)
	}

	go func() {
		time.Sleep(100 * time.Millisecond)
		tryGetCertificate(hostname)
	}()

	httpsTLSConfig := *m.TLSConfig()
	httpsTLSConfig.GetCertificate = loggingGetCertificate
	s := &http.Server{
		Addr:      hostname + ":443",
		TLSConfig: &httpsTLSConfig,
	}
	log.Printf("listening for https")
	log.Fatal(s.ListenAndServeTLS("", ""))
}

type dirCache autocert.DirCache

func (d dirCache) Delete(ctx context.Context, name string) error {
	err := autocert.DirCache(d).Delete(ctx, name)
	if err != nil {
		log.Printf("deleting %q from dir cache: %s", name, err)
	} else if !strings.HasSuffix(name, "+token") {
		log.Printf("cert delete %q", name)
	}
	return err
}

func (d dirCache) Get(ctx context.Context, name string) ([]byte, error) {
	buf, err := autocert.DirCache(d).Get(ctx, name)
	if err != nil {
		log.Printf("getting %q from dir cache: %s", name, err)
	} else if !strings.HasSuffix(name, "+token") {
		log.Printf("cert get %q", name)
	}
	return buf, err
}

func (d dirCache) Put(ctx context.Context, name string, data []byte) error {
	err := autocert.DirCache(d).Put(ctx, name, data)
	if err != nil {
		log.Printf("storing %q in dir cache: %s", name, err)
	} else if !strings.HasSuffix(name, "+token") {
		log.Printf("cert store %q", name)
	}
	return err
}

And go.mod:

module acmecert

go 1.17

require golang.org/x/crypto v0.0.0-20211215153901-e495a2d5b3d3

require (
        golang.org/x/net v0.0.0-20211112202133-69e39bad7dc2 // indirect
        golang.org/x/text v0.3.6 // indirect
)

@mjl-
Copy link
Author

mjl- commented Jan 15, 2022

An ECDSA TLS1.3 connection indeed fails:

$ openssl s_client -tls1_3 -connect host.example:443 -sigalgs "ECDSA+SHA256"
CONNECTED(00000004)
139741787190656:error:14094410:SSL routines:ssl3_read_bytes:sslv3 alert handshake failure:../ssl/record/rec_layer_s3.c:1543:SSL alert number 40
---
no peer certificate available
---
No client certificate CA names sent
---
SSL handshake has read 7 bytes and written 215 bytes
Verification: OK
---
New, (NONE), Cipher is (NONE)
Secure Renegotiation IS NOT supported
Compression: NONE
Expansion: NONE
No ALPN negotiated
Early data was not sent
Verify return code: 0 (ok)
---

Logging from acmecert as posted above (with an additional log line added to "supportsECDSA", according to current detection of support):

hello:
&tls.ClientHelloInfo{
        CipherSuites:[]uint16{0x1302, 0x1303, 0x1301, 0xff},
        ServerName:"host.example",
        SupportedCurves:[]tls.CurveID{0x1d, 0x17, 0x1e, 0x19, 0x18},
        SupportedPoints:[]uint8{0x0, 0x1, 0x2},
        SignatureSchemes:[]tls.SignatureScheme{0x403},
        SupportedProtos:[]string(nil),
        SupportedVersions:[]uint16{0x304},
...
}
client supports ECDSA: false
http: TLS handshake error from 0.0.0.0:44898: tls: peer doesn't support any of the certificate's signature algorithms

@FiloSottile
Copy link
Contributor

supportsECDSA is indeed an extremely limited and partial check. The assumption is that RSA is always the safe fallback, too, which is not necessarily true. I added ClientHelloInfo.SupportsCertificate in Go 1.14 to address this. It's been in the standard library long enough that we should just switch autocert to it and delete supportsECDSA.

@FiloSottile FiloSottile self-assigned this Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants