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

nixos/tests/acme: Fix fullchain validation #346023

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

m1cr0man
Copy link
Contributor

@m1cr0man m1cr0man commented Oct 2, 2024

Blocks #331721

In the next release of Pebble, the certificate
subject is no longer populated with a useful domain name. This change will refactor the fullchain validation assertions to avoid checking the subject line.

This was tested with both pebble 2.4.0 and 2.6.0

CC @NixOS/acme

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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

In the next release of Pebble, the certificate
subject is no longer populated with a useful domain name.
This change will refactor the fullchain validation assertions
to avoid checking the subject line.
@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Oct 2, 2024
@m1cr0man m1cr0man requested a review from a team October 2, 2024 22:56
@m1cr0man m1cr0man self-assigned this Oct 2, 2024
@m1cr0man m1cr0man mentioned this pull request Oct 2, 2024
14 tasks
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Oct 3, 2024
@afh
Copy link
Member

afh commented Oct 3, 2024

Result of nixpkgs-review pr 346023 run on aarch64-darwin 1

Silly me: nixos tests aren't run on darwin using nixpkgs 😅

@afh
Copy link
Member

afh commented Oct 3, 2024

Thanks for putting up this PR, @m1cr0man, very much appreciated 🙏

Unfortunately my knowledge on acme in general this part of nixpkgs is too limited to confidently review this PR.
I'd appreciate more context on the underlying issue, the proposed changes, and how to test them on a nixos system. That said I'd understand if explaining takes too much time or effort and your time is elsewhere is more useful.

@flokli
Copy link
Contributor

flokli commented Oct 3, 2024

This replaces the openssl crl2pkcs7 -nocrl -certfile /var/lib/acme/{cert_name}/fullchain.pem invocation to a command only taking the first cert (everything before the first "END CERTIFICATE", and then peeks at the DNS names, rather than subject, which contains all names, including what's in the subject.

The PR comment also says why:

In the next release of Pebble, the certificate
subject is no longer populated with a useful domain name. This change will refactor the fullchain validation assertions to avoid checking the subject line.

Changes LGTM, and were tested with both pebble versions, so let's merge this!

@flokli flokli merged commit 3398bb1 into NixOS:master Oct 3, 2024
27 checks passed
@afh
Copy link
Member

afh commented Oct 3, 2024

Thanks for the helpful context, @flokli, much appreciated.

@m1cr0man
Copy link
Contributor Author

m1cr0man commented Oct 3, 2024

That's exactly right @flokli , thank you for doing the write up and merge! 🙂

@m1cr0man m1cr0man deleted the acme-subject-test-fix branch October 3, 2024 10:51
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 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants