-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Support ACME for certificate provisioning #4384
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #4384 +/- ##
===========================================
- Coverage 73.7% 73.58% -0.12%
===========================================
Files 300 301 +1
Lines 29705 29822 +117
Branches 4882 4894 +12
===========================================
+ Hits 21895 21946 +51
- Misses 6385 6446 +61
- Partials 1425 1430 +5 |
Note for reviewers (@richvdh , @erikjohnston ): this removes the I'm guessing I'll need to add another newsfile, or split that into a different PR. More info about why plain DHE is bad: https://weakdh.org/sysadmin.html |
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.
This is looking nice now.
Something I don't quite understand here. I thought you needed an account key to get certs from LE, and that you were supposed to reuse the same account key for subsequent renewals. I'm not seeing anything about that here... what am I missing?
As for the dh_params thing... I would love it if it could be pulled out to a completely separate PR, but if that's a bit of a faff I won't argue.
CI is sad, at least partially about dh_params.
Co-Authored-By: hawkowl <[email protected]>
@richvdh This is because it registers and uses a consistent |
@richvdh I'll pull the |
…/matrix-org/synapse into hawkowl/acme-portable-certificates
synapse/config/tls.py
Outdated
) | ||
self.acme_port = acme_config.get("port", 8449) | ||
self.acme_bind_addresses = acme_config.get("bind_addresses", ["127.0.0.1"]) | ||
self.acme_reprovision_threshold = acme_config.get("reprovision_threshold", 10) |
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.
LetsEncrypt as the main cert provider via ACME suggest renewal after 60 days (at 30 days to go). They currently send emails to warn of un-renewed certs at 20 and 10 days, so this may want to be about 30 days as a default.
synapse/config/tls.py
Outdated
acme_config = config.get("acme", {}) | ||
self.acme_enabled = acme_config.get("enabled", False) | ||
self.acme_url = acme_config.get( | ||
"url", "https://acme-staging.api.letsencrypt.org/directory" |
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.
Don't forget to set the default to be the live endpoint
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.
Also, do we want to consider using the v2 API now , if it's a simple change in our use of the library, to prevent us having to move to v2 when they decommission (timeline unspecified, so if it's a lot of work it is definitelyworth punting down the road)
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.
txacme does not yet support v2. We can probably put a little bit of work in to make it support it, later.
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.
WRT live endpoint -- I think it should be the staging one by default, since the real one has rate limits, and we dont' want someone to get accidentally blacklisted while setting up their server, I think.
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.
that feels like a lower risk then them wondering why their cert doesn't work, tbh. The fact that the whole thing is disabled by default is enough of a safetynet imho.
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.
hmm, okay
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.
lgtm otherwise, modulo @michaelkaye 's comments
Co-Authored-By: hawkowl <[email protected]>
…/matrix-org/synapse into hawkowl/acme-portable-certificates
No description provided.