acme/autocert: remove tls-sni-xx challenge support
These challenge types have been deprecated by popular ACME providers
due to security issues in the ecosystem.
Fixes golang/go#28370
Change-Id: I3270a6f5d3e5fbc53e4347a9a802df5f603c87de
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/194658
Run-TryBot: Alex Vaghin <ddos@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
diff --git a/acme/autocert/autocert.go b/acme/autocert/autocert.go
index 70ab355..5256bc3 100644
--- a/acme/autocert/autocert.go
+++ b/acme/autocert/autocert.go
@@ -88,9 +88,9 @@
}
// Manager is a stateful certificate manager built on top of acme.Client.
-// It obtains and refreshes certificates automatically using "tls-alpn-01",
-// "tls-sni-01", "tls-sni-02" and "http-01" challenge types,
-// as well as providing them to a TLS server via tls.Config.
+// It obtains and refreshes certificates automatically using "tls-alpn-01"
+// or "http-01" challenge types, as well as providing them to a TLS server
+// via tls.Config.
//
// You must specify a cache implementation, such as DirCache,
// to reuse obtained certificates across program restarts.
@@ -184,10 +184,8 @@
// to be provisioned.
// The entries are stored for the duration of the authorization flow.
httpTokens map[string][]byte
- // certTokens contains temporary certificates for tls-sni and tls-alpn challenges
- // and is keyed by token domain name, which matches server name of ClientHello.
- // Keys always have ".acme.invalid" suffix for tls-sni. Otherwise, they are domain names
- // for tls-alpn.
+ // certTokens contains temporary certificates for tls-alpn-01 challenges
+ // and is keyed by the domain name which matches the ClientHello server name.
// The entries are stored for the duration of the authorization flow.
certTokens map[string]*tls.Certificate
// nowFunc, if not nil, returns the current time. This may be set for
@@ -226,7 +224,7 @@
// GetCertificate implements the tls.Config.GetCertificate hook.
// It provides a TLS certificate for hello.ServerName host, including answering
-// tls-alpn-01 and *.acme.invalid (tls-sni-01 and tls-sni-02) challenges.
+// tls-alpn-01 challenges.
// All other fields of hello are ignored.
//
// If m.HostPolicy is non-nil, GetCertificate calls the policy before requesting
@@ -235,9 +233,7 @@
// This does not affect cached certs. See HostPolicy field description for more details.
//
// If GetCertificate is used directly, instead of via Manager.TLSConfig, package users will
-// also have to add acme.ALPNProto to NextProtos for tls-alpn-01, or use HTTPHandler
-// for http-01. (The tls-sni-* challenges have been deprecated by popular ACME providers
-// due to security issues in the ecosystem.)
+// also have to add acme.ALPNProto to NextProtos for tls-alpn-01, or use HTTPHandler for http-01.
func (m *Manager) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certificate, error) {
if m.Prompt == nil {
return nil, errors.New("acme/autocert: Manager.Prompt not set")
@@ -269,13 +265,10 @@
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
defer cancel()
- // Check whether this is a token cert requested for TLS-SNI or TLS-ALPN challenge.
+ // Check whether this is a token cert requested for TLS-ALPN challenge.
if wantsTokenCert(hello) {
m.tokensMu.RLock()
defer m.tokensMu.RUnlock()
- // It's ok to use the same token cert key for both tls-sni and tls-alpn
- // because there's always at most 1 token cert per on-going domain authorization.
- // See m.verify for details.
if cert := m.certTokens[name]; cert != nil {
return cert, nil
}
@@ -318,8 +311,7 @@
if len(hello.SupportedProtos) == 1 && hello.SupportedProtos[0] == acme.ALPNProto {
return true
}
- // tls-sni-xx
- return strings.HasSuffix(hello.ServerName, ".acme.invalid")
+ return false
}
func supportsECDSA(hello *tls.ClientHelloInfo) bool {
@@ -688,7 +680,7 @@
func (m *Manager) verify(ctx context.Context, client *acme.Client, domain string) error {
// The list of challenge types we'll try to fulfill
// in this specific order.
- challengeTypes := []string{"tls-alpn-01", "tls-sni-02", "tls-sni-01"}
+ challengeTypes := []string{"tls-alpn-01"}
m.tokensMu.RLock()
if m.tryHTTP01 {
challengeTypes = append(challengeTypes, "http-01")
@@ -776,20 +768,6 @@
}
m.putCertToken(ctx, domain, &cert)
return func() { go m.deleteCertToken(domain) }, nil
- case "tls-sni-01":
- cert, name, err := client.TLSSNI01ChallengeCert(chal.Token)
- if err != nil {
- return nil, err
- }
- m.putCertToken(ctx, name, &cert)
- return func() { go m.deleteCertToken(name) }, nil
- case "tls-sni-02":
- cert, name, err := client.TLSSNI02ChallengeCert(chal.Token)
- if err != nil {
- return nil, err
- }
- m.putCertToken(ctx, name, &cert)
- return func() { go m.deleteCertToken(name) }, nil
case "http-01":
resp, err := client.HTTP01ChallengeResponse(chal.Token)
if err != nil {
diff --git a/acme/autocert/autocert_test.go b/acme/autocert/autocert_test.go
index 91f47e5..6020e55 100644
--- a/acme/autocert/autocert_test.go
+++ b/acme/autocert/autocert_test.go
@@ -51,14 +51,9 @@
"status": "pending",
"challenges": [
{
- "uri": "{{.}}/challenge/1",
- "type": "tls-sni-01",
- "token": "token-01"
- },
- {
- "uri": "{{.}}/challenge/2",
- "type": "tls-sni-02",
- "token": "token-02"
+ "uri": "{{.}}/challenge/tls-alpn-01",
+ "type": "tls-alpn-01",
+ "token": "token-alpn"
},
{
"uri": "{{.}}/challenge/dns-01",
@@ -184,28 +179,46 @@
return json.Unmarshal(payload, v)
}
-func clientHelloInfo(sni string, ecdsaSupport bool) *tls.ClientHelloInfo {
+type algorithmSupport int
+
+const (
+ algRSA algorithmSupport = iota
+ algECDSA
+)
+
+func clientHelloInfo(sni string, alg algorithmSupport) *tls.ClientHelloInfo {
hello := &tls.ClientHelloInfo{
ServerName: sni,
CipherSuites: []uint16{tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305},
}
- if ecdsaSupport {
+ if alg == algECDSA {
hello.CipherSuites = append(hello.CipherSuites, tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305)
}
return hello
}
+// tokenCertFn returns a function suitable for startACMEServerStub.
+// The returned function simulates a TLS hello request from a CA
+// during validation of a tls-alpn-01 challenge.
+func tokenCertFn(man *Manager, alg algorithmSupport) getCertificateFunc {
+ return func(sni string) (*tls.Certificate, error) {
+ hello := clientHelloInfo(sni, alg)
+ hello.SupportedProtos = []string{acme.ALPNProto}
+ return man.GetCertificate(hello)
+ }
+}
+
func TestGetCertificate(t *testing.T) {
man := &Manager{Prompt: AcceptTOS}
defer man.stopRenew()
- hello := clientHelloInfo("example.org", true)
+ hello := clientHelloInfo("example.org", algECDSA)
testGetCertificate(t, man, "example.org", hello)
}
func TestGetCertificate_trailingDot(t *testing.T) {
man := &Manager{Prompt: AcceptTOS}
defer man.stopRenew()
- hello := clientHelloInfo("example.org.", true)
+ hello := clientHelloInfo("example.org.", algECDSA)
testGetCertificate(t, man, "example.org", hello)
}
@@ -213,10 +226,10 @@
man := &Manager{Prompt: AcceptTOS}
defer man.stopRenew()
- hello := clientHelloInfo("σσσ.com", true)
+ hello := clientHelloInfo("σσσ.com", algECDSA)
testGetCertificate(t, man, "xn--4xaaa.com", hello)
- hello = clientHelloInfo("σςΣ.com", true)
+ hello = clientHelloInfo("σςΣ.com", algECDSA)
testGetCertificate(t, man, "xn--4xaaa.com", hello)
}
@@ -224,10 +237,10 @@
man := &Manager{Prompt: AcceptTOS}
defer man.stopRenew()
- hello := clientHelloInfo("example.org", true)
+ hello := clientHelloInfo("example.org", algECDSA)
testGetCertificate(t, man, "example.org", hello)
- hello = clientHelloInfo("EXAMPLE.ORG", true)
+ hello = clientHelloInfo("EXAMPLE.ORG", algECDSA)
testGetCertificate(t, man, "example.org", hello)
}
@@ -238,7 +251,7 @@
ForceRSA: true,
}
defer man.stopRenew()
- hello := clientHelloInfo(exampleDomain, true)
+ hello := clientHelloInfo(exampleDomain, algECDSA)
testGetCertificate(t, man, exampleDomain, hello)
// ForceRSA was deprecated and is now ignored.
@@ -254,10 +267,10 @@
func TestGetCertificate_nilPrompt(t *testing.T) {
man := &Manager{}
defer man.stopRenew()
- url, finish := startACMEServerStub(t, getCertificateFromManager(man, true), "example.org")
+ url, finish := startACMEServerStub(t, tokenCertFn(man, algECDSA), "example.org")
defer finish()
man.Client = &acme.Client{DirectoryURL: url}
- hello := clientHelloInfo("example.org", true)
+ hello := clientHelloInfo("example.org", algECDSA)
if _, err := man.GetCertificate(hello); err == nil {
t.Error("got certificate for example.org; wanted error")
}
@@ -291,7 +304,7 @@
// The expired cached cert should trigger a new cert issuance
// and return without an error.
- hello := clientHelloInfo(exampleDomain, true)
+ hello := clientHelloInfo(exampleDomain, algECDSA)
testGetCertificate(t, man, exampleDomain, hello)
}
@@ -323,7 +336,7 @@
},
}
defer man.stopRenew()
- hello := clientHelloInfo(exampleDomain, true)
+ hello := clientHelloInfo(exampleDomain, algECDSA)
if _, err := man.GetCertificate(hello); err == nil {
t.Error("GetCertificate: err is nil")
}
@@ -340,9 +353,9 @@
}
// testGetCertificate_tokenCache tests the fallback of token certificate fetches
-// to cache when Manager.certTokens misses. ecdsaSupport refers to the CA when
-// verifying the certificate token.
-func testGetCertificate_tokenCache(t *testing.T, ecdsaSupport bool) {
+// to cache when Manager.certTokens misses.
+// algorithmSupport refers to the CA when verifying the certificate token.
+func testGetCertificate_tokenCache(t *testing.T, tokenAlg algorithmSupport) {
man1 := &Manager{
Cache: newMemCache(t),
Prompt: AcceptTOS,
@@ -356,10 +369,10 @@
// Send the verification request to a different Manager from the one that
// initiated the authorization, when they share caches.
- url, finish := startACMEServerStub(t, getCertificateFromManager(man2, ecdsaSupport), "example.org")
+ url, finish := startACMEServerStub(t, tokenCertFn(man2, tokenAlg), "example.org")
defer finish()
man1.Client = &acme.Client{DirectoryURL: url}
- hello := clientHelloInfo("example.org", true)
+ hello := clientHelloInfo("example.org", algECDSA)
if _, err := man1.GetCertificate(hello); err != nil {
t.Error(err)
}
@@ -370,10 +383,10 @@
func TestGetCertificate_tokenCache(t *testing.T) {
t.Run("ecdsaSupport=true", func(t *testing.T) {
- testGetCertificate_tokenCache(t, true)
+ testGetCertificate_tokenCache(t, algECDSA)
})
t.Run("ecdsaSupport=false", func(t *testing.T) {
- testGetCertificate_tokenCache(t, false)
+ testGetCertificate_tokenCache(t, algRSA)
})
}
@@ -381,11 +394,11 @@
cache := newMemCache(t)
man := &Manager{Prompt: AcceptTOS, Cache: cache}
defer man.stopRenew()
- url, finish := startACMEServerStub(t, getCertificateFromManager(man, true), "example.org")
+ url, finish := startACMEServerStub(t, tokenCertFn(man, algECDSA), "example.org")
defer finish()
man.Client = &acme.Client{DirectoryURL: url}
- cert, err := man.GetCertificate(clientHelloInfo("example.org", true))
+ cert, err := man.GetCertificate(clientHelloInfo("example.org", algECDSA))
if err != nil {
t.Error(err)
}
@@ -393,7 +406,7 @@
t.Error("an ECDSA client was served a non-ECDSA certificate")
}
- cert, err = man.GetCertificate(clientHelloInfo("example.org", false))
+ cert, err = man.GetCertificate(clientHelloInfo("example.org", algRSA))
if err != nil {
t.Error(err)
}
@@ -401,10 +414,10 @@
t.Error("a RSA client was served a non-RSA certificate")
}
- if _, err := man.GetCertificate(clientHelloInfo("example.org", true)); err != nil {
+ if _, err := man.GetCertificate(clientHelloInfo("example.org", algECDSA)); err != nil {
t.Error(err)
}
- if _, err := man.GetCertificate(clientHelloInfo("example.org", false)); err != nil {
+ if _, err := man.GetCertificate(clientHelloInfo("example.org", algRSA)); err != nil {
t.Error(err)
}
if numCerts := cache.numCerts(); numCerts != 2 {
@@ -416,7 +429,7 @@
cache := newMemCache(t)
man := &Manager{Prompt: AcceptTOS, Cache: cache}
defer man.stopRenew()
- url, finish := startACMEServerStub(t, getCertificateFromManager(man, true), exampleDomain)
+ url, finish := startACMEServerStub(t, tokenCertFn(man, algECDSA), exampleDomain)
defer finish()
man.Client = &acme.Client{DirectoryURL: url}
@@ -443,7 +456,7 @@
}
// The RSA cached cert should be silently ignored and replaced.
- cert, err := man.GetCertificate(clientHelloInfo(exampleDomain, true))
+ cert, err := man.GetCertificate(clientHelloInfo(exampleDomain, algECDSA))
if err != nil {
t.Error(err)
}
@@ -455,25 +468,36 @@
}
}
-func getCertificateFromManager(man *Manager, ecdsaSupport bool) func(string) error {
- return func(sni string) error {
- _, err := man.GetCertificate(clientHelloInfo(sni, ecdsaSupport))
- return err
- }
-}
+type getCertificateFunc func(domain string) (*tls.Certificate, error)
// startACMEServerStub runs an ACME server
// The domain argument is the expected domain name of a certificate request.
// TODO: Drop this in favour of x/crypto/acme/autocert/internal/acmetest.
-func startACMEServerStub(t *testing.T, getCertificate func(string) error, domain string) (url string, finish func()) {
- // echo token-02 | shasum -a 256
- // then divide result in 2 parts separated by dot
- tokenCertName := "4e8eb87631187e9ff2153b56b13a4dec.13a35d002e485d60ff37354b32f665d9.token.acme.invalid"
+func startACMEServerStub(t *testing.T, tokenCert getCertificateFunc, domain string) (url string, finish func()) {
verifyTokenCert := func() {
- if err := getCertificate(tokenCertName); err != nil {
- t.Errorf("verifyTokenCert: GetCertificate(%q): %v", tokenCertName, err)
+ tlscert, err := tokenCert(domain)
+ if err != nil {
+ t.Errorf("verifyTokenCert: tokenCert(%q): %v", domain, err)
return
}
+ crt, err := x509.ParseCertificate(tlscert.Certificate[0])
+ if err != nil {
+ t.Errorf("verifyTokenCert: x509.ParseCertificate: %v", err)
+ }
+ if err := crt.VerifyHostname(domain); err != nil {
+ t.Errorf("verifyTokenCert: %v", err)
+ }
+ // TODO: Update OID to the latest value 1.3.6.1.5.5.7.1.31
+ // See https://tools.ietf.org/html/draft-ietf-acme-tls-alpn-05#section-5.1
+ oid := asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 1, 30, 1}
+ for _, x := range crt.Extensions {
+ if x.Id.Equal(oid) {
+ // No need to check the extension value here.
+ // This is done in acme package tests.
+ return
+ }
+ }
+ t.Error("verifyTokenCert: no id-pe-acmeIdentifier extension found")
}
// ACME CA server stub
@@ -501,8 +525,8 @@
if err := authzTmpl.Execute(w, ca.URL); err != nil {
t.Errorf("authzTmpl: %v", err)
}
- // accept tls-sni-02 challenge
- case "/challenge/2":
+ // accept tls-alpn-01 challenge
+ case "/challenge/tls-alpn-01":
verifyTokenCert()
w.Write([]byte("{}"))
// authorization status
@@ -552,7 +576,7 @@
tick := time.NewTicker(100 * time.Millisecond)
defer tick.Stop()
for {
- if err := getCertificate(tokenCertName); err != nil {
+ if _, err := tokenCert(domain); err != nil {
return
}
select {
@@ -576,7 +600,7 @@
// tests man.GetCertificate flow using the provided hello argument.
// The domain argument is the expected domain name of a certificate request.
func testGetCertificate(t *testing.T, man *Manager, domain string, hello *tls.ClientHelloInfo) {
- url, finish := startACMEServerStub(t, getCertificateFromManager(man, true), domain)
+ url, finish := startACMEServerStub(t, tokenCertFn(man, algECDSA), domain)
defer finish()
man.Client = &acme.Client{DirectoryURL: url}
@@ -658,11 +682,8 @@
if err := authzTmpl.Execute(w, ca.URL); err != nil {
t.Errorf("authzTmpl: %v", err)
}
- // Accept tls-sni-02.
- case "/challenge/2":
- w.Write([]byte("{}"))
- // Reject tls-sni-01.
- case "/challenge/1":
+ // Reject tls-alpn-01.
+ case "/challenge/tls-alpn-01":
http.Error(w, "won't accept tls-sni-01", http.StatusBadRequest)
// Should not accept dns-01.
case "/challenge/dns-01":
@@ -674,10 +695,9 @@
verifyHTTPToken()
w.Write([]byte("{}"))
// Authorization statuses.
- // Make tls-sni-xxx invalid.
- case "/authz/1", "/authz/2":
+ case "/authz/1": // tls-alpn-01
w.Write([]byte(`{"status": "invalid"}`))
- case "/authz/3", "/authz/4":
+ case "/authz/2": // http-01
w.Write([]byte(`{"status": "valid"}`))
default:
http.NotFound(w, r)
@@ -700,10 +720,10 @@
if err := m.verify(ctx, client, "example.org"); err != nil {
t.Errorf("m.verify: %v", err)
}
- // Only tls-sni-01, tls-sni-02 and http-01 must be accepted
+ // Only tls-alpn-01 and http-01 must be accepted.
// The dns-01 challenge is unsupported.
- if authzCount != 3 {
- t.Errorf("authzCount = %d; want 3", authzCount)
+ if authzCount != 2 {
+ t.Errorf("authzCount = %d; want 2", authzCount)
}
if !didAcceptHTTP01 {
t.Error("did not accept http-01 challenge")
@@ -716,8 +736,8 @@
// each tried within a newly created authorization.
// This means each authorization URI corresponds to a different challenge type.
revokedAuthz := map[string]bool{
- "/authz/0": false, // tls-sni-02
- "/authz/1": false, // tls-sni-01
+ "/authz/0": false, // tls-alpn-01
+ "/authz/1": false, // http-01
"/authz/2": false, // no viable challenge, but authz is created
}
@@ -752,12 +772,12 @@
t.Errorf("authzTmpl: %v", err)
}
authzCount++
- // tls-sni-02 challenge "accept" request.
- case "/challenge/2":
+ // tls-alpn-01 challenge "accept" request.
+ case "/challenge/tls-alpn-01":
// Refuse.
- http.Error(w, "won't accept tls-sni-02 challenge", http.StatusBadRequest)
- // tls-sni-01 challenge "accept" request.
- case "/challenge/1":
+ http.Error(w, "won't accept tls-alpn-01 challenge", http.StatusBadRequest)
+ // http-01 challenge "accept" request.
+ case "/challenge/http-01":
// Accept but the authorization will be "expired".
w.Write([]byte("{}"))
// Authorization requests.
@@ -795,8 +815,9 @@
m := &Manager{
Client: &acme.Client{DirectoryURL: ca.URL},
}
+ m.HTTPHandler(nil) // enable http-01 challenge type
// Should fail and revoke 3 authorizations.
- // The first 2 are tsl-sni-02 and tls-sni-01 challenges.
+ // The first 2 are tls-alpn-01 and http-01 challenges.
// The third time an authorization is created but no viable challenge is found.
// See revokedAuthz above for more explanation.
if _, err := m.createCert(context.Background(), exampleCertKey); err == nil {
@@ -1058,7 +1079,7 @@
{"fo.o", "cache.Get of fo.o"},
}
for _, tt := range tests {
- _, err := m.GetCertificate(clientHelloInfo(tt.name, true))
+ _, err := m.GetCertificate(clientHelloInfo(tt.name, algECDSA))
got := fmt.Sprint(err)
if got != tt.wantErr {
t.Errorf("GetCertificate(SNI = %q) = %q; want %q", tt.name, got, tt.wantErr)
diff --git a/acme/autocert/renewal_test.go b/acme/autocert/renewal_test.go
index 5d1c63f..294f902 100644
--- a/acme/autocert/renewal_test.go
+++ b/acme/autocert/renewal_test.go
@@ -173,7 +173,7 @@
}
// trigger renew
- hello := clientHelloInfo(exampleDomain, true)
+ hello := clientHelloInfo(exampleDomain, algECDSA)
if _, err := man.GetCertificate(hello); err != nil {
t.Fatal(err)
}
@@ -299,7 +299,7 @@
}
// assert the expiring cert is returned from state
- hello := clientHelloInfo(exampleDomain, true)
+ hello := clientHelloInfo(exampleDomain, algECDSA)
tlscert, err := man.GetCertificate(hello)
if err != nil {
t.Fatal(err)
@@ -317,7 +317,7 @@
t.Fatal("renew took too long to occur")
case <-done:
// assert the new cert is returned from state after renew
- hello := clientHelloInfo(exampleDomain, true)
+ hello := clientHelloInfo(exampleDomain, algECDSA)
tlscert, err := man.GetCertificate(hello)
if err != nil {
t.Fatal(err)
