indiana-university / puppet-duo_unix

The duo_unix module handles the deployment of duo_unix (login_duo or pam_duo) across a range of Linux distributions.
BSD 3-Clause "New" or "Revised" License
2 stars 14 forks source link

Supporting CERN configuration for Duo #38

Closed mboisson closed 5 months ago

mboisson commented 8 months ago

This is my attempt at fixing #35 I unfortunately do not have a way to test for Debian, but this works on RHEL8.

markaddonizio commented 8 months ago

Hi, sorry for the delay. Have a couple comments/questions.

1) I'm blocked from merging since our repository requires signed commits.

2) The pam_ssh_user_auth package appears to be RedHat only, and will fail any other distro since it was added to init.pp. Also, putting it there will install it even if the user isn't going to use it.

3) Just double-checking as I've not encountered this syntax yet: $keyonly ? { true => 'set AuthenticationMethods "publickey,keyboard-interactive:pam"', false => 'set AuthenticationMethods "publickey,keyboard-interactive:pam keyboard-interactive:pam,keyboard-interactive:pam"' }

Shorthand if/else? If keyonly, restrict auth to publickey then keyboard. If not, auth can be publickey then keyboard, or keyboard then keyboard again?

Thanks!

mboisson commented 8 months ago

Hi, sorry for the delay. Have a couple comments/questions.

  1. I'm blocked from merging since our repository requires signed commits.

Ok, I can look at signing the commits later this week.

  1. The pam_ssh_user_auth package appears to be RedHat only, and will fail any other distro since it was added to init.pp. Also, putting it there will install it even if the user isn't going to use it.

Indeed, I do not have access to test on other distros. I am open to suggestions on how to best handle this.

  1. Just double-checking as I've not encountered this syntax yet: $keyonly ? { true => 'set AuthenticationMethods "publickey,keyboard-interactive:pam"', false => 'set AuthenticationMethods "publickey,keyboard-interactive:pam keyboard-interactive:pam,keyboard-interactive:pam"' }

Shorthand if/else? If keyonly, restrict auth to publickey then keyboard. If not, auth can be publickey then keyboard, or keyboard then keyboard again?

Correct, that is a short-hand if/else. And correct, if keyonly is true, then the two factors are key + keyboard (Duo), otherwise it can be either key + Duo or password + Duo (both of which are keyboard-interactive in this later case).

Thanks!

mboisson commented 7 months ago

I signed the commits yesterday. I am still thinking on how to handle the ubuntu vs redhat situation.

mboisson commented 7 months ago

Hi @markaddonizio, I had a deeper look at how to implement this for Ubuntu. It seems that CERN's PAM module has only been bundled into RHEL, and would need to be compiled manually for Debian base distributions: https://cern-cert.github.io/pam_2fa/ https://github.com/CERN-CERT/pam_2fa/blob/master/pam_ssh_user_auth.c

Therefore, I suggest to only support this on RedHat. I propose to wrap the various bits of this configuration into if/else or cases based on $facts['os']['family'] like what is done in params.pp.

That being said, I think that setting usage::pam is currently broken https://github.com/indiana-university/puppet-duo_unix/issues/37 with the puppet module. I need to that for this configuration to work, but it will also change the behavior on Ubuntu/Debian. Presumably, it will fix the behavior to what it should be, but I cannot test that. Should I fix the broken behavior only for RedHat too ?

markaddonizio commented 7 months ago

I think we're safe to fix the usage::pam for both redhat and ubuntu/debian. You're right, that was a bug on our end, and we never noticed since at least our setup is all ubuntu w/ duo_login (and nobody else reported it as an issue).

Thank you!

mboisson commented 7 months ago

Ok, I made the changes to apply only to RedHat and not Debian based distributions. I confirmed it to work on our RHEL-base node.

mboisson commented 7 months ago

Do you know if this can be merged now, or if it needs more changes ? if it can be, do you have some ETA ?

mboisson commented 6 months ago

What are the steps missing to merge this ?

markaddonizio commented 6 months ago

Hi, sorry about the delay, we keep running into issues trying to run the acceptance tests.

Currently, pdk validate has a few minor formatting complaints. The bigger issue is the unit tests (pdk test unit) are failing.

The main issues seem to be: -pam_config.pp now tries to notify the sshd service, but the service isn't ever added to the catalog. -similar issue in pam_sshd_config.pp, trying to require the RedHat exclusive pam_ssh_user_auth_package, but it's not in the catalog.

I think these are more problems with how puppet runs unit tests rather than the code itself as we expect anyone using this to go through init.pp which would add the relevant packages/services. I can 'solve' these by adding an if (!defined(Package/Service[??]) block.

The last (?) 'big' issue is now that we're adding ExposeAuth yes and the various AuthenticationMethods... on the RedHat family, and the tests are complaining about the additions when it's not expecting them (and not looking for them when it should be... and the keyonly true/false differences).

I have fixes for all of these. I'll get it signed and issue a pull request on your branch, then I think we should be able to get it merged here.

mboisson commented 6 months ago

I just noticed from a fresh build that there is a missing dependency. Installing pam_ssh_user_auth requires the epel yum repository (it works in our environment, but on the second puppet run):

Apr 03 15:33:10 login1.int.archimedes.c3.ca puppet-agent[1016]: (/Stage[main]/Duo_unix::Repo/Yumrepo[duosecurity]/ensure) created
Apr 03 15:33:30 login1.int.archimedes.c3.ca puppet-agent[1016]: (/Stage[main]/Duo_unix/Package[duo_unix]/ensure) created
Apr 03 15:33:31 login1.int.archimedes.c3.ca puppet-agent[1016]: Execution of '/usr/bin/dnf -d 0 -e 1 -y install pam_ssh_user_auth' returned 1: Error: Unable to find a match: pam_ssh_user_auth
Apr 03 15:33:31 login1.int.archimedes.c3.ca puppet-agent[1016]: (/Stage[main]/Duo_unix/Package[pam_ssh_user_auth]/ensure) change from 'purged' to 'present' failed: Execution of '/usr/bin/dnf -d 0 -e 1 -y install pam_ssh_user_auth' returned 1: Error: Unable to find a match: pam_ssh_user_auth
Apr 03 15:33:31 login1.int.archimedes.c3.ca puppet-agent[1016]: (/Stage[main]/Duo_unix::Pam_ssh_config/Augeas[Duo Security SSH Configuration]) Dependency Package[pam_ssh_user_auth] has failures: true
Apr 03 15:33:31 login1.int.archimedes.c3.ca puppet-agent[1016]: (/Stage[main]/Duo_unix::Pam_ssh_config/Augeas[Duo Security SSH Configuration]) Skipping because of failed dependencies
Apr 03 15:33:33 login1.int.archimedes.c3.ca puppet-agent[1016]: (/Stage[main]/Duo_unix::Pam_config/Augeas[Duo Security PAM Configuration]/returns) executed successfully
Apr 03 15:33:33 login1.int.archimedes.c3.ca puppet-agent[1016]: (/Stage[main]/Duo_unix/File[/etc/duo/pam_duo.conf]/content) content changed '{sha256}9e86bf796cce49d0f8ee5dc6630f23282f07945659f58986b973a95f47be4a93' to '{sha256}dac57de8a59bb3a29483533533b99b5ac0428345c98da24129c1e09ea090fa64'
Apr 03 15:33:33 login1.int.archimedes.c3.ca puppet-agent[1016]: (/Stage[main]/Epel/Epel::Rpm_gpg_key[EPEL-8]/Exec[import-EPEL-8]/returns) executed successfully
Apr 03 15:33:33 login1.int.archimedes.c3.ca puppet-agent[1016]: (/Stage[main]/Epel/Yumrepo[epel]/ensure) created

This can be fixed either within the Duo_Unix puppet repository itself, or rely on some local documentation. Any preference ?

mboisson commented 6 months ago

I can add a

require => Yumrepo['epel']

to the pam_ssh_user_auth package

markaddonizio commented 6 months ago

Yup, we're good with the 'require' fix. Thanks!

mboisson commented 6 months ago

Yup, we're good with the 'require' fix. Thanks!

done