From 660a36bd1311369da9033fd02aa9da9e50b0b804 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Fri, 17 Jan 2025 23:43:22 +0100 Subject: [PATCH 1/3] 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 --- client/acme.go | 113 ++++++++++++++++++++------------------------- client/defaults.go | 2 + 2 files changed, 52 insertions(+), 63 deletions(-) diff --git a/client/acme.go b/client/acme.go index 3a4eab1..5de0422 100644 --- a/client/acme.go +++ b/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 { @@ -210,41 +205,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 +237,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 +257,58 @@ 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, + } - myACME := certmagic.NewACMEIssuer(certCfg, certmagic.ACMEIssuer{ // TODO: UX around user passed emails + agreement + // 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, + Logger: mgrCfg.log.Desugar(), + }) + + // 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 + 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"), }, TrustedRoots: mgrCfg.trustedRoots, - Logger: certCfg.Logger, + Logger: mgr.certmagic.Logger, }) + 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 { @@ -359,17 +342,20 @@ func NewP2PForgeCertMgr(opts ...P2PForgeCertMgrOptions) (*P2PForgeCertMgr, error } 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 +419,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 +436,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) diff --git a/client/defaults.go b/client/defaults.go index fb7f301..d8261ca 100644 --- a/client/defaults.go +++ b/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 From 60f66f89e261cf0960c24db5d2854de5a13fa6ca Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Sat, 18 Jan 2025 00:33:29 +0100 Subject: [PATCH 2/3] refactor: named acme-broker logger --- client/acme.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/client/acme.go b/client/acme.go index 5de0422..b1dc9d5 100644 --- a/client/acme.go +++ b/client/acme.go @@ -289,6 +289,7 @@ func NewP2PForgeCertMgr(opts ...P2PForgeCertMgrOptions) (*P2PForgeCertMgr, error }) // 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, @@ -300,10 +301,10 @@ func NewP2PForgeCertMgr(opts ...P2PForgeCertMgrOptions) (*P2PForgeCertMgr, error modifyForgeRequest: mgrCfg.modifyForgeRequest, userAgent: mgrCfg.userAgent, allowPrivateForgeAddresses: mgrCfg.allowPrivateForgeAddresses, - log: mgrCfg.log.Named("dns01solver"), + log: acmeLog.Named("dns01solver"), }, TrustedRoots: mgrCfg.trustedRoots, - Logger: mgr.certmagic.Logger, + Logger: acmeLog.Desugar(), }) mgr.certmagic.Issuers = []certmagic.Issuer{brokeredDNS01Issuer} From d44d4afac06770d59abc705a508a1683a7b9b907 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Wed, 22 Jan 2025 04:01:02 +0100 Subject: [PATCH 3/3] test: OnCertRenewed basic test that confirms cert renewal works as expected when expiration event is triggered --- client/acme.go | 35 +++++++++++++++++++++++++++++++++-- e2e_test.go | 43 ++++++++++++++++++++++++++++++++++++------- 2 files changed, 69 insertions(+), 9 deletions(-) diff --git a/client/acme.go b/client/acme.go index b1dc9d5..2b6bfbe 100644 --- a/client/acme.go +++ b/client/acme.go @@ -79,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 @@ -171,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. @@ -278,8 +296,9 @@ func NewP2PForgeCertMgr(opts ...P2PForgeCertMgrOptions) (*P2PForgeCertMgr, error } magicCache := certmagic.NewCache(certmagic.CacheOptions{ - GetConfigForCert: configGetter, - Logger: mgrCfg.log.Desugar(), + GetConfigForCert: configGetter, + RenewCheckInterval: mgrCfg.renewCheckInterval, + Logger: mgrCfg.log.Desugar(), }) // Wire up final certmagic config by calling upstream New with sanity checks @@ -336,6 +355,18 @@ 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 } diff --git a/e2e_test.go b/e2e_test.go index 4a57443..b6354ee 100644 --- a/e2e_test.go +++ b/e2e_test.go @@ -126,6 +126,7 @@ func TestMain(m *testing.M) { // Need to handle .forgeDomain to return NODATA rather than NXDOMAIN per https://datatracker.ietf.org/doc/html/rfc8020 func TestRFC8020(t *testing.T) { + t.Parallel() _, pk, err := crypto.GenerateEd25519Key(rand.Reader) if err != nil { t.Fatal(err) @@ -200,6 +201,7 @@ func TestIPSubdomainsNonExistentRecords(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() domain := fmt.Sprintf("%s.%s.%s.", tt.subdomain, peerIDb36, forge) m := new(dns.Msg) m.Question = make([]dns.Question, 1) @@ -220,6 +222,7 @@ func TestIPSubdomainsNonExistentRecords(t *testing.T) { } func TestSetACMEChallenge(t *testing.T) { + t.Parallel() ctx, cancel := context.WithCancel(context.Background()) defer cancel() sk, _, err := crypto.GenerateEd25519Key(rand.Reader) @@ -332,6 +335,7 @@ func TestIPv4Lookup(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() m := new(dns.Msg) m.Question = make([]dns.Question, 1) m.Question[0] = dns.Question{Qclass: dns.ClassINET, Name: fmt.Sprintf("%s.%s.%s.", tt.subdomain, peerIDb36, forge), Qtype: tt.qtype} @@ -448,6 +452,7 @@ func TestIPv6Lookup(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() m := new(dns.Msg) m.Question = make([]dns.Question, 1) m.Question[0] = dns.Question{Qclass: dns.ClassINET, Name: fmt.Sprintf("%s.%s.%s.", tt.subdomain, peerIDb36, forge), Qtype: tt.qtype} @@ -492,15 +497,26 @@ func TestLibp2pACMEE2E(t *testing.T) { } tests := []struct { - name string - clientOpts []client.P2PForgeCertMgrOptions - isValidForgeAddr func(addr string) bool + name string + clientOpts []client.P2PForgeCertMgrOptions + isValidForgeAddr func(addr string) bool + caCertValidityPeriod uint64 // 0 means default from letsencrypt/pebble/ca/v2 will be used + awaitOnCertRenewed bool // include renewal test }{ { name: "default opts", clientOpts: []client.P2PForgeCertMgrOptions{}, isValidForgeAddr: isValidResolvedForgeAddr, }, + { + name: "expired cert gets renewed and triggers OnCertRenewed", + clientOpts: []client.P2PForgeCertMgrOptions{ + client.WithRenewCheckInterval(5 * time.Second), + }, + isValidForgeAddr: isValidResolvedForgeAddr, + caCertValidityPeriod: 30, // letsencrypt/pebble/v2/ca uses int as seconds + awaitOnCertRenewed: true, + }, { name: "explicit WithShortForgeAddrs(true)", clientOpts: []client.P2PForgeCertMgrOptions{client.WithShortForgeAddrs(true)}, @@ -515,10 +531,11 @@ func TestLibp2pACMEE2E(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() db := pebbleDB.NewMemoryStore() logger := log.New(os.Stdout, "", 0) - ca := pebbleCA.New(logger, db, "", 0, 1, 0) + ca := pebbleCA.New(logger, db, "", 0, 1, tt.caCertValidityPeriod) va := pebbleVA.New(logger, 0, 0, false, dnsServerAddress, db) wfeImpl := pebbleWFE.New(logger, db, va, ca, false, false, 3, 5) @@ -530,7 +547,7 @@ func TestLibp2pACMEE2E(t *testing.T) { } defer acmeHTTPListener.Close() - // Generate the self-signed certificate and private key + // Generate the self-signed certificate and private key for mocked ACME endpoint certPEM, privPEM, err := generateSelfSignedCert("127.0.0.1") if err != nil { log.Fatalf("Failed to generate self-signed certificate: %v", err) @@ -562,6 +579,7 @@ func TestLibp2pACMEE2E(t *testing.T) { acmeEndpoint := fmt.Sprintf("https://%s%s", acmeHTTPListener.Addr(), pebbleWFE.DirectoryPath) certLoaded := make(chan bool, 1) + certRenewed := make(chan bool, 1) clientOpts := append([]client.P2PForgeCertMgrOptions{ client.WithForgeDomain(forge), client.WithForgeRegistrationEndpoint(fmt.Sprintf("http://127.0.0.1:%d", httpPort)), client.WithCAEndpoint(acmeEndpoint), client.WithTrustedRoots(cas), @@ -574,6 +592,9 @@ func TestLibp2pACMEE2E(t *testing.T) { client.WithOnCertLoaded(func() { certLoaded <- true }), + client.WithOnCertRenewed(func() { + certRenewed <- true + }), }, tt.clientOpts...) certMgr, err := client.NewP2PForgeCertMgr(clientOpts...) @@ -654,6 +675,14 @@ func TestLibp2pACMEE2E(t *testing.T) { t.Fatal(err) } + if tt.awaitOnCertRenewed { + select { + case <-certRenewed: + case <-time.After(30 * time.Second): + t.Fatal("timed out waiting for certificate renewal") + } + } + }) } } @@ -672,10 +701,10 @@ func generateSelfSignedCert(ipAddr string) ([]byte, []byte, error) { template := x509.Certificate{ SerialNumber: serialNumber, Subject: pkix.Name{ - Organization: []string{"My Organization"}, + Organization: []string{"Test Mocked ACME Endpoint"}, }, NotBefore: time.Now(), - NotAfter: time.Now().Add(365 * 24 * time.Hour), // Valid for 1 year + NotAfter: time.Now().Add(1 * time.Hour), KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature, ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, BasicConstraintsValid: true,