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

MSC3861: load the issuer and account management URLs from OIDC discovery #17407

Merged
merged 12 commits into from
Aug 30, 2024
Merged

MSC3861: load the issuer and account management URLs from OIDC discovery #17407

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
65120fe
MSC3861: load the issuer and account management URLs from OIDC discovery
sandhose Jul 8, 2024
86d7fee
Newsfile.
sandhose Jul 8, 2024
ee0cd1a
Import `cast` from `typing` instead of `typing_extensions`.
sandhose Jul 8, 2024
2ff5321
Fix loading the account management URL from the issuer
sandhose Jul 8, 2024
90b6a4f
Merge remote-tracking branch 'origin/develop' into quenting/msc3861-l…
sandhose Jul 8, 2024
0119d40
Use the cached metadata call
sandhose Jul 11, 2024
b035669
Merge remote-tracking branch 'origin/develop' into quenting/msc3861-l…
sandhose Aug 30, 2024
5d9d269
Don't fallback to the config issuer if loading the metadata fails
sandhose Aug 30, 2024
f4dc986
Merge remote-tracking branch 'origin/develop' into quenting/msc3861-l…
sandhose Aug 30, 2024
089a542
Use the right account management URL in the UIA fallback
sandhose Aug 30, 2024
17a90f7
Fix tests now that we're always fetching the issuer metadata
sandhose Aug 30, 2024
40c854c
Fix well-known tests now that we're fetching through the metadata
sandhose Aug 30, 2024
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
1 change: 1 addition & 0 deletions changelog.d/17407.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
MSC3861: load the issuer and account management URLs from OIDC discovery.
36 changes: 36 additions & 0 deletions synapse/api/auth/msc3861_delegated.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,42 @@ async def _load_metadata(self) -> OpenIDProviderMetadata:
# metadata.validate_introspection_endpoint()
return metadata

async def issuer(self) -> str:
"""
Get the configured issuer

This will use the issuer value set in the metadata,
falling back to the one set in the config in case the metadata can't be loaded
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

is this the right precedence? Generally I would expect config to override autodetection

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 guess the problem is that the issuer in the config is used for discovery, and might be slightly different than the one you should actually advertise. Client will fail if the issuer advertised by /auth_issuer is not exactly the same as the one in {issuer}/.well-known/openid-configuration. We've had problems in the past of mismatches because of trailing slashes being in one place but not at the other one

So IMO, it can be confusing, but I think that pragmatically this is the right precedence

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, even still, it still feels wrong to me. Suppose the homeserver fails to fetch the discovery at some point, you're still going to hand out the wrong issuer value to clients then, it will just be extra confusing that this was caused by the discovery failing.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a really good point. Should we log a warning if there is a mismatch, but still work? Because the discovery has to work just once in Synapse's lifetime, then everything is cached, so hopefully that sort of outage would be very temporary

Copy link
Contributor

@reivilibre reivilibre Jul 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can we check it on start up and then refuse to start up if it's wrong instead? I think this would be justified for what is a configuration error. The problem with warnings is that virtually nobody reads them.

I think by 'once in Synapse's lifetime' you mean once every process lifetime?

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 think you're right. I preferred to not catch the error if it fails loading. The impact of this, is that /.well-known/matrix/client and /_matrix/client/*/auth_issuer will 501 in case Synapse was never able to contact MAS, instead of potentially showing wrong informations. I think this is better

issuer: Optional[str] = None
try:
metadata = await self._load_metadata()
sandhose marked this conversation as resolved.
Show resolved Hide resolved
issuer = metadata.issuer
# We don't want to raise here if we can't load the metadata
except Exception:
logger.warning("Failed to load metadata:", exc_info=True)

# Fallback to the config value if the metadata can't be loaded
# or if the metadata doesn't have an issuer set
return issuer or self._config.issuer

async def account_management_url(self) -> Optional[str]:
"""
Get the configured account management URL

This will discover the account management URL from the issuer if it's not set in the config
"""
if self._config.account_management_url is not None:
return self._config.account_management_url

try:
metadata = await self._load_metadata()
sandhose marked this conversation as resolved.
Show resolved Hide resolved
return metadata.get("account_management_uri", None)
# We don't want to raise here if we can't load the metadata
except Exception:
logger.warning("Failed to load metadata:", exc_info=True)
return None

async def _introspection_endpoint(self) -> str:
"""
Returns the introspection endpoint of the issuer
Expand Down
10 changes: 8 additions & 2 deletions synapse/rest/client/auth_issuer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# limitations under the License.
import logging
import typing
from typing import Tuple
from typing import Tuple, cast

from synapse.api.errors import Codes, SynapseError
from synapse.http.server import HttpServer
Expand Down Expand Up @@ -43,10 +43,16 @@ class AuthIssuerServlet(RestServlet):
def __init__(self, hs: "HomeServer"):
super().__init__()
self._config = hs.config
self._auth = hs.get_auth()

async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
if self._config.experimental.msc3861.enabled:
return 200, {"issuer": self._config.experimental.msc3861.issuer}
# If MSC3861 is enabled, we can assume self._auth is an instance of MSC3861DelegatedAuth
# We import lazily here because of the authlib requirement
from synapse.api.auth.msc3861_delegated import MSC3861DelegatedAuth

auth = cast(MSC3861DelegatedAuth, self._auth)
return 200, {"issuer": await auth.issuer()}
else:
# Wouldn't expect this to be reached: the servelet shouldn't have been
# registered. Still, fail gracefully if we are registered for some reason.
Expand Down
16 changes: 11 additions & 5 deletions synapse/rest/client/keys.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import re
from collections import Counter
from http import HTTPStatus
from typing import TYPE_CHECKING, Any, Dict, Optional, Tuple
from typing import TYPE_CHECKING, Any, Dict, Optional, Tuple, cast

from synapse.api.errors import Codes, InvalidAPICallError, SynapseError
from synapse.http.server import HttpServer
Expand Down Expand Up @@ -397,11 +397,17 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
# explicitly mark the master key as replaceable.
if self.hs.config.experimental.msc3861.enabled:
if not master_key_updatable_without_uia:
config = self.hs.config.experimental.msc3861
if config.account_management_url is not None:
url = f"{config.account_management_url}?action=org.matrix.cross_signing_reset"
# If MSC3861 is enabled, we can assume self.auth is an instance of MSC3861DelegatedAuth
# We import lazily here because of the authlib requirement
from synapse.api.auth.msc3861_delegated import MSC3861DelegatedAuth

auth = cast(MSC3861DelegatedAuth, self.auth)

uri = await auth.account_management_url()
if uri is not None:
url = f"{uri}?action=org.matrix.cross_signing_reset"
else:
url = config.issuer
url = await auth.issuer()

raise SynapseError(
HTTPStatus.NOT_IMPLEMENTED,
Expand Down
2 changes: 1 addition & 1 deletion synapse/rest/client/login.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, LoginResponse]:
approval_notice_medium=ApprovalNoticeMedium.NONE,
)

well_known_data = self._well_known_builder.get_well_known()
well_known_data = await self._well_known_builder.get_well_known()
if well_known_data:
result["well_known"] = well_known_data
return 200, result
Expand Down
37 changes: 21 additions & 16 deletions synapse/rest/well_known.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,13 @@
#
#
import logging
from typing import TYPE_CHECKING, Optional
from typing import TYPE_CHECKING, Optional, Tuple, cast

from twisted.web.resource import Resource
from twisted.web.server import Request

from synapse.http.server import set_cors_headers
from synapse.api.errors import NotFoundError
from synapse.http.server import DirectServeJsonResource
from synapse.http.site import SynapseRequest
from synapse.types import JsonDict
from synapse.util import json_encoder
Expand All @@ -38,8 +39,9 @@
class WellKnownBuilder:
def __init__(self, hs: "HomeServer"):
self._config = hs.config
self._auth = hs.get_auth()

def get_well_known(self) -> Optional[JsonDict]:
async def get_well_known(self) -> Optional[JsonDict]:
if not self._config.server.serve_client_wellknown:
return None

Expand All @@ -52,13 +54,20 @@ def get_well_known(self) -> Optional[JsonDict]:

# We use the MSC3861 values as they are used by multiple MSCs
if self._config.experimental.msc3861.enabled:
# If MSC3861 is enabled, we can assume self._auth is an instance of MSC3861DelegatedAuth
# We import lazily here because of the authlib requirement
from synapse.api.auth.msc3861_delegated import MSC3861DelegatedAuth

auth = cast(MSC3861DelegatedAuth, self._auth)

result["org.matrix.msc2965.authentication"] = {
"issuer": self._config.experimental.msc3861.issuer
"issuer": await auth.issuer(),
}
if self._config.experimental.msc3861.account_management_url is not None:
account_management_url = await auth.account_management_url()
sandhose marked this conversation as resolved.
Show resolved Hide resolved
if account_management_url is not None:
result["org.matrix.msc2965.authentication"][
"account"
] = self._config.experimental.msc3861.account_management_url
] = account_management_url

if self._config.server.extra_well_known_client_content:
for (
Expand All @@ -71,26 +80,22 @@ def get_well_known(self) -> Optional[JsonDict]:
return result


class ClientWellKnownResource(Resource):
class ClientWellKnownResource(DirectServeJsonResource):
"""A Twisted web resource which renders the .well-known/matrix/client file"""

isLeaf = 1

def __init__(self, hs: "HomeServer"):
Resource.__init__(self)
super().__init__()
self._well_known_builder = WellKnownBuilder(hs)

def render_GET(self, request: SynapseRequest) -> bytes:
set_cors_headers(request)
r = self._well_known_builder.get_well_known()
async def _async_render_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
r = await self._well_known_builder.get_well_known()
if not r:
request.setResponseCode(404)
request.setHeader(b"Content-Type", b"text/plain")
return b".well-known not available"
raise NotFoundError(".well-known not available")

logger.debug("returning: %s", r)
request.setHeader(b"Content-Type", b"application/json")
return json_encoder.encode(r).encode("utf-8")
Comment on lines -74 to -93
Copy link
Member Author

Choose a reason for hiding this comment

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

Changes here don't change the response when the WK can be built:

Before:

curl -i localhost:8008/.well-known/matrix/client
HTTP/1.1 200 OK
Server: Synapse/1.110.0
Date: Mon, 08 Jul 2024 10:10:19 GMT
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: GET, HEAD, POST, PUT, DELETE, OPTIONS
Access-Control-Allow-Headers: X-Requested-With, Content-Type, Authorization, Date, If-Match, If-None-Match
Access-Control-Expose-Headers: ETag, Location, X-Max-Bytes
Content-Type: application/json
Content-Length: 178

{"m.homeserver":{"base_url":"https://oidc.sandhose.fr/"},"org.matrix.msc2965.authentication":{"issuer":"https://oidc.sandhose.fr/","account":"https://oidc.sandhose.fr/account/"}}

After:

curl -i localhost:8008/.well-known/matrix/client
HTTP/1.1 200 OK
Transfer-Encoding: chunked
Server: Synapse/1.110.0
Date: Mon, 08 Jul 2024 10:10:57 GMT
Content-Type: application/json
Cache-Control: no-cache, no-store, must-revalidate
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: GET, HEAD, POST, PUT, DELETE, OPTIONS
Access-Control-Allow-Headers: X-Requested-With, Content-Type, Authorization, Date, If-Match, If-None-Match
Access-Control-Expose-Headers: ETag, Location, X-Max-Bytes

{"m.homeserver":{"base_url":"https://oidc.sandhose.fr/"},"org.matrix.msc2965.authentication":{"issuer":"https://oidc.sandhose.fr/","account":"https://oidc.sandhose.fr/account/"}}

It does although change the response when the public_baseurl isn't set:

Before:

curl -i localhost:8008/.well-known/matrix/client
HTTP/1.1 404 Not Found
Server: Synapse/1.110.0
Date: Mon, 08 Jul 2024 10:13:35 GMT
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: GET, HEAD, POST, PUT, DELETE, OPTIONS
Access-Control-Allow-Headers: X-Requested-With, Content-Type, Authorization, Date, If-Match, If-None-Match
Access-Control-Expose-Headers: ETag, Location, X-Max-Bytes
Content-Type: text/plain
Content-Length: 25

.well-known not available

After:

curl -i localhost:8008/.well-known/matrix/client
HTTP/1.1 404 Not Found
Transfer-Encoding: chunked
Server: Synapse/1.110.0
Date: Mon, 08 Jul 2024 10:13:10 GMT
Content-Type: application/json
Cache-Control: no-cache, no-store, must-revalidate
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: GET, HEAD, POST, PUT, DELETE, OPTIONS
Access-Control-Allow-Headers: X-Requested-With, Content-Type, Authorization, Date, If-Match, If-None-Match
Access-Control-Expose-Headers: ETag, Location, X-Max-Bytes

{"errcode":"M_NOT_FOUND","error":".well-known not available"}

Which is IMO perfectly acceptable

return 200, r


class ServerWellKnownResource(Resource):
Expand Down
Loading