Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Downgrade some OIDC exceptions to warnings #12723

Merged
merged 3 commits into from
May 18, 2022
Merged

Downgrade some OIDC exceptions to warnings #12723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
df3c432
Downgrade some OIDC exceptions to warnings
May 12, 2022
d186cf5
Changelog
May 12, 2022
016fc20
Keep MacaroonInitException logged as exceptions
May 18, 2022
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/12723.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Downgrade some OIDC errors to warnings in the logs, to reduce the noise of Sentry reports.
4 changes: 2 additions & 2 deletions synapse/handlers/oidc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ async def handle_oidc_callback(self, request: SynapseRequest) -> None:
self._sso_handler.render_error(request, "invalid_session", str(e))
return
except MacaroonInvalidSignatureException as e:
logger.exception("Could not verify session for OIDC callback")
Copy link
Member

Choose a reason for hiding this comment

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

This means the signature on the macaroon was wrong, so either the server just rotated their macaroon secret, or they have something really wrong in their setup (like different workers not sharing the same secret). I would probably keep this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's still kept as a warning. Is the traceback useful to a server operator?

MacaroonInvalidSignatureException: Signatures do not match
  File "synapse/handlers/oidc.py", line 220, in handle_oidc_callback
    session, state
  File "synapse/handlers/oidc.py", line 1198, in verify_oidc_session_token
    v.verify(macaroon, self._macaroon_secret_key)
  File "pymacaroons/verifier.py", line 55, in verify
    discharge_macaroons
  File "pymacaroons/verifier.py", line 77, in verify_discharge
    raise MacaroonInvalidSignatureException('Signatures do not match')

With the proposed change, we would log the message Invalid session for OIDC callback: Signatures do not match as a warning.

I could throw in the name of e's class (i.e. Invalid session for OIDC callback - MacaroonInvalidSingatureException: Signatures do not match) if that's clearer?

Copy link
Member

Choose a reason for hiding this comment

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

With the proposed change, we would log the message Invalid session for OIDC callback: Signatures do not match as a warning.

That looks fine to me 👍

logger.warning("Could not verify session for OIDC callback: %s", e)
self._sso_handler.render_error(request, "mismatching_session", str(e))
return

Expand Down Expand Up @@ -827,7 +827,7 @@ async def handle_oidc_callback(
logger.debug("Exchanging OAuth2 code for a token")
token = await self._exchange_code(code)
except OidcError as e:
logger.exception("Could not exchange OAuth2 code")
Copy link
Member

Choose a reason for hiding this comment

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

This one can be an indicator of a misconfiguration, or just happen if there is a problem communicating to the IdP? It should not be that common (or we have other things to worry about)

Copy link
Member

Choose a reason for hiding this comment

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

Looking at Sentry, it happens a lot because codes get exchanged twice, which could happen if that callback handler is loaded multiple times. So it's probably safe to ignore in most cases, but useful to log when OIDC is being setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll still log this as a warning with the proposed change: Could not exchange OAuth2 code: bad_verification_code: The code passed is incorrect or expired.

logger.warning("Could not exchange OAuth2 code: %s", e)
self._sso_handler.render_error(request, e.error, e.error_description)
return

Expand Down