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

Add support for X-Forwarded-Proto #9472

Merged
merged 5 commits into from
Feb 24, 2021
Merged

Add support for X-Forwarded-Proto #9472

merged 5 commits into from
Feb 24, 2021

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Feb 23, 2021

rewrite XForwardedForRequest to set isSecure() based on X-Forwarded-Proto. Also implement getClientAddress() while we're here.

@richvdh
Add support for X-Forwarded-Proto
df6ab3d 
rewrite XForwardedForRequest to set `isSecure()` based on
`X-Forwarded-Proto`. Also implement `getClientAddress()` while we're here.
@richvdh
Update reverse-proxy documentation
271e091 
add X-Forwarded-Proto to revproxy docs
@richvdh
changelog
7dbb0f6
richvdh
richvdh commented Feb 23, 2021
View reviewed changes
docs/reverse_proxy.md
@@ -102,6 +103,7 @@ example.com:8448 {
SSLEngine on
ServerName matrix.example.com;

RequestHeader set "X-Forwarded-Proto" expr=%{REQUEST_SCHEME}
Copy link
Member Author

Choose a reason for hiding this comment

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

Apache and caddy both set X-Forwarded-For by default, though in a way that #9471 is problematic.

@richvdh richvdh requested a review from a team February 23, 2021 15:21
clokep
clokep reviewed Feb 23, 2021
View reviewed changes
docs/reverse_proxy.md Outdated Show resolved Hide resolved
@richvdh richvdh requested a review from clokep February 24, 2021 12:30
clokep
clokep reviewed Feb 24, 2021
View reviewed changes
synapse/http/site.py
return super().getClientAddress()


@implementer(IAddress)
Copy link
Member

Choose a reason for hiding this comment

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

I've always found this interface bizarre that it has no fields...

synapse/http/site.py Outdated Show resolved Hide resolved
synapse/http/site.py
return True
return super().isSecure()

def getClientIP(self) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

getClientIP will now potentially return private IPs (of the reverse proxy) instead of - if the header isn't set.

It seems that this gets used to fill the user_ips table, which can be retrieved via the whois call, which is a public API.

This is a misconfiguration, but is a change in behavior so thought it was worth calling out.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, that behaviour always seemed a bit magical to me. Why -?

Note that /whois is not strictly "public": it can only be called by admin users and the user themselves.

Copy link
Member

Choose a reason for hiding this comment

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

My point was more of that a private IP could potentially be exposed to users who are not system administrators. (This is what I meant by public.)

Copy link
Member Author

Choose a reason for hiding this comment

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

right, yes.

Well, I feel like it's a fairly basic configuration error. Don't do that.

@richvdh richvdh requested a review from clokep February 24, 2021 17:07
clokep
clokep approved these changes Feb 24, 2021
View reviewed changes
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

LGTM. I wonder if we should call something out in upgrade.rst?

@richvdh
Copy link
Member Author

richvdh commented Feb 24, 2021

I wonder if we should call something out in upgrade.rst?

I don't think there's anything hugely backwards-incompatible here. The warning is harmless and clear enough imho.

@richvdh richvdh merged commit d8e95e5 into develop Feb 24, 2021
@richvdh richvdh deleted the rav/x_forwarded_proto branch February 24, 2021 18:11
@erikjohnston
Copy link
Member

I wonder if we should call something out in upgrade.rst?

I don't think there's anything hugely backwards-incompatible here. The warning is harmless and clear enough imho.

I feel like if its important enough for us to log at WARNING (for every request) we should probably call it out? If nothing else almost doubling our log size isn't ideal

@richvdh
Copy link
Member Author

richvdh commented Feb 26, 2021

I feel like if its important enough for us to log at WARNING (for every request) we should probably call it out?

well it's called out in the newsfile...

but fair enough I'll stick something in UPGRADE.rst too.

@richvdh richvdh mentioned this pull request Feb 26, 2021
Merged
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Mar 14, 2021
@Midar
Update chat/matrix-synapse to 1.29.0
d5b1cc5 
Synapse 1.29.0 (2021-03-08)
===========================

Note that synapse now expects an `X-Forwarded-Proto` header when used with a reverse proxy. Please see [UPGRADE.rst](UPGRADE.rst#upgrading-to-v1290) for more details on this change.


No significant changes.


Synapse 1.29.0rc1 (2021-03-04)
==============================

Features
--------

- Add rate limiters to cross-user key sharing requests. ([\#8957](matrix-org/synapse#8957))
- Add `order_by` to the admin API `GET /_synapse/admin/v1/users/<user_id>/media`. Contributed by @dklimpel. ([\#8978](matrix-org/synapse#8978))
- Add some configuration settings to make users' profile data more private. ([\#9203](matrix-org/synapse#9203))
- The `no_proxy` and `NO_PROXY` environment variables are now respected in proxied HTTP clients with the lowercase form taking precedence if both are present. Additionally, the lowercase `https_proxy` environment variable is now respected in proxied HTTP clients on top of existing support for the uppercase `HTTPS_PROXY` form and takes precedence if both are present. Contributed by Timothy Leung. ([\#9372](matrix-org/synapse#9372))
- Add a configuration option, `user_directory.prefer_local_users`, which when enabled will make it more likely for users on the same server as you to appear above other users. ([\#9383](matrix-org/synapse#9383), [\#9385](matrix-org/synapse#9385))
- Add support for regenerating thumbnails if they have been deleted but the original image is still stored. ([\#9438](matrix-org/synapse#9438))
- Add support for `X-Forwarded-Proto` header when using a reverse proxy. ([\#9472](matrix-org/synapse#9472), [\#9501](matrix-org/synapse#9501), [\#9512](matrix-org/synapse#9512), [\#9539](matrix-org/synapse#9539))


Bugfixes
--------

- Fix a bug where users' pushers were not all deleted when they deactivated their account. ([\#9285](matrix-org/synapse#9285), [\#9516](matrix-org/synapse#9516))
- Fix a bug where a lot of unnecessary presence updates were sent when joining a room. ([\#9402](matrix-org/synapse#9402))
- Fix a bug that caused multiple calls to the experimental `shared_rooms` endpoint to return stale results. ([\#9416](matrix-org/synapse#9416))
- Fix a bug in single sign-on which could cause a "No session cookie found" error. ([\#9436](matrix-org/synapse#9436))
- Fix bug introduced in v1.27.0 where allowing a user to choose their own username when logging in via single sign-on did not work unless an `idp_icon` was defined. ([\#9440](matrix-org/synapse#9440))
- Fix a bug introduced in v1.26.0 where some sequences were not properly configured when running `synapse_port_db`. ([\#9449](matrix-org/synapse#9449))
- Fix deleting pushers when using sharded pushers. ([\#9465](matrix-org/synapse#9465), [\#9466](matrix-org/synapse#9466), [\#9479](matrix-org/synapse#9479), [\#9536](matrix-org/synapse#9536))
- Fix missing startup checks for the consistency of certain PostgreSQL sequences. ([\#9470](matrix-org/synapse#9470))
- Fix a long-standing bug where the media repository could leak file descriptors while previewing media. ([\#9497](matrix-org/synapse#9497))
- Properly purge the event chain cover index when purging history. ([\#9498](matrix-org/synapse#9498))
- Fix missing chain cover index due to a schema delta not being applied correctly. Only affected servers that ran development versions. ([\#9503](matrix-org/synapse#9503))
- Fix a bug introduced in v1.25.0 where `/_synapse/admin/join/` would fail when given a room alias. ([\#9506](matrix-org/synapse#9506))
- Prevent presence background jobs from running when presence is disabled. ([\#9530](matrix-org/synapse#9530))
- Fix rare edge case that caused a background update to fail if the server had rejected an event that had duplicate auth events. ([\#9537](matrix-org/synapse#9537))


Improved Documentation
----------------------

- Update the example systemd config to propagate reloads to individual units. ([\#9463](matrix-org/synapse#9463))


Internal Changes
----------------

- Add documentation and type hints to `parse_duration`. ([\#9432](matrix-org/synapse#9432))
- Remove vestiges of `uploads_path` configuration setting. ([\#9462](matrix-org/synapse#9462))
- Add a comment about systemd-python. ([\#9464](matrix-org/synapse#9464))
- Test that we require validated email for email pushers. ([\#9496](matrix-org/synapse#9496))
- Allow python to generate bytecode for synapse. ([\#9502](matrix-org/synapse#9502))
- Fix incorrect type hints. ([\#9515](matrix-org/synapse#9515), [\#9518](matrix-org/synapse#9518))
- Add type hints to device and event report admin API. ([\#9519](matrix-org/synapse#9519))
- Add type hints to user admin API. ([\#9521](matrix-org/synapse#9521))
- Bump the versions of mypy and mypy-zope used for static type checking. ([\#9529](matrix-org/synapse#9529))
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@clokep clokep clokep approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@richvdh @erikjohnston @clokep