Remove tls_fingerprints option #9280

jerinjtitus · 2021-01-31T01:18:31Z

Signed-off-by: Jerin J Titus [email protected]

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
Pull request includes a sign off
Code style is correct (run the linters)

Signed-off-by: Jerin J Titus <[email protected]>

anoadragon453 · 2021-02-02T14:49:13Z

The diff in this PR looked to be a little mixed due to a bad merge or similar. I've removed everything besides the intended changes.

clokep · 2021-02-02T17:22:33Z

This has some unused imports:

synapse/config/tls.py:20:1: F401 'hashlib.sha256' imported but unused
synapse/config/tls.py:23:1: F401 'unpaddedbase64.encode_base64' imported but unused
scripts-dev/convert_server_keys.py:1:1: F401 'hashlib' imported but unused

Signed-off-by: Jerin J Titus <[email protected]>

clokep · 2021-02-04T15:43:38Z

I wonder if we should raise an error if this value is added to the config? I don't know our process for removing values from the config. 😢

richvdh · 2021-02-04T16:00:44Z

I don't know our process for removing values from the config.

IIRC there are a number of removed config options that make synapse print/log errors if you try to use them, so that's an option.

In this case I don't think setting tls_fingerprints is actively harmful, so I don't think there's any need to error out.

clokep

I think this looks good overall. There seems to be a reference in synapse.app._base to the tls_fingerprints which shows that the read_certificate_from_disk method can be further simplified...I think. 😄

changelog.d/9280.misc

jerinjtitus · 2021-02-05T13:40:08Z

I think this looks good overall. There seems to be a reference in synapse.app._base to the tls_fingerprints which shows that the read_certificate_from_disk method can be further simplified...I think.

Yes, I had that doubt. It would be great if you can give some insight into it's working, so that I could simplify it. The comment
# we only need the certificate for the tls_fingerprints.
made me think that the whole function's purpose was around tls_fingerprints option, but not sure how exactly.

Signed-off-by: Jerin J Titus <[email protected]>

clokep · 2021-02-05T13:49:12Z

I think this looks good overall. There seems to be a reference in synapse.app._base to the tls_fingerprints which shows that the read_certificate_from_disk method can be further simplified...I think.

Yes, I had that doubt. It would be great if you can give some insight into it's working, so that I could simplify it. The comment
# we only need the certificate for the tls_fingerprints.
made me think that the whole function's purpose was around tls_fingerprints option, but not sure how exactly.

In the case where there's no TLS listeners the only reason that access to the certificate is needed is to add it to the tls_fingerprints option, so if we're not loading that anymore.

If we take a look at read_certificate_from_disk with your changes and consider require_cert_and_key to be False, then the only thing it does is (maybe) set self.tls_certificate. So I think the call to it from _base with require_cert_and_key=False can go away and then we can simplify the code to always load the certificates.

Looking at it a bit more I'm actually unsure that tls_certificate is even necessary at all after this change?

jerinjtitus · 2021-02-15T15:08:42Z

Looking at it a bit more I'm actually unsure that tls_certificate is even necessary at all after this change?

I am doubtful about that too. Should we file a new issue on it?

clokep · 2021-02-16T13:24:11Z

Looking at it a bit more I'm actually unsure that tls_certificate is even necessary at all after this change?

I am doubtful about that too. Should we file a new issue on it?

I see no reason not to remove it as part of this.

jerinjtitus · 2021-02-17T15:09:21Z

I see no reason not to remove it as part of this.

I suggested it because there was an ambiguity on if it's necessary or not. If we are sure it's not, then I can start working on it.

clokep · 2021-04-08T12:32:48Z

I see no reason not to remove it as part of this.

I suggested it because there was an ambiguity on if it's necessary or not. If we are sure it's not, then I can start working on it.

I didn't see it used, but it is worth double checking! We can also remove it and see if anything breaks. 🤷

clokep · 2021-04-23T17:39:52Z

I merged develop into this since it is quite out of date.

richvdh · 2021-05-13T17:58:22Z

we should decide if this is ready to merge, or if it should be closed.

erikjohnston · 2021-05-14T09:54:17Z

I think it makes sense to merge? No server since v1.0 should be looking at TLS fingerprints right?

(I added need discussion to make sure we actually make a decision sometime this cycle)

richvdh · 2021-05-14T12:00:09Z

I think it makes sense to merge? No server since v1.0 should be looking at TLS fingerprints right?

yes, we can certainly remove the tls_fingerprints value from the /keys response (if that's not already done). The question is whether this PR does that - it just needs a review.

clokep · 2021-05-17T12:31:18Z

I think after this PR there's some additional dead code which could be removed. We could do that after the fact though.

richvdh · 2021-05-24T15:50:52Z

scripts-dev/convert_server_keys.py

@@ -54,15 +53,9 @@ def convert_v1_to_v2(server_name, valid_until, keys, certificate):
        "server_name": server_name,
        "verify_keys": {key_id: {"key": key} for key_id, key in keys.items()},
        "valid_until_ts": valid_until,
-        "tls_fingerprints": [fingerprint(certificate)],


I don't think this is necessary, but on the other hand, I don't think this script is useful any more.

richvdh · 2021-05-24T15:55:13Z

synapse/rest/key/v2/local_key_resource.py

        json_object = {
            "valid_until_ts": self.valid_until_ts,
            "server_name": self.config.server_name,
            "verify_keys": verify_keys,
            "old_verify_keys": old_verify_keys,
-            "tls_fingerprints": tls_fingerprints,


for reference: this has been unspecced since at least the first version of the server-server API: https://matrix.org/docs/spec/server_server/r0.1.0#get-matrix-key-v2-server-keyid.

we don't need to reload the tls cert if we don't have any tls listeners. Follow-up to #9280.

#9280 has made it into a release, so we need a new changelog

we don't need to reload the tls cert if we don't have any tls listeners. Follow-up to #9280.

@junquera

Synapse 1.35.1 (2021-06-03) =========================== Bugfixes -------- - Fix a bug introduced in v1.35.0 where invite-only rooms would be shown to all users in a space, regardless of if the user had access to it. ([\#10109](matrix-org/synapse#10109)) Synapse 1.35.0 (2021-06-01) =========================== Note that [the tag](https://github.com/matrix-org/synapse/releases/tag/v1.35.0rc3) and [docker images](https://hub.docker.com/layers/matrixdotorg/synapse/v1.35.0rc3/images/sha256-34ccc87bd99a17e2cbc0902e678b5937d16bdc1991ead097eee6096481ecf2c4?context=explore) for `v1.35.0rc3` were incorrectly built. If you are experiencing issues with either, it is recommended to upgrade to the equivalent tag or docker image for the `v1.35.0` release. Deprecations and Removals ------------------------- - The core Synapse development team plan to drop support for the [unstable API of MSC2858](https://github.com/matrix-org/matrix-doc/blob/master/proposals/2858-Multiple-SSO-Identity-Providers.md#unstable-prefix), including the undocumented `experimental.msc2858_enabled` config option, in August 2021. Client authors should ensure that their clients are updated to use the stable API (which has been supported since Synapse 1.30) well before that time, to give their users time to upgrade. ([\#10101](matrix-org/synapse#10101)) Bugfixes -------- - Fixed a bug causing replication requests to fail when receiving a lot of events via federation. Introduced in v1.33.0. ([\#10082](matrix-org/synapse#10082)) - Fix HTTP response size limit to allow joining very large rooms over federation. Introduced in v1.33.0. ([\#10093](matrix-org/synapse#10093)) Internal Changes ---------------- - Log method and path when dropping request due to size limit. ([\#10091](matrix-org/synapse#10091)) Synapse 1.35.0rc2 (2021-05-27) ============================== Bugfixes -------- - Fix a bug introduced in v1.35.0rc1 when calling the spaces summary API via a GET request. ([\#10079](matrix-org/synapse#10079)) Synapse 1.35.0rc1 (2021-05-25) ============================== Features -------- - Add experimental support to allow a user who could join a restricted room to view it in the spaces summary. ([\#9922](matrix-org/synapse#9922), [\#10007](matrix-org/synapse#10007), [\#10038](matrix-org/synapse#10038)) - Reduce memory usage when joining very large rooms over federation. ([\#9958](matrix-org/synapse#9958)) - Add a configuration option which allows enabling opentracing by user id. ([\#9978](matrix-org/synapse#9978)) - Enable experimental support for [MSC2946](matrix-org/matrix-spec-proposals#2946) (spaces summary API) and [MSC3083](matrix-org/matrix-spec-proposals#3083) (restricted join rules) by default. ([\#10011](matrix-org/synapse#10011)) Bugfixes -------- - Fix a bug introduced in v1.26.0 which meant that `synapse_port_db` would not correctly initialise some postgres sequences, requiring manual updates afterwards. ([\#9991](matrix-org/synapse#9991)) - Fix `synctl`'s `--no-daemonize` parameter to work correctly with worker processes. ([\#9995](matrix-org/synapse#9995)) - Fix a validation bug introduced in v1.34.0 in the ordering of spaces in the space summary API. ([\#10002](matrix-org/synapse#10002)) - Fixed deletion of new presence stream states from database. ([\#10014](matrix-org/synapse#10014), [\#10033](matrix-org/synapse#10033)) - Fixed a bug with very high resolution image uploads throwing internal server errors. ([\#10029](matrix-org/synapse#10029)) Updates to the Docker image --------------------------- - Fix bug introduced in Synapse 1.33.0 which caused a `Permission denied: '/homeserver.log'` error when starting Synapse with the generated log configuration. Contributed by Sergio Miguéns Iglesias. ([\#10045](matrix-org/synapse#10045)) Improved Documentation ---------------------- - Add hardened systemd files as proposed in [#9760](matrix-org/synapse#9760) and added them to `contrib/`. Change the docs to reflect the presence of these files. ([\#9803](matrix-org/synapse#9803)) - Clarify documentation around SSO mapping providers generating unique IDs and localparts. ([\#9980](matrix-org/synapse#9980)) - Updates to the PostgreSQL documentation (`postgres.md`). ([\#9988](matrix-org/synapse#9988), [\#9989](matrix-org/synapse#9989)) - Fix broken link in user directory documentation. Contributed by @junquera. ([\#10016](matrix-org/synapse#10016)) - Add missing room state entry to the table of contents of room admin API. ([\#10043](matrix-org/synapse#10043)) Deprecations and Removals ------------------------- - Removed support for the deprecated `tls_fingerprints` configuration setting. Contributed by Jerin J Titus. ([\#9280](matrix-org/synapse#9280)) Internal Changes ---------------- - Allow sending full presence to users via workers other than the one that called `ModuleApi.send_local_online_presence_to`. ([\#9823](matrix-org/synapse#9823)) - Update comments in the space summary handler. ([\#9974](matrix-org/synapse#9974)) - Minor enhancements to the `@cachedList` descriptor. ([\#9975](matrix-org/synapse#9975)) - Split multipart email sending into a dedicated handler. ([\#9977](matrix-org/synapse#9977)) - Run `black` on files in the `scripts` directory. ([\#9981](matrix-org/synapse#9981)) - Add missing type hints to `synapse.util` module. ([\#9982](matrix-org/synapse#9982)) - Simplify a few helper functions. ([\#9984](matrix-org/synapse#9984), [\#9985](matrix-org/synapse#9985), [\#9986](matrix-org/synapse#9986)) - Remove unnecessary property from SQLBaseStore. ([\#9987](matrix-org/synapse#9987)) - Remove `keylen` param on `LruCache`. ([\#9993](matrix-org/synapse#9993)) - Update the Grafana dashboard in `contrib/`. ([\#10001](matrix-org/synapse#10001)) - Add a batching queue implementation. ([\#10017](matrix-org/synapse#10017)) - Reduce memory usage when verifying signatures on large numbers of events at once. ([\#10018](matrix-org/synapse#10018)) - Properly invalidate caches for destination retry timings every (instead of expiring entries every 5 minutes). ([\#10036](matrix-org/synapse#10036)) - Fix running complement tests with Synapse workers. ([\#10039](matrix-org/synapse#10039)) - Fix typo in `get_state_ids_for_event` docstring where the return type was incorrect. ([\#10050](matrix-org/synapse#10050))

@junquera

Synapse 1.35.0 (2021-06-01) =========================== Note that [the tag](https://github.com/matrix-org/synapse/releases/tag/v1.35.0rc3) and [docker images](https://hub.docker.com/layers/matrixdotorg/synapse/v1.35.0rc3/images/sha256-34ccc87bd99a17e2cbc0902e678b5937d16bdc1991ead097eee6096481ecf2c4?context=explore) for `v1.35.0rc3` were incorrectly built. If you are experiencing issues with either, it is recommended to upgrade to the equivalent tag or docker image for the `v1.35.0` release. Deprecations and Removals ------------------------- - The core Synapse development team plan to drop support for the [unstable API of MSC2858](https://github.com/matrix-org/matrix-doc/blob/master/proposals/2858-Multiple-SSO-Identity-Providers.md#unstable-prefix), including the undocumented `experimental.msc2858_enabled` config option, in August 2021. Client authors should ensure that their clients are updated to use the stable API (which has been supported since Synapse 1.30) well before that time, to give their users time to upgrade. ([\#10101](matrix-org/synapse#10101)) Bugfixes -------- - Fixed a bug causing replication requests to fail when receiving a lot of events via federation. Introduced in v1.33.0. ([\#10082](matrix-org/synapse#10082)) - Fix HTTP response size limit to allow joining very large rooms over federation. Introduced in v1.33.0. ([\#10093](matrix-org/synapse#10093)) Internal Changes ---------------- - Log method and path when dropping request due to size limit. ([\#10091](matrix-org/synapse#10091)) Synapse 1.35.0rc2 (2021-05-27) ============================== Bugfixes -------- - Fix a bug introduced in v1.35.0rc1 when calling the spaces summary API via a GET request. ([\#10079](matrix-org/synapse#10079)) Synapse 1.35.0rc1 (2021-05-25) ============================== Features -------- - Add experimental support to allow a user who could join a restricted room to view it in the spaces summary. ([\#9922](matrix-org/synapse#9922), [\#10007](matrix-org/synapse#10007), [\#10038](matrix-org/synapse#10038)) - Reduce memory usage when joining very large rooms over federation. ([\#9958](matrix-org/synapse#9958)) - Add a configuration option which allows enabling opentracing by user id. ([\#9978](matrix-org/synapse#9978)) - Enable experimental support for [MSC2946](matrix-org/matrix-spec-proposals#2946) (spaces summary API) and [MSC3083](matrix-org/matrix-spec-proposals#3083) (restricted join rules) by default. ([\#10011](matrix-org/synapse#10011)) Bugfixes -------- - Fix a bug introduced in v1.26.0 which meant that `synapse_port_db` would not correctly initialise some postgres sequences, requiring manual updates afterwards. ([\#9991](matrix-org/synapse#9991)) - Fix `synctl`'s `--no-daemonize` parameter to work correctly with worker processes. ([\#9995](matrix-org/synapse#9995)) - Fix a validation bug introduced in v1.34.0 in the ordering of spaces in the space summary API. ([\#10002](matrix-org/synapse#10002)) - Fixed deletion of new presence stream states from database. ([\#10014](matrix-org/synapse#10014), [\#10033](matrix-org/synapse#10033)) - Fixed a bug with very high resolution image uploads throwing internal server errors. ([\#10029](matrix-org/synapse#10029)) Updates to the Docker image --------------------------- - Fix bug introduced in Synapse 1.33.0 which caused a `Permission denied: '/homeserver.log'` error when starting Synapse with the generated log configuration. Contributed by Sergio Miguéns Iglesias. ([\#10045](matrix-org/synapse#10045)) Improved Documentation ---------------------- - Add hardened systemd files as proposed in [#9760](matrix-org/synapse#9760) and added them to `contrib/`. Change the docs to reflect the presence of these files. ([\#9803](matrix-org/synapse#9803)) - Clarify documentation around SSO mapping providers generating unique IDs and localparts. ([\#9980](matrix-org/synapse#9980)) - Updates to the PostgreSQL documentation (`postgres.md`). ([\#9988](matrix-org/synapse#9988), [\#9989](matrix-org/synapse#9989)) - Fix broken link in user directory documentation. Contributed by @junquera. ([\#10016](matrix-org/synapse#10016)) - Add missing room state entry to the table of contents of room admin API. ([\#10043](matrix-org/synapse#10043)) Deprecations and Removals ------------------------- - Removed support for the deprecated `tls_fingerprints` configuration setting. Contributed by Jerin J Titus. ([\#9280](matrix-org/synapse#9280)) Internal Changes ---------------- - Allow sending full presence to users via workers other than the one that called `ModuleApi.send_local_online_presence_to`. ([\#9823](matrix-org/synapse#9823)) - Update comments in the space summary handler. ([\#9974](matrix-org/synapse#9974)) - Minor enhancements to the `@cachedList` descriptor. ([\#9975](matrix-org/synapse#9975)) - Split multipart email sending into a dedicated handler. ([\#9977](matrix-org/synapse#9977)) - Run `black` on files in the `scripts` directory. ([\#9981](matrix-org/synapse#9981)) - Add missing type hints to `synapse.util` module. ([\#9982](matrix-org/synapse#9982)) - Simplify a few helper functions. ([\#9984](matrix-org/synapse#9984), [\#9985](matrix-org/synapse#9985), [\#9986](matrix-org/synapse#9986)) - Remove unnecessary property from SQLBaseStore. ([\#9987](matrix-org/synapse#9987)) - Remove `keylen` param on `LruCache`. ([\#9993](matrix-org/synapse#9993)) - Update the Grafana dashboard in `contrib/`. ([\#10001](matrix-org/synapse#10001)) - Add a batching queue implementation. ([\#10017](matrix-org/synapse#10017)) - Reduce memory usage when verifying signatures on large numbers of events at once. ([\#10018](matrix-org/synapse#10018)) - Properly invalidate caches for destination retry timings every (instead of expiring entries every 5 minutes). ([\#10036](matrix-org/synapse#10036)) - Fix running complement tests with Synapse workers. ([\#10039](matrix-org/synapse#10039)) - Fix typo in `get_state_ids_for_event` docstring where the return type was incorrect. ([\#10050](matrix-org/synapse#10050))

jerinjtitus added 2 commits January 30, 2021 07:44

Remove tls_fingerprints option

c686fa8

Signed-off-by: Jerin J Titus <[email protected]>

Added changelog file

6a359e5

Signed-off-by: Jerin J Titus <[email protected]>

anoadragon453 force-pushed the remove-tls_fingerprints branch from 75840af to ed56bbb Compare February 2, 2021 14:48

anoadragon453 requested a review from a team February 2, 2021 14:49

clokep removed the request for review from a team February 2, 2021 17:22

removed unused imports

5de842c

Signed-off-by: Jerin J Titus <[email protected]>

jerinjtitus force-pushed the remove-tls_fingerprints branch 2 times, most recently from 1f63937 to 5de842c Compare February 3, 2021 01:53

fixing sample config

9fa38f7

Signed-off-by: Jerin J Titus <[email protected]>

jerinjtitus force-pushed the remove-tls_fingerprints branch from a7486f0 to 9fa38f7 Compare February 3, 2021 02:31

clokep requested a review from a team February 3, 2021 12:27

clokep reviewed Feb 5, 2021

View reviewed changes

changelog.d/9280.misc Outdated Show resolved Hide resolved

Updated changelog.d file

e0e561f

Signed-off-by: Jerin J Titus <[email protected]>

Merge branch 'develop' into remove-tls_fingerprints

3cbea42

richvdh requested a review from a team May 13, 2021 17:57

erikjohnston added the X-Needs-Discussion label May 14, 2021

richvdh removed the X-Needs-Discussion label May 24, 2021

richvdh reviewed May 24, 2021

View reviewed changes

richvdh approved these changes May 24, 2021

View reviewed changes

Merge branch 'develop' into remove-tls_fingerprints

40d3875

richvdh enabled auto-merge (squash) May 24, 2021 16:02

richvdh merged commit 057ce7b into matrix-org:develop May 24, 2021

richvdh added a commit that referenced this pull request May 24, 2021

Remove redundant code to reload tls cert

c173e2b

we don't need to reload the tls cert if we don't have any tls listeners. Follow-up to #9280.

richvdh mentioned this pull request May 24, 2021

Remove redundant code to reload tls cert #10054

Merged

richvdh added a commit that referenced this pull request May 25, 2021

update changelog

d1044e1

#9280 has made it into a release, so we need a new changelog

richvdh added a commit that referenced this pull request May 27, 2021

Remove redundant code to reload tls cert (#10054)

fe5dad4

we don't need to reload the tls cert if we don't have any tls listeners. Follow-up to #9280.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove tls_fingerprints option #9280

Remove tls_fingerprints option #9280

jerinjtitus commented Jan 31, 2021 •

edited

Loading

anoadragon453 commented Feb 2, 2021 •

edited

Loading

clokep commented Feb 2, 2021

clokep commented Feb 4, 2021

richvdh commented Feb 4, 2021

clokep left a comment

jerinjtitus commented Feb 5, 2021

clokep commented Feb 5, 2021

jerinjtitus commented Feb 15, 2021

clokep commented Feb 16, 2021

jerinjtitus commented Feb 17, 2021 •

edited

Loading

clokep commented Apr 8, 2021

clokep commented Apr 23, 2021

richvdh commented May 13, 2021

erikjohnston commented May 14, 2021

richvdh commented May 14, 2021

clokep commented May 17, 2021

richvdh May 24, 2021

richvdh May 24, 2021

Remove tls_fingerprints option #9280

Remove tls_fingerprints option #9280

Conversation

jerinjtitus commented Jan 31, 2021 • edited Loading

Pull Request Checklist

anoadragon453 commented Feb 2, 2021 • edited Loading

clokep commented Feb 2, 2021

clokep commented Feb 4, 2021

richvdh commented Feb 4, 2021

clokep left a comment

Choose a reason for hiding this comment

jerinjtitus commented Feb 5, 2021

clokep commented Feb 5, 2021

jerinjtitus commented Feb 15, 2021

clokep commented Feb 16, 2021

jerinjtitus commented Feb 17, 2021 • edited Loading

clokep commented Apr 8, 2021

clokep commented Apr 23, 2021

richvdh commented May 13, 2021

erikjohnston commented May 14, 2021

richvdh commented May 14, 2021

clokep commented May 17, 2021

richvdh May 24, 2021

Choose a reason for hiding this comment

richvdh May 24, 2021

Choose a reason for hiding this comment

jerinjtitus commented Jan 31, 2021 •

edited

Loading

anoadragon453 commented Feb 2, 2021 •

edited

Loading

jerinjtitus commented Feb 17, 2021 •

edited

Loading