Federation .well-known requests don't follow 308 redirects

[tulir]

Description

Synapse does not follow HTTP 308 (Permanent Redirect) redirects on .well-known files when resolving hosts for federation.

The problem seems to be that Synapse uses Twisted's RedirectAgent for handling .well-known redirects, which for whatever reason doesn't follow 308 redirects

Steps to reproduce

• Set up a server with .well-known delegation behind a 308 redirect
• Try to federate with that server

Expected result: federation works fine
Actual result: other servers will fail to fetch the .well-known and fall back to 8448, leading to federation not working

2020-08-07 21:50:39,593 - synapse.http.matrixfederationclient - 414 - INFO - GET-1685382- {GET-O-838751} [onscale.co.nz] Sending request: GET matrix://onscale.co.nz/_matrix/key/v2/server/ed25519%3Aa_YoJp; timeout 10.000000s
2020-08-07 21:50:39,594 - synapse.http.federation.well_known_resolver - 245 - INFO - GET-1685382- Fetching https://onscale.co.nz/.well-known/matrix/server
2020-08-07 21:50:40,851 - synapse.http.federation.well_known_resolver - 192 - INFO - GET-1685382- Error parsing well-known for b'onscale.co.nz': Non-200 response 308
2020-08-07 21:50:40,913 - synapse.http.federation.matrix_federation_agent - 250 - INFO - GET-1685382- Connecting to onscale.co.nz:8448
2020-08-07 21:50:49,598 - synapse.http.federation.matrix_federation_agent - 261 - INFO - GET-1685382- Failed to connect to onscale.co.nz:8448: HostnameAddress(hostname=b'onscale.co.nz', port=8448)

Version information

• Version: 1.18

[ryanc-me]

It's also worth noting that the Kubernetes Nginx Ingress controller uses HTTP 308 redirects by default (for http-to-https, apex-to-www, etc). The current behavior excludes most standard bare-metal Kubernetes setups.

Also note that the Matrix spec explicitly states "30x redirects should be followed" in the Resolving server names section (point #3).

As for fixing this issue, I guess the best approach is opening an issue/PR with twisted?

[clokep]

which for whatever reason doesn't follow 308 redirects

It likely has just never been implemented. I searched Twisted briefly and didn't find any ticket requesting it be implemented.

It's also worth noting that the Kubernetes Nginx Ingress controller uses HTTP 308 redirects by default (for http-to-https, apex-to-www, etc). The current behavior excludes most standard bare-metal Kubernetes setups.

Thanks for the context!

As for fixing this issue, I guess the best approach is opening an issue/PR with twisted?

That would be great! Seems like it should be fairly easy to support there. We could also attempt to fix it in the Synapse code-base until an updated version of Twisted is released.

[ryanc-me]

Thanks @clokep. I've opened a ticket with Twisted, and made a quick PR. This is a very small change so hopefully they can push it through quickly!

[ryanc-me]

Added a note to the docs regarding this issue, with some temporary workarounds: #8120

[ryanc-me]

Quick update: This issue has been fixed at the Twisted repo (twisted/twisted#1376). Now, we just need to wait for a release, then update the Twisted version being used by Synapse!

However, because this issue originates with the destination server during a federation attempt, all servers must update to the newer version of Twisted before the issue is fully resolved. If you're using 308 redirects on your homeserver, you will not be able to federate with any servers that are using an older version of Twisted. It could be 6-12 months before the majority of servers are migrated to the newer version.

If you're facing this problem, you should probably just use a work-around (see here).

[ryanc-me]

Twisted released a new version (21.2.0) last month, so 308 redirects are now officially supported!

It looks like Synapse is currently requiring Twisted 18.9.0. Bumping the version to 21.2.0 would ensure that users are getting the fix, but it feels like that could be a large change.

@clokep do you think it's worth me making a PR to bump the Twisted version, or is that a project that would be chosen/handled by the Synapse team?

[richvdh]

I could go either way on this. Normally we don't bump our dependency versions as a matter of course, to allow freedom to our downstream packages to use whatever version is most appropriate for them. But arguably Synapse is actually buggy with older Twisted versions so we should require 21.2 to fix the bug. What do other folks on @matrix-org/synapse-core think?

As an aside: it would be good to see updates to the spec, and additions to the Complement or Sytest test suites, to ensure that other homeserver implementations don't forget about 308.

[clokep]

There's a couple of other issues fixed by that Twisted release (#6211, we could remove a hack for #7441) so I'd be for it. I think some of the packagers have quite old versions though.

[ryanc-me]

@richvdh I can add Complement and Sytest coverage for the 308 redirects, but the spec updates sound a little out-of-scope for me (unless you could send me a few pointers!)

I can also open a PR to bump the Twisted version @clokep, if that's suitable.

[auscompgeek]

I think the spec already says 30x redirects should be followed for .well-known resolution. Correct me if I'm wrong though.

[richvdh]

I think the spec already says 30x redirects should be followed for .well-known resolution. Correct me if I'm wrong though.

it does, though I thought it might be nice to enumerate exactly which codes that covers. If we forgot 308, there are chances that others might do too. @ryanc-me I think it's as simple as a PR against https://github.com/matrix-org/matrix-doc/blob/master/specification/server_server_api.rst (though there might be some rearrangement afoot in that repo, so the PR might sit for a while.)

There's a couple of other issues fixed by that Twisted release (#6211, we could remove a hack for #7441)

the thing is, I don't think either of those are reason enough to break compat with old Twisted. On the other hand, not honouring the federation spec (and thus making it incompatible with other servers) is enough reason, imho. So yes, @ryanc-me, I think a PR to bump the Twisted version would be appreciated.

[ryanc-me]

Thanks for the feedback @richvdh. FWIW, I think that spec compatibility is a good reason too, especially considering that Synapse is the de-facto reference server. This was a very difficult error to track down.

I'll work on some PRs over the weekend!