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

Add reverse proxy setting #331

Merged
merged 6 commits into from
Jan 24, 2020
Merged

Add reverse proxy setting #331

merged 6 commits into from
Jan 24, 2020

Conversation

martin-css
Copy link
Contributor

@martin-css martin-css commented Dec 16, 2019

Description

Adds a new ReverseProxy option that controls whether the application accepts HTTP headers that can be set by a reverse proxy to indicate the real user etc.

Currently, this only affects whether the logger respects the X-Real-Ip header. If set, this will be used if present. If not set, this will be ignored, even with a value as a malicious user could be trying to log a false IP address.

In the future this could also control other common headers, e.g. X-Forwarded-For etc.

Big discussion point is that this is a BREAKING CHANGE for users currently behind a reverse proxy. The default value has been deliberately set to false as a user who runs this with a minimal configuration (without fully reading docs) should be left with a secure configuration by default. If being hosted behind a reverse proxy then this flag needs to be enabled.

We should consider:

  1. Best way to communicate clear notice to existing users about breaking change
  2. Should we temporarily leave default as true to allow for a period of bedding in then change to false after a suitable time? If so, how long?

Motivation and Context

Corrects the security issue by always accepting the X-Real-Ip header (described in #321).

How Has This Been Tested?

Quick and dirty test with curl - should be checked by others.

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

Documentation has been updated but left the changelog until discussion has been had around best way to implement (if at all).

@martin-css martin-css requested a review from a team December 16, 2019 20:09
Copy link
Member

@loshz loshz left a comment

Choose a reason for hiding this comment

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

Code looks sensible to me. I've added a few nits if you wouldn't mind fixing them and adding an entry to the CHANGELOG.

pkg/logger/logger.go Outdated Show resolved Hide resolved
pkg/logger/logger.go Show resolved Hide resolved
@martin-css
Copy link
Contributor Author

The CHANGELOG has been updated and minor change requested was added. Anything else needing done for this PR?

Copy link
Member

@loshz loshz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@loshz
Copy link
Member

loshz commented Jan 8, 2020

@JoelSpeed @steakunderscore can you also review this please.

Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

LGTM

Just for clarification though, the only place the header X-Real-IP is used is for logging right? If not, where else is it used?

@JoelSpeed JoelSpeed merged commit d9362d3 into oauth2-proxy:master Jan 24, 2020
ploxiln added a commit to ploxiln/oauth2_proxy that referenced this pull request Feb 19, 2020
this PR inspired by oauth2-proxy/oauth2-proxy#331
by Martin Campbell <[email protected]>

this option name inspired by the python Tornado framework HTTPServer
ploxiln added a commit to ploxiln/oauth2_proxy that referenced this pull request Feb 19, 2020
this PR inspired by oauth2-proxy/oauth2-proxy#331
by Martin Campbell <[email protected]>

this option name inspired by the python Tornado framework HTTPServer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants