Add possibility to handle proxy info differently depending on upstream IP#11
Merged
Conversation
Owner
|
I will take the weekend to look into this. |
pires
force-pushed
the
master
branch
2 times, most recently
from
e4eae17 to
b6563d9
Compare
Owner
|
I like this idea a lot! Can you please rebase and fix conflicts? I'm totally fine with merging and breaking the API since there's no release and people can simply rely on the commit of their choosing. |
…m ip Without such a possibility usage of this library is prone to security attacks enabling simple faking of client ip addresses by accepting any proxy protocol header regardless of whether the upstream source is allowed to send one or not. This introduces the possibility to decide on each new incoming connection whether the proxy header should be used, ignored, or reject the connection. It also allows to reject the connection when a proxy header is required but not send.
mikey179
force-pushed
the
proxy_handling_policy
branch
from
b2ca7a5 to
fa756e1
Compare
Author
|
Rebased and fixed conflicts. |
Closed
Owner
jefferai
reviewed
Jan 8, 2020
jefferai
reviewed
Jan 8, 2020
jefferai
reviewed
Jan 9, 2020
jefferai
reviewed
Jan 9, 2020
jefferai
reviewed
Jan 9, 2020
added 5 commits
February 10, 2020 11:05
…ed before first read
Author
|
I think I addressed all concerns raised so far, but in case I missed anything just ping me. |
jefferai
reviewed
Feb 10, 2020
|
Other than my suggestion around CIDRs I think this is looking good! |
This introduces the possibility to use IP ranges next to IP adresses for the whitelists. To ensure the provided values are valid they are parsed, invalid IP ranges or IP addresses are rejected. However, this requires LaxWhiteListPolicy() and StrictWhiteListPolicy() to return an error in such cases. To cope with that, similar to other Must* functions in the standard library, two new functions MustLaxWhiteListPolicy() and MustStrictWhiteListPolicy() which panic instead are introduced.
Owner
|
@jefferai can you please give it a last review? |
jefferai
approved these changes
Feb 13, 2020
|
I think it looks good! |
pires
approved these changes
Feb 13, 2020
pires
left a comment
pires
left a comment
Owner
There was a problem hiding this comment.
Thank you very much for this contribution and for being so collaborative! <3
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This introduces the possibility to decide on each new incoming connection whether the proxy header should be used, ignored, or the connection be rejected. It also allows to reject the connection when a proxy header is required but not send at all.
Without such a possibility usage of this library is prone to security attacks enabling simple faking of client IP addresses by accepting any proxy protocol header regardless of whether the upstream source is allowed to send one or not. The need for this is derived from our use case, where we have an application which accepts connections both through a proxy and direct (there's a reason for that). If we would allow the proxy header for direct connections it would allow clients to pretend they are from another IP address which we don't want.
While
NewConnis part of the public API I decided to change it nevertheless as no official release or version of this library exists yet. In case you want to keep it as is I'd introduce another functionNewConnWithPolicyinstead where the caller can provide the policy, and fall back to theUSEpolicy inNewConn.