search

saltstack-formulas / sudoers-formula

http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
41 stars 166 forks source link

[BUG] Rule ordering #82

Open tacerus opened 10 months ago

tacerus commented 10 months ago

Your setup

Formula commit hash / release tag

Versions reports (master & minion)

Pillar / config used

Bug details

Describe the bug

Currently, rules are run through dictsort, which causes issues with users/groups which match multiple rules. Sudo takes the last matching rule - which may not always be desirable - for example if one wants to have NOPASSWD for a specific command take priority over a general password enforced rule.

Example:

sudoers:
  groups:
    wheel:
      - 'ALL=(ALL) ALL'
    hypervisor.cluster-admins:
      - >-
        {{ grains['host'] }}=(root) NOPASSWD:
        /sbin/multipath -f [[\:alnum\:]]*,

Returns:

%hypervisor.cluster-admins falkor21=(root) NOPASSWD: /sbin/multipath -f [[\:alnum\:]]*
%wheel ALL=(ALL) ALL

It should be vice versa, as now, for users who are in both groups (wheel and hypervisor.cluster-admins) the NOPASSWD rule never matches, as the wheel rule takes priority.

I'm not sure if just removing dictsort is the right solution either though, I think it requires some other logic.

Steps to reproduce the bug

Expected behaviour

Attempts to fix the bug

Additional context

tacerus commented 10 months ago

It seems this was already attempted to be tackled in https://github.com/saltstack-formulas/sudoers-formula/pull/67 some years ago but without a conclusion.

daks commented 10 months ago

@tacerus the idea was to change the implementation to use lists instead of a dict to preserve the rules order. And introduce a BREAKING CHANGE. But in fact no implementation was made

Don't hesitate to try to re-implement it, even starting from the previous PR if it's still applicable. Don't hesitate to seek help from other formulas maintainers/users (see salt community slack and #formulas channel). (I'm myself no longer active on salt right now)

tacerus commented 10 months ago

Hi, thanks for getting back. I think the patch in https://github.com/saltstack-formulas/sudoers-formula/pull/67 is perfectly reasonable. The pillar in Salt is stored as OrderedDict these days, making .items() reasonably idempotent. Only when new lines are introduced, existing ones might be slightly reordered.