X-Forwarded-For can contain multiple IP addresses #352

fuhrysteve · 2014-12-28T13:37:29Z

We should be using whatever the last IP in X-Forwarded-For is. Note that in order for this to work properly behind a trusted proxy (i.e. if you are using nginx / similar to forward to gunicorn / similar, as is common), you must use ProxyFix as described in the werkzeug and flask documentation. I've added a note to the docs for SECURITY_TRACKABLE to point that out, since it's probably not obvious. Perhaps a note should be added elsewhere too? Not sure.

From the nginx docs: http://nginx.org/en/docs/http/ngx_http_proxy_module.html

$proxy_add_x_forwarded_for
    the “X-Forwarded-For” client request header field with the $remote_addr variable
    appended to it, separated by a comma. If the “X-Forwarded-For” field is not present
    in the client request header, the $proxy_add_x_forwarded_for variable is equal to
    the $remote_addr variable.

Here's a stack trace illustrating the issue I was having while using ProxyFix behind a proxy, which in this case was 127.0.0.1. The client's actual IP address in this case should be 189.254.205.210 (a Mexican IP address - the client was actually in Mexico at the time), though they were being proxied by an ec2 host at 172.21.5.123 (some indent added for clarity):

DataError: (raised as a result of Query-invoked autoflush; consider using a 
session.no_autoflush block if this flush is occurring prematurely) (DataError)
invalid input syntax for type inet: "172.21.5.123, 189.254.205.210"
LINE 1: ...-27T00:00:25.154208'::timestamp, current_login_ip='172.21.5....
                                                             ^
 'UPDATE user_account 
SET
last_login_at=%(last_login_at)s,
current_login_at=%(current_login_at)s,
current_login_ip=%(current_login_ip)s,
login_count=%(login_count)s, modifieddate=now()
WHERE
user_account.user_id = %(user_account_user_id)s' {
    'current_login_at': datetime.datetime(2014, 12, 27, 0, 0, 25, 154208),
    'login_count': 14,
    'user_account_user_id': 3,
    'current_login_ip': '172.21.5.123, 189.254.205.210',
    'last_login_at': datetime.datetime(
        2014, 12, 26, 21, 21, 21, 456305,
        tzinfo=psycopg2.tz.FixedOffsetTimezone(offset=0, name=None))
}

Stacktrace (most recent call last):

  File "flask/app.py", line 1817, in wsgi_app
    response = self.full_dispatch_request()
  File "flask/app.py", line 1477, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "flask/app.py", line 1381, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "flask/_compat.py", line 33, in reraise
    raise value
  File "flask/app.py", line 1475, in full_dispatch_request
    rv = self.dispatch_request()
  File "flask/app.py", line 1461, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "flask_security/decorators.py", line 205, in wrapper
    return f(*args, **kwargs)
  File "flask_security/views.py", line 76, in login
    login_user(form.user, remember=form.remember.data)
  File "flask_security/utils.py", line 82, in login_user
    identity=Identity(user.id))
  File "blinker/base.py", line 267, in send
    for receiver in self.receivers_for(sender)]
  File "blinker/base.py", line 267, in <listcomp>
    for receiver in self.receivers_for(sender)]
  File "site-packages/flask_principal.py", line 469, in _on_identity_changed
    self.set_identity(identity)
  File "site-packages/flask_principal.py", line 418, in set_identity
    self._set_thread_identity(identity)
  File "site-packages/flask_principal.py", line 463, in _set_thread_identity
    identity=identity)
  File "blinker/base.py", line 267, in send
    for receiver in self.receivers_for(sender)]
  File "blinker/base.py", line 267, in <listcomp>
    for receiver in self.receivers_for(sender)]
  File "flask_security/core.py", line 214, in _on_identity_loaded
    for role in current_user.roles:
  File "werkzeug/local.py", line 338, in __getattr__
    return getattr(self._get_current_object(), name)
  File "sqlalchemy/orm/attributes.py", line 239, in __get__
    return self.impl.get(instance_state(instance), dict_)
  File "sqlalchemy/orm/attributes.py", line 591, in get
    value = self.callable_(state, passive)
  File "sqlalchemy/orm/strategies.py", line 534, in _load_for_state
    return self._emit_lazyload(session, state, ident_key, passive)
  File "<string>", line 1, in <lambda>
  File "sqlalchemy/orm/strategies.py", line 603, in _emit_lazyload
    result = q.all()
  File "sqlalchemy/orm/query.py", line 2320, in all
    return list(self)
  File "sqlalchemy/orm/query.py", line 2437, in __iter__
    self.session._autoflush()
  File "sqlalchemy/orm/session.py", line 1208, in _autoflush
    util.raise_from_cause(e)
  File "sqlalchemy/util/compat.py", line 188, in raise_from_cause
    reraise(type(exception), exception, tb=exc_tb, cause=exc_value)
  File "sqlalchemy/util/compat.py", line 182, in reraise
    raise value
  File "sqlalchemy/orm/session.py", line 1198, in _autoflush
    self.flush()
  File "sqlalchemy/orm/session.py", line 1919, in flush
    self._flush(objects)
  File "sqlalchemy/orm/session.py", line 2037, in _flush
    transaction.rollback(_capture_exception=True)
  File "sqlalchemy/util/langhelpers.py", line 60, in __exit__
    compat.reraise(exc_type, exc_value, exc_tb)
  File "sqlalchemy/util/compat.py", line 182, in reraise
    raise value
  File "sqlalchemy/orm/session.py", line 2001, in _flush
    flush_context.execute()
  File "sqlalchemy/orm/unitofwork.py", line 372, in execute
    rec.execute(self)
  File "sqlalchemy/orm/unitofwork.py", line 526, in execute
    uow
  File "sqlalchemy/orm/persistence.py", line 60, in save_obj
    mapper, table, update)
  File "sqlalchemy/orm/persistence.py", line 518, in _emit_update_statements
    execute(statement, params)
  File "sqlalchemy/engine/base.py", line 729, in execute
    return meth(self, multiparams, params)
  File "sqlalchemy/sql/elements.py", line 322, in _execute_on_connection
    return connection._execute_clauseelement(self, multiparams, params)
  File "sqlalchemy/engine/base.py", line 826, in _execute_clauseelement
    compiled_sql, distilled_params
  File "sqlalchemy/engine/base.py", line 958, in _execute_context
    context)
  File "sqlalchemy/engine/base.py", line 1159, in _handle_dbapi_exception
    exc_info
  File "sqlalchemy/util/compat.py", line 188, in raise_from_cause
    reraise(type(exception), exception, tb=exc_tb, cause=exc_value)
  File "sqlalchemy/util/compat.py", line 181, in reraise
    raise value.with_traceback(tb)
  File "sqlalchemy/engine/base.py", line 951, in _execute_context
    context)
  File "sqlalchemy/engine/default.py", line 436, in do_execute
    cursor.execute(statement, parameters)

coveralls · 2014-12-28T13:42:09Z

Coverage decreased (-0.08%) when pulling 94fc9304c5217210c06db631b51603a8fd57a507 on fuhrysteve:develop into c7d0ea9 on mattupstate:develop.

coveralls · 2014-12-28T15:15:39Z

Coverage increased (+0.01%) when pulling aadfe70b9f7637268b4f12ec18cbd42840e22249 on fuhrysteve:develop into c7d0ea9 on mattupstate:develop.

fuhrysteve · 2014-12-28T15:27:44Z

Some references:

#234

58b7fa8

coveralls · 2014-12-29T13:22:03Z

Coverage increased (+0.01%) when pulling ab47fe21cab542a931ba061c28024e398cec18cf on fuhrysteve:develop into c7d0ea9 on mattupstate:develop.

coveralls · 2014-12-29T13:26:46Z

Coverage remained the same when pulling a140996eb093998f2830a43ce4eaff3ac69d0ec4 on fuhrysteve:develop into c7d0ea9 on mattupstate:develop.

coveralls · 2014-12-29T13:29:36Z

Coverage remained the same when pulling a140996eb093998f2830a43ce4eaff3ac69d0ec4 on fuhrysteve:develop into c7d0ea9 on mattupstate:develop.

From the nginx docs: http://nginx.org/en/docs/http/ngx_http_proxy_module.html > $proxy_add_x_forwarded_for > the “X-Forwarded-For” client request header field with the $remote_addr > variable appended to it, separated by a comma. If the “X-Forwarded-For” > field is not present in the client request header, the > $proxy_add_x_forwarded_for variable is equal to the $remote_addr > variable. Use the last IP address in X-Forwarded-For. For this to work properly behind a trusted proxy, you must be using ProxyFix as described in the flask & werkzeug documentation.

coveralls · 2014-12-29T13:37:34Z

Coverage remained the same when pulling 923ad72 on fuhrysteve:develop into c7d0ea9 on mattupstate:develop.

fuhrysteve · 2014-12-29T13:37:59Z

I did a little more research on this and discovered that there are some issues with simply choosing the first address in X-Forwarded-For. See X-Forwarded-For for a little more info on that.

There is also a potential issue when using the last address on the list if you are using a trusted proxy server to proxy web requests. The recommended solution in the werkzeug and flask documentation is to use ProxyFix in these situations.

See:
http://werkzeug.pocoo.org/docs/0.9/contrib/fixers/#werkzeug.contrib.fixers.ProxyFix
http://flask.pocoo.org/docs/0.10/deploying/wsgi-standalone/#proxy-setups

I've rebased my commits and cleaned up the original post for clarity.

mattupstate · 2015-05-02T17:46:19Z

Good stuff. Thanks!.

X-Forwarded-For can contain multiple IP addresses

fuhrysteve force-pushed the develop branch from a140996 to 923ad72 Compare December 29, 2014 13:32

mattupstate pushed a commit that referenced this pull request May 2, 2015

Merge pull request #352 from fuhrysteve/develop

2e08ec8

X-Forwarded-For can contain multiple IP addresses

mattupstate merged commit 2e08ec8 into pallets-eco:develop May 2, 2015

lnielsen mentioned this pull request Aug 17, 2016

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

X-Forwarded-For can contain multiple IP addresses #352

X-Forwarded-For can contain multiple IP addresses #352

fuhrysteve commented Dec 28, 2014

coveralls commented Dec 28, 2014

coveralls commented Dec 28, 2014

fuhrysteve commented Dec 28, 2014

coveralls commented Dec 29, 2014

coveralls commented Dec 29, 2014

coveralls commented Dec 29, 2014

coveralls commented Dec 29, 2014

fuhrysteve commented Dec 29, 2014

mattupstate commented May 2, 2015

X-Forwarded-For can contain multiple IP addresses #352

X-Forwarded-For can contain multiple IP addresses #352

Conversation

fuhrysteve commented Dec 28, 2014

coveralls commented Dec 28, 2014

coveralls commented Dec 28, 2014

fuhrysteve commented Dec 28, 2014

coveralls commented Dec 29, 2014

coveralls commented Dec 29, 2014

coveralls commented Dec 29, 2014

coveralls commented Dec 29, 2014

fuhrysteve commented Dec 29, 2014

mattupstate commented May 2, 2015