From 1090250d69ef22a9b609ecc4350e0bad265da38a Mon Sep 17 00:00:00 2001
From: Alex Vaghin <ddos@google.com>
Date: Tue, 25 Apr 2017 10:32:32 +0200
Subject: [PATCH] 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 <bradfitz@golang.org>
---
 acme/autocert/autocert.go      |  8 +++++---
 acme/autocert/autocert_test.go | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/acme/autocert/autocert.go b/acme/autocert/autocert.go
index 98842b4..679ff4d 100644
--- 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
index 6df4cf3..3166c1f 100644
--- 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()) {