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

agnos: init at 0.1.0, nixos/agnos: init #351678

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

justinas
Copy link
Member

@justinas justinas commented Oct 27, 2024

Fixes #219372

Agnos lets you obtain certificates (including wildcards) from Let's Encrypt using DNS-01 challenges. It answers Let's Encrypt DNS queries on its own, bypassing the need for API calls to your DNS provider.

This PR introduces a derivation for agnos, as well as a NixOS module with a systemd timer for running it.

As of currently, the module is not as flexible/powerful as security.acme. For example, we don't do the dance that security.acme does to figure out whether certificates were actually renewed. For my use, I simply reload the services depending on TLS certs unconditionally (whether certs were renewed or not).

This module also implements a pattern that I have not seen used in nixpkgs previously: temporarilyOpenFirewall opens the required port (usually 53) in ExecStartPre, and closes it again in ExecStopPost. This suits agnos well, as agnos is intended to be a short-lived unit activated by a timer. If the reviewers see any problems with this approach, we could change this to a conventional, persistent openFirewall.

I have been successfully running this PR on top of 24.05 for about 3 months now on my personal servers.

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.

@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/` labels Oct 27, 2024
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 labels Oct 27, 2024
@krtab
Copy link
Contributor

krtab commented Oct 27, 2024

Thanks a lot for working on this! I am no nix expert, so I can merely say that I am impressed and happy with how you have integrated configuration and testing in the package.

For testing, you can have a look at the docker based infrastructure I am using here: https://github.com/krtab/agnos/tree/23590e3d2261169c3426ad6d4b9562772da8b454/test-docker. I am not too happy with it and was in the process of moving it to nix but the pebble and agnos conf can perhaps be reused here. (The bind9 is not used, I should delete it).

Also, the repo has a bunch of things that are not needed to build agnos itself, so if you want to be extra pedantic you can either pull the crate from crates io (but then you don't get the systemd units and test confs) or build an include list like the one I made in krtab/agnos@0358d4f. The biggest culprit in terms of size is the resources folder though so you can just exclude that if you're not building the doc.

In any case, please let me know if you need things upstream to help with the packaging effort, and thanks again.

@h7x4 h7x4 added 8.has: module (new) This PR adds a module in `nixos/` 8.has: tests This PR has tests labels Oct 28, 2024
@justinas
Copy link
Member Author

Also, the repo has a bunch of things that are not needed to build agnos itself

I don't think that's a huge deal. We often just fetchFromGitHub here in the Nixpkgs land, and a few extra files in the repo do not really hurt. 🙂

For testing, you can have a look at the docker based infrastructure I am using here

Oh I did look at it a lot for my implementation 🙂 I'm afraid re-using these configs verbatim might not be viable due to various reasons, but they were of huge help for me writing the NixOS test here.

We could in theory re-use the systemd units from the agnos repo via systemd.packages. But the units are so "thin" (not a lot of options defined) that I don't think it's necessarily worth the effort in this case - we have to override several things in the NixOS module anyway, and at that point, why bother.

@justinas justinas marked this pull request as ready for review October 30, 2024 11:50
@justinas justinas changed the title agnos: init at 0.1.0-beta.4, nixos/agnos: init agnos: init at 0.1.0, nixos/agnos: init Oct 30, 2024
@justinas
Copy link
Member Author

Bumped to v0.1.0.

Copy link
Contributor

@evanrichter evanrichter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me! I am running this PR in my homelab with nginx, replacing acme and acme-dns

I really like the temporarilyOpenFirewall option!

Agnos really seems to work well with the systemd timer trigger. I had such a hack going for temporarily running acme-dns (which expects to run indefinitely)

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 6, 2024
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 27, 2024
Copy link
Contributor

@evanrichter evanrichter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-approving, as this looks like just a rebase onto a newer checkout.

I also just tested the auto renewal timer by setting days = 75 using krtab/agnos#67 and everything works great

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 1, 2024
@justinas
Copy link
Member Author

Force-pushed with a single change: in the case where the nftables-based NixOS firewall is used, temporarilyOpenFirewall will use the existing temp-ports set instead of an additional rule for packets bound for agnos. Hopefully this is a better solution, although temp-ports seems to mostly be intended for use by nixos-firewall-tool.

after = [
"firewall.target"
"network-online.target"
"nftables.service"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if !useNftables will the service never start? or will systemd realize there is no nftables.service, and start agnos as if it wasn't specified in the service ordering?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, After does not imply Wants (that's why After is usually paired with Wants) nor Requires, only the latter of which would make the service fail to start if the referenced unit does not exist or fails to start.

The iptables and no-firewall varieties of the integration test should also ensure this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, thanks!

Copy link
Contributor

@evanrichter evanrichter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if these approvals help, but I would like to see this merged sometime

pkgs/by-name/ag/agnos/package.nix Outdated Show resolved Hide resolved
nixos/tests/all-tests.nix Outdated Show resolved Hide resolved
@justinas
Copy link
Member Author

justinas commented Jan 8, 2025

@FliegendeWurst thanks, I've applied your suggestions.

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 8, 2025
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 9, 2025
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 (new) This PR adds a module in `nixos/` 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 8.has: tests This PR has tests 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 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.

Package request: agnos
6 participants