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

Revert "nixos/nginx: not "before" ACME certs using DNS validation" #382401

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Feb 15, 2025

This reverts commit 03122b4.

Consider the following setup:

  • a.example.com uses the HTTP-01 challenge.
  • b.example.com uses the DNS-01 challenge (and creates a wildcard cert for *.b.example.com).

This results in the following dependency cycle:

┌──────────────────┐    ┌─────────────────────┐
│acme-b.example.com│◄───┼acme-account-X.target│
└─────┬────────────┘    └───────▲─────────────┘
      │                         │
      │                         │
┌─────▼───────┐         ┌───────┼──────────┐
│nginx.service┼─────────►acme-a.example.com│
└─────────────┘         └──────────────────┘
  • acme-account-X.target runs after acme-a.example.com (this is the "leader", i.e the cert that gets created first without other challenges in parallel to create the LE account without a race condition).

  • acme-account-X.target is scheduled to be reached before acme-b.example.com.

  • Because of the change I'm suggesting to revert, acme-a.example.com and acme-b.example.com are scheduled after/before nginx which is the reason for this cycle.

    This is only the case if the "leader" ACME unit (i.e. the cert for the first host - alphabetically sorted) is using the HTTP-01 challenge.

When all these units are started in one transaction (e.g. when multi-user.target is started), this cycle gets noticed by systemd.


cc @ThinkChaos

@NixOS/acme would you be OK with a backport?

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

This reverts commit 03122b4.

Consider the following setup:

* a.example.com uses the HTTP-01 challenge.
* b.example.com uses the DNS-01 challenge (and creates a wildcard cert
  for *.b.example.com).

This results in the following dependency cycle:

    ┌──────────────────┐    ┌─────────────────────┐
    │acme-b.example.com│◄───┼acme-account-X.target│
    └─────┬────────────┘    └───────▲─────────────┘
	  │                         │
	  │                         │
    ┌─────▼───────┐         ┌───────┼──────────┐
    │nginx.service┼─────────►acme-a.example.com│
    └─────────────┘         └──────────────────┘

* `acme-account-X.target` runs *after* `acme-a.example.com`
  (this is the "leader", i.e the cert that gets created first
  without other challenges in parallel to create the LE account
  without a race condition).

* `acme-account-X.target` is scheduled to be reached *before*
  `acme-b.example.com`.

* Because of the change I'm suggesting to revert, `acme-a.example.com`
  and `acme-b.example.com` are scheduled after/before `nginx` which
  is the reason for this cycle.

  This is only the case if the "leader" ACME unit (i.e. the cert for the
  first host - alphabetically sorted) is using the HTTP-01 challenge.

When all these units are started in one transaction (e.g. when
`multi-user.target` is started), this cycle gets noticed by systemd.
@Ma27 Ma27 requested review from fpletz, RaitoBezarius, K900 and a team February 15, 2025 19:43
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Feb 15, 2025
@ThinkChaos
Copy link
Contributor

It's probably best to also revert the change in the other webservers (apache and caddy).
Related PR is #336412

@Ma27
Copy link
Member Author

Ma27 commented Feb 16, 2025

CC maintainers of caddy & httpd @lovek323 @techknowlogick

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Feb 17, 2025
@ThinkChaos
Copy link
Contributor

I didn't remember making that PR because it's been in limbo for a while, but I think this is better fixed by #355054. It ensures the service that creates the account is a cert that uses DNS validation, if one exists.
So in the example from above, we break the "acme-a.example.com before acme-account-X.target" dependency:

          ┌──────────────────┐
          │acme-b.example.com│    (DNS validation)
          └───┬────────────┬─┘
              │            │
              │            │
┌─────────────▼───────┐  ┌─▼───────────┐
│acme-account-X.target│  │nginx.service│
└─────────────┬───────┘  └─┬───────────┘
              │            │
              │            │
          ┌───▼────────────▼─┐
          │acme-a.example.com│    (HTTP validation)
          └──────────────────┘

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants