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 feature CF-Connecting-IP header #217

Closed
wants to merge 2 commits into from

Conversation

gaikov-everstake
Copy link

Description

This is PoC how to use header if you deploy Chproxy behind Cloudflare.
Fixes this issue

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Checklist

  • Linter passes correctly
  • Add tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary

Does this introduce a breaking change?

  • Yes
  • No

Further comments

@mga-chka
Copy link
Collaborator

mga-chka commented Aug 30, 2022

Hi,
thanks for the PR.
A few comments:

  • why do you need to put the same logic in many places? The one in main.go should be enough.
  • In main.go it might be better to add your code only when we need it (for example I don't think you'll need it for /metrics http queries that fetch stats from chproxy). IHMO, you should put your code herehttps://github.com/ContentSquare/chproxy/blob/master/main.go#L235
  • you should also add a couple of tests in main_test.go with an http call with/without your header and the same with https to be sure that your feature work as expected and won't be erased by someone else in the future. If you look at the file you'll see it's a couple tests that creates a chproxy instance then run http queries on it and look at the answer so it's not difficult to add new tests
  • can you use another header than CF-Connecting-IP because this one seems specific to cloudflare and it might be better to use a "generic" one so that it will work with other load balancers
  • last but not least, by looking at a library doing the same kind of stuff, they explicitly say that this feature should work only for a program behind a proxy (nginx, apache, ...) otherwise it may lead to a security issue. So you'll have to create a setting (with the associated doc) so that chproxy user activate it and it will be unactivated by default (cf https://github.com/gorilla/handlers/blob/v1.5.1/proxy_headers.go#L43). If you want to see how to add a settings, you can look at this PR https://github.com/ContentSquare/chproxy/pull/191/files

@ank-everstake
Copy link

We just showed that it is possible and what we currently use.
Everything you propose is totally right, but I am sorry that we cannot put a lot of time to follow the guidelines.
Yep, flags to enable /disable and set specific header would do it better and with safety in mind.

We would be SO grateful if you would took it to planned features as a whole with proper planning.

@mga-chka
Copy link
Collaborator

Hi,
This feature is not in our priority right now. If you can wait a couple of months we could do it (but I can't promise it because we could have other top priority tasks in between).
But if you want it sooner you'd better do it or finding someone doing it (I my previous comment I gave most of the information that should make it easy to do it).

@mga-chka
Copy link
Collaborator

closing this PR since another PR is solving the same issue
#225

@mga-chka mga-chka closed this Sep 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants