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

Open
wants to merge 2 commits into
base: master
from

Conversation

@rkoe
Copy link
Contributor

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 15, 2020
sync fork
- add a note about account-rate-limits
- add a note about ec384-certificates as default
Copy link
Contributor

flokli left a comment

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

This comment has been minimized.

Copy link
@flokli

flokli May 18, 2020

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

This comment has been minimized.

Copy link
@flokli

flokli May 18, 2020

Contributor

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

This comment has been minimized.

Copy link
@flokli

flokli May 18, 2020

Contributor

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

This comment has been minimized.

Copy link
@flokli

flokli May 18, 2020

Contributor

email address

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

This comment has been minimized.

Copy link
@flokli

flokli May 18, 2020

Contributor
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>

This comment has been minimized.

Copy link
@flokli

flokli May 18, 2020

Contributor

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.

This comment has been minimized.

Copy link
@flokli

flokli May 18, 2020

Contributor

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

This comment has been minimized.

Copy link
@rkoe

rkoe May 18, 2020

Author Contributor

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

This comment has been minimized.

Copy link
@flokli

flokli May 18, 2020

Contributor

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

This comment has been minimized.

Copy link
@emilazy

emilazy May 18, 2020

Member

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

This comment has been minimized.

Copy link
@flokli

flokli May 25, 2020

Contributor

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 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?

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.

None yet

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