Repository: caddyserver/caddy (language: go)

Issue:
acme_server sometimes delivers outdated intermediate certificate when clients ask for certificate renewal
This is a reminder for the issue reported over there in the Caddy Community: 
https://caddy.community/t/internal-ca-certificate-renewal-does-not-refresh-intermediate-cert-properly

Observed with Caddy version 2.4.6

Quoting @francislavoie for a summary of the observations:

> Strange. So it sounds like the acme_server directive still had the old intermediate cert in memory when performing renewal? There might be missing a thing to reload the certs used by the ACME server when the intermediate is renewed. I’ll try to take a look at the code soon to see if anything stands out.

First feedback (by @francislavoie):

> At a glance, it looks like the fix would be a bit complicated because of how the actual CA renewal process and the ACME server are decoupled. I have some work in progress to implement an event system in Caddy, and the event system would make this much easier to resolve (i.e. CA intermediate cert renewal would trigger an event, the ACME server could subscribe to that event and update the intermediate cert at that point).

> The other option is we could add a timer inside the ACME server to reload the intermediate cert from storage daily-ish which might be good enough.

As a workaround, force reloading Caddy every once in a while might help (as suggested by @francislavoie):

> Alright well as a workaround I recommend force reloading Caddy at least once a week (daily even better) for the time being so that it has a chance to refresh the intermediate cert in memory before the ACME server tries to issue certs. You can do this with docker-compose exec -w /etc/caddy caddy caddy reload --force I think. You can put this in a cron job or something.


Discussion / hints:
@maraino if you don't mind the ping, I think we could use some help to fix this one. The problem:

- Caddy maintains (renews) the intermediate cert, which has a 1-week lifetime, via the PKI app (global singleton instance)
- The `acme_server` module loads the intermediate cert in [`ca.NewAuthority`](https://github.com/caddyserver/caddy/blob/b8dbecb841bef1f1ec67419b871d23d66af7a3b1/modules/caddypki/acmeserver/acmeserver.go#L133)
- This sets up the authority, with a [`authority.WithX509Signer`](https://github.com/caddyserver/caddy/blob/4b75f3e2f09b77b98783ea4d6602391c7283f984/modules/caddypki/ca.go#L202) option
- If Caddy runs for over a week without reloading, the intermediate expires (renewed after 2/3 of its lifetime), but the `acme_server` module still has the old intermediate in memory
- I don't see any way to replace the intermediate in an already-configured authority
- Workaround is to tell users to reload Caddy daily to prevent the issue... but that requires external scripting or cron. Annoying. Or users can turn on `sign_with_root` which makes it use a cert that expires after 10 years... but that's also a terrible idea for other reasons obviously.

Maybe you have a better idea for how this could be fixed, but I'm thinking what would be nice is if there was a variant of `authority.WithX509Signer` that could take a closure that returns an intermediate cert, so we could always feed it the latest one on-demand, instead of loading it once and never being able to change it. Maybe like `authority.WithX509SignerFunc(func() (crt *x509.Certificate, s crypto.Signer))` as the signature?
@francislavoie I think it makes sense to implement something like that and it won't be hard to do. The way we do this in our hosted product is by automatically reloading the CA when the configuration changes, there're ways to do this without downtime. 

Besides adding the new configuration option that you propose, using a chain of intermediates (`[]*x509.Certificate`), the solution would be to change this [`SoftCAS`](https://github.com/smallstep/certificates/blob/a7959ac563ac3ce18e2b6bb5234ab0d24bf41c5f/cas/softcas/softcas.go#L34-L48) to be able to use the injected function to load the intermediate chain and signer, so for example the [`CreateCertificate`](https://github.com/smallstep/certificates/blob/a7959ac563ac3ce18e2b6bb5234ab0d24bf41c5f/cas/softcas/softcas.go#L51) function can get the signer and chain like this:


```golang
chain, signer := c.getIssuer()
```

And as very hacky solution would be to replace the certificate with a custom `http.ResponseWriter`, but doing this would be way more difficult than doing a PR in certificates. 

I should be able to implement this next, week, but we always welcome a PR.
Cool! Thanks!

Yeah I tried to dig through the call chain in `smallstep/certificates` and I kinda got lost since it's structured pretty differently to how I'm used to (I really call myself more of a "Caddy developer" than a "Go developer", I don't play around in other projects much), so probably better you than I.

No rush on this, it's not mission-critical right now.
PR was merged to master https://github.com/smallstep/certificates/pull/879

Fix patch:
diff --git a/go.mod b/go.mod
index f1b1166791d..1cdf88e8edf 100644
--- a/go.mod
+++ b/go.mod
@@ -17,7 +17,7 @@ require (
 	github.com/mholt/acmez v1.0.2
 	github.com/naoina/toml v0.1.1
 	github.com/prometheus/client_golang v1.12.1
-	github.com/smallstep/certificates v0.18.3-0.20220328195804-49de04661b85
+	github.com/smallstep/certificates v0.18.3-0.20220329212333-b42c1dfe64cf
 	github.com/smallstep/cli v0.18.0
 	github.com/smallstep/nosql v0.4.0
 	github.com/smallstep/truststore v0.11.0
diff --git a/go.sum b/go.sum
index 5ddf302bbc7..3abea980678 100644
--- a/go.sum
+++ b/go.sum
@@ -950,8 +950,8 @@ github.com/smallstep/assert v0.0.0-20180720014142-de77670473b5/go.mod h1:TC9A4+R
 github.com/smallstep/assert v0.0.0-20200723003110-82e2b9b3b262 h1:unQFBIznI+VYD1/1fApl1A+9VcBk+9dcqGfnePY87LY=
 github.com/smallstep/assert v0.0.0-20200723003110-82e2b9b3b262/go.mod h1:MyOHs9Po2fbM1LHej6sBUT8ozbxmMOFG+E+rx/GSGuc=
 github.com/smallstep/certificates v0.18.0/go.mod h1:8eHwHNg/bRWvNZo9S0uWFVMkS+LSpDYxM4//EgBhkFM=
-github.com/smallstep/certificates v0.18.3-0.20220328195804-49de04661b85 h1:iXoo8eraLn0rFbn92eVd8dNjj1jcHcYsOQaqssUPTZ4=
-github.com/smallstep/certificates v0.18.3-0.20220328195804-49de04661b85/go.mod h1:K4gQuZo7j4kzQb0zF0A2s6/kiPh3ZhmFThlHh2B+Gwg=
+github.com/smallstep/certificates v0.18.3-0.20220329212333-b42c1dfe64cf h1:Ky0hrHTjd/sXrLZj1rKEBYRo/3nmOUIk3xyqnrs04jY=
+github.com/smallstep/certificates v0.18.3-0.20220329212333-b42c1dfe64cf/go.mod h1:K4gQuZo7j4kzQb0zF0A2s6/kiPh3ZhmFThlHh2B+Gwg=
 github.com/smallstep/certinfo v1.5.2/go.mod h1:gA7HBbue0Wwr3kD60P2UtgTIFfMAOC66D3rzYhI0GZ4=
 github.com/smallstep/cli v0.18.0 h1:BslbUHuMfj/LbVHxuZ4Hv1sL+vAHHidqia4JRoCBwXs=
 github.com/smallstep/cli v0.18.0/go.mod h1:C8ZSfMm/pKdCHnN1C3Pc44bjIOkBbyuoyq6XjS/K9lI=
diff --git a/modules/caddypki/ca.go b/modules/caddypki/ca.go
index c9bdeee6beb..ca9ba469f5d 100644
--- a/modules/caddypki/ca.go
+++ b/modules/caddypki/ca.go
@@ -182,30 +182,51 @@ func (ca CA) IntermediateKey() interface{} {
 }
 
 // NewAuthority returns a new Smallstep-powered signing authority for this CA.
-func (ca CA) NewAuthority(authorityConfig AuthorityConfig) (*authority.Authority, error) {
+// Note that we receive *CA (a pointer) in this method to ensure the closure within it, which
+// executes at a later time, always has the only copy of the CA so it can access the latest,
+// renewed certificates since NewAuthority was called. See #4517 and #4669.
+func (ca *CA) NewAuthority(authorityConfig AuthorityConfig) (*authority.Authority, error) {
 	// get the root certificate and the issuer cert+key
 	rootCert := ca.RootCertificate()
-	var issuerCert *x509.Certificate
-	var issuerKey interface{}
+
+	// set up the signer; cert/key which signs the leaf certs
+	var signerOption authority.Option
 	if authorityConfig.SignWithRoot {
+		// if we're signing with root, we can just pass the
+		// cert/key directly, since it's unlikely to expire
+		// while Caddy is running (long lifetime)
+		var issuerCert *x509.Certificate
+		var issuerKey interface{}
 		issuerCert = rootCert
 		var err error
 		issuerKey, err = ca.RootKey()
 		if err != nil {
 			return nil, fmt.Errorf("loading signing key: %v", err)
 		}
+		signerOption = authority.WithX509Signer(issuerCert, issuerKey.(crypto.Signer))
 	} else {
-		issuerCert = ca.IntermediateCertificate()
-		issuerKey = ca.IntermediateKey()
+		// if we're signing with intermediate, we need to make
+		// sure it's always fresh, because the intermediate may
+		// renew while Caddy is running (medium lifetime)
+		signerOption = authority.WithX509SignerFunc(func() ([]*x509.Certificate, crypto.Signer, error) {
+			issuerCert := ca.IntermediateCertificate()
+			issuerKey := ca.IntermediateKey().(crypto.Signer)
+			ca.log.Debug("using intermediate signer",
+				zap.String("serial", issuerCert.SerialNumber.String()),
+				zap.String("not_before", issuerCert.NotBefore.String()),
+				zap.String("not_after", issuerCert.NotAfter.String()))
+			return []*x509.Certificate{issuerCert}, issuerKey, nil
+		})
 	}
 
 	opts := []authority.Option{
 		authority.WithConfig(&authority.Config{
 			AuthorityConfig: authorityConfig.AuthConfig,
 		}),
-		authority.WithX509Signer(issuerCert, issuerKey.(crypto.Signer)),
+		signerOption,
 		authority.WithX509RootCerts(rootCert),
 	}
+
 	// Add a database if we have one
 	if authorityConfig.DB != nil {
 		opts = append(opts, authority.WithDatabase(*authorityConfig.DB))


Test patch: