Cannot deny access via location

[Hufschmidt]

The following location (Deny access to .htaccess/.htpasswd files on nginx reverse proxy) does not work:

location ~ /\.ht(access|passwd)$ {
  deny all;
}

This location gets rejected by: manifests/resource/location.pp:325

if (($www_root == undef) and ($proxy == undef) and ($location_alias == undef) and ($stub_status == undef) and ($fastcgi == undef) and ($uwsgi == undef) and ($location_custom_cfg == undef) and ($internal == false)) {
    fail('Cannot create a location reference without a www_root, proxy, location_alias, fastcgi, uwsgi, stub_status, internal, or location_custom_cfg defined')
  }

Website gets deployed and kept in sync on proxies and nginx loadbalancer via git, so removing htaccess on the loadbalancer is not a (good) solution.

[zaroth]

I was under the impression that that's what location_custom_cfg is for? The following works for me:

nginx::resource::location {'htaccess block':
  location => '~ /\.ht(access|passwd)$',
  location_custom_cfg => { 'deny' => 'all' },
  vhost => 'myhost',
}

[Hufschmidt]

well there a location_deny and location_allow setting on nginx::resource::location, so it feals kind of counter-intuitive having to resort back to using location_custom_cfg. (Which I'm currently using, since there is no other way)

This warning should either be extended to check location_deny, location_allow or be removed all together and let nginx complain about invalid configurations.
I'd much prefer the later over puppet modules 'trying to be smart'...

[wyardley]

You're right, doesn't seem to let you create a location resource without one of those things.

nginx::nginx_vhosts:
  'test_80':
    www_root: '/var/www'
    ensure: present
    listen_port: 80
    server_name: 
      - localhost
    listen_options: 'default_server'
    use_default_location: false

nginx::nginx_locations:
  '~ /\.ht(access|passwd)$':
    location_deny:
      - 'all'
    location_allow:
      - '127.0.0.1'
    vhost: 'test_80'

I'm not sure what the ramifications would be of totally dropping the check in line 352 of manifests/resource/location.pp, though does seem like the workaround of putting something in location_custom_cfg, even if it's not the allow / deny conditions.

[wyardley]

Would adding $location_deny / $location_allow to that long list of parameters be a reasonable solution? or are there too many other things that could also be in a location block?

The following works for me in a very quick test, and also has the benefit of being a tiny bit shorter and easier to read (IMHO):

--- a/modules/nginx/manifests/resource/location.pp
+++ b/modules/nginx/manifests/resource/location.pp
@@ -343,10 +343,10 @@ define nginx::resource::location (
   if ($vhost == undef) {
     fail('Cannot create a location reference without attaching to a virtual host')
   }
-  if (($www_root == undef) and ($proxy == undef) and ($location_alias == undef) and ($stub_status == undef) and ($fastcgi == undef) and ($uwsgi == undef) and ($location_custom_cfg == undef) and ($internal == false) and ($try_files == undef)) {
-    fail('Cannot create a location reference without a www_root, proxy, location_alias, fastcgi, uwsgi, stub_status, internal, or location_custom_cfg defined')
+  if !($www_root or $proxy or $location_alias or $stub_status or $fastcgi or $uwsgi or $location_custom_cfg or $internal or $try_files or $location_allow or $location_deny) {
+    fail('Cannot create a location reference without a www_root, proxy, location_alias, fastcgi, uwsgi, stub_status, internal, location_custom_cfg, location_allow or location_deny defined')
   }
-  if (($www_root != undef) and ($proxy != undef)) {
+  if ($www_root and $proxy) {
     fail('Cannot define both directory and proxy in a virtual host')
   }

[wyardley]

I've created a PR that suggests a possible fix for this.

[wyardley]

I added try_files in the PR above as well.

I am sure there are other cases where www_root is being inherited, or for other reasons, none of these things need to be in the block... for now, I'm going to take the conservative path in terms of changing the code, but I do think there's a case to be made for removing this check entirely.

[wyardley]

I think the merged PR (which is in the latest release) will fix this. Please test if possible.