-
-
Notifications
You must be signed in to change notification settings - Fork 15.1k
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
simp_le: 0.9.0 -> 0.16.0 #71291
simp_le: 0.9.0 -> 0.16.0 #71291
Conversation
@GrahamcOfBorg test acme |
Trying to update There's something wrong with the acme-ci integration test setup:
Coming from there: https://github.com/certbot/certbot/blob/master/certbot-ci/certbot_integration_tests/conftest.py#L85 Investigating. |
Should I disable this test suite ( |
34c7826
to
00949e5
Compare
I have to go offline for today. Since this is a somehow critical situation (letsencrypt is broken), I disabled the certbot tests altogether. In order to re-enable them, we have to pass the I am not familiar enough with the nixos python infrastructure to know how to do it and did not manage to track down were this override should happen. I'll have a second look to this issue that tomorrow. In the meantime, feel free to either merge this PR with the disabled tests or edit it to pass the appropriate flag to the test runner. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in the changelog, this version does not work out of the box with the previous configuration, so the module needs an update as well.
Adding the required flag did the trick for me, but I'd like someone else who is more familiar with the whole acme module to do a review before merging :)
diff --git a/nixos/modules/security/acme.nix b/nixos/modules/security/acme.nix
index b321c04e574..e35ea0c7b92 100644
--- a/nixos/modules/security/acme.nix
+++ b/nixos/modules/security/acme.nix
@@ -69,9 +69,9 @@ let
plugins = mkOption {
type = types.listOf (types.enum [
"cert.der" "cert.pem" "chain.pem" "external.sh"
- "fullchain.pem" "full.pem" "key.der" "key.pem" "account_key.json"
+ "fullchain.pem" "full.pem" "key.der" "key.pem" "account_key.json" "account_reg.json"
]);
- default = [ "fullchain.pem" "full.pem" "key.pem" "account_key.json" ];
+ default = [ "fullchain.pem" "full.pem" "key.pem" "account_key.json" "account_reg.json" ];
description = ''
Plugins to enable. With default settings simp_le will
store public certificate bundle in <filename>fullchain.pem</filename>,
00949e5
to
66a3765
Compare
Thanks @WilliButz , Just updated with your requested change. The nixos test still fails though:
Will look into it in the next ~4 hours. In the meantime, if somebody familiar with this whole area understands what's happening, feel free to edit this PR. |
66a3765
to
2ac7594
Compare
Updated nixos tests to use acme-V02 endpoints. We are currently using an outdated We probably want to:
I won't have enough spare time to do that today, would someone be willing to share the migration effort here? |
@NinjaTrappeur I took a few minutes and packaged up Boulder in #71324 |
FWIW, i'm using this with the fix from @WilliButz and it works fine :) |
FWIW, i'm using this with the fix ***@***.*** and it works fine :)
What's the fix? Can you link to it? ;-)
|
@flokli I made a branch at manveru@956169b |
2ac7594
to
eeb275d
Compare
pkgs/tools/admin/certbot/default.nix
Outdated
josepy | ||
parsedatetime | ||
psutil | ||
pyRFC3339 | ||
pyopenssl | ||
pytest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this go to checkInputs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@GrahamcOfBorg build boulder certbot pebble simp_le |
(nitpick) |
I'm kind of puzzled by those ofborg errors. I tried to reproduce that locally on my machine and did not manage to trigger any of those.
This PR adds both the pebble and the boulder derivation to I'll wait for azlig's decision - is this pebble-based test good enough or should we rollback and fix the boulder one - to drop the useless commit. |
@NinjaTrappeur would you mind filing a backport PR? |
Sure. |
Backporting ACME v2 fix (#71291) to 19.09
Error build certbot in master branch:
|
With setuptools v41.2.0 same error. |
It does build on top of 0a4373a. We should probably bisect from there. Can you open a proper issue for tracking this? |
In becfba9 - error build |
The failure happens since we updated setuptools. If I am correct it is due to pypa/setuptools#1847. Upstream packages (in this case certbot) needs to fix this. |
@FRidh with setuptools v41.2.0 same error. |
Let's please not clutter this PR, but instead open a new issue, and properly start |
fixed certbot in 2db400d |
@FRidh does this fix need to be backported to 19.09 too? |
@FRidh does this fix need to be backported to 19.09 too?
I don't think so, 19.09 still uses setuptools `41.2.0`.
We are spamming notifications by sending messages to this PR. We should
open a new issue and ping the appropriate people if needed in the
future.
|
Motivation for this change
As of today, let's encrypt dropped ACME V1 support. We need to update
simp_le
to0.16.0
to support ACME V2.Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)Notify maintainers
cc @arianvp @flokli