search

captncraig / caddy-realip

Real-IP middleware for caddy
MIT License
18 stars 9 forks source link

Modify default behavior of maxhops to truncate #10

Open sporkmonger opened 7 years ago

sporkmonger commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Yes, that's correct.

sporkmonger commented 7 years ago

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.