Add tests for passwdqc pwqcheck #41

Zaiba-S · 2025-02-07T16:24:03Z

Currently, there are no tests implemented for the package, so this serves as an initial attempt to implement basic testing coverage.
This PR introduces an initial set of tests for the pwqcheck binary of the passwdqc package.

Any feedback or suggestions would be appreciated.

solardiz

Thank you for the contribution. Yes, we need tests.

solardiz · 2025-02-09T05:16:12Z

Makefile

@@ -307,6 +307,17 @@ remove_lib_wrapped:
 remove_locales_wrapped:
 	for f in $(LANGUAGES); do $(RM) $(DESTDIR)$(LOCALEDIR)/$$f/LC_MESSAGES/$(PACKAGE).mo; done

+test:


I think we want this target called check, and it should exit non-zero on any test failures.

And we should run it from GitHub Actions, but that may be a separate PR or at least separate commit.

solardiz · 2025-02-09T05:23:05Z

Makefile

+	@echo "Running tests..."
+	# Set LD_LIBRARY_PATH to include the current directory
+	@export LDFLAGS="-Wl,-rpath,$(CURDIR)"; \
+	export LD_LIBRARY_PATH=$(CURDIR):$$LD_LIBRARY_PATH; \


I think this is excessive. For my manual testing, I simply set LD_LIBRARY_PATH=. and it just works.

The change of LDFLAGS wouldn't take effect at this point anyway, because the test scripts being invoked from here assume the tool has already been built.

solardiz · 2025-02-09T05:30:27Z

My thinking was to start with what I've been doing in manual testing - running pwqcheck on a large file and confirming the hash of its output isn't unexpectedly changed. This wouldn't directly test the individual checks, but it would ensure stable behavior. So if we ever make code changes that change the behavior on such large test input, we'll need to explicitly acknowledge this and update the hash. A drawback is the large input file would need to be stored somewhere in here or download from somewhere external (e.g., the run/password.lst file we have in the john repo) or reproducibly generated.

$ LD_LIBRARY_PATH=. ./pwqcheck -1 --multi < ~/john/run/password.lst | grep -c ^OK:
1733
$ LD_LIBRARY_PATH=. ./pwqcheck -1 --multi < ~/john/run/password.lst | grep -c ^Bad
1793975
$ LD_LIBRARY_PATH=. ./pwqcheck -1 --multi < ~/john/run/password.lst | md5sum 
3eccb6ef49eb84b494c61954c240e76c  -

I see that you also test a few features that the above would not.

solardiz · 2025-02-10T15:10:26Z

tests/test-pwqcheck-basic.sh

+test_basic_password " " "fail" "Single space"
+test_basic_password "" "fail" "Empty password"
+test_basic_password "$(printf 'a%.0s' {1..71})" "fail" "Very long password"
+test_basic_password "ljy8zk9aBJ3hA3TXAAMAQe61ytFohJM4SuPFbA4L1xDqV2JDE1n8BCnLN96evcJMWyTkr9y3" "pass" "Max lenght password"


Typo: lenght.

Also, if we do these tests, there should probably be one for a password that's just above max length, with expected fail.

Also, shouldn't we expect specific fail reasons?

Thanks for the review !

I will correct the typo and add test case for a password just above the max length and ensure it fails as expected.

Reagarding the fail reason,Could you please clarify if you are expecting specific fail reasons for each test case?

Reagarding the fail reason,Could you please clarify if you are expecting specific fail reasons for each test case?

Yes, like you already have in another script where you check against expected output. Instead of the word fail, you could include those reason strings.

Zaiba-S · 2025-02-12T04:23:09Z

My thinking was to start with what I've been doing in manual testing - running pwqcheck on a large file and confirming the hash of its output isn't unexpectedly changed. This wouldn't directly test the individual checks, but it would ensure stable behavior. So if we ever make code changes that change the behavior on such large test input, we'll need to explicitly acknowledge this and update the hash. A drawback is the large input file would need to be stored somewhere in here or download from somewhere external (e.g., the run/password.lst file we have in the john repo) or reproducibly generated.
$ LD_LIBRARY_PATH=. ./pwqcheck -1 --multi < ~/john/run/password.lst | grep -c ^OK:
1733
$ LD_LIBRARY_PATH=. ./pwqcheck -1 --multi < ~/john/run/password.lst | grep -c ^Bad
1793975
$ LD_LIBRARY_PATH=. ./pwqcheck -1 --multi < ~/john/run/password.lst | md5sum 
3eccb6ef49eb84b494c61954c240e76c  -
I see that you also test a few features that the above would not.

Thanks for sharing your approach
Your method of hashing the output of pwqcheck on a large input file sounds like a great way to ensure stability across changes.
Are you suggesting that we implement this approach in addition to the individual tests I proposed?
If so, For storing the large input file and expected output hash for comparison, do you have a preference on where it should be stored ?

solardiz · 2025-02-13T03:37:48Z

This PR currently fails our whitespace-errors test:


Run git diff-index --check --cached 4b825dc642cb6eb9a060e54bf8d69288fbee4904
tests/test-pwqcheck-length.sh:14: trailing whitespace.
+    
tests/test-pwqcheck-length.sh:18: trailing whitespace.
+    
tests/test-pwqcheck-length.sh:22: trailing whitespace.
+    
tests/test-pwqcheck-multi.sh:13: trailing whitespace.
+    
tests/test-pwqcheck-multi.sh:16: trailing whitespace.
+    
tests/test-pwqcheck-multi.sh:25: trailing whitespace.
+    
tests/test-pwqcheck-multi.sh:39: trailing whitespace.
+    
tests/test-pwqcheck-multi.sh:85: trailing whitespace.
+  
tests/test-pwqcheck-multi.sh:109: trailing whitespace.
+            echo "StrongP@ss${i}!" 
tests/test-pwqcheck-multi.sh:111: trailing whitespace.
+            echo "weak${i}" 
tests/test-pwqcheck-similarity.sh:94: new blank line at EOF.

solardiz · 2025-02-13T03:43:15Z

Are you suggesting that we implement this approach in addition to the individual tests I proposed?

In general, yes. In this same PR, no.

If so, For storing the large input file and expected output hash for comparison, do you have a preference on where it should be stored ?

Currently no. I'd like to hear from @ldv-alt on this. Off the top of my head, the options are:

In this repo and included in release archive (so that make check can use it from there as well).
In this repo, but excluded from release archive (available for use in GitHub Actions).
External reference (what kind?) to john/run/password.lst.
Generate a large list into a pipe programmatically (the lines wouldn't look so much like real passwords, but that's OK if we're only testing stability).

solardiz · 2025-02-13T04:04:09Z

The color output could be undesired when redirected to a build log file. I suggest to use color only when outputting to a tty, which I think you can test with test -t 1, e.g. if [ -t 1 ]; then.

We use 8-wide tabs for indentation in this source tree, and I think it makes sense to continue with this style for the shell scripts in here as well (not switch to 4 spaces).

solardiz · 2025-02-13T04:11:06Z

tests/test-pwqcheck-basic.sh

+    exit_code=$?
+
+    if [[ "$expected" == "pass" && $exit_code -eq 0 ]] || \
+       [[ "$expected" == "fail" && $exit_code -ne 0 ]]; then


This could be more portable (to other shells) if written as a test expression (so in single brackets and using -a and -o for AND and OR) rather than as two bash conditional expressions.

should we set the default shell to sh instead of bash?

We could, if we actually test these scripts with non-bash as well.

solardiz · 2025-02-13T04:12:58Z

Please also add your copyright statements and references to LICENSE as source code comments near the start of your scripts, similarly to how we have this in existing source files.

solardiz · 2025-02-18T16:07:02Z

This PR currently fails our whitespace-errors test:
Run git diff-index --check --cached 4b825dc642cb6eb9a060e54bf8d69288fbee4904

This is still failing. You can run the above git command on your computer to see (and then fix) these errors before you push.

Signed-off-by: Zaiba Sanglikar <[email protected]>

solardiz reviewed Feb 9, 2025

View reviewed changes

solardiz reviewed Feb 10, 2025

View reviewed changes

solardiz reviewed Feb 13, 2025

View reviewed changes

Zaiba-S force-pushed the add-tests branch from 0b15f19 to d1c81c4 Compare February 18, 2025 06:02

Zaiba-S force-pushed the add-tests branch from 573635c to ca258ee Compare February 19, 2025 07:01

Zaiba-S added 2 commits February 19, 2025 07:26

Include tests to test pwqcheck

721f378

Signed-off-by: Zaiba Sanglikar <[email protected]>

Updated Makefile target

ca258ee

Signed-off-by: Zaiba Sanglikar <[email protected]>

solardiz requested a review from ldv-alt February 20, 2025 00:30

Zaiba-S mentioned this pull request Feb 25, 2025

Add tests for passwdqc pwqgen #42

Open

Zaiba-S mentioned this pull request Mar 10, 2025

Add tests for mode option of pwqfilter #43

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for passwdqc pwqcheck #41

Add tests for passwdqc pwqcheck #41

Zaiba-S commented Feb 7, 2025

solardiz left a comment

solardiz Feb 9, 2025

solardiz Feb 9, 2025

solardiz commented Feb 9, 2025

solardiz Feb 10, 2025

Zaiba-S Feb 12, 2025

solardiz Feb 13, 2025

Zaiba-S commented Feb 12, 2025

solardiz commented Feb 13, 2025

solardiz commented Feb 13, 2025

solardiz commented Feb 13, 2025

solardiz Feb 13, 2025

Zaiba-S Feb 20, 2025

solardiz Feb 20, 2025

solardiz commented Feb 13, 2025

solardiz commented Feb 18, 2025

Add tests for passwdqc pwqcheck #41

Are you sure you want to change the base?

Add tests for passwdqc pwqcheck #41

Conversation

Zaiba-S commented Feb 7, 2025

solardiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solardiz commented Feb 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zaiba-S commented Feb 12, 2025

solardiz commented Feb 13, 2025

solardiz commented Feb 13, 2025

solardiz commented Feb 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solardiz commented Feb 13, 2025

solardiz commented Feb 18, 2025