Verify federation certs #133
Conversation
There was a problem hiding this comment.
so I'd like to see this behaviour be driven by a config option, so that we can get the code landed without breaking things.
We might also end up needing a whitelist for domains that can be trusted without a valid cert, or accepting certs signed by a given CA, but we can probably punt them for sydent for now.
|
Now uses a config option with the default to not verify federation certificates. We can change this in the future. |
Notwithstanding the business of whether it should be beside a config flag, this is incorrect. There is no need to wait for Synapse 1.0: we can enable this check as soon as we take a view that most of the ecosystem has fixed their certificates. For sure that is one of the requirements of a synapse 1.0 release, but there may be other reasons that Synapse 1.0 is not released. In short: a synapse 1.0 release is sufficient but not necessary. |
| # We don't use config options yet | ||
| self._options = ssl.CertificateOptions(verify=False) | ||
| def __init__(self, config): | ||
| self._options = ssl.CertificateOptions(verify=config.get("http", "federation.verifycerts")) |
There was a problem hiding this comment.
Are you sure this does what you want? "False" is a non-empty string...
(hint: https://docs.python.org/2.7/library/configparser.html#ConfigParser.RawConfigParser.getboolean)
sydent/sydent.py
Outdated
| @@ -77,6 +77,7 @@ | |||
| 'replication.https.bind_address': '::', | |||
| 'replication.https.port': '4434', | |||
| 'obey_x_forwarded_for': 'False', | |||
| 'federation.verifycerts': 'False', | |||
There was a problem hiding this comment.
I think there's a strong argument for setting this to True by default now; I'd prefer not to have to remember to revisit this. I don't think we have anyone pulling sydent updates regularly, and we're already at 60% and rising. Anyone who does update sydent (including matrix.org/vector.im/dinsic) can use the config setting to disable the check until they are ready to enforce it.
There was a problem hiding this comment.
Got it, setting to True then 👍
Closes #95
Should only be merged once we've released Synapse 1.0Behind an option flag now so nvm.Currently not tested.