Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Rule ordering #82

Open
tacerus opened this issue Jan 13, 2024 · 3 comments
Open

[BUG] Rule ordering #82

tacerus opened this issue Jan 13, 2024 · 3 comments
Labels

Comments

@tacerus
Copy link

tacerus commented Jan 13, 2024

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 tacerus added the bug label Jan 13, 2024
@tacerus
Copy link
Author

tacerus commented Jan 13, 2024

It seems this was already attempted to be tackled in #67 some years ago but without a conclusion.

@daks
Copy link
Member

daks commented Jan 19, 2024

@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
Copy link
Author

tacerus commented Jan 19, 2024

Hi, thanks for getting back.
I think the patch in #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants