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

Implements --real-client-ip-header option. #503

Merged
merged 19 commits into from
May 12, 2020

Conversation

Izzette
Copy link
Contributor

@Izzette Izzette commented Apr 25, 2020

Description

  • The -real-client-ip-header determines what HTTP header is used for
    determining the "real client IP" of the remote client.
  • The -real-client-ip-header option supports the following headers:
    X-Forwarded-For X-ProxyUser-IP and X-Real-IP (default).
  • Introduces new realClientIPParser interface to allow for multiple
    polymorphic classes to decide how to determine the real client IP.
  • TODO: implement the more standard, but more complex Forwarded HTTP
    header.

Motivation and Context

  • Support other headers besides X-Real-IP for logging.
  • Support a future feature to allow whitelisted IPs behind a reverse-proxy to bypass authentication.

How Has This Been Tested?

  • Wrote new standard go tests to test the features implemented.
  • TODO: write tests to confirm logging backwards compatibility.
    • Would love some advice on how capturing logs could be implemented.

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.

Fixes #424

Sorry, something went wrong.

* The -real-client-ip-header determines what HTTP header is used for
  determining the "real client IP" of the remote client.
* The -real-client-ip-header option supports the following headers:
  X-Forwarded-For X-ProxyUser-IP and X-Real-IP (default).
* Introduces new realClientIPParser interface to allow for multiple
  polymorphic classes to decide how to determine the real client IP.
* TODO: implement the more standard, but more complex `Forwarded` HTTP
  header.
CHANGELOG.md Outdated Show resolved Hide resolved
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.

@Izzette Thanks for re-doing this PR, I like the way this is done more now than before :) Added a few comments, if you could take a look, that'd be great!

pkg/logger/logger.go Show resolved Hide resolved
realclientip.go Show resolved Hide resolved
docs/configuration/configuration.md Outdated Show resolved Hide resolved
realclientip.go Show resolved Hide resolved
Izzette and others added 6 commits April 27, 2020 14:02

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@Izzette Izzette requested a review from JoelSpeed April 27, 2020 12:54
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.

Added a couple of minor suggestions, but otherwise looks good

docs/configuration/configuration.md Outdated Show resolved Hide resolved
realclientip.go Outdated Show resolved Hide resolved
pkg/logger/logger.go Show resolved Hide resolved
Izzette and others added 4 commits April 29, 2020 19:48

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-Authored-By: Joel Speed <Joel.speed@hotmail.co.uk>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
* Simplify the language around dependance on -reverse-proxy

Co-Authored-By: Joel Speed <Joel.speed@hotmail.co.uk>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
CHANGELOG.md Outdated Show resolved Hide resolved
@RichiCoder1
Copy link

Jumping in a bit late, but if you're adding support for all these, might be worth adding Forwarded support too since that's the most recent standard.

@Izzette
Copy link
Contributor Author

Izzette commented May 4, 2020

Jumping in a bit late, but if you're adding support for all these, might be worth adding Forwarded support too since that's the most recent standard.

It's already noted as a TODO in the initial description, but because the specification of the forwarded header is more complex and my implementation is type-polymorphic I'm choosing the leave this out of this PR. It can be included in another PR after this one is merged.

Izzette and others added 3 commits May 4, 2020 09:32

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
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.

Bar the one outstanding comment re getClient vs getClientFunc I think I'm happy with this

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
CHANGELOG.md Outdated Show resolved Hide resolved

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@Izzette Izzette changed the title Implements -real-client-ip-header option. Implements --real-client-ip-header option. May 11, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@loshz loshz removed their request for review May 12, 2020 14:36

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@Izzette Izzette requested a review from JoelSpeed May 12, 2020 15:56
@Izzette
Copy link
Contributor Author

Izzette commented May 12, 2020

@steakunderscore @JoelSpeed, are we all happy, should we merge this?

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Isabelle COWAN-BERGMAN <Izzette@users.noreply.github.com>
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.

Thanks for all your work on this @Izzette, apologies it's taken so long to get it in

@JoelSpeed JoelSpeed merged commit 111d17e into oauth2-proxy:master May 12, 2020
ploxiln added a commit to ploxiln/oauth2_proxy that referenced this pull request May 24, 2020
also remove the recently-added --xheaders option (it was never in a release),
in favor of disabling trust of the X-Real-IP header by setting the new
real-client-ip-header option to a blank/empty string

inspired by oauth2-proxy/oauth2-proxy#503
ploxiln added a commit to ploxiln/oauth2_proxy that referenced this pull request May 24, 2020
also remove the recently-added --xheaders option (it was never in a release),
in favor of disabling trust of the X-Real-IP header by setting the new
real-client-ip-header option to a blank/empty string

inspired by oauth2-proxy/oauth2-proxy#503
ploxiln added a commit to ploxiln/oauth2_proxy that referenced this pull request May 24, 2020
also remove the recently-added --xheaders option, in favor
of disabling trust of the X-Real-IP header by setting the
real-client-ip-header option to a blank/empty string

inspired by oauth2-proxy/oauth2-proxy#503
ploxiln added a commit to ploxiln/oauth2_proxy that referenced this pull request May 24, 2020
also remove the recently-added --xheaders option, in favor
of disabling trust of the X-Real-IP header by setting the
real-client-ip-header option to a blank/empty string

inspired by oauth2-proxy/oauth2-proxy#503
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.

X-Real-IP only meaningful to mostly CloudFlare users
4 participants