Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Security decorator should log client IP address #415

Closed
wants to merge 7 commits into from
Closed

Security decorator should log client IP address #415

wants to merge 7 commits into from

Conversation

danballance
Copy link
Contributor

Fixes #410

Changes proposed in this pull request:

  • trusted_ips list optionally passed in at application start. If passed then we'll only trust X-Forwarded-For if remote_addr is in the list of trusted IPs (i.e. known proxies under our control)
  • "for client IP 'x.x.x.x" format string is added to log lines
  • testfixtures added to setup.py in order to make use of LogCapture. I wouldn't normally recommend testing log lines (it does feel fragile) but in this case we need to be sure the correct IP is logged.

@coveralls
Copy link

coveralls commented Mar 14, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 5f8a71b on danballance:log_client_ips into e9db7fe on zalando:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5f8a71b on danballance:log_client_ips into e9db7fe on zalando:master.

@rafaelcaricio
Copy link
Collaborator

Could you please rebase this PR? I am seeing a lot of changes related to the PR #414.

# 1) remote_addr, no x-forwarded-for, invalid token (401)
with LogCapture() as l:
wrapped_func = verify_oauth('https://example.org/tokeninfo', set(['admin']), [], func)
request, headers = _setup_request_mock(remote_addr="123.123.123.123")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better than doing all this mocking is to use test_request_context. It is safer because we relay on how Flask actually works and not how the mocks are set up.

api_client.application.test_request_context('/').push()

Reference:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, no problem, I will review this code. A few things to clean up. Thanks for the feedback.

@rafaelcaricio
Copy link
Collaborator

@danballance Thank you for the PR. Would be nice if we could have some documentation on what it does and how this works. What do you think? Do not need to be long. Basically a summary of your comments from #410.

@rafaelcaricio
Copy link
Collaborator

@danballance Please review the changes made in this PR. It needs rebasing.

@danballance
Copy link
Contributor Author

Hi,

I've just been looking for a gap here where I can look again at this. Tomorrow may be possible - certainly this week.

@danballance
Copy link
Contributor Author

I am looking at this, but 1.1.7 appears to be a major re-factor and lots of things have broken. I still hope to resolve everything this week, but it's not going to be tomorrow now.

@rafaelcaricio rafaelcaricio dismissed their stale review April 5, 2017 14:23

Will review again after refactor.

@rafaelcaricio
Copy link
Collaborator

@danballance Yes, sorry about that. It was an expected internal refactor to move the project forward (see #380).

@danballance
Copy link
Contributor Author

No problem - it makes complete sense to decouple Flask. It's a big change though! I can see you guys are still changing quite a lot of code so I think I'll wait until the dust settles before I return to this :)

@rafaelcaricio
Copy link
Collaborator

@danballance Sure, no problem. Yeah, now we are fixing some things since the decoupling started.

@jmcs
Copy link
Contributor

jmcs commented Jul 6, 2018

I'm closing this PR since it wasn't updated in more than 1 year. If you are still interested in adding this change please reopen it.

@jmcs jmcs closed this Jul 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Security decorator should log client IP address
4 participants