Pass additional headers from the worker to the main process. #7797

clokep · 2020-07-07T14:02:55Z

I believe that this will fix #6396 by passing the X-Forwarded-For and User-Agent headers from the worker to the main process when proxying requests.

Note that a User-Agent is usually generated, but will get overridden if provided as a header:

synapse/synapse/http/client.py

Lines 400 to 406 in a3f1156

    
           actual_headers = { 
        
               b"Content-Type": [b"application/json"], 
        
               b"User-Agent": [self.user_agent], 
        
               b"Accept": [b"application/json"], 
        
           } 
        
           if headers: 
        
               actual_headers.update(headers)

clokep · 2020-07-07T14:06:31Z

I wasn't really sure how to test this, but I think it is doing the right thing.

synapse/app/generic_worker.py

erikjohnston · 2020-07-07T18:55:58Z

It might also be nice at the persistence stage to ignore/error when we try and persist ips/user agents that are our own, or something

synapse/app/generic_worker.py

richvdh

lgtm!

clokep · 2020-07-09T11:34:03Z

It might also be nice at the persistence stage to ignore/error when we try and persist ips/user agents that are our own, or something

We talked a little bit about this and there's a few pieces of it that would likely be worth doing separately:

The XForwardedForRequest should potentially filter out private IPs.
We should potentially not save - into the database (but this theoretically shouldn't happen anymore...)
User agents are a bit more problematic since they can really be any string...

For now I'm going to leave this.

…anoa/amorgan.xyz * 'release-v1.17.0' of github.com:matrix-org/synapse: (117 commits) 1.17.0 update changelog fix migration, again fix changelog 1.16.1 Drop incorrectly-added table `local_rejections_stream`. (#7816) 1.17.0rc1 Fix some spelling mistakes / typos. (#7811) `update_membership` declaration: now always returns an event id. (#7809) Improve stacktraces from exceptions in background processes (#7808) Fix `can only concatenate list (not "tuple") to list` exception (#7810) Pass original request headers from workers to the main process. (#7797) Generate real events when we reject invites (#7804) Add `HomeServer.signing_key` property (#7805) Revert "Update the installation docs on apt-transport-https (#7801)" Do not use simplejson in Synapse. (#7800) Stop passing bytes when dumping JSON (#7799) Update the installation docs on apt-transport-https (#7801) shuffle changelog slightly 1.16.0 ...

* commit '43726783e': (22 commits) 1.17.0rc1 Fix some spelling mistakes / typos. (#7811) `update_membership` declaration: now always returns an event id. (#7809) Improve stacktraces from exceptions in background processes (#7808) Fix `can only concatenate list (not "tuple") to list` exception (#7810) Pass original request headers from workers to the main process. (#7797) Generate real events when we reject invites (#7804) Add `HomeServer.signing_key` property (#7805) Revert "Update the installation docs on apt-transport-https (#7801)" Do not use simplejson in Synapse. (#7800) Stop passing bytes when dumping JSON (#7799) Update the installation docs on apt-transport-https (#7801) shuffle changelog slightly Change Caddy links (old is deprecated) (#7789) Stop populating unused table `local_invites`. (#7793) Refactor getting replication updates from database v2. (#7740) Add libwebp dependency to Dockerfile (#7791) Add documentation for JWT login type and improve sample config. (#7776) Convert the appservice handler to async/await. (#7775) Don't ignore `set_tweak` actions with no explicit `value`. (#7766) ...

clokep added 2 commits July 7, 2020 09:52

Pass additional headers from the worker to the main process.

a120931

Add a changelog entry.

5a566c4

clokep marked this pull request as ready for review July 7, 2020 14:05

clokep requested a review from a team July 7, 2020 15:12

clokep commented Jul 7, 2020

View reviewed changes

synapse/app/generic_worker.py Outdated Show resolved Hide resolved

Always override the expected headers.

17b916d

erikjohnston approved these changes Jul 7, 2020

View reviewed changes

richvdh reviewed Jul 8, 2020

View reviewed changes

synapse/app/generic_worker.py Outdated Show resolved Hide resolved

clokep added 3 commits July 8, 2020 12:54

Ensure that X-Forwarded-For exists and add the previous hop.

7033f19

Add type check for the client.

b0ff671

Lint.

826543c

clokep requested a review from richvdh July 8, 2020 17:51

richvdh reviewed Jul 8, 2020

View reviewed changes

synapse/app/generic_worker.py Outdated Show resolved Hide resolved

Construct a new list.

f74ffc4

clokep requested a review from richvdh July 8, 2020 18:51

richvdh approved these changes Jul 9, 2020

View reviewed changes

clokep merged commit 8fa7fdd into develop Jul 9, 2020

clokep deleted the clokep/worker-ip branch July 9, 2020 11:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass additional headers from the worker to the main process. #7797

Pass additional headers from the worker to the main process. #7797

clokep commented Jul 7, 2020

clokep commented Jul 7, 2020

erikjohnston commented Jul 7, 2020

richvdh left a comment

clokep commented Jul 9, 2020

	actual_headers = {
	b"Content-Type": [b"application/json"],
	b"User-Agent": [self.user_agent],
	b"Accept": [b"application/json"],
	}
	if headers:
	actual_headers.update(headers)

Pass additional headers from the worker to the main process. #7797

Pass additional headers from the worker to the main process. #7797

Conversation

clokep commented Jul 7, 2020

clokep commented Jul 7, 2020

erikjohnston commented Jul 7, 2020

richvdh left a comment

Choose a reason for hiding this comment

clokep commented Jul 9, 2020