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

Jump to bottom

nixos/acme: renew after rebuild and on boot #81371

Merged
merged 1 commit into from Mar 1, 2020
Merged

nixos/acme: renew after rebuild and on boot #81371

merged 1 commit into from Mar 1, 2020

Conversation

mweinelt
Copy link
Member

@mweinelt mweinelt commented Feb 29, 2020 •
edited

Fixes #81069

Motivation for this change

Changes to security.acme.certs.<fqdn> (like chmod, chown, extraDomains) should be realized after a rebuild, therefore add the acme service to multi-user.target as proposed by @infinisil in #81069.

This has the added benefit that the certificate is also checked for validity on boot.

Since the renewal check is local this will not be affected by any rate limits on the remote ACME service.

This is completely untested, I will get to that later tonight.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@aanderse @emilazy @flokli @infinisil @arianvp @m1cr0man

@emilazy
Copy link
Member

emilazy commented Feb 29, 2020

Looks good to me!

Doesn't need to be addressed in this PR, but a related issue is that the certificates aren't force-renewed after relevant certificate settings (like the ocspMustStaple I add in #80900) are changed. I'm not sure if there's a good way to achieve that automatically, or if it'd even be desired to, but at least a simple way for the end-user to explicitly force a renewal would be appreciated.

@lukateras
Copy link
Member

I've been running systemctl start acme-example.com manually, which is somewhat of a workaround :/

Doesn't need to be addressed in this PR, but a related issue is that the certificates aren't force-renewed after relevant certificate settings (like the ocspMustStaple I add in #80900) are changed.

On our side, I guess one thing that comes to mind is to hash all flags together and concoct some shell script that will force renewal when hash changes. lego upstream might be better placed to deal with this, however, and handling it there would benefit users of other distributions as well.

lukateras
lukateras approved these changes Feb 29, 2020
View reviewed changes
Copy link
Member

@lukateras lukateras left a comment

Choose a reason for hiding this comment

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

Makes sense per #81069 (comment) and lego renew behavior.

@emilazy
Copy link
Member

emilazy commented Feb 29, 2020

I've been running systemctl start acme-example.com manually, which is somewhat of a workaround :/

Is this intended for the PR issue or the one I pointed out? If the latter, then I'm not sure this works, since it'll just see that the cert is new enough and doesn't need renewing. I had to manually set validMinDays high to get it to renew.

@lukateras
Copy link
Member

Right, sorry. Doing that won't renew the cert only if options changed, for reasons you've described, and can only be useful if no certs were issued at all (like the issue that's addressed in this PR).

@lukateras lukateras merged commit 98cbc40 into NixOS:master Mar 1, 2020
@mweinelt mweinelt deleted the pr/acme-autostart branch March 1, 2020 13:39
@mweinelt mweinelt mentioned this pull request Mar 1, 2020
Merged
10 tasks
infinisil added a commit that referenced this pull request Mar 7, 2020
@infinisil
Merge pull request #81435 from mweinelt/pr/20.03/acme
883217d 
[20.03] nixos/acme: backport #81369 and #81371
@Ma27
Copy link
Member

Ma27 commented Mar 28, 2020

@mweinelt @yegortimoshenko @worldofpeace this seems to break nixos-containers: unfortunately a nixos-container only has an uplink after the boot is completed[1], however this dependency assumes that during the boot an ACME cert can be (re)requested.

I'd suggest that we only add this dependency if e.g. boot.isContainer is false.

Any further opinions? :)

[1] The host-side interface for a nixos-container is configured after it is started up (

nixpkgs/nixos/modules/virtualisation/containers.nix

Lines 178 to 229 in 90a3908

postStartScript = (cfg:
let
ipcall = cfg: ipcmd: variable: attribute:
if cfg.${attribute} == null then
''
if [ -n "${variable}" ]; then
${ipcmd} add ${variable} dev $ifaceHost
fi
''
else
''${ipcmd} add ${cfg.${attribute}} dev $ifaceHost'';
renderExtraVeth = name: cfg:
if cfg.hostBridge != null then
''
# Add ${name} to bridge ${cfg.hostBridge}
ip link set dev ${name} master ${cfg.hostBridge} up
''
else
''
echo "Bring ${name} up"
ip link set dev ${name} up
# Set IPs and routes for ${name}
${optionalString (cfg.hostAddress != null) ''
ip addr add ${cfg.hostAddress} dev ${name}
''}
${optionalString (cfg.hostAddress6 != null) ''
ip -6 addr add ${cfg.hostAddress6} dev ${name}
''}
${optionalString (cfg.localAddress != null) ''
ip route add ${cfg.localAddress} dev ${name}
''}
${optionalString (cfg.localAddress6 != null) ''
ip -6 route add ${cfg.localAddress6} dev ${name}
''}
'';
in
''
if [ -n "$HOST_ADDRESS" ] || [ -n "$LOCAL_ADDRESS" ] ||
[ -n "$HOST_ADDRESS6" ] || [ -n "$LOCAL_ADDRESS6" ]; then
if [ -z "$HOST_BRIDGE" ]; then
ifaceHost=ve-$INSTANCE
ip link set dev $ifaceHost up
${ipcall cfg "ip addr" "$HOST_ADDRESS" "hostAddress"}
${ipcall cfg "ip -6 addr" "$HOST_ADDRESS6" "hostAddress6"}
${ipcall cfg "ip route" "$LOCAL_ADDRESS" "localAddress"}
${ipcall cfg "ip -6 route" "$LOCAL_ADDRESS6" "localAddress6"}
fi
${concatStringsSep "\n" (mapAttrsToList renderExtraVeth cfg.extraVeths)}
fi
''
);
), so the uplink is broken until the container is up. (For the record: this is a known issue and e.g. reported in #69414.)

@mweinelt
Copy link
Member Author

SGTM.

Ma27 added a commit to Ma27/nixpkgs that referenced this pull request Mar 29, 2020
@Ma27
nixos/acme: don't depend on multi-user.target inside a container
1a5289f 
On boot, a container doesn't have an uplink and would run into a timeout
while waiting for cert renewal[1].

[1] NixOS#81371 (comment)
@Ma27 Ma27 mentioned this pull request Mar 29, 2020
Merged
10 tasks
Ma27 added a commit that referenced this pull request Mar 31, 2020
@Ma27
nixos/acme: don't depend on multi-user.target inside a container
55b0101 
On boot, a container doesn't have an uplink and would run into a timeout
while waiting for cert renewal[1].

[1] #81371 (comment)

(cherry picked from commit 1a5289f)
dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request May 21, 2020
@Ma27 @dtzWill
nixos/acme: don't depend on multi-user.target inside a container
b5478b6 
On boot, a container doesn't have an uplink and would run into a timeout
while waiting for cert renewal[1].

[1] NixOS#81371 (comment)

(cherry picked from commit 1a5289f)
stigok pushed a commit to stigok/nixpkgs that referenced this pull request Jun 12, 2020
@Ma27 @stigok
nixos/acme: don't depend on multi-user.target inside a container
461d485 
On boot, a container doesn't have an uplink and would run into a timeout
while waiting for cert renewal[1].

[1] NixOS#81371 (comment)

(cherry picked from commit 1a5289f)
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/issues-using-nixos-container-to-set-up-an-etcd-cluster/8438/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukateras lukateras lukateras approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ACME module does not request certificate afte rebuild
5 participants
@mweinelt @emilazy @lukateras @Ma27 @nixos-discourse