ghoneycutt / puppet-module-pam

Puppet module to manage PAM
Other
18 stars 79 forks source link

Fixed a compatibility issue that effects listsep changes #215

Closed mears148 closed 5 years ago

mears148 commented 5 years ago

In environments that are tied into Active Directory using SSSD or other methods, default Windows groups may contain spaces, which are not accepted in pam access.conf. This is due to spaces and tabs being the default line separators used in this file. Alternative line separators can be set via a pam_access.so module option ('listsep=,' specifically) to better integrate with these mixed environments. This is suggested in the pam_access man page and can be referenced at ('https://linux.die.net/man/8/pam_access').

The issue is that when you make a line separator change to anything other than the default, including additional spaces between field separators (':') causes group parsing issues that can only be viewed by adding the debug option next to the pam_access.so module.

All that to say, removing the extra spaces between field separators in the access.conf file, ensures compatibility with unique field separators set in the pam_access.so module.

ghoneycutt commented 5 years ago

@mears148 thanks for the contribution and great write up on why we need it!

The readme should also be changed to show the usage without spaces. Would also be helpful to show a real world example in the README.

When you rebase your commits, could you also please put your comment above in the commit.

mears148 commented 5 years ago

The spaces between the field separators have no bearing on the way access.conf works by default, only if you substitute line separators via pam_access.so. In my deployment, I ensured the param pam::sshd_pam_access is set to 'absent', and then passed in the params to use a custom password-auth and system-auth, which included the line 'account required pam_access.so listsep=,'. I think we could change the default RHEL7 template for access.conf to add an addition param in the template, which allows passing in options next to the module, but I would probably need assistance writing the spec tests for that one, as I don't have spec testing experience really.

As for the README, I have edited the content to reflect the change to access.conf. Good catch!

mears148 commented 5 years ago

@mears148 thanks for the contribution and great write up on why we need it!

The readme should also be changed to show the usage without spaces. Would also be helpful to show a real world example in the README.

When you rebase your commits, could you also please put your comment above in the commit.

I am not sure I understood your request to add my comment to the commit. It is really long for a commit message but I did it anyway, and figured you'd let me know if that's what you were looking for.

ghoneycutt commented 5 years ago

Could you give us the code or hiera data for how you are using this with listsep? I think that would be really helpful for folks using this module to provide AD integration as you are doing.

ghoneycutt commented 5 years ago

For the commit message, the first line should be what you had, followed by a blank line, followed by whatever text that is wrapped. Such as

Fix compatibility issue that with listsep to allow better integration with AD

In environments that are tied into Active Directory using SSSD or other
methods, default Windows groups may contain spaces, which are not
accepted in pam access.conf. This is due to spaces and tabs being the
default line separators used in this file. Alternative line separators
can be set via a pam_access.so module option ('listsep=,' specifically)
to better integrate with these mixed environments. This is suggested in
the pam_access man page and can be referenced at
('https://linux.die.net/man/8/pam_access').

The issue is that when you make a line separator change to anything
other than the default, including additional spaces between field
separators (':') causes group parsing issues that can only be viewed by
adding the debug option next to the pam_access.so module.

All that to say, removing the extra spaces between field separators in
the access.conf file, ensures compatibility with unique field separators
set in the pam_access.so module.
mears148 commented 5 years ago

I think the latest push should be closer to what you are looking for :)

ghoneycutt commented 5 years ago

Tested with vagrant and looks great!

ghoneycutt commented 5 years ago

Released in v3.3.1