Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

acme_server sometimes delivers outdated intermediate certificate when clients ask for certificate renewal #4517

Closed
ronau opened this issue Jan 10, 2022 · 4 comments · Fixed by #4669
Labels
bug 🐞 Something isn't working
Milestone

Comments

@ronau
Copy link

ronau commented Jan 10, 2022

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.

@francislavoie francislavoie added the bug 🐞 Something isn't working label Jan 10, 2022
@mholt mholt added the help wanted 🆘 Extra attention is needed label Feb 9, 2022
@francislavoie
Copy link
Member

francislavoie commented Mar 25, 2022

@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
  • This sets up the authority, with a authority.WithX509Signer 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?

@maraino
Copy link
Contributor

maraino commented Mar 25, 2022

@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 to be able to use the injected function to load the intermediate chain and signer, so for example the CreateCertificate function can get the signer and chain like this:

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.

@francislavoie
Copy link
Member

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.

@maraino
Copy link
Contributor

maraino commented Mar 29, 2022

PR was merged to master smallstep/certificates#879

francislavoie added a commit that referenced this issue Mar 29, 2022
Fixes #4517

Big thanks to @maraino for adding an API in `smallstep/certificates` so that we can fix this
francislavoie added a commit that referenced this issue Apr 7, 2022
Fixes #4517

Big thanks to @maraino for adding an API in `smallstep/certificates` so that we can fix this
mholt added a commit that referenced this issue Apr 13, 2022
* caddypki: Load intermediate for signing on-the-fly

Fixes #4517

Big thanks to @maraino for adding an API in `smallstep/certificates` so that we can fix this

* Debug log

* Trying a hunch, does it need to be a pointer receiver?

* Clarify pointer receiver

Co-authored-by: Matt Holt <[email protected]>

Co-authored-by: Matt Holt <[email protected]>
@mholt mholt removed the help wanted 🆘 Extra attention is needed label Apr 13, 2022
@mholt mholt added this to the v2.5.0 milestone Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants