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.

Sign up for GitHub

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

MSC4148: Permitting HTTP(S) URLs for SSO IdP icons #4148

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

MSC4148: Permitting HTTP(S) URLs for SSO IdP icons #4148

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions proposals/4148-https-sso-icons.md
View file
Open in desktop
Copy link
Member Author

Choose a reason for hiding this comment

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

Implementation requirements:

  • None required, in my opinion. This MSC is logically possible to implement considering a client's use of the media download and thumbnail APIs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Having written a whole speech about the status of this MSC, I think the best thing for it is to cancel proposed-fcp and restart it once the new vs existing field discussion can conclude. The TLDR is client and server developers don't appear to be overly concerned with the broken behaviour, the author (me) doesn't have the time to maintain this, and given the debate on new vs existing fields it feels important to give SCT members a chance to re-review before sending the MSC along (necessitating FCP cancel and restart).

If there's no significant objections by about December 9th, 2024, I'll cancel the FCP and return this MSC to the backlog.

The longer speech is/was:

In practice, this MSC has all the checkboxes it needs to enter FCP, but has 2 notable blocking concerns preventing that from happening. It's been like this for about 5 months, which is very long. Meanwhile, client (and server) developers don't appear overly bothered with the lack of progress on this particular MSC, making it feel not as urgent as when it was opened. The author (me) has also had extremely limited availability to work on it, clearly.

The concern for removing the 64x64 requirement doesn't materially affect the MSC and can be resolved safely, however the concern about using a new field vs existing field may be breaking enough to cancel proposed-fcp and restart the process. Even if the MSC doesn't change, it doesn't feel fair to suddenly start a 5 day FCP with 5 month-old reviews - SCT members in particular should be given a chance to re-review before the MSC enters FCP.

Typically, we'd cancel and restart FCP if the MSC materially changes (it's a whole new MSC at that point), however we don't usually have such a long delay between achieving checkmarks and identifying a stalled MSC. Where we do want to give SCT members a chance to re-review an MSC, we typically be extremely loud about the MSC entering FCP, but this has had underwhelming success in the past (and also lights a fire where a fire isn't required). Because of the 5 month delay alone, it feels most appropriate to cancel proposed-fcp and restart it once/if the new vs existing field discussion resolves, giving all SCT members a chance to re-review and decide differently to last season's conclusions.

On the client/server implementation side: typically MSCs which are important to developers see some amount of traffic, either in the form of "drive by" comments or a ton of references hidden in the GitHub PR timeline. This MSC has neither, indicating it may not actually be solving a problem felt in practice. This feels untrue though, as surely there is a client out there showing a broken image on a button somewhere - maybe they're referencing the issue in a place where GitHub can't see it? Either way, this year end cleanup post offers developers a chance to say "hey, my users care!" more loudly, which will feed into the MSC's relative priority down the line.

Considering the expected priority from affected implementations and the proposed cancellation of FCP, it's likely the MSC will fall to the infinite backlog until either the author has enough free time (highly unlikely) or a number not much higher than zero developers advocate for the MSC on behalf of their users.

Thoughts from the community and SCT are welcome on this. It is proposed that the MSC cancel its FCP no sooner than December 9th, 2024 to ensure already-received checkmarks can be maintained if desired.

Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
# MSC4148: Permitting HTTP(S) URLs for SSO IdP icons

The current [`m.login.sso` flow schema](https://spec.matrix.org/v1.10/client-server-api/#definition-mloginsso-flow-schema)
includes an optional `icon` field for clients to use when representing the Identity Provider.

Use of this icon currently relies on unauthenticated media download/thumbnail endpoints, which are
slated for deprecation and eventual removal through [MSC3916](https://github.com/matrix-org/matrix-spec-proposals/pull/3916).
Clients do not yet have credentials for the user and therefore may not be able to access these icons.
MSC3916 recommends that servers continue to allow the icons on the unauthenticated endpoints, though
this is only helpful so long as the endpoints exist.

This proposal introduces yet another temporary measure for handling these icons in the face of authenticated
media and the upcoming transition to OIDC: servers are permitted to use HTTP(S) URLs instead.

## Proposal

`icon` for the [`m.login.sso` flow schema](https://spec.matrix.org/v1.10/client-server-api/#definition-mloginsso-flow-schema)
Copy link
Member

Choose a reason for hiding this comment

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

I don't want to bikeshed too much on an MSC that's only meant as a temporary measure, but couldn't we stick the HTTP(S) URI in a different property, maybe something stupid like icon_location? This would allow both old and new clients to work properly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure it'd materially improve the experience, honestly. Clients which don't update will still get broken icons.

Copy link
Contributor

Choose a reason for hiding this comment

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

I concur that this change will break clients. Clients currently expect mxc urls and actively prevent loading https urls. Changing the same property to https will break existing clients and prevent even the fallback to allow loading old media using the old endpoints. Meanwhile I see no goo reason not to use a different property name for these urls. Clients which don't update won't get broken icons immediately without this MSC. With this MSC they will. And servers can even provide a quality of implementation "feature" and allow uploading media for unauthenticated access for the idp urls. So this should really be a separate property.

Copy link
Member Author

Choose a reason for hiding this comment

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

Using a separate property would lead to clients not showing avatars, which it sounds like would also happen on clients not prepared for non-mxc URLs in the existing field. While it's a technically breaking change, the effective behaviour is the same regardless of the approach.

Note: clients which crash because of non-mxc URLs being present should be fixed regardless of this MSC's direction.

Copy link
Member

Choose a reason for hiding this comment

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

I think the idea is if you provide both it allows clients to migrate more gracefully by a server giving both an MXC and HTTPS URL?

Copy link
Member

@dbkr dbkr Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This thread should probably be resolved one way or the other, so I'll add as concern for it. For what it's worth, having it as a separate field seems sensible to me because servers can send both as a transition period. Since the HTTP(S) icon will only support a single size, you could even call it icon_64.

Copy link
Member

Choose a reason for hiding this comment

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

My preference would be a separate field, but I don't think it's a huge issue either way, given it's a temporary measure, and just aesthetic.

Copy link
Member

Choose a reason for hiding this comment

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

If we use a separate field here, I wonder what the implementation would look like. Currently, Synapse passes the idp_icon setting from its config straight though (see doc, which says "must be an MXC URI"). To take advantage of separate fields here, we'd need to support separate fields in Synapse's config, which seems like it is complicating matters for the average Synapse admin.

I also see @turt2live's point that it probably won't help much in practice. Either: your server allows the mxc: uri, in which case send that; else it doesn't, in which case what's the point of a separate field. The only reason to send both is to help you keep track of the number of old clients that aren't honoring the new field (and thus presumably don't support https: uris).

IMHO the downsides of a separate field outweigh the upsides.

-0.5.

Copy link
Member

Choose a reason for hiding this comment

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

An extra field means extra maintenance, on both (client and server) sides. Given that it's mainly an aesthetic issue, I'm fine with temporary limited breakage as long as we get straight to the to-be state. We have more than enough legacy cruft in the protocol already, let's not add to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

In theory OIDC is expected this year, but I'm not tracking that work. @hughns do you have insight here?

has the following changes:

1. `mxc://` URI usage is *deprecated*, [pending removal](https://spec.matrix.org/v1.10/#deprecation-policy).
2. HTTPS and HTTP URLs are permitted.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can those be on any host? That would suddenly send the IP of clients to possibly an arbitrary amount of third parties. Imo it is best if the homeserver hosts the media. This is somewhat guaranteed for mxc URIs and servers could decide to just not enforce authentication for the IDP icons with very little impact. But another option could be for the homeserver to provide the https url using some internal mechanism. But in general I do think we should avoid a situation where homeservers provide icon urls pointing to some third party server.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's up to the HS which servers it delegates to and therefore exposes user data to. I don't think this belongs in the spec.

3. Servers SHOULD prefer to use HTTPS URLs over HTTP or MXC URLs.
4. Icons SHOULD be 64x64 pixels in size, accounting for a wide range of screen resolutions.
Copy link
Member

Choose a reason for hiding this comment

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

bleurgh. it seems crazy to mandate 64x64 for an icon - that's tiny. macOS icons are 512x512 for instance (or 1024x1024 at retina). can we just delete this in favour of "Icons should be high enough resolution to render pleasantly on a wide variety of clients" or just leave it unspecified.

Copy link
Member

Choose a reason for hiding this comment

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

(other than that, i'm fine with the msc tho)

* This is because the thumbnail APIs are no longer accessible to clients with HTTP(S) URLs, so
"reasonable" sizes should be picked. This is not a strong requirement as deployment-specific
considerations may apply (larger sign-in button on a custom client, etc).

Some suggested spec text for `icon` to capture these changes would be:

> Optional. An HTTP(S) URL to provide an image/icon representing the IdP. Intended to be shown alongside
> `name` if provided.
>
> Servers SHOULD prefer HTTPS URLs over HTTP. `mxc://` URIs SHOULD NOT be used as their usage is deprecated
> here, though are permitted. Clients may have to use the deprecated, unauthenticated, media download
> and thumbnail endpoints to access content addressed by `mxc://` URIs.
>
> Images SHOULD be 64x64 pixels in size.

## Potential issues

Existing clients may block non-`mxc://` URI usage and show no icon. Though this can lead to subpar
user experience, the actual impact is expected to be temporary.
Copy link
Member

Choose a reason for hiding this comment

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

It's probably worth noting that mxc URLs also give the ability for clients to request different sized thumbnails, and this MSC effectively hard codes the quality of the thumbnail to be used.


## Alternatives

* As it turns out, Element Web [overrides](https://github.com/matrix-org/matrix-react-sdk/blob/679b170bc5ae5eeb9c04c47fc5eeb663cb45b30c/src/components/views/elements/SSOButtons.tsx#L42)
the icon if it recognizes the `brand`. This behaviour prevents authenticated media from breaking the
icons, and avoids issues with HTTP(S) URL compatibility for the client. Other clients may wish to
pursue a similar approach, particularly for popular brands/sign in methods.

* This entire SSO login mechanism could be replaced, which is what is happening with OIDC (as of
writing). This MSC is intended to fill a gap between 'now' and when OIDC lands rather than being
a complete solution.

## Security considerations

MXC URIs are intended to keep media 'inside' Matrix to reduce opportunities for user information to
be harvested by external entities. Unlike random image uploads to a room though, the server the user
is about to register an account on is the only entity able to control these URLs, reducing the risk
surface. Clients may wish to look into an approach similar to Element Web for handling common icons,
and possibly ways of not using icons if they feel "too sketchy". For example, if the icons aren't
served by a subdomain of the Client-Server API URL (`https://*.matrix.org` in the case of
turt2live marked this conversation as resolved.
Show resolved Hide resolved
`https://matrix-client.matrix.org`), the icon could be omitted.

## Unstable prefix

Placing this MSC behind an unstable prefix becomes very difficult very quickly. Instead, clients and
servers are suggested to *not* implement this proposal until it is considered stable.

## Dependencies

No direct dependencies. [MSC3916](https://github.com/matrix-org/matrix-spec-proposals/pull/3916)
increases the need for this MSC, however.