pam/integration-tests/ssh: Some tests cleanups and enforcements #979
Conversation
3v1n0
force-pushed
the
ssh-tests-cleanups
branch
2 times, most recently
from
229946b to
2a9c077
Compare
Do not show the sshd messages unless we have a failure, this avoids spamming CI, but also it seems that in such context they get scrambled. At the same time they're useful during debugging, so don't change the behavior when we're in an interactive terminal
…orks It's kind implicit in the previous tests, but still let's test the non interactive behavior too
Otherwise mkhomedir may fail in trying to create it, and so SSH: Could not chdir to home directory /home/user-integration-cached: No such file or directory
In case the user is considered already existent: - Create the directory when parsing the DB - Ensure the directory already exists at test time Otherwise: - Ensure no user home directory exists before user has been created - Ensure the user home exists after user logged in
3v1n0
force-pushed
the
ssh-tests-cleanups
branch
from
2a9c077 to
7f5af63
Compare
Let's give some time to ssh to print all the environment first
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #979 +/- ##
=======================================
Coverage 85.61% 85.61%
=======================================
Files 82 82
Lines 5792 5792
Branches 111 111
=======================================
Hits 4959 4959
Misses 777 777
Partials 56 56 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| user-pre-check: '{"Name":"user-pre-check","UID":1974679960,"GID":1974679960,"Gecos":"gecos for user-pre-check","Dir":"/home/user-pre-check","Shell":"/bin/sh","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"2025-05-30T18:24:42.349318925+02:00"}' | ||
| user-integration-UPPER-CASE: '{"Name":"user-integration-UPPER-CASE","UID":1911746559,"GID":1911746559,"Gecos":"gecos for user-integration-UPPER-CASE","Dir":"@HOME_BASE@/user-integration-UPPER-CASE","Shell":"/bin/sh","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"2025-05-30T18:20:04.201035581+02:00"}' |
There was a problem hiding this comment.
Use a valid home for already-existing users
Otherwise mkhomedir may fail in trying to create it, and so SSH:
Could not chdir to home directory /home/user-integration-cached:
No such file or directory
Why does mkhomedir fail when trying to create a directory /home/user-pre-check?
There was a problem hiding this comment.
Well, it does in this test environment of course... It's just running as user, so it won't be able to make a such directory.
Indeed if we would make the daemon run inside bwrap, things would change, but that's for another day.
There was a problem hiding this comment.
ah, I see. could we change the home directories in the testdata to /tmp/user-pre-check etc. instead? could we avoid the extra redaction + insertion logic that way?
There was a problem hiding this comment.
Eh, I don't like to have fixed and hardcoded paths, since otherwise we may end up having false positive/negatives when repeating the tests locally, and if a test expects something from the environment (like being able to write to /tmp) is already a problem IMHO.
I did initially some simple redaction, but then I though the one I ended up was more prone to potential future issues.
| // Z_ForTests_CreateDBFromYAMLWithBaseHome creates the database inside destDir and loads the src file content into it. | ||
| // | ||
| // nolint:revive,nolintlint // We want to use underscores in the function name here. | ||
| func Z_ForTests_CreateDBFromYAMLWithBaseHome(src, destDir, baseHomeDir string) (err error) { |
There was a problem hiding this comment.
Why is this in the old bbolt package?
There was a problem hiding this comment.
Because that's a function to generate a bolt db, no?
I just added another function similar to where the one we had was...
I initially was just doing this as part of the integration tests helpers, but it felt something to do in its own spot (also to be able to access to private stuff).
There was a problem hiding this comment.
I think the integration tests should not use a bbolt database anymore, except for test cases which test migration from bbolt to sqlite. I see useOldDatabaseEnv is used in many other test cases as well
There was a problem hiding this comment.
ah but it just returns if oldDB is empty, which it is for all test cases except for the ones which test migration
There was a problem hiding this comment.
but that means that Z_ForTests_CreateDBFromYAMLWithBaseHome is also only used for those test cases which test migration. is that expected?
There was a problem hiding this comment.
do we not need to create the home directory in the other cases, where we use a SQLite db?
There was a problem hiding this comment.
do we not need to create the home directory in the other cases, where we use a SQLite db?
Oh, we should, but IIRC we don't have such tests yet... I need to double check it.
See commits for details.
Base of #971