Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Support ACME for certificate provisioning #4384

Merged
merged 71 commits into from
Jan 23, 2019

Conversation

hawkowl
Copy link
Contributor

@hawkowl hawkowl commented Jan 14, 2019

No description provided.

@codecov-io
Copy link

codecov-io commented Jan 17, 2019

Codecov Report

Merging #4384 into develop will decrease coverage by 0.11%.
The diff coverage is 50%.

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

@hawkowl hawkowl requested a review from a team January 21, 2019 13:40
@hawkowl
Copy link
Contributor Author

hawkowl commented Jan 21, 2019

Note for reviewers (@richvdh , @erikjohnston ): this removes the dh_params functionality, and therefore the DHE (but not ECDHE) support. It is unlikely this was used in practice at all, as DHE has been outclassed by ECDHE and is only required for things like IE6. If it breaks anyone, they were using a scarily old and insecure TLS stack, and not one on a platform we support (nor should support).

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

Copy link
Member

@richvdh richvdh left a 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.

synapse/app/homeserver.py Show resolved Hide resolved
synapse/app/homeserver.py Outdated Show resolved Hide resolved
synapse/app/homeserver.py Outdated Show resolved Hide resolved
synapse/app/homeserver.py Outdated Show resolved Hide resolved
synapse/config/tls.py Outdated Show resolved Hide resolved
synapse/config/tls.py Show resolved Hide resolved
synapse/config/tls.py Outdated Show resolved Hide resolved
synapse/config/tls.py Outdated Show resolved Hide resolved
synapse/python_dependencies.py Show resolved Hide resolved
synapse/python_dependencies.py Show resolved Hide resolved
Co-Authored-By: hawkowl <hawkowl@atleastfornow.net>
@hawkowl
Copy link
Contributor Author

hawkowl commented Jan 22, 2019

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?

@richvdh This is because it registers and uses a consistent client.key. See https://github.com/matrix-org/synapse/pull/4384/files#diff-149d848a6b79ba9483b2e5e40ee029feR82 -- txacme terms it as a client key, not an account key.

@hawkowl
Copy link
Contributor Author

hawkowl commented Jan 22, 2019

@richvdh I'll pull the dh_params out, as it does need a bit of cleanup in other areas + documentation, and could really use its own newsfile. The changing of the cipher string shouldn't result in anything bad (as what's there should equate to what I've added) but will be more consistent across different versions of OpenSSL and is understandable without reading a man page (which I still had trouble deciphering), but is worth calling out anyway.

synapse/config/tls.py Outdated Show resolved Hide resolved
synapse/config/tls.py Outdated Show resolved Hide resolved
synapse/config/tls.py Outdated Show resolved Hide resolved
synapse/config/tls.py Show resolved Hide resolved
@hawkowl hawkowl requested a review from a team January 22, 2019 12:46
)
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)
Copy link
Contributor

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.

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"
Copy link
Contributor

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

Copy link
Contributor

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)

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, okay

Copy link
Member

@richvdh richvdh left a 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

synapse/app/homeserver.py Outdated Show resolved Hide resolved
richvdh and others added 4 commits January 23, 2019 15:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants