Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSC1700: Improving .well-known discovery #1700

Closed
wants to merge 4 commits into from

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Oct 19, 2018

Rendered

Note: This is a summary of my thoughts on the matter of .well-known discovery validation.

@turt2live turt2live added proposal-ready-for-review proposal A matrix spec change proposal proposal-pr client-server Client-Server API labels Oct 19, 2018
@turt2live turt2live changed the title MSC to improve .well-known discovery MSC 1700: Improving .well-known discovery Oct 19, 2018
@ara4n
Copy link
Member

ara4n commented Oct 19, 2018

This looks very very sane to me.

Parsing a hostname isn't an insane feat, however that doesn't change the argument regarding port usage within matrix.
@dbkr
Copy link
Member

dbkr commented Oct 22, 2018

I would probably avoid changing what port is used when a port is present in the server_name since this would be something any existing impls would have to go back & change, etc. All for simplifying the other steps though.

Copy link
Contributor

@maxidorius maxidorius left a comment

Choose a reason for hiding this comment

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

Overall, I think the proposal lacks any substential material/code to illustrate its points and should focus on that part first.

developers, without sacrificing critical functionality.

In general, the process should be shortened to two steps: get the JSON and use it if possible. Currently the
process employs a lot of verification methods that don't seem to prevent anything.
Copy link
Contributor

@maxidorius maxidorius Oct 26, 2018

Choose a reason for hiding this comment

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

Many of those verification steps/methods are actually part of most SDK or libraries, making them available and used by default.
The current process is also having steps in an order which allow for their UX states to be "grouped", ensuring you actually don't need to discern them if your coding language/tools allow you to bundle them up.

Not all languages/tools/SDKs/libraries give you that high-level access and therefore the exact steps need to be written down to ensure it is not language specific. This is the same principle which is used in the S2S spec for state resolution - if you were to actually implement it, many steps would actually be grouped together. But that doesn't mean the algorithm shouldn't be documented or guidance not given for implementations.
I think this strips away the needed algorithm which is agnostic of language, SDK or tools and promotes high-level languages like Javascript at the expense of low(er)-level languages like Java, C(++), Go, etc.

I would suggest to give advises for languages on what can and cannot be "bunched up" instead.

A lot of the validation here doesn't need to be done for the following reasons. In general, the validation steps make
it difficult for libraries to provide the functionality to support their UI counterparts. By reducing the amount
of validation required, an expressive UI is still possible while also making it easier for automated clients and
libraries to use the functionality.
Copy link
Contributor

@maxidorius maxidorius Oct 26, 2018

Choose a reason for hiding this comment

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

I think this should be demonstrated with code, showing both implementations (the current spec one, and this MSC one). At this point, this is mere speculation.

If code was produced following the current spec, with notes/comments on how the various elements are not working, it would give the occasion for the community to actually review the code to ensure the problem is with the specification itself instead of:

  • The approach used by the person writting the code
  • The framework used (matrix-react-sdk)
  • The libraries used (react, etc.)
  • The language used (Javascript)

On the other hand, the algorithm is implemented in the Matrix Java SDK and used by at least two clients, Palaver and SimpleMatrix - This means that so far we have conflicting evidences, not allowing us to know if a problem exists and if yes, where.


The first check removed is checking whether the `base_url` is actually a URL. There is no definition as to what
makes a valid URL, which is completely unhelpful for the people having to implement this. Instead, the unwritten
rule where the client should be verifying the information is appropriate for its own purpose should come into
Copy link
Contributor

@maxidorius maxidorius Oct 26, 2018

Choose a reason for hiding this comment

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

I think letting anyone decide what qualifies as a "valid URL" instead of simply clarifying what "valid" means is very extreme in the approach. Clarifying the term valid would fit the issue and achieve the same goal of being "helpful for the people having to implement this".

Copy link

@mvgorcum mvgorcum Nov 8, 2018

Choose a reason for hiding this comment

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

I have a hard time defining what would constitute a 'valid URL' in this case. Do you have a suggestion of what a clarified valid would look like, @maxidor?
I don't really get much further than: ASCII not containing any unsafe characters.

Copy link
Contributor

@maxidorius maxidorius Nov 8, 2018

Choose a reason for hiding this comment

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

@mvgorcum See the comment just below.

there is an appropriate `m.homeserver` configuration in the response.

The first check removed is checking whether the `base_url` is actually a URL. There is no definition as to what
makes a valid URL, which is completely unhelpful for the people having to implement this. Instead, the unwritten
Copy link
Contributor

@maxidorius maxidorius Oct 26, 2018

Choose a reason for hiding this comment

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

Given that I wrote the original text of the .well-known proposal and those terms were not changed, the meaning simply was: "to fit the latest RFC definining what a URL is", which is RFC #1738 unless mistaken. URL is a well-defined term, so a simple clarification should be enough?

rule where the client should be verifying the information is appropriate for its own purpose should come into
effect. This also allows application-specific clients to verify the URL independently of the specification, or
not at all if it chooses to. The risk of an invalid URL here is relatively minimal, as a responsible client would
be communicating to the user what the URL it is about to use is for the user to verify.
Copy link
Contributor

@maxidorius maxidorius Oct 26, 2018

Choose a reason for hiding this comment

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

Since there is no concept of showing the URL about to be used in the current spec, is this effectively a strong recommendation, or a requirement?

URL. The URL was given to the client for the sole purpose of assisting the user into the system, and is unlikely
to be wrong. If it were wrong, the administrator would be more than likely interested in fixing it for their users.
In the event of a security breach where the .well-known configuration is changed to point at a malicious party,
the client's responsibility to show which URL it is about to connect to comes into play.
Copy link
Contributor

@maxidorius maxidorius Oct 26, 2018

Choose a reason for hiding this comment

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

No such thing as "responsibility" or "responsible client" is ever defined in the specification. What is the expectation here?

makes a valid URL, which is completely unhelpful for the people having to implement this. Instead, the unwritten
rule where the client should be verifying the information is appropriate for its own purpose should come into
effect. This also allows application-specific clients to verify the URL independently of the specification, or
not at all if it chooses to. The risk of an invalid URL here is relatively minimal, as a responsible client would
Copy link
Contributor

@maxidorius maxidorius Oct 26, 2018

Choose a reason for hiding this comment

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

Would leeway given in "application-specific" behaviour allowing bypass of the check and requirement for HTTPS?

The first check removed is checking whether the `base_url` is actually a URL. There is no definition as to what
makes a valid URL, which is completely unhelpful for the people having to implement this. Instead, the unwritten
rule where the client should be verifying the information is appropriate for its own purpose should come into
effect. This also allows application-specific clients to verify the URL independently of the specification, or
Copy link
Contributor

Choose a reason for hiding this comment

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

If clients can choose to verify a URL independently of the specification, is there a point to put any kind of verification system in the specification?

the client's responsibility to show which URL it is about to connect to comes into play.

Further to the point of not having to verify if a Matrix server exists on the endpoint, the risk of a conflicting
service is minimal. There are not many services out there that use both "matrix" and "m.homeserver" to do .well-known
Copy link
Contributor

Choose a reason for hiding this comment

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

This was never given as a reason for validating the .well-known data.


Also for reference, here is the last step of the discovery process (the validation):

> 3. Make a `GET` request to `https://hostname/.well-known/matrix/client`.
Copy link
Member

Choose a reason for hiding this comment

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

this is illegible in the rendered version.

@richvdh richvdh changed the title MSC 1700: Improving .well-known discovery MSC1700: Improving .well-known discovery Dec 12, 2018
@turt2live
Copy link
Member Author

I'm closing this until I have the motivation to rewrite it.

@turt2live turt2live closed this Dec 17, 2018
@richvdh richvdh added abandoned A proposal where the author/shepherd is not responsive and removed proposal-in-review labels Dec 27, 2018
@turt2live turt2live added kind:maintenance MSC which clarifies/updates existing spec and removed proposal-pr labels Apr 20, 2020
@behrmann
Copy link

@turt2live Just found this spec, thanks for writing it. As someone bitten by the missing 30* support in the spec I'd still very much like to see this proposal happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned A proposal where the author/shepherd is not responsive client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants