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

Modify default behavior of maxhops to truncate #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sporkmonger
Copy link
Contributor

@sporkmonger sporkmonger commented Jun 23, 2017

Since maxhops is currently undocumented, I made a small change to move
existing error behavior to strict mode, in line with other strict mode
errors.

In non-strict mode, maxhops will truncate the list down to whatever the
maxhops value is set to. This allows for handling of scenarios where the
set of CIDR ranges is either unknown, too long to list, or too
frequently changing. Instead you can set your CIDR range to 0.0.0.0/0
and, e.g. for a Caddy server behind a single load balancer, you can set
maxhops to 1. Any IPs to the left of the 1 IP address allowed will
simply be truncated. This allows you to trivially eliminate IPs internal
to other networks which are provided by forward proxies, and
consistently obtain the IP you're actually looking for in upstream code.

While it's a breaking change, it's a breaking change to an undocumented
feature and this seems like a better behavioral fit for a plugin named
'realip', and it doesn't prevent people from getting the original
behavior if they so desire.

Also documented the full behavior of maxhops and added a bunch more
tests, including explicit tests of strict mode.

Since maxhops is currently undocumented, I made a small change to move
existing error behavior to strict mode, in line with other strict mode
errors.

In non-strict mode, maxhops will truncate the list down to whatever the
maxhops value is set to. This allows for handling of scenarios where the
set of CIDR ranges is either unknown, too long to list, or too
frequently changing. Instead you can set your CIDR range to 0.0.0.0/0
and, e.g. for a Caddy server behind a single load balancer, you can set
maxhops to 1. Any IPs to the left of the 1 IP address allowed will
simply be truncated. This allows you to trivially eliminate IPs internal
to other networks which are provided by forward proxies, and
consistently obtain the IP you're actually looking for in upstream code.

While it's a breaking change, it's a breaking change to an undocumented
feature and this seems like a better behavioral fit for a plugin named
'realip', and it doesn't prevent people from getting the original
behavior if they so desire.

Also documented the full behavior of maxhops and added a bunch more
tests, including explicit tests of strict mode.
@captncraig
Copy link
Owner

I'm not sure how I feel about this. It seems wrong to allow arbitrary length proxy chains. I understand that strict mode is a thing, but now I kinda wish we had reversed that. strict should be the default and you should need to opt-in to the riskier behaviours with unsafe or something.

On the other hand, it still does not trust anything coming from an untrusted proxy, as long as the most recent N are trusted, correct?

@sporkmonger
Copy link
Contributor Author

Yes, that's correct.

@sporkmonger
Copy link
Contributor Author

sporkmonger commented Jun 27, 2017

I'd argue, BTW, the other direction on strict / unsafe. Normally, as a security person, I argue in favor of secure defaults. However, the way strict mode works, plenty of stuff that is outside the implementer's control would trip those errors. In fact, plenty of stuff outside both the implementor and the client's direct control would trip those errors. You end up with errors that potentially neither party can fix because of intermediaries. This is likely to cause frustration and frustration is the leading cause of security tools being turned off. Meanwhile configuration options named unsafe that are in fact perfectly reasonable are a great way to freak out your PCI auditor. This is a great example of where simple principles of least surprise should take precedence over paranoid defaults.

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.

2 participants