diff --git a/config.go b/config.go index e2924ed5..3b033bd1 100644 --- a/config.go +++ b/config.go @@ -162,6 +162,8 @@ type Config struct { // cache with a renewed certificate beside the key it replaced. // A certificate the local cache cannot supply a usable pair for // is reloaded from Storage, which replaces the local copies. + // So is one that is due for renewal, since the local cache does + // not learn of renewals done by other instances. // // Beware that this stores private keys on every instance that // serves them, so the local cache should be at least as secure diff --git a/crypto.go b/crypto.go index 5fea2339..774c7aad 100644 --- a/crypto.go +++ b/crypto.go @@ -239,6 +239,8 @@ func (cfg *Config) loadCertResourceAnyIssuer(ctx context.Context, certNamesKey s // loadCertResource loads a certificate resource from the given issuer's storage // location, using the given storage: see cachedStorage and groundTruthStorage. +// A resource from the local cache that is due for renewal is read from Storage +// instead, in case another instance already renewed it. func (cfg *Config) loadCertResource(ctx context.Context, issuer Issuer, certNamesKey string, storage Storage) (CertificateResource, error) { certRes := CertificateResource{issuerKey: issuer.IssuerKey()} @@ -267,6 +269,17 @@ func (cfg *Config) loadCertResource(ctx context.Context, issuer Issuer, certName return CertificateResource{}, fmt.Errorf("decoding certificate metadata: %v", err) } + // the local cache only learns of renewals this instance does, so a cached + // certificate due for renewal is the one a peer most likely replaced; the + // Storage read refreshes the local copy, so this is paid once per renewal + if ls, ok := storage.(localCacheStorage); ok && ls.readLocal { + if _, _, stale := cfg.managedCertNeedsRenewal(certRes, false); stale { + if current, err := cfg.loadCertResource(ctx, issuer, certNamesKey, cfg.groundTruthStorage()); err == nil { + return current, nil + } + } + } + return certRes, nil } diff --git a/localcache_test.go b/localcache_test.go index 547c4402..2e147111 100644 --- a/localcache_test.go +++ b/localcache_test.go @@ -123,8 +123,9 @@ func TestLocalCache(t *testing.T) { cfg, am, storage, localCache := testLocalCacheConfig(t) cfg.LocalCache = localCache certKey := StorageKeys.SiteCert(am.IssuerKey(), domain) + saved := testIssuedCertResource(t, am, domain, time.Now()) - err := cfg.saveCertResource(ctx, am, testCertResource(am, domain)) + err := cfg.saveCertResource(ctx, am, saved) if err != nil { t.Fatalf("Expected no error saving cert resource, got: %v", err) } @@ -136,8 +137,8 @@ func TestLocalCache(t *testing.T) { if err != nil { t.Fatalf("Expected no error loading cert resource, got: %v", err) } - if string(certRes.CertificatePEM) != "certificate" { - t.Errorf("Expected 'certificate', got: %s", certRes.CertificatePEM) + if !bytes.Equal(certRes.CertificatePEM, saved.CertificatePEM) { + t.Error("Expected the saved certificate to be loaded") } if len(storage.calls) > 0 { t.Errorf("Expected no storage calls, got: %v", storage.calls) @@ -221,6 +222,58 @@ func TestLocalCacheTornWrite(t *testing.T) { } } +func TestLocalCacheStaleCertRenewedByPeer(t *testing.T) { + ctx := context.Background() + const domain = "example.com" + now := time.Now() + + cfg, preferred, storage, localCache := testLocalCacheConfig(t) + fallback := &ACMEIssuer{CA: "https://fallback.example.com/acme/directory"} + fallback.config = cfg + cfg.Issuers = []Issuer{preferred, fallback} + cfg.LocalCache = localCache + cfg.OCSP = OCSPConfig{DisableStapling: true} + + // this instance has served the preferred issuer's certificate, now + // expired, and has a fallback certificate that is still fine + for issuer, issued := range map[Issuer]time.Time{preferred: now.Add(-100 * 24 * time.Hour), fallback: now.Add(-30 * 24 * time.Hour)} { + if err := cfg.saveCertResource(ctx, issuer, testIssuedCertResource(t, issuer, domain, issued)); err != nil { + t.Fatalf("Expected no error saving cert resource, got: %v", err) + } + } + + // another instance renewed the preferred certificate, which only + // reaches storage; the fallback would mask that forever, since it + // never needs renewal itself and so never triggers a storage read + renewed := testIssuedCertResource(t, preferred, domain, now) + cfg.LocalCache = nil + if err := cfg.saveCertResource(ctx, preferred, renewed); err != nil { + t.Fatalf("Expected no error saving renewed cert resource, got: %v", err) + } + cfg.LocalCache = localCache + + cert, err := cfg.CacheManagedCertificate(ctx, domain) + if err != nil { + t.Fatalf("Expected no error caching managed certificate, got: %v", err) + } + expected, err := makeCertificate(renewed.CertificatePEM, renewed.PrivateKeyPEM) + if err != nil { + t.Fatalf("Expected no error making the renewed certificate, got: %v", err) + } + if !bytes.Equal(cert.Leaf.Raw, expected.Leaf.Raw) { + t.Error("Expected the renewed certificate from the preferred issuer to be loaded") + } + + // the local cache has caught up, so the next load stays local + storage.calls = nil + if _, err = cfg.CacheManagedCertificate(ctx, domain); err != nil { + t.Fatalf("Expected no error caching managed certificate, got: %v", err) + } + if len(storage.calls) > 0 { + t.Errorf("Expected no storage calls once the local cache caught up, got: %v", storage.calls) + } +} + func TestLocalCacheUnknownDomainLoadedOnce(t *testing.T) { ctx := context.Background() @@ -334,7 +387,7 @@ func TestWarmLocalCache(t *testing.T) { cfg, am, storage, localCache := testLocalCacheConfig(t) // no local cache yet, so this only writes to storage - err := cfg.saveCertResource(ctx, am, testCertResource(am, domain)) + err := cfg.saveCertResource(ctx, am, testIssuedCertResource(t, am, domain, time.Now())) if err != nil { t.Fatalf("Expected no error saving cert resource, got: %v", err) }