Are profile_pam_access::deny_rules position settings correct?

[billglick]

I just flipped a user from profile_pam_access::allow_rules to profile_pam_access::deny_rules. The new deny rule was after the pre-existing allow rule, which defeats the purpose.

puppet-profile_pam_access/manifests/init.pp

Line 69 in 7e7799d

'position' => 'after',

I definitely do not understand pam_access position parameters, but it seems weird/odd to have deny rules always be the last in the access.conf file.

[billglick]

Update: If an existing access rule for the given user does not already exist in the file, it seems to add the deny rule at the top of the list. But if an existing access rule for the given user name DOES exist, the deny rule is going in at the end of the file.

[billglick]

ALSO... just be setting the profile_pam_access::deny_rules parameter, then on a fresh deployment (first run) the following line deny all line gets moved before the end of the file, breaking other expected allow rules:

- : ALL : ALL

e.g.

+ : root : LOCAL
- : (all_disabled_usr) : ALL
+ : root : cron crond 127.0.0.1 :0 tty
- : ALL : ALL
+ : (org_asd) : LOCAL
+ : (org_irst) : LOCAL
+ : (org_asd) : 141.142.148.5
+ : (org_asd) : 141.142.236.22
+ : (org_asd) : 141.142.236.23
+ : (org_asd) : 141.142.148.24
+ : (org_asd) : 141.142.146.0/24
+ : (org_irst) : 141.142.148.5
+ : (org_irst) : 141.142.236.22
+ : (org_irst) : 141.142.236.23
+ : (org_irst) : 141.142.148.24
+ : (org_irst) : 141.142.146.0/24
+ : (qualys) : 141.142.148.51
+ : qualys : 141.142.148.51
+ : root : 172.28.18.18

...making most of the allow rules useless.

[billglick]

Ran into this again with profile_pam_access::deny_first_rules. Adding a new entry to the profile_pam_access::deny_first_rules hash resulted in it getting added before the following line which is expected to always be first:

+ : root : LOCAL

See https://jira.ncsa.illinois.edu/browse/SECURITY-1517?focusedCommentId=668572&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-668572 for more details.

After reviewing MiamiOH/puppet-pam_access#7 I think this is more of an issue specific to https://github.com/ncsa/puppet-profile_pam_access/blame/7e7799d5e0f270c6f1653252c19f785b3a4239b2/manifests/init.pp#L47-L77, how we are merging Hiera data, and the order in which the pam_access::entry defined type gets called. If a Puppet change is triggering a few new lines to get added, they may end up being added out of order from what would normally be expected.

For example, if /etc/security/access.conf is empty of any rules it will ensure them in the expected order. But if existing rules already exist, the ordering can get applied in a way unexpected.

[jjackzhn]

This sounds like a similar issue we had (and still has) with sshd match blocks - Augeas doesn't really recognize existing rules well. Purging access.conf is always a good idea because most of the time Augeas won't remove the old rules.

As for ordering, since exact matching of the rules is not implemented here, I believe 'before' and 'after' are effectively acting as 'before first' and 'after last', with the first and last rules being what's in the file at the time of addition.