acme/autocert: treat invalid cert as a cache miss

A cached cert data may be corrupted or simply contain an expired certificate, which results in GetCertificate returning an error. This change makes the Manager ignore those invalid and expired cache entries, treating them as nonexistent. Fixes golang/go#20035. Change-Id: I5345291ecb1aab1cf19671cf0a383135c7102038 Reviewed-on: https://go-review.googlesource.com/41690 Reviewed-by: Brad Fitzpatrick <[email protected]>
LewiGoddard · Apr 25, 2017 · 1090250 · 1090250
1 parent 61de90c
commit 1090250
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 3 deletions.
diff --git a/acme/autocert/autocert.go b/acme/autocert/autocert.go
@@ -244,6 +244,7 @@ func (m *Manager) cert(ctx context.Context, name string) (*tls.Certificate, erro
 }
 
 // cacheGet always returns a valid certificate, or an error otherwise.
+// If a cached certficate exists but is not valid, ErrCacheMiss is returned.
 func (m *Manager) cacheGet(ctx context.Context, domain string) (*tls.Certificate, error) {
 	if m.Cache == nil {
 		return nil, ErrCacheMiss
@@ -256,7 +257,7 @@ func (m *Manager) cacheGet(ctx context.Context, domain string) (*tls.Certificate
 	// private
 	priv, pub := pem.Decode(data)
 	if priv == nil || !strings.Contains(priv.Type, "PRIVATE") {
-		return nil, errors.New("acme/autocert: no private key found in cache")
+		return nil, ErrCacheMiss
 	}
 	privKey, err := parsePrivateKey(priv.Bytes)
 	if err != nil {
@@ -274,13 +275,14 @@ func (m *Manager) cacheGet(ctx context.Context, domain string) (*tls.Certificate
 		pubDER = append(pubDER, b.Bytes)
 	}
 	if len(pub) > 0 {
-		return nil, errors.New("acme/autocert: invalid public key")
+		// Leftover content not consumed by pem.Decode. Corrupt. Ignore.
+		return nil, ErrCacheMiss
 	}
 
 	// verify and create TLS cert
 	leaf, err := validCert(domain, pubDER, privKey)
 	if err != nil {
-		return nil, err
+		return nil, ErrCacheMiss
 	}
 	tlscert := &tls.Certificate{
 		Certificate: pubDER,

diff --git a/acme/autocert/autocert_test.go b/acme/autocert/autocert_test.go
@@ -178,6 +178,38 @@ func TestGetCertificate_nilPrompt(t *testing.T) {
 	}
 }
 
+func TestGetCertificate_expiredCache(t *testing.T) {
+	// Make an expired cert and cache it.
+	pk, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	if err != nil {
+		t.Fatal(err)
+	}
+	tmpl := &x509.Certificate{
+		SerialNumber: big.NewInt(1),
+		Subject:      pkix.Name{CommonName: "example.org"},
+		NotAfter:     time.Now(),
+	}
+	pub, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, &pk.PublicKey, pk)
+	if err != nil {
+		t.Fatal(err)
+	}
+	tlscert := &tls.Certificate{
+		Certificate: [][]byte{pub},
+		PrivateKey:  pk,
+	}
+
+	man := &Manager{Prompt: AcceptTOS, Cache: newMemCache()}
+	defer man.stopRenew()
+	if err := man.cachePut(context.Background(), "example.org", tlscert); err != nil {
+		t.Fatalf("man.cachePut: %v", err)
+	}
+
+	// The expired cached cert should trigger a new cert issuance
+	// and return without an error.
+	hello := &tls.ClientHelloInfo{ServerName: "example.org"}
+	testGetCertificate(t, man, "example.org", hello)
+}
+
 // startACMEServerStub runs an ACME server
 // The domain argument is the expected domain name of a certificate request.
 func startACMEServerStub(t *testing.T, man *Manager, domain string) (url string, finish func()) {