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

Use ProxyFix instead of inspecting X-Forwarded-For header #542

Closed

Conversation

lnielsen
Copy link
Collaborator

@lnielsen lnielsen commented Aug 17, 2016

PR #352 fixed a security issue in extracting a correct remote IP address from X-Forwarded-For header.

The fix however doesn't take multiple trusted proxies into account, which you typically have in a load balanced setup (e.g. HAProxy + Nginx). The problem is located here, and can quickly be demonstrated with this example where it's always the last address in X-Forwarded-For being chosen:

>>> x_forwarded_for = '33.33.33.33, 22.22.22.22, 11.11.11.11'
>>> x_forwarded_for.rpartition(' ')[-1]
'11.11.11.11'

The problem with the current code is that if you use ProxyFix to set a correct IP address in request.remote_addr, then Flask-Security doesn't pick it up because X-Forwarded-For headers are inspected first. The workaround so far, is in addition to ProxyFix to use HeaderRewriterFix to remove the X-Forwarded-For headers completely from the request.

Proposal

IMHO, Flask-Security (and any other Flask extension for that matter) should rely solely on request.remote_addr to get the client IP address, and only document that you should use ProxyFix in order to ensure that remote_addr is set correctly (already done in Flask-Security).

@coveralls
Copy link

coveralls commented Aug 17, 2016

Coverage Status

Coverage decreased (-0.009%) to 94.274% when pulling bfcb8ab8d59033dc40a072f12394c380d00338b0 on lnielsen:feature-xforwardedfor-removal into 9e5865d on mattupstate:develop.

@coveralls
Copy link

coveralls commented Aug 17, 2016

Coverage Status

Coverage decreased (-0.009%) to 94.274% when pulling 0676c29 on lnielsen:feature-xforwardedfor-removal into 9e5865d on mattupstate:develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.009%) to 94.274% when pulling 0676c29 on lnielsen:feature-xforwardedfor-removal into 9e5865d on mattupstate:develop.

@lnielsen
Copy link
Collaborator Author

PyPy build is failing probably due to latest peewee release (2.8.2 release on Aug 9)

@mattupstate
Copy link
Collaborator

This is great. Thanks for this. Are you able to rebase off of develop and get the build passing?

Changes trackable feature to always rely on having a correctly set IP
address in request.remote_addr via e.g. Werkzeug's ProxyFix instead of
inspecting X-Forwarded-For headers (in which the current
implementation doesn't take multiple trusted proxies into account).
@lnielsen lnielsen force-pushed the feature-xforwardedfor-removal branch from 0676c29 to 8ea7e50 Compare January 25, 2017 15:19
boydgreenfield added a commit to onecodex/flask-security that referenced this pull request Mar 29, 2017
@jirikuncar
Copy link
Collaborator

Merged as 0676c29 via #617.

@jirikuncar jirikuncar closed this Apr 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants