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: more 20.03 release notes updates #87911

Closed
wants to merge 2 commits into from

Conversation

rkoe
Copy link
Contributor

@rkoe rkoe commented May 15, 2020

Motivation for this change

Failures during 20.03 upgrade.

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.

rkoe added 2 commits May 16, 2020 00:13
- add a note about account-rate-limits
- add a note about ec384-certificates as default
Copy link
Contributor

@flokli flokli left a comment

Choose a reason for hiding this comment

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

Some notes. Also, please rebase the commit on top of master, squash it to one commit, and update the PR title to comething matching CONTRIBUTING.md.

have consequences if you embed your public key in apps.
</para>
<para>
A separate Let's-Encyrpt-account is registered for each distinctive
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
A separate Let's-Encyrpt-account is registered for each distinctive
A separate Let's-Encrypt-account is registered for each distinctive

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the rest of the changelog/documentation seems to be pretty Let's-Encrypt unspecific - the module uses the ACME protocol, and Let's-Encrypt is the default provider. I'd propose to reword this to match this style.

<para>
A separate Let's-Encyrpt-account is registered for each distinctive
<literal>security.acme.email</literal>/<literal>security.acme.certs.&lt;name&gt;.email</literal>.
If you have several certificates, this may trigger Let's-Encrypt account rate limits during the update
Copy link
Contributor

Choose a reason for hiding this comment

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

ACME here, too.

A separate Let's-Encyrpt-account is registered for each distinctive
<literal>security.acme.email</literal>/<literal>security.acme.certs.&lt;name&gt;.email</literal>.
If you have several certificates, this may trigger Let's-Encrypt account rate limits during the update
(currently max. 10 accounts each 3h). To prevent this, you can use the same email-address for all your
Copy link
Contributor

Choose a reason for hiding this comment

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

email address

certificates.
</para>
</listitem>
<listitem><para>By default, now "ec384"-certificates are created instead of rsa-certificates.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<listitem><para>By default, now "ec384"-certificates are created instead of rsa-certificates.
<listitem><para>By default, now "ec384"-certificates are created instead of RSA certificates.

</para>
</listitem>
<listitem><para>By default, now "ec384"-certificates are created instead of rsa-certificates.
You may want to change this by setting <literal>security.acme.certs.&lt;name&gt;.keyType</literal>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a link referring to the option specified here.

</listitem>
<listitem><para>By default, now "ec384"-certificates are created instead of rsa-certificates.
You may want to change this by setting <literal>security.acme.certs.&lt;name&gt;.keyType</literal>
back to e.g. <literal>rsa4096</literal>, especially if you are using the certificate for a mailserver.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate a bit on this? Why do my mailservers need different certificates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For interoperatbility, mailservers need RSA certificates.
There are several mailservers out there, which do not yet support ec384-certificates. Now, if you use ec384-certificates, these mailservers cannot send mail to you and send bounce-messages back to the sender (even if encryption is optional in your mailserver). So, if you do this, you will lose mails, cause bounces and be unsubscribed from mailinglists.

So, for a mailserver, you must have an RSA certificate.
See also: http://postfix.1071664.n5.nabble.com/TLS-problem-no-shared-cipher-tp105970p105979.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks to the explanation! We should add a small part of it to the security.acme.certs.<name>.keyType option, WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Note that on master (but not 20.03) the default key type is ec256 instead; see #83121 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

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

We backported the change to use ec256 to 20.03 as well, as that's what lego will default to in their next version: go-acme/lego#1091

@flokli
Copy link
Contributor

flokli commented May 18, 2020

cc @NixOS/acme for the content.

@flokli flokli added the 9.needs: port to stable A PR needs a backport to the stable release. label May 18, 2020
@flokli flokli changed the title Rkoe release notes 20.03 acme nixos/acme: more 20.03 release notes updates May 18, 2020
@arianvp
Copy link
Member

arianvp commented May 18, 2020

Content is fine LGTM!

@flokli
Copy link
Contributor

flokli commented May 25, 2020

@rkoe can you address the proposed changes?

@stale
Copy link

stale bot commented Nov 21, 2020

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 21, 2020
@arianvp arianvp closed this Nov 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: nixos 8.has: changelog 8.has: documentation 9.needs: port to stable A PR needs a backport to the stable release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants