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.

Sign up for GitHub

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

Jump to bottom

x/crypto/acme/autocert: handle RSA-only clients better #22066

Closed
rsc opened this issue Sep 27, 2017 · 13 comments
Closed

x/crypto/acme/autocert: handle RSA-only clients better #22066

rsc opened this issue Sep 27, 2017 · 13 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@rsc
Copy link
Contributor

rsc commented Sep 27, 2017

https://letsencrypt.org/docs/integration-guide/ says "Our recommendation is to serve a dual-cert config, offering an RSA certificate by default, and a (much smaller) ECDSA certificate to those clients that indicate support."

Today, autocert can only use one kind of cert. I think we should fix that and make it manage both a cached ECDSA cert and a cached RSA cert, offering the RSA certificate to clients that don't support ECDSA. Otherwise you just get weird "no supported cipher suite" errors with older clients. I'm using ruby on an up-to-date Mac and it wants to do TLS 1.0 with RSA. I spent way too long debugging why it was failing, because the failures were very non-obvious. (I don't know why Ruby on my Mac wants TLS 1.0 with RSA, but surely there are other clients like mine out there. And maybe there are RSA-only TLS 1.2 clients out there too.)

I see the ForceRSA flag in the Manager, but that doesn't do what the integration guide suggests: it uses RSA always, not just when the client doesn't support ECDSA.

As a workaround, I am using two managers, with two separate clients and caches, and a custom GetCertificate that figures out which to use:

m := autocert.Manager{
	Client:     &acme.Client{DirectoryURL: dir},
	Cache:      autocertcache.NewGoogleCloudStorageCache(client, "mycert"),
	Prompt:     autocert.AcceptTOS,
	HostPolicy: autocert.HostWhitelist("my.host"),
}
mRSA := autocert.Manager{
	Client:     &acme.Client{DirectoryURL: dir},
	Cache:      autocertcache.NewGoogleCloudStorageCache(client, "mycert-rsa"),
	Prompt:     autocert.AcceptTOS,
	HostPolicy: autocert.HostWhitelist("my.host"),
	ForceRSA:   true,
}
s := &http.Server{
	Addr:    ":https",
	Handler: handler,
	TLSConfig: &tls.Config{
		MinVersion:     tls.VersionSSL30,
		GetCertificate: fallbackSNI(mRSA.GetCertificate, m.GetCertificate, "my.host"),
	},
}

func fallbackSNI(getCertRSA, getCert func(*tls.ClientHelloInfo) (*tls.Certificate, error), host string) func(*tls.ClientHelloInfo) (*tls.Certificate, error) {
	return func(hello *tls.ClientHelloInfo) (*tls.Certificate, error) {
		saveHello(hello)
		if hello.ServerName == "" {
			h := *hello
			hello = &h
			hello.ServerName = host
		}
		var cert *tls.Certificate
		var err error
		if len(hello.SupportedVersions) > 0 && hello.SupportedVersions[0] >= tls.VersionTLS12 {
			cert, err = getCert(hello)
			if strings.HasSuffix(hello.ServerName, ".acme.invalid") && err != nil {
				cert, err = getCertRSA(hello)
			}
		} else {
			cert, err = getCertRSA(hello)
		}
		if err != nil {
			fmt.Fprintf(os.Stderr, "getCert: %v\n", err)
		}
		return cert, err
	}
}

This code also includes a workaround for #18785, and the RSA decision is wrong (it should look at the supported ciphers etc) but was good enough for my purposes.

/cc @x1ddos @bradfitz
@rsc rsc added this to the Unreleased milestone Sep 27, 2017
@x1ddos
Copy link

x1ddos commented Sep 28, 2017

Hey Russ, why not just use hello.ServerName != "" to check whether the client supports SNI? This is what https://godoc.org/crypto/tls#ClientHelloInfo docs say:

// ServerName indicates the name of the server requested by the client
// in order to support virtual hosting. ServerName is only set if the
// client is using SNI (see
// http://tools.ietf.org/html/rfc4366#section-3.1).

But agree either way, autocert should handle RSA-only clients.

@rsc
Copy link
Contributor Author

rsc commented Sep 28, 2017

@x1ddos, maybe the code is just unclear with everything else going on, but that's the test I do use for SNI (the five lines starting at if hello.ServerName == "" {). I just jammed the RSA patch into it too because I already had a custom GetCertificate.

@bradfitz bradfitz added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Nov 2, 2017
@x1ddos x1ddos mentioned this issue Feb 7, 2018
Closed
@bradfitz
Copy link
Contributor

bradfitz commented Feb 7, 2018

Some other discussion is in #23698.

@FiloSottile
Copy link
Contributor

Obtaining two certificates off the bat sounds like a waste of quota, but how about we issue a RSA certificate automatically if a TLS handshake from a client that doesn't support ECDSA comes?

@bradfitz
Copy link
Contributor

bradfitz commented Feb 7, 2018

@FiloSottile, SGTM. But I also don't think 2 cert requests every 2-3 months per domain name is too bad either.

@FiloSottile FiloSottile self-assigned this Feb 12, 2018
@gopherbot
Copy link

Change https://golang.org/cl/114501 mentions this issue: acme/autocert: support both RSA and ECDSA clients on the fly

@x1ddos
Copy link

x1ddos commented Jun 3, 2018

@niemeyer FYI because you've posted #17744.

@primalmotion
Copy link

this commit broke our builds

vendor/golang.org/x/crypto/acme/autocert/autocert.go:277:9: undefined: tls.ECDSAWithSHA1

@FiloSottile
Copy link
Contributor

Wow, that was fast.

And sorry, that symbol was added in Go 1.10. I'll fix it ASAP, in the meantime you can workaround by updating to 1.10 if that's possible for you.

@gopherbot
Copy link

Change https://golang.org/cl/116496 mentions this issue: acme/autocert: fix build in Go 1.9

@primalmotion
Copy link

Oh I see it's only for go 1.9. we haven't migrated our pipelines yet which explains why it works on my laptop :)

gopherbot pushed a commit to golang/crypto that referenced this issue Jun 6, 2018
@FiloSottile
acme/autocert: fix build in Go 1.9
d162186 
Updates golang/go#22066

Change-Id: I7eb6a60deb6680003245815760e2ce6a8f7d8b15
Reviewed-on: https://go-review.googlesource.com/116496
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@FiloSottile
Copy link
Contributor

Should be fixed now. @primalmotion can you confirm?

@primalmotion
Copy link

yep it works now, thanks, that was fast too ;)

@jsha jsha mentioned this issue Jun 11, 2018
Closed
@golang golang locked and limited conversation to collaborators Jun 6, 2019
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
@FiloSottile
acme/autocert: support both RSA and ECDSA clients on the fly
e1831b3 
GetCertificate has all the information it needs to know if a client
supports ECDSA in ClientHelloInfo. Deprecate and ignore ForceRSA, and
just obtain a RSA certificate on the fly when a client that doesn't
support ECDSA connects.

This changes the cache key format to have a "+rsa" suffix for RSA
certificates. The default (ForceRSA = false) cache key is unchanged,
so most DirCache instances will still be valid. Caches created with
ForceRSA set will be silently ignored and certificates reissued.

The cache keys for HTTP tokens and the account key are changed to be
guaranteed not to overlap with valid domain names as well.

Note that ECDSA support detection is more strict in following RFC 5246
than crypto/tls, which ignores signature_algorithms.

Fixes golang/go#22066

Change-Id: I70227747b563d6849cb693f83a950d57040b3f39
Reviewed-on: https://go-review.googlesource.com/114501
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
c-expert-zigbee added a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@c-expert-zigbee
acme/autocert: support both RSA and ECDSA clients on the fly
dc79383 
GetCertificate has all the information it needs to know if a client
supports ECDSA in ClientHelloInfo. Deprecate and ignore ForceRSA, and
just obtain a RSA certificate on the fly when a client that doesn't
support ECDSA connects.

This changes the cache key format to have a "+rsa" suffix for RSA
certificates. The default (ForceRSA = false) cache key is unchanged,
so most DirCache instances will still be valid. Caches created with
ForceRSA set will be silently ignored and certificates reissued.

The cache keys for HTTP tokens and the account key are changed to be
guaranteed not to overlap with valid domain names as well.

Note that ECDSA support detection is more strict in following RFC 5246
than crypto/tls, which ignores signature_algorithms.

Fixes golang/go#22066

Change-Id: I70227747b563d6849cb693f83a950d57040b3f39
Reviewed-on: https://go-review.googlesource.com/114501
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
c-expert-zigbee added a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@c-expert-zigbee
acme/autocert: support both RSA and ECDSA clients on the fly
77eccb9 
GetCertificate has all the information it needs to know if a client
supports ECDSA in ClientHelloInfo. Deprecate and ignore ForceRSA, and
just obtain a RSA certificate on the fly when a client that doesn't
support ECDSA connects.

This changes the cache key format to have a "+rsa" suffix for RSA
certificates. The default (ForceRSA = false) cache key is unchanged,
so most DirCache instances will still be valid. Caches created with
ForceRSA set will be silently ignored and certificates reissued.

The cache keys for HTTP tokens and the account key are changed to be
guaranteed not to overlap with valid domain names as well.

Note that ECDSA support detection is more strict in following RFC 5246
than crypto/tls, which ignores signature_algorithms.

Fixes golang/go#22066

Change-Id: I70227747b563d6849cb693f83a950d57040b3f39
Reviewed-on: https://go-review.googlesource.com/114501
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
@FiloSottile
acme/autocert: support both RSA and ECDSA clients on the fly
2b7f6c9 
GetCertificate has all the information it needs to know if a client
supports ECDSA in ClientHelloInfo. Deprecate and ignore ForceRSA, and
just obtain a RSA certificate on the fly when a client that doesn't
support ECDSA connects.

This changes the cache key format to have a "+rsa" suffix for RSA
certificates. The default (ForceRSA = false) cache key is unchanged,
so most DirCache instances will still be valid. Caches created with
ForceRSA set will be silently ignored and certificates reissued.

The cache keys for HTTP tokens and the account key are changed to be
guaranteed not to overlap with valid domain names as well.

Note that ECDSA support detection is more strict in following RFC 5246
than crypto/tls, which ignores signature_algorithms.

Fixes golang/go#22066

Change-Id: I70227747b563d6849cb693f83a950d57040b3f39
Reviewed-on: https://go-review.googlesource.com/114501
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
@FiloSottile
acme/autocert: support both RSA and ECDSA clients on the fly
d3c80d3 
GetCertificate has all the information it needs to know if a client
supports ECDSA in ClientHelloInfo. Deprecate and ignore ForceRSA, and
just obtain a RSA certificate on the fly when a client that doesn't
support ECDSA connects.

This changes the cache key format to have a "+rsa" suffix for RSA
certificates. The default (ForceRSA = false) cache key is unchanged,
so most DirCache instances will still be valid. Caches created with
ForceRSA set will be silently ignored and certificates reissued.

The cache keys for HTTP tokens and the account key are changed to be
guaranteed not to overlap with valid domain names as well.

Note that ECDSA support detection is more strict in following RFC 5246
than crypto/tls, which ignores signature_algorithms.

Fixes golang/go#22066

Change-Id: I70227747b563d6849cb693f83a950d57040b3f39
Reviewed-on: https://go-review.googlesource.com/114501
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@bradfitz @x1ddos @rsc @primalmotion @FiloSottile @gopherbot