Sanity check identity server passed to bind/unbind.

[dkasak]

Signed-off-by: Denis Kasak dkasak@termina.org.uk

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)

[richvdh]

it's not (necessarily) supposed to be a valid Matrix server name.

Identity servers don't use the matrix server-name resolution algorithm (fully specced at https://matrix.org/docs/spec/server_server/r0.1.4#resolving-server-names): rather they just go straight into an https url.

so I think "what is valid here" is "what is valid in the 'authority' part of an HTTPS URL", which I think is defined by RFC3986 ?

It's also not entirely clear to me that the IS API must be exposed at the root of the path hierarchy (I don't think this is mandated anywhere in the spec), so maybe id_server should be allowed to include / components?

[dkasak]

Uh, sorry, I missed this. My email rules still need a bit of tweaking, it seems.

Identity servers don't use the matrix server-name resolution algorithm (fully specced at https://matrix.org/docs/spec/server_server/r0.1.4#resolving-server-names): rather they just go straight into an https url.

Ah, I missed this. I did check the C-S API spec and the "hostname plus optional port" language made me think of the Matrix server name algorithm. The language used in the spec seems inconsistent/colourful enough, so this now looks to me as a bit of a spec bug:

The identity server to unbind all of the user's 3PIDs from.

The hostname+port of the identity server which should be used for third party identifier lookups.

The hostname of the identity server to communicate with. May optionally include a port.

The identity server to use.

url of identity server authed with, e.g. 'matrix.org:8090'

---

It's also not entirely clear to me that the IS API must be exposed at the root of the path hierarchy (I don't think this is mandated anywhere in the spec), so maybe id_server should be allowed to include / components?

Per the above, only one place mentions the word "url" (which wouldn't work if followed literally), but then proceeds to give a hostname+port example. In other places, the wording seems to flat out disallow using a hostname + path components hybrid. I also don't see this added flexibility as being too useful practically, but maybe I'm missing some scenarios?

so I think "what is valid here" is "what is valid in the 'authority' part of an HTTPS URL", which I think is defined by RFC3986 ?

The differences of RFC3986's authority rules from the Matrix server algorithm seem very exotic:

• Allowing the optional userinfo "@" part (which seems non-sensical in this context)
• Allowing percent-encoded characters
• Allowing sub-delims: "!" / "$" / "&" / "'" / "(" / ")" / "*" / "+" / "," / ";" / "="

I'm wondering whether the best course of action here would be to change the spec to mandate a Matrix server name here, since it seems it is overwhelmingly likely that this is what everyone running an IS is doing.

[dkasak]

I checked and passing either the userinfo, percent-encoded chars or the chars from sub-delims fails shortly afterwards, with either twisted or treq complaining that it isn't a valid hostname. So it seems like the remaining question is whether a root path should be allowed.

Given the existing wording of the spec, it seems unlikely to me that anyone is using that. The spec should be clarified in either case.

[richvdh]

The language used in the spec seems inconsistent/colourful enough, so this now looks to me as a bit of a spec bug:

The spec may be unclear, but synapse has always been very clear that we do not use the matrix federation routing algorithm for ISes. For example, if no explicit port is given, we use port 443 (federation uses 8448). Since Synapse has done this forever, that behaviour is de-facto part of the spec, even if the actual words of the spec don't make that clear.

In other places, the wording seems to flat out disallow using a hostname + path components hybrid.

Could you cite any specific examples?

I also don't see this added flexibility as being too useful practically, but maybe I'm missing some scenarios?

It's just that sometimes it's easier to deploy things at a sub-path. There are parallels to the ongoing debate about pusher URLs; we've even heard of people setting up Synapse's C-S API at a subpath (#9574).

I'm on the fence about this, tbh. I could go either way. We should make a decision and then be explicit about it, though.

I'm wondering whether the best course of action here would be to change the spec to mandate a Matrix server name here, since it seems it is overwhelmingly likely that this is what everyone running an IS is doing.

I wouldn't have any particular objection to defining a grammar for IS names which happens to be the same as the grammar for Matrix server_names - as you say the current differences are minor. However, I would oppose the use of any phrasing like "must be a valid Matrix server name", because server_names bring with them the implication of using the "Resolving server names" algorithm - which, as above, is not what happens.

[dkasak]

Could you cite any specific examples?

My reading of e.g. https://matrix.org/docs/spec/client_server/r0.6.1#post-matrix-client-r0-account-password-email-requesttoken, which says the following for the id_server parameter

The hostname of the identity server to communicate with. May optionally include a port.

Is that it doesn't allow a path.

I'm on the fence about this, tbh. I could go either way. We should make a decision and then be explicit about it, though.

Agreed. Since this used to allow specifying a root path, I'm going to change the PR so that it doesn't forbid it. Once a decision has been made, we can easily make it stricter again.

I wouldn't have any particular objection to defining a grammar for IS names which happens to be the same as the grammar for Matrix server_names - as you say the current differences are minor. However, I would oppose the use of any phrasing like "must be a valid Matrix server name", because server_names bring with them the implication of using the "Resolving server names" algorithm - which, as above, is not what happens.

Gotcha. I pushed a new commit which does away with this wording (but still uses the parse_and_validate_server_name function internally to enforce the same rules).

[richvdh]

lgtm