Security decorator should log client IP address

[danballance]

Description

We use a project called Fail2Ban on most of our web services in order to block brute force attacks. Fail2Ban can be configured to scan custom application logs and look for authentication failures and block the offending IPs. Obviously in order to do this it needs the IP address to be recorded in the log.

Expected behaviour

Logs lines look like this:

• Token info (200) for client IP 123.123.123.123
• ... User scopes (set([u'reporting:write', u'reporting:read'])) do not match the scopes necessary to call endpoint (set(['pbx.provisioning:read'])). Aborting with 403 for client IP 123.123.123.123.

Actual behaviour

Logs lines look like this:

• Token info (200)
• ... User scopes (set([u'reporting:write', u'reporting:read'])) do not match the scopes necessary to call endpoint (set(['pbx.provisioning:read'])). Aborting with 403.

Steps to reproduce

Set up authentication and try to authenticate with an invalid access token and then insufficient scope.

Output of the commands:

• python --version
• pip show connexion | grep "^Version\:"

Python 2.7.12
Version: 2016.0.dev1

Would you accept a pull request for me to add the IPs into the log lines with the above format with request.remote_addr? The decorator is already accessing the Flask request object, so it should be a simple change to make.

[hjacobs]

@danballance I think logging the client IP would be valuable (but please note that X_FORWARDED_FOR is needed if behind LB/proxy).

[danballance]

Okay thanks, I'll be sure to handle the X_FORWARDED_FOR case as well then.

[danballance]

There's one additional security related issue I need to check as well. I know in PHP that the remote IP address in PHP is secure because it is taken from the TCP layer and not just read blindly from the HTTP packet. Since HTTP relies on TCP and its 3 way handshake, we can be sure that any IP address taken from the TCP layer has not been spoofed. Anything from the actual HTTP packet should not be trusted from a security PoV.

So I need to check that this behaviour is definitely the same in Python and also how X_FORWARDED_FOR works in terms of security. I'm assuming that when a trusted proxy is involved we can probably trust X_FORWARDED_FOR. However I'd need to be sure that if there is no proxy expected then a hacker can't set X_FORWARDED_FOR and use that to spoof their IP address. For that reason I'm thinking that perhaps a setting would be best so that this proxy IP logging feature can be switched on when the proxy is known and trusted @hjacobs ?

[danballance]

@hjacobs So I think we're safe from a security PoV to trust request.remote_addr. X_FORWARDED_FOR all depends on whether we are behind a trusted proxy or not. The only way I can see of indicating that the application is running behind a trusted proxy would be with an application configuration setting.

One option here, for a quick and clean commit, might be to log remote_addr and accept that this logging feature is not accurate behind a proxy (it will log the proxy). Obviously it's just a log line and no clients will get blocked unless a service such as Fail2ban is set up on the server. In some ways I'd prefer this option than add the risk of introducing spoofable IP addresses into the application.

[danballance]

Sorry to bump this one again but this security feature is quite important for my project. So I guess we have two options:

1. Don't worry about logging x-forwarded-for and acept this as a limitation
2. Add a trusted_ips list to the application configuration for known trusted proxy IPs. If remote_addr is in this list, then trust and log x-forwarded-for

My only hesitation about 2. is that it's not that easy to access global configuration settings and we don't yet have a config.json type file for these settings (AFAIK) so I'm worried to add yet more settings at constructions of the api.

You guys will know what's best for your project though - what would you prefer?

[danballance]

Okay so I've just sent across PR #415 with my proposed solution. It's based on my previous PR #414 because I know it will generate a conflict if the two branches are merged. Hope that's okay?

Does this seem like a reasonable way to implement this feature securely? Obviously nobody is obliged to make use of these logged IP addresses, but if somebody does then I think this should help to guarantee that it's not a hacker imitating a proxy or load balancer.

[danballance]

What do people think? Could this PR be merged in possibly? It just adds logging of IP addresses in the security decorator and also logs X-Forwarded-For if remote_addr is in a list of trusted IPs. (They are just single IP addresses for now and it does not handle subnets at this stage)

[kfot]

@danballance Recently I have been intensively using connexion when creating applications that are supposed to be very secure and I miss this functionality

[RobbeSneyders]

I don't think this should be in Connexion by default. You can provide your own security middleware since #1514