fix(client): race in cert renewal (#42)

* fix: race in cert renewal

this refactor ensures unique cache per instance is initialized,
and the global default cert management in certmagic is not triggered.

circular dependency is solved in alrernative way:
1. recursive call for newCertmagicConfig is removed and replaced by simpler configGetter
2. TLSConfig() ensures correct GetCertificate() is used

* refactor: named acme-broker logger
* test: OnCertRenewed

basic test that confirms cert renewal works as expected when expiration
event is triggered

Author: lidel

client/acme.go

@@ -36,7 +36,7 @@ type P2PForgeCertMgr struct {
 	ProvideHost                func(host.Host)
 	hostFn                     func() host.Host
 	hasHost                    func() bool
-	cfg                        *certmagic.Config
+	certmagic                  *certmagic.Config
 	log                        *zap.SugaredLogger
 	allowPrivateForgeAddresses bool
 	produceShortAddrs          bool
@@ -45,11 +45,6 @@ type P2PForgeCertMgr struct {
 	certCheckMx sync.RWMutex
 }
 
-var (
-	defaultCertCache   *certmagic.Cache
-	defaultCertCacheMu sync.Mutex
-)
-
 func isRelayAddr(a multiaddr.Multiaddr) bool {
 	found := false
 	multiaddr.ForEach(a, func(c multiaddr.Component) bool {
@@ -84,9 +79,11 @@ type P2PForgeCertMgrConfig struct {
 	storage                    certmagic.Storage
 	modifyForgeRequest         func(r *http.Request) error
 	onCertLoaded               func()
+	onCertRenewed              func()
 	log                        *zap.SugaredLogger
 	allowPrivateForgeAddresses bool
 	produceShortAddrs          bool
+	renewCheckInterval         time.Duration
 }
 
 type P2PForgeCertMgrOptions func(*P2PForgeCertMgrConfig) error
@@ -176,6 +173,22 @@ func WithTrustedRoots(trustedRoots *x509.CertPool) P2PForgeCertMgrOptions {
 	}
 }
 
+// WithOnCertRenewed is optional callback executed on cert renewal event
+func WithOnCertRenewed(fn func()) P2PForgeCertMgrOptions {
+	return func(config *P2PForgeCertMgrConfig) error {
+		config.onCertRenewed = fn
+		return nil
+	}
+}
+
+// WithRenewCheckInterval is meant for testing
+func WithRenewCheckInterval(renewCheckInterval time.Duration) P2PForgeCertMgrOptions {
+	return func(config *P2PForgeCertMgrConfig) error {
+		config.renewCheckInterval = renewCheckInterval
+		return nil
+	}
+}
+
 // WithAllowPrivateForgeAddrs is meant for testing or skipping all the
 // connectivity checks libp2p node needs to pass before it can request domain
 // and start ACME DNS-01 challenge.
@@ -210,41 +223,13 @@ func WithLogger(log *zap.SugaredLogger) P2PForgeCertMgrOptions {
 	}
 }
 
-// newCertmagicConfig is p2p-forge/client-specific version of
-// certmagic.NewDefault() that ensures we have our own cert cache. This is
-// necessary to ensure cert maintenance spawned by NewCache does not share
-// global certmagic.Default.Storage, and certmagic.Default.Logger and uses
-// storage path specific to p2p-forge, and no other instance of certmagic in
-// golang application.
-func newCertmagicConfig(mgrCfg *P2PForgeCertMgrConfig) *certmagic.Config {
-	clog := mgrCfg.log.Desugar()
-
-	defaultCertCacheMu.Lock()
-	if defaultCertCache == nil {
-		defaultCertCache = certmagic.NewCache(certmagic.CacheOptions{
-			GetConfigForCert: func(certmagic.Certificate) (*certmagic.Config, error) {
-				// default getter that does not depend on certmagic defaults
-				// and respects Config.Storage path
-				return newCertmagicConfig(mgrCfg), nil
-			},
-			Logger: clog,
-		})
-	}
-	certCache := defaultCertCache
-	defaultCertCacheMu.Unlock()
-
-	return certmagic.New(certCache, certmagic.Config{
-		Storage: mgrCfg.storage,
-		Logger:  clog,
-	})
-}
-
 // NewP2PForgeCertMgr handles the creation and management of certificates that are automatically granted by a forge
 // to a libp2p host.
 //
 // Calling this function signifies your acceptance to
 // the CA's Subscriber Agreement and/or Terms of Service. Let's Encrypt is the default CA.
 func NewP2PForgeCertMgr(opts ...P2PForgeCertMgrOptions) (*P2PForgeCertMgr, error) {
+	// Init config + apply optional user settings
 	mgrCfg := &P2PForgeCertMgrConfig{}
 	for _, opt := range opts {
 		if err := opt(mgrCfg); err != nil {
@@ -270,14 +255,11 @@ func NewP2PForgeCertMgr(opts ...P2PForgeCertMgrOptions) (*P2PForgeCertMgr, error
 			return nil, fmt.Errorf("must specify the forge registration endpoint if using a non-default forge")
 		}
 	}
-
-	const defaultStorageLocation = "p2p-forge-certs"
 	if mgrCfg.storage == nil {
-		mgrCfg.storage = &certmagic.FileStorage{Path: defaultStorageLocation}
+		mgrCfg.storage = &certmagic.FileStorage{Path: DefaultStorageLocation}
 	}
 
-	certCfg := newCertmagicConfig(mgrCfg)
-
+	// Wire up p2p-forge manager instance
 	hostChan := make(chan host.Host, 1)
 	provideHost := func(host host.Host) { hostChan <- host }
 	hasHostChan := make(chan struct{})
@@ -293,39 +275,60 @@ func NewP2PForgeCertMgr(opts ...P2PForgeCertMgrOptions) (*P2PForgeCertMgr, error
 		defer close(hasHostChan)
 		return <-hostChan
 	})
+	mgr := &P2PForgeCertMgr{
+		forgeDomain:                mgrCfg.forgeDomain,
+		forgeRegistrationEndpoint:  mgrCfg.forgeRegistrationEndpoint,
+		ProvideHost:                provideHost,
+		hostFn:                     hostFn,
+		hasHost:                    hasHostFn,
+		log:                        mgrCfg.log,
+		allowPrivateForgeAddresses: mgrCfg.allowPrivateForgeAddresses,
+		produceShortAddrs:          mgrCfg.produceShortAddrs,
+	}
+
+	// NOTE: callback getter is necessary to avoid circular dependency
+	// but also structure code to avoid issues like https://github.com/ipshipyard/p2p-forge/issues/28
+	configGetter := func(cert certmagic.Certificate) (*certmagic.Config, error) {
+		if mgr.certmagic == nil {
+			return nil, errors.New("P2PForgeCertmgr.certmagic is not set")
+		}
+		return mgr.certmagic, nil
+	}
+
+	magicCache := certmagic.NewCache(certmagic.CacheOptions{
+		GetConfigForCert:   configGetter,
+		RenewCheckInterval: mgrCfg.renewCheckInterval,
+		Logger:             mgrCfg.log.Desugar(),
+	})
 
-	myACME := certmagic.NewACMEIssuer(certCfg, certmagic.ACMEIssuer{ // TODO: UX around user passed emails + agreement
+	// Wire up final certmagic config by calling upstream New with sanity checks
+	mgr.certmagic = certmagic.New(magicCache, certmagic.Config{
+		Storage: mgrCfg.storage,
+		Logger:  mgrCfg.log.Desugar(),
+	})
+
+	// Wire up Issuer that does brokered DNS-01 ACME challenge
+	acmeLog := mgrCfg.log.Named("acme-broker")
+	brokeredDNS01Issuer := certmagic.NewACMEIssuer(mgr.certmagic, certmagic.ACMEIssuer{
 		CA:     mgrCfg.caEndpoint,
 		Email:  mgrCfg.userEmail,
 		Agreed: true,
 		DNS01Solver: &dns01P2PForgeSolver{
 			forgeRegistrationEndpoint:  mgrCfg.forgeRegistrationEndpoint,
 			forgeAuth:                  mgrCfg.forgeAuth,
-			hostFn:                     hostFn,
+			hostFn:                     mgr.hostFn,
 			modifyForgeRequest:         mgrCfg.modifyForgeRequest,
 			userAgent:                  mgrCfg.userAgent,
 			allowPrivateForgeAddresses: mgrCfg.allowPrivateForgeAddresses,
-			log:                        mgrCfg.log.Named("dns01solver"),
+			log:                        acmeLog.Named("dns01solver"),
 		},
 		TrustedRoots: mgrCfg.trustedRoots,
-		Logger:       certCfg.Logger,
+		Logger:       acmeLog.Desugar(),
 	})
+	mgr.certmagic.Issuers = []certmagic.Issuer{brokeredDNS01Issuer}
 
-	certCfg.Issuers = []certmagic.Issuer{myACME}
-
-	mgr := &P2PForgeCertMgr{
-		forgeDomain:                mgrCfg.forgeDomain,
-		forgeRegistrationEndpoint:  mgrCfg.forgeRegistrationEndpoint,
-		ProvideHost:                provideHost,
-		hostFn:                     hostFn,
-		hasHost:                    hasHostFn,
-		cfg:                        certCfg,
-		log:                        mgrCfg.log,
-		allowPrivateForgeAddresses: mgrCfg.allowPrivateForgeAddresses,
-		produceShortAddrs:          mgrCfg.produceShortAddrs,
-	}
-
-	certCfg.OnEvent = func(ctx context.Context, event string, data map[string]any) error {
+	// Wire up onCertLoaded callback
+	mgr.certmagic.OnEvent = func(ctx context.Context, event string, data map[string]any) error {
 		if event == "cached_managed_cert" {
 			sans, ok := data["sans"]
 			if !ok {
@@ -352,24 +355,39 @@ func NewP2PForgeCertMgr(opts ...P2PForgeCertMgrOptions) (*P2PForgeCertMgr, error
 			}
 			return nil
 		}
+
+		// Execute user function for on certificate cert renewal
+		if event == "cert_obtained" && mgrCfg.onCertRenewed != nil {
+			if renewal, ok := data["renewal"].(bool); ok && renewal {
+				name := certName(hostFn().ID(), mgrCfg.forgeDomain)
+				if id, ok := data["identifier"].(string); ok && id == name {
+					mgrCfg.onCertRenewed()
+				}
+			}
+			return nil
+		}
+
 		return nil
 	}
 
 	return mgr, nil
 }
 
 func (m *P2PForgeCertMgr) Start() error {
-	if m.cfg == nil || m.hostFn == nil {
+	if m.certmagic == nil || m.hostFn == nil {
 		return errors.New("unable to start without a certmagic and libp2p host")
 	}
+	if m.certmagic.Storage == nil {
+		return errors.New("unable to start without a certmagic Cache and Storage set up")
+	}
 	m.ctx, m.cancel = context.WithCancel(context.Background())
 	go func() {
 		log := m.log.Named("start")
 		h := m.hostFn()
 		name := certName(h.ID(), m.forgeDomain)
-		certExists := localCertExists(m.ctx, m.cfg, name)
+		certExists := localCertExists(m.ctx, m.certmagic, name)
 		startCertManagement := func() {
-			if err := m.cfg.ManageAsync(m.ctx, []string{name}); err != nil {
+			if err := m.certmagic.ManageAsync(m.ctx, []string{name}); err != nil {
 				log.Error(err)
 			}
 		}
@@ -433,8 +451,9 @@ func (m *P2PForgeCertMgr) Stop() {
 
 // TLSConfig returns a tls.Config that managed by the P2PForgeCertMgr
 func (m *P2PForgeCertMgr) TLSConfig() *tls.Config {
-	tlsCfg := m.cfg.TLSConfig()
+	tlsCfg := m.certmagic.TLSConfig()
 	tlsCfg.NextProtos = nil // remove the ACME ALPN
+	tlsCfg.GetCertificate = m.certmagic.GetCertificate
 	return tlsCfg
 }
 
@@ -449,7 +468,7 @@ func (m *P2PForgeCertMgr) AddrStrings() []string {
 // This should be used with the libp2p.AddrsFactory option to ensure that a libp2p host with forge managed addresses
 // only announces those that are active and valid.
 func (m *P2PForgeCertMgr) AddressFactory() config.AddrsFactory {
-	tlsCfg := m.cfg.TLSConfig()
+	tlsCfg := m.certmagic.TLSConfig()
 	tlsCfg.NextProtos = []string{"h2", "http/1.1"} // remove the ACME ALPN and set the HTTP 1.1 and 2 ALPNs
 
 	return m.createAddrsFactory(m.allowPrivateForgeAddresses, m.produceShortAddrs)

client/defaults.go

@@ -20,6 +20,8 @@ const (
 	// ForgeAuthHeader optional HTTP header that client should include when
 	// talking to a limited access registration endpoint
 	ForgeAuthHeader = "Forge-Authorization"
+
+	DefaultStorageLocation = "p2p-forge-certs"
 )
 
 // defaultUserAgent is used as a fallback to inform HTTP server which library