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/acme: update documentation #86347

Merged
merged 3 commits into from May 4, 2020
Merged

nixos/acme: update documentation #86347

merged 3 commits into from May 4, 2020

Conversation

@m1cr0man
Copy link
Contributor

m1cr0man commented Apr 29, 2020

Motivation for this change

Closes #86114

Revamped the documentation for generating certs as a whole, on top of
adding instructions for DNS validation. I also updated the docs for security.acme.certs.<name>.extraDomains since alt webroots are not supported.

These docs should be updated again when #83474 is merged, since it is unlikely users
want to set up a whole DNS server just for certificates.

Things done
  • Ensured the docs can be built
  • 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.
@Mic92
Copy link
Contributor

Mic92 commented Apr 29, 2020

Thanks for writing this!

Copy link
Member

emilazy left a comment

LGTM. Recommending BIND pains me, but it's good to have a working DNS-01 example :)

@flokli flokli requested a review from NinjaTrappeur Apr 30, 2020
Copy link
Contributor

flokli left a comment

Thanks a lot for writing this together :-) Some comments.

A list of extra domain names, which are included in the one certificate to be issued, with their
own server roots if needed.
A list of extra domain names, which are included in the one certificate to be issued.
Setting a distinct server root is deprecated and not functional in 20.03+

This comment has been minimized.

Copy link
@flokli

flokli Apr 30, 2020

Contributor
Suggested change
Setting a distinct server root is deprecated and not functional in 20.03+
Support for configuring distinct challenge webroots has been dropped.
Challenges are saved from the same folder.

I had to think two times here what "server root" means.

This was a way to provide an alternative webroot to serve challenges for that specific domain.

Am I right to assume lego doesn't support that anymore, and we put all challenges into the same folder (and nginx for example defaults the per-vhost acmeRoot option to the same locaiton).

We should also add this to the 20.03 release notes.

This comment has been minimized.

Copy link
@m1cr0man

m1cr0man May 1, 2020

Author Contributor

Correct, no support from lego. I'll add it to the release notes.

@@ -10,88 +10,238 @@
for Let's Encrypt. The alternative ACME client <literal>lego</literal> is

This comment has been minimized.

Copy link
@flokli

flokli Apr 30, 2020

Contributor

we support more than Let's Encrypt, via security.acme.server. One could also use Buypass, or an internal ACME server.

This comment has been minimized.

Copy link
@m1cr0man

m1cr0man May 1, 2020

Author Contributor

Yeah, that was hangover from the original docs. I'll make it more open about other providers :)

This address is only used for registration and renewal reminders,
and cannot be used to administer the certificates in any way.
</para>

This comment has been minimized.

Copy link
@flokli

flokli Apr 30, 2020

Contributor

We should add a note here explaining security.acme.server too.

<xref linkend="opt-security.acme.acceptTerms" /> = true;
<xref linkend="opt-security.acme.email" /> = "admin+acme@example.com";
services.nginx = {
<link linkend="opt-services.nginx.enable">enable = true;</link>

This comment has been minimized.

Copy link
@flokli

flokli Apr 30, 2020

Contributor
Suggested change
<link linkend="opt-services.nginx.enable">enable = true;</link>
<link linkend="opt-services.nginx.enable"> = true;</link>

or does this render differently?

I'd personally prefer the <link linkend="…">…</link> syntax, as used below.

This comment has been minimized.

Copy link
@m1cr0man

m1cr0man May 1, 2020

Author Contributor

Yeah link renders nothing on its own whereas xref puts in the option path. Although, I noticed the close tag was in the wrong place, fixed now.

</para>
</section>
<section xml:id="module-security-acme-configuring">
<title>Configuring</title>
<title>Manual configuration of HTTP-01 validation</title>

This comment has been minimized.

Copy link
@flokli

flokli Apr 30, 2020

Contributor

When do we need the manual way?

Doesn't the module system provide a much more convenient way to do this?

This comment has been minimized.

Copy link
@m1cr0man

m1cr0man May 1, 2020

Author Contributor

I feel it's good to keep this for 2 reasons, one is so people know how to integrate it with existing setups where maybe they do want to use port 80 for things and have a vhost there, and secondly explaining how the system works so that it can be adapted for other web servers is important.

This comment has been minimized.

Copy link
@flokli

flokli May 1, 2020

Contributor

I'm mostly worried about people just quickly skimming over this, and thinking "OMG, is this complicated". But as the easy setup is just on the same page, it should be fine. 🤔

This comment has been minimized.

Copy link
@NinjaTrappeur

NinjaTrappeur May 1, 2020

Contributor

I like this section. It's always good to show as many example setup as possible. It's in a different section clearly stating this is a manual conf. I think this is fine!

certs are overwritten when the ACME certs arrive. For
<literal>foo.example.com</literal> the config would look like.
This is useful if you want to generate a wildcard certificate, since
Let's Encrypt will only hand out wildcard certs over DNS validation.

This comment has been minimized.

Copy link
@flokli

flokli Apr 30, 2020

Contributor
Suggested change
Let's Encrypt will only hand out wildcard certs over DNS validation.
ACME servers will only hand out wildcard certs over DNS validation.
Nix config. Instead, generate them one time with these commands:
</para>

<programlisting>

This comment has been minimized.

Copy link
@flokli

flokli Apr 30, 2020

Contributor

There's no such /var/secrets in nixpkgs. There's /run/keys for nonlocal secrets, and some occurences of/var/lib/secrets for more persistent ones. I'd propose to use the latter in this example.

This comment has been minimized.

Copy link
@m1cr0man

m1cr0man May 1, 2020

Author Contributor

/var/secrets is a personal preference, I'll change it to /var/lib/secrets 👍

<para>
Now you're all set to generate certs! You should monitor the first invokation
by running <literal>systemctl start acme-example.com.service &amp;
journalctl -fu acme-example.com.service</literal> and watching for errors.

This comment has been minimized.

Copy link
@flokli

flokli Apr 30, 2020

Contributor
Suggested change
journalctl -fu acme-example.com.service</literal> and watching for errors.
journalctl -fu acme-example.com.service</literal> and watching for its log output.

That's a bit pessimistic :-D

@m1cr0man
Copy link
Contributor Author

m1cr0man commented May 1, 2020

Thanks for the solid review @flokli ! I've made those changes now :)

Copy link
Contributor

NinjaTrappeur left a comment

Thanks a lot for this, it's super useful <3!

LGTM.

See the comments for a remark and a couple style nitpicks.

A list of extra domain names, which are included in the one certificate to be issued, with their
own server roots if needed.
A list of extra domain names, which are included in the one certificate to be issued.
Setting a distinct server root is deprecated and not functional in 20.03+

This comment has been minimized.

Copy link
@NinjaTrappeur

NinjaTrappeur May 1, 2020

Contributor

This might be off-topic for this PR, but on top of this message, could we generate a eval-time warning using the assertion module

warnings = mkOption {
if acmeRoot!=null here?

Do we plan to backport this commit to 19.09? If not, I think this is safe to assume this option is deprecated and not functional, hence dropping the in 20.03+ part of this message.

This comment has been minimized.

Copy link
@NinjaTrappeur

NinjaTrappeur May 1, 2020

Contributor

Also, maybe it's also time to remove the acme root from this example:

extraDomains = { "www.example.com" = null; "foo.example.com" = "/var/www/foo/"; };

This comment has been minimized.

Copy link
@m1cr0man

m1cr0man May 1, 2020

Author Contributor

So I actually tried doing that back when I did the DNS-01 update, but I couldn't figure out how to add a deprecation warning to a submodule (which is what the certs are). When I rewrite the whole module I'm actually going to add a warning and a rewrite rule to change them to a list.

</para>

<para>
You will need an HTTP server or DNS server for verification. For HTTP,

This comment has been minimized.

Copy link
@NinjaTrappeur

NinjaTrappeur May 1, 2020

Contributor
Suggested change
You will need an HTTP server or DNS server for verification. For HTTP,
You will need an HTTP or DNS server to perform the verification. For an HTTP verification,

Style nitpick, feel free to ignore if you don't like it.

This comment has been minimized.

Copy link
@m1cr0man

m1cr0man May 1, 2020

Author Contributor

Well, the verification is more of a process so a proper noun rather than a concrete noun, personally I think it reads more naturally the way it is.

<filename>.well-known/acme-challenge</filename>. This directory must be
writeable by the user that will run the ACME client.
writeable by the user that will run the ACME client. For DNS, you must

This comment has been minimized.

Copy link
@NinjaTrappeur

NinjaTrappeur May 1, 2020

Contributor
Suggested change
writeable by the user that will run the ACME client. For DNS, you must
writeable by the user that will run the ACME client. For a DNS verification, you must

Style nitpick, feel free to ignore if you don't like it.

<para>
Automatic cert validation and configuration for Apache and Nginx virtual
hosts is included in NixOS, however if you would like to generate a wildcard
cert or you are not using a web server you will have to configure DNS

This comment has been minimized.

Copy link
@NinjaTrappeur

NinjaTrappeur May 1, 2020

Contributor
Suggested change
cert or you are not using a web server you will have to configure DNS
cert or you are not using a web server you will have to use a DNS

Style nitpick, feel free to ignore if you don't like it.

</para>
</section>
<section xml:id="module-security-acme-configuring">
<title>Configuring</title>
<title>Manual configuration of HTTP-01 validation</title>

This comment has been minimized.

Copy link
@NinjaTrappeur

NinjaTrappeur May 1, 2020

Contributor

I like this section. It's always good to show as many example setup as possible. It's in a different section clearly stating this is a manual conf. I think this is fine!

@flokli flokli requested review from NinjaTrappeur and emilazy May 3, 2020
@flokli flokli merged commit 7457c78 into NixOS:master May 4, 2020
14 checks passed
14 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="037ef70"; rev="037ef70d5cbacc9850f5c52d19b16cc41cd5efae"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="037ef70"; rev="037ef70d5cbacc9850f5c52d19b16cc41cd5efae"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="037ef70"; rev="037ef70d5cbacc9850f5c52d19b16cc41cd5efae"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="037ef70"; rev="037ef70d5cbacc9850f5c52d19b16cc41cd5efae"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="037ef70"; rev="037ef70d5cbacc9850f5c52d19b16cc41cd5efae"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="037ef70"; rev="037ef70d5cbacc9850f5c52d19b16cc41cd5efae"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="037ef70"; rev="037ef70d5cbacc9850f5c52d19b16cc41cd5efae"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@flokli
Copy link
Contributor

flokli commented May 4, 2020

Thanks everyone! @m1cr0man this can probably also be backported to 20.03, right?

@m1cr0man
Copy link
Contributor Author

m1cr0man commented May 4, 2020

Yeah, it can. I'll create a backport PR.

@m1cr0man m1cr0man mentioned this pull request May 4, 2020
0 of 10 tasks complete
@flokli flokli mentioned this pull request May 11, 2020
6 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.