location_allow / location_deny are broken outside the most simple cases

[carljohnstone]

From the nginx docs:

location / {
    deny  192.168.1.1;
    allow 192.168.1.0/24;
    allow 10.1.1.0/16;
    allow 2001:0db8::/32;
    deny  all;
}

The rules are checked in sequence until the first match is found. In this example, access is allowed only for IPv4 networks 10.1.1.0/16 and 192.168.1.0/24 excluding the address 192.168.1.1, and for IPv6 network 2001:0db8::/32.

If this is configured using:

        location_deny => [
          '192.168.1.1',
          'all',
        ],
        location_allow => [
          '192.168.1.0/24',
          '10.1.1.0/16',
          '2001:0db8::/32',
        ],

The resultant nginx config would be:

location / {
    allow 192.168.1.0/24;
    allow 10.1.1.0/16;
    allow 2001:0db8::/32;
    deny 192.168.1.1;
    deny all;
}

Which doesn't work correctly because allow 192.168.1.0/24 would allow a request from 192.168.1.1 before nginx got to the deny block.

Other ways of adding config options eg location_cfg_append also fail because the hash keys are sorted so that allow always appear before deny (although arguably ordering of hashes is undetermined anyway).

Currently the only way to add the directives in the correct order would be to use something like raw_append.

Whilst it might appear that a simple solution could be to put all the deny directives before allow, except deny all which would be put at the end, that would fail with more complicated examples where there might be a deny-allow-deny-allow-deny etc pattern of less specific rules. The suggests to me that just ordering the rules based on the specificity of the IP addresses with deny rules before allow rules might be all that's needed to make this work correctly - however I'd like to hear more feedback on that from somebody with more nginx experience.

The best solution may be to just change the interface so that an array of rules is fed in and order maintained in the generated config file. eg:

    http_access => [
      { type => deny, cidr => '192.168.1.1' },
      { type => allow, cidr => '192.168.1.0/24' },
      { type => deny, cidr => 'all' },
  ]

[carljohnstone]

I probably should have said that whilst I can't promise anything, I'm interested in providing a patch for the issue. What I was hoping to do was illicit some discussion regarding the preferred direction to take this, as I don't want to spend some time on a PR that will be summarily disregarded as a poor fix.