-
Notifications
You must be signed in to change notification settings - Fork 694
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
Remove common-auth pam customizations #4021
Conversation
Started reviewing this, haven't run through all the steps yet. On the surface this looks like exactly what what we want. On clean Trusty & Xenial VMs (i.e. before any SD-specific customizations are applied), the After verifying the rest of the functionality as described, I foresee two small additions:
Before adding the latter test in particular, I'll a) confirm that the log message readily appears on develop and b) paste example log lines into #3963 to use in structure the test. |
PAM customizations were necessary to allow 2FA for console logins. Since, these configurations are no longer necessary due to the phasing our of 2FA for console logins and `/var/log/auth.log` in trusty and `syslog` in xenial, (encryptfs.so), let's replace the FPF-configured pam.d/common-auth file with the upstream-maintained common-auth.
6730390
to
03844e1
Compare
03844e1
to
3cb6d5c
Compare
Codecov Report
@@ Coverage Diff @@
## develop #4021 +/- ##
=======================================
Coverage 84.7% 84.7%
=======================================
Files 43 43
Lines 2765 2765
Branches 300 300
=======================================
Hits 2342 2342
Misses 355 355
Partials 68 68 Continue to review full report at Codecov.
|
Thanks for the tests and the rebase, @emkll. Verified that there are no unwanted side-effects on the Trusty story, and all config tests are passing locally. For Xenial, SSH functionality is working as expected, and all the config tests are passing (except #3916, since I ran several provisioning commands). However, I'm I notice that auth.log contains:
Given that the pam_cap include was taken from the upstream config, and no problems were observed with SSH access, I suggest we ignore this for now and move forward with the other migration tasks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving for merge, especially with the shiny new tests.
For the record, I have not run through the upgrade portion of the test plan, but based on the positive results of both the Trusty and Xenial flows, I'm confident based on visual review that the config changes here work as advertised.
Status
Ready for review
Description of Changes
PAM customizations were necessary to allow 2FA for console logins. Since, these configurations are no longer necessary due to the phasing out of 2FA for console logins and
/var/log/auth.log
in trusty andsyslog
in xenial, (encryptfs.so), let's replace the FPF-configuredpam.d/common-auth file with the upstream-maintained common-auth.Closes #3963 for new installs or installs on which
./securedrop-admin install
is run.Testing
/etc/pam.d/common-auth
provided by this PR is consistent with the upstream version(s) in Ubuntu 14.04 and 16.04.Clean install
molecule converge -s libvirt-staging
) and ensure SSH access is preserved on app and mon servers.molecule converge -s libvirt-staging-xenial
) and ensure SSH access is preserved on app and mon servers./lib/security/pam_encryptfs.so: cannot open shared object file: No such file or directory
does not appear in/var/log/auth.log
(Trusty) orjournalctl -e
(Xenial) does not contain after running the playbooks.Upgrade testing
git checkout develop
develop
(using./securedrop-admin install
)./securedrop-admin install
/lib/security/pam_encryptfs.so: cannot open shared object file: No such file or directory
does not appear in/var/log/auth.log
(Trusty) orjournalctl -e
(Xenial) does not contain after running the playbooks.Deployment
Since there is no negative impact on running instances (Trusty or Xenial) other than errors in the logs, let's minimize the risk to automatically patch the file on running instances via postinst, and instead only ship this change via Ansible:
Checklist
If you made changes to the system configuration:
If you made non-trivial code changes: