Add configuration option for app service auth (header vs. query string)

[bradjones1]

Description:

After #13996 we now send the HS token in an authentication header in addition to the legacy query string. It would be good to make this configurable in the AS config, e.g. so that logs which include the query string no longer expose this security token or require manual redaction.

[clokep]

It would be good to make this configurable in the AS config

I think this was not included in the MSC: https://github.com/matrix-org/matrix-spec-proposals/blob/main/proposals/2832-appservice-auth-fix.md#alternatives

Or are you suggesting a way in the Synapse config to not send these for all appservices or something? How are you expecting to be able to configure this?

[bradjones1]

Or are you suggesting a way in the Synapse config to not send these for all appservices or something? How are you expecting to be able to configure this?

Correct. The spec says you should only send the header, but in the spirit of BC you can send both. (Interestingly, this broke BC for me on the appserver side since this is also an application that uses OAuth2 client bearer tokens, so it was trying to validate the token from the homeserver... easy enough to work around for that route but just interesting datapoint.)

So I am imagining a configuration option, perhaps even globally, that says, be strict to the spec and just send the header. Mainly the motivation is to mask the token from logs where query strings are displayed, but the authorization header is not.

[bradjones1]

Actually just noticed that the MSC specifically calls this out as a security consideration/motivation for the change. So I guess to put it another way, Synapse's BC layer for still including this in the query string deprives us of one of the main reasons to have implemented it (beyond simple spec compliance.)

Not fixing this causes access tokens to be logged in many bridges.

[clokep]

Did #13855 not fix this?

[bradjones1]

Did #13855 not fix this?

AFAICT that has to do with a proxy for requests and not the construction of the outgoing request to the AS.

The request still sends both the header and the query string on develop:

synapse/synapse/appservice/api.py

Lines 123 to 127 in a84744f

	response = await self.get_json(
	uri,
	{"access_token": service.hs_token},
	headers={"Authorization": [f"Bearer {service.hs_token}"]},
	)

And the PR that made the change you specifically noted:

I chose to send the token in both locations, which is allowed by the MSC.

[bradjones1]

And to put a finer point on it, the log leaks I'm speaking of would be external to Synapse, e.g. logs from a reverse proxy or in my case, GCP's Cloud Run tracing.

[clokep]

Did #13855 not fix this?

AFAICT that has to do with a proxy for requests and not the construction of the outgoing request to the AS.

The application service calling code in Synapse uses the ProxyAgent which is being fixed in #13855, so I think this would fix the issue in Synapse logs.

The request still sends both the header and the query string on develop:

Yes, no one is refuting that both are sent.

And to put a finer point on it, the log leaks I'm speaking of would be external to Synapse, e.g. logs from a reverse proxy or in my case, GCP's Cloud Run tracing.

This is a good point to bring up. 👍