pam provider should use control as parameter instead of a property

[raphink]

The scap-security-guide has a few security rules it checks for that involve having two different entries with the same type and module but a different control. I can't speak for whether or not the recommended pam config is valid (and I don't even care if anyone is ever able to log into this puppet configured server but if someone runs a security scan I want these checks to "pass").

@raphink says:
Indeed, control is a property, not a parameter, so it is meant to converge for a given combination of service and type. If you think this is a bug, please open a new ticket to request control to be used as a parameter instead.

Here is the verbiage from the scap scan report (there are couple other security findings that are addressed by the same two pam entries).

Result for Set Deny For Failed Password Attempts
Result: fail
Rule ID: accounts_passwords_pam_faillock_deny
Time: 2014-05-29 16:07
Severity: medium

To configure the system to lock out accounts after a number of incorrect login attempts using pam_faillock.so:

Add the following lines immediately below the pam_unix.so statement in AUTH section of both /etc/pam.d/system-auth and /etc/pam.d/password-auth:

auth [default=die] pam_faillock.so authfail deny=3 unlock_time=604800 fail_interval=900
auth required pam_faillock.so authsucc deny=3 unlock_time=604800 fail_interval=900

Locking out user accounts after a number of incorrect attempts prevents direct password guessing attacks.

Security identifiers
CCE-26844-1

[raphink]

I've just attached some code to this issue which makes control a parameter. Can you test and let me know if that would fix your issue?

[gregswift]

so control is the '3rd' field of a pam entry (which is really the 2nd because the filename is the first when using pam.d/service files)
There are 2 possible types of control: single keyword and square bracket enclosed k=v pairs.
By setting control to be a parameter you are using it to select a resource, which makes it impossible to change an existing entry.
For a good chunk of implementations this will probably not be noticed, but there are 3 primary scenarios where it will cause a need for workarounds:

1: If someone wants to change the control of an entry that was preloaded on the box
2: If someone wants to change the control of an entry that they added themselves. This is most likely going to happen for someone using the [k=v k2=v2] control syntax.

Both of these can be worked around by explicitly added a ensure => absent that matches the old entry and then adding a new one, but really thats not very friendly.

In talking with @raphink the thought of making control_is_a_param a toggleable boolean. I'm not sure what we would actually call the property or what teh default state would be.

[hdeadman]

I tested out your branch and it sort of worked. I had some other pam entries where I hadn't specified a control and it was defaulting to "requisite" so I had to add a value for control to those entries since as a parameter it was "required" to be present in the pam provider.

I say sort of because with the following:

pam { 'Set invalid login 3 times deny in password-auth -fail':
  ensure    => present,
  service   => 'password-auth',
  type      => 'auth',
  control   => '[default=die]',
  module    => 'pam_faillock.so',
  arguments => ['authfail','deny=3','unlock_time=604800','fail_interval=900'],
  position  => 'after module pam_unix.so',
}

pam { 'Set invalid login 3 times deny in password-auth -success':
  ensure    => present,
  service   => 'password-auth',
  type      => 'auth',
  control   => 'required',
  module    => 'pam_faillock.so',
  arguments => ['authsucc','deny=3','unlock_time=604800','fail_interval=900'],
  position  => 'before module pam_succeed_if.so',
}

the first time I ran through it added the "authsucc" entry and got the following error on the authfail entry:

[Set invalid login 3 times deny in password-auth -fail]/ensure: created
Debug: Puppet::Type::Pam::ProviderAugeas: Save failure details:
/augeas/files/etc/pam.d/password-auth/error/path = /files/etc/pam.d/password-auth/01
/augeas/files/etc/pam.d/password-auth/error/lens = /usr/share/augeas/lenses/dist/pam.aug:58.21-.51:
/augeas/files/etc/pam.d/password-auth/error/message = Failed to match
    { /optional/ }?{ /type/ = /auth|session|account|password/ }{ /control/ = /\\
[[^]\001-\004\n#]*\\]|[^\001-\004\t [][^\001-\004\t ]*/ }{ /module/ = /[^\001-\0
04\t\n #]+/ }{ /argument/ = /[^\001-\004\t\n #]+/ }*({ /#comment/ = /[^\001-\004
\t\n\r ][^\001-\004\n]*[^\001-\004\t\n\r ]|[^\001-\004\t\n\r ]/ } | ())
  with tree

I ran it again and with the existing pam_faillock.so entry present it was able to run puppet to completion and both pam_faillock.so entries were present. Result looked like this:

auth    [default=die]   pam_faillock.so authfail        deny=3  unlock_time=604800      fail_interval=900
auth    required        pam_faillock.so authsucc        deny=3  unlock_time=604800      fail_interval=900

I think a parameter to make treating control as a parameter an option would be good. Is there anything I could change in my config to have both entries get added on the first pass?

[raphink]

I've just adapted the patch to expose a new control_is_param parameter. When set, resources are selected based on the control value, so control doesn't converge anymore. Let me know if this fixes the issue and is acceptable to you @gregswift .

[gregswift]

👍

[hdeadman]

I tried it out the latest from this issue branch and it worked for me. I still get an error the first time I run when both the entries I am adding are removed but after a second run the second entry is added and it is good from there on out.

[raphink]

Can you be more specific about what happens during the first run?

[hdeadman]

In my comment from 12:43PM yesterday I think I was fairly specific but the shorter version is that when I run it the first time, I get this entry added ->

auth    required        pam_faillock.so authsucc        deny=3  unlock_time=604800      fail_interval=900

but I get this error from puppet (running with --verbose --debug):

[Set invalid login 3 times deny in password-auth -fail]/ensure: created
Debug: Puppet::Type::Pam::ProviderAugeas: Save failure details:
/augeas/files/etc/pam.d/password-auth/error/path = /files/etc/pam.d/password-auth/01
/augeas/files/etc/pam.d/password-auth/error/lens = /usr/share/augeas/lenses/dist/pam.aug:58.21-.51:
/augeas/files/etc/pam.d/password-auth/error/message = Failed to match
    { /optional/ }?{ /type/ = /auth|session|account|password/ }{ /control/ = /\\
[[^]\001-\004\n#]*\\]|[^\001-\004\t [][^\001-\004\t ]*/ }{ /module/ = /[^\001-\0
04\t\n #]+/ }{ /argument/ = /[^\001-\004\t\n #]+/ }*({ /#comment/ = /[^\001-\004
\t\n\r ][^\001-\004\n]*[^\001-\004\t\n\r ]|[^\001-\004\t\n\r ]/ } | ())
  with tree

If I just re-run again (with the only difference being that the password-auth and/or system-auth files contain the new entry), the second entry is then added and the file contains both of these entries:

auth    [default=die]   pam_faillock.so authfail        deny=3  unlock_time=604800      fail_interval=900
auth    required        pam_faillock.so authsucc        deny=3  unlock_time=604800      fail_interval=900

I can re-create the problem by deleting both entries and running again. The entries involved are the two I listed in yesterday's 12:43PM post but they are part of a module making a bunch of other changes, some of which are in pam, but I don't think the other puppet configurations should be changing anything b/c the entries they are managing are already present.

I am fairly new to puppet (and ruby, git, augeas, etc) so I could easily be doing something wrong. How hard would it be to use entries like the ones I am trying to use as part of a test case or example of the control_is_param parameter?

[raphink]

Bug #101 was actually still not fixed for pam. The last commit fixes it (requires tests).

[raphink]

There, tests added.

[raphink]

I committed the fix for #101 into master, so there's only one commit left here. Please test and confirm if this works fine now.

[hdeadman]

I eventually hope to be less of a git noob and be able to grab the fixes on the issues/114 branch from raphink/augeasproviders along with this fix in hercules-team/augeasproviders master but I probably won't be able to figure out how to do that today since I have some other things I need to get done. I will test when the changes are in one place or I up my git skills. Thanks.

[raphink]

No problem. In order to test my branch, you need to add the remote:

$ git remote add gh-raphink https://github.com/raphink/augeasproviders.git
$ git remote update gh-raphink

and then use the branch:

$ git checkout issue/114 gh-raphink/issue/114

[hdeadman]

Thanks for the git help. Did you edit your commands? I could have sworn it said git reset before. I used that and it seemed to fix all my problems when I ran using the augeasproviders master branch plus issue/114. (I also changed the url to augeasproviders.git instead o augeas.git).

Thanks.

[raphink]

The tests pass. The one thing I'd still like to ponder is the interface. The parameter is currently named control_is_param, but I find this a bit ugly. Any better suggestions? sync_control (defaulting to true), match_control, static_control?

[raphink]

@hdeadman Yes, I had given an example with reset before, I edited it later:

$ git reset --hard gh-raphink issue/114

[hdeadman]

I sort of wished control_is_param was shorter because puppet-lint will probably make me indent my other attributes so they line up but I can't think of a better name and it is descriptive. Most users of the pam provider won't be bothered by it since the default is false and those that need it (like me) will just be glad something is available regardless of the name.