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

Clean ACME certificates #10782

Open
wants to merge 2 commits into
base: v3.0
Choose a base branch
from

Conversation

ldez
Copy link
Contributor

@ldez ldez commented Jun 4, 2024

What does this PR do?

Removes dead ACME certificates and revoked ACME certificates.

Motivation

Fixes #3376

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

Closes #8647

@traefiker traefiker added this to the 3.0 milestone Jun 4, 2024
@ldez ldez force-pushed the feat/acme-clean-certificates-3 branch 5 times, most recently from b5146f7 to cb69d8a Compare June 4, 2024 23:31
@kevinpollet kevinpollet self-assigned this Jun 6, 2024
@mmatur
Copy link
Member

mmatur commented Jun 7, 2024

Seems an interesting fix for v2.11. @kevinpollet @rtribotte @nmengin WDYT?

@kevinpollet
Copy link
Member

IMHO, bringing the OCSP check before the renewal in v2.11 is needed.

@ldez ldez force-pushed the feat/acme-clean-certificates-3 branch 3 times, most recently from c054d26 to d8fb597 Compare June 7, 2024 14:16
@nmengin
Copy link
Contributor

nmengin commented Jun 11, 2024

Hello there,

@mmatur, maybe I’ve missed something but, for me, this PR is an enhancement, not a bugfix.

Indeed, currently, unexpected certificates are renewed and this PR improves their management but the current behavior does not avoid using Traefik. For this reason IMO it’s an enhancement.

@ldez About the code itself I have a couple of questions:

OSCP check

Shouldn't this modification be part of a dedicated PR?
It’s like a feature in the feature, that may be brought as a second step?

Dead period

First, concerning the wording we could call it GracefulPeriod.

Then, even if allowing the users to set this period is a good addition to the original PR, I still have one concern: what happens if Traefik deletes unexpected certificates?

Let me explain the scenario I have in mind:

As a user, I generate my certificates using the DNS challenge, and I’ve set a GracefulPeriod. I do not know it but, my DNS credentials were changed. I still have routers that refer to the certificate resolver in error.

In the current situation, for existing certificates, Traefik generates an error once a day per certificate during their renewal.

With the proposal, except if I’ve missed something, the certificates are deleted and Traefik tries to create certificates during each configuration reload: the number of errors can become unmanageable.

For this reason, WDYT to add a check to ensure that no router are using a certificate before deleting it?

@ldez
Copy link
Contributor Author

ldez commented Jun 11, 2024

I’ve missed something but, for me, this PR is an enhancement, not a bugfix.

It's a bug fix from the POV of what OSCP revocation means.

Shouldn't this modification be part of a dedicated PR?
It’s like a feature in the feature, that may be brought as a second step?

The dead ACME certificates and the OSCP check are the same topic, so I will not split this PR.
I already talk about that with other maintainers.

what happens if Traefik deletes unexpected certificates?

This cannot happen based on the rules I apply.

With the proposal, except if I’ve missed something, the certificates are deleted and Traefik tries to create certificates during each configuration reload: the number of errors can become unmanageable.

You do not understand how Traefik currently works: Traefik tries to renew ACME certificates, even if they are not used, even if it is impossible to renew them, this creates a lot of errors.

With my PR:

  • this problem will be fixed and no error will happen.
  • the ACME certificates are NOT removed during configuration reload but during the renewal (24h)
  • the ACME certificates should NEVER be removed during configuration reload.

WDYT to add a check to ensure that no router are using a certificate before deleting it?

The certificates will be removed only when revoked or out-of-date (because no router uses it), and only during the renewal.
So no need for an extra check.

@emilevauge
Copy link
Member

There are ongoing discussions on this PR (bugfix or enhancement, need design review, etc). More news soon.

@ldez ldez changed the base branch from v3.0 to master June 19, 2024 16:46
@ldez ldez changed the base branch from master to v3.0 June 19, 2024 16:46
@ldez ldez force-pushed the feat/acme-clean-certificates-3 branch from 317f93f to c85387f Compare June 19, 2024 16:49
@rtribotte rtribotte self-assigned this Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants