search

Luzifer / nginx-sso

SSO authentication provider for the auth_request nginx module
Apache License 2.0
282 stars 41 forks source link

Multiple rulesets not working as expected #44

Closed Tigger2014 closed 5 years ago

Tigger2014 commented 5 years ago

Hello,

I have the below ACL

acl:
  rule_sets:
  - rules:
    - field: "X-Host"
      equals: "ssh.ti***.co.uk"
    allow: ["@admins"]

  rule_sets:
  - rules:
    - field: "X-Host"
      equals: "netbox.ti***.co.uk"
    allow: ["@netbox"]

However only the netbox host allows connection, when logged in as a member of the admin group i always get a forbidden on the ssh subdomain, If i comment out the netbox entry the ssh works as expected, am i doing something wrong or is this a bug?

Luzifer commented 5 years ago

That's expected behavior according to your config:

Your second rule_sets key overwrites the first one (which is standard YAML behavior). Putting your sniplet through yamllint produces an error accordingly:

# yamllint test.yml
test.yml
  11:3      error    duplication of key "rule_sets" in mapping  (key-duplicates)

I think what you want to achieve is this:

acl:
  rule_sets:
  - rules:
    - field: "X-Host"
      equals: "ssh.ti***.co.uk"
    allow: ["@admins"]

  - rules:
    - field: "X-Host"
      equals: "netbox.ti***.co.uk"
    allow: ["@netbox"]

As rule_sets is an array / slice of rule sets it now contains two entries and you should get the results you're expecting.

Tigger2014 commented 5 years ago

Whoops, that was it indeed, Not super familiar with yaml, although I guess the question it now raises for me is what is the purpose of Rule_sets instead of just going into the rules directly?

Luzifer commented 5 years ago

That's simple: Imagine the acl field directly is an array of rule-sets and another option is needed within acl: Then it's a breaking change as acl needs to be converted from an array of rule-sets to a struct containing rule-sets and the new option.

The way it's currently designed the new option does not need a breaking change but just adds the new option. It's built with a future expansion in mind.