feat(pam/integration-tests): Add SSH authentication tests #583
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #583 +/- ##
==========================================
+ Coverage 82.98% 83.24% +0.25%
==========================================
Files 80 80
Lines 8587 8617 +30
Branches 75 75
==========================================
+ Hits 7126 7173 +47
+ Misses 1130 1115 -15
+ Partials 331 329 -2 ☔ View full report in Codecov by Sentry. |
adombeck
reviewed
Oct 15, 2024
...tdata/TestCLIAuthenticate/golden/authenticate_user_and_reset_password_while_enforcing_policy
Show resolved
Hide resolved
pam/integration-tests/testdata/TestSSHAuthenticate/golden/prevent_user_from_switching_username
Outdated
Show resolved
Hide resolved
adombeck
reviewed
Oct 16, 2024
3v1n0
force-pushed
the
sshd-tests
branch
6 times, most recently
from
ce95ffa
to
bae768e
Compare
adombeck
reviewed
Nov 8, 2024
adombeck
reviewed
Nov 8, 2024
3v1n0
force-pushed
the
sshd-tests
branch
3 times, most recently
from
0a12040
to
c3c8e13
Compare
adombeck
approved these changes
Nov 13, 2024
This was meant to be removed by commit ab6e2e6, but it was actually duplicated :(
We used to share the tests artifact folder for each run, but better to split it for each test run so that's easier to inspect
… side We may want to replicate some tapes for multiple tests but with different commands, so we need to define the command in a generic way instead of repeating it in each single tape. The variables are evaluated before running the tape, so no changes in the golden files are needed.
So that we can avoid troubles when running the tests in parallel
We were running multiple instances of authd daemon for each test that in order to make some specific cases to be tested. However this is unneeded for the great majority of the test cases and it doesn't allow us to test the daemon concurrency properly. So, just run a specific daemon if the test case requires it, and so in just in case we need to do root-checks or to ensure that the local groups are updated. The only downside of this is that if a test generates a gpasswd file we are going to fail also in other tests that are not affected by the issue but that's still something that can be easily debugged checking the logs or temporary enabling the single-authd instance to be used all the times
…using the shell
We relied on vhs's prompt (">") being written to ensure we exited authd
but it's better to actually ensure that the terminal is still responsive
We can use the same code to build any C module, not just the PAM ones
So we can use it for multiple needs-reset tests
As per commit 79f21be we've a new layout on the native model but this is not applied to the new password view, so follow the same rules here.
We have tests simulating SSH behavior, but it's definitely better to ensure that SSH works as expected using the actual server and client when used with authd. In order to get sshd to be fully usable for this simulation, however, we need to "mock" it by using a LD_PRELOAD'ed library that has to be in C (as the cgo version I initially done would trigger the well known issues we have with go libraries and threads) and that we use it for mocking the sshd requests on getpwnam and to make sshd to open our pam file (that is hardcoded in sshd). To handle the getpwnam we could even have used __nss_configure_lookup() with a fake module or our own, but this is just a simpler solution for now, while in future we may want to add full integration tests where also our own NSS library is used instead, but this was outside the scope of this change, that is mainly focused on the behavior of the PAM module only. As for the rest, just repeat all the native tests that make sense using SSH instead, by de facto re-using the same tape files, minus the removal of the user selection.
SSH and native tests are basically using the same UI, so let's share the same tape files when possible. We can't do it for all since most of native ones rely on the user being selected during the interaction but we can change that at later point
…braries Sadly we can't cover the preloaded library, otherwise it will cause signals being emitted which break SSHd behavior. At the same time, using a library with ASAN support when prelaoding is too complex for being implemented here. We can avoid this though since the code paths are already covered in other tests.
In some tests we may want to re-connect to it multiple times so make this possible. This commit also opens the gates to potentially running all the tests in a single SSHd session, to test the ability of our library to run when loaded in a concurrent way.
… remembered Thanks to the previous commit we can handle the test by just launching ssh as a demon, that will accept multiple connections.
We do this request multiple times, but it's not something that can change, so perform it just once
…e requests We may want to be sure that a single instance of SSH with multiple requests coming in parallel is properly handled by our stack. This is something we didn't test before but having sshd as a daemon allows us to do it properly, simulating a more real scenario. However, we only perform such tests in race mode not to increase the testing time too much
This is what the great majority of PAM-based tools do, so also with the experience of CVE-2024-9313 it's just better to test this case by default while keeping the cases where the user selection is happening as the special ones. Doing this for the native model authentication only for now, since this allows to share most of the tapes with SSH test cases, but that's something we should do also for CLI tests and passwd cases
This is something that we don't support anymore as per commit e91ab76 and if we'd do it, it wouldn't work well anyways since it would imply changing the PAM user, which as we know may lead to logging-in wrongly as CVE-2024-9313 taught us
These are not supported by some SSH clients, so better to be reliable and support characters that are visible the same ways in all the known cases
…ed OS SSH output changes from old jammy (where CI is) to noble and greater versions (as per the OpenSSH server changes we carry on), so to be able to run the tests in a reliable way we need to be on such context. While we target noble, we didn't force our CI to be updated, so for now let's just enable the tests in older ubuntu versions where CI resides skipping it otherwise. Added also a further check so that when CI changes we get an error about
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Repeat all the tests using the actual SSH daemon to simulate better the real environment.
It comes con various cleanups that are prerequisite to achieve all this reducing duplications.
This is not using the NSS module yet because that's something to be done outside the PAM scope.
See each commit for details, from the main one:
UDENG-4691