caddy-realip icon indicating copy to clipboard operation
caddy-realip copied to clipboard

Modify default behavior of maxhops to truncate

Open sporkmonger opened this issue 8 years ago • 3 comments

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.

sporkmonger avatar Jun 23 '17 06:06 sporkmonger

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?

captncraig avatar Jun 27 '17 16:06 captncraig

Yes, that's correct.

sporkmonger avatar Jun 27 '17 17:06 sporkmonger

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.

sporkmonger avatar Jun 27 '17 17:06 sporkmonger