Merge ~ahasenack/ubuntu/+source/libfido2:focal-libfido2-enable-tests into ubuntu/+source/libfido2:ubuntu/devel

[Andreas Hasenack]

Run regress tests at build time, in an override d/rules target.

The normal build won't run them, so I override dh_auto_test and do another build with Debug enabled, which enables those tests.

To be sure they actually ran (they are invisible when they succeed), I do a second run with an injected failure, and verify that the failure happened.

This is what it looks like when the injected failure did not happen: https://paste.ubuntu.com/p/gnP7FFRSqp/ (scroll to the bottom)

I wondered about patching the code to run the regress tests in the normal build, not just the debug one, but found this commit[1] which changed that behavior because of a bug, so I felt it's better to follow upstream on this one.

PPA: https://launchpad.net/~ahasenack/+archive/ubuntu/openssh-fido/

1. https://github.com/Yubico/libfido2/commit/99a9be59b77fc9e6513a2953c222b44b0dbbe91e

[Christian Ehrhardt  (paelzer)]

Odd tests for sure...
Can't we make "echo "SUCCESS: regression tests passed"" to only appear if the tests worked.
Isn't there an RC or anything in the output that we can check.

At lesat the re-run with an error that we introduce seems extra effort that is error-prone and IMHO likely to break on updates.

I'd even be fine to have this behavior:
 echo "Starting tests"
  # Tests that might be silent
 echo "Tests completed - if there is no output since 'Starting tests' they are all good"

Maybe you just need to talk to me about the details, but only looking at the MP I'm not buying it that this is the right way :-)

review: Needs Information

[Andreas Hasenack (ahasenack)]

This is how the tests are built and run:
CMakeLists.txt:
...
if(NOT WIN32)
    if(CMAKE_BUILD_TYPE STREQUAL "Debug")
        if(NOT MSAN AND NOT LIBFUZZER)
            subdirs(regress) <--- adds the regress/ directory
        endif()
..

regress/CMakeLists.txt, once per test:
# cred
add_executable(regress_cred cred.c)
target_link_libraries(regress_cred fido2_shared)
add_custom_command(TARGET regress_cred POST_BUILD COMMAND regress_cred)

So it's just a post build command, and the output is silent. It's not even telling you the test is being run. If the test fails, it core dumps with an assertion error (all tests are asserts it seems).

Someone looking at this output (the good case) and who doesn't know this package won't know if tests are being run or not.

There is no specific test case target. If for some reason, or bug, upstream no longer includes "regress/" in the build, and the build succeeds, we will think the tests also succeeded. That's why I think the injected error is an important safeguard.

[Andreas Hasenack (ahasenack)]

Oh, and security kind of liked this error injection:
 <sarnold> ahasenack: "Expected regression test failure did not happen", nice
 <sarnold> ahasenack: it's not the easiest thing to try to convey that something should have failed, but it didn't fail, and that's a failure :)
 <sarnold> ahasenack: but this does a good job of it, well done

[Christian Ehrhardt  (paelzer)]

Thanks for explaining, then it is maybe just my expectation that was wrong.
I mean it works - and that much better than before.

Who cares that I find the change odd :-)

I'd appreciate ever extra line of comment in d/rules to make this clear for the next one looking at it. But other than that I'm +1 then.

review: Approve

[Andreas Hasenack (ahasenack)]

Added more comments to d/rules. I'll leave this commit unsquashed, and it can get cleaned up in the next merge/upload.

[Andreas Hasenack (ahasenack)]

This change doesn't require an FFe, so I'm tagging and uploading 1d151615c3164b276fbaa817b397bebe29cfe6d8

$ git push pkg upload/1.3.1-1ubuntu1
Enumerating objects: 22, done.
Counting objects: 100% (22/22), done.
Delta compression using up to 4 threads
Compressing objects: 100% (17/17), done.
Writing objects: 100% (17/17), 2.65 KiB | 2.65 MiB/s, done.
Total 17 (delta 11), reused 0 (delta 0)
To ssh://git.launchpad.net/~usd-import-team/ubuntu/+source/libfido2
 * [new tag] upload/1.3.1-1ubuntu1 -> upload/1.3.1-1ubuntu1

$ dput ubuntu ../libfido2_1.3.1-1ubuntu1_source.changes
Checking signature on .changes
gpg: ../libfido2_1.3.1-1ubuntu1_source.changes: Valid signature from AC983EB5BF6BCBA9
Checking signature on .dsc
gpg: ../libfido2_1.3.1-1ubuntu1.dsc: Valid signature from AC983EB5BF6BCBA9
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading libfido2_1.3.1-1ubuntu1.dsc: done.
  Uploading libfido2_1.3.1-1ubuntu1.debian.tar.xz: done.
  Uploading libfido2_1.3.1-1ubuntu1_source.buildinfo: done.
  Uploading libfido2_1.3.1-1ubuntu1_source.changes: done.
Successfully uploaded packages.

[Andreas Hasenack (ahasenack)]

Also submitted to Debian: https://salsa.debian.org/auth-team/libfido2/-/merge_requests/4

[Andreas Hasenack (ahasenack)]

Oh, I should have waited for the new libcbor, bummer. Oh well, no-change-rebuild it is (after).

diff --git a/debian/changelog b/debian/changelog
index cf94a2a..7334673 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+libfido2 (1.3.1-1ubuntu1) focal; urgency=medium
+
+  * d/rules: run regression tests at build time, and one more time
+    with an injected failure to verify it's caught.
+
+ -- Andreas Hasenack <[e-mail]>  Tue, 24 Mar 2020 16:29:13 -0300
+
 libfido2 (1.3.1-1build1) focal; urgency=medium
 
   * No-change rebuild to pick up i386
diff --git a/debian/control b/debian/control
index 9a5dcaa..713b2f7 100644
--- a/debian/control
+++ b/debian/control
@@ -1,7 +1,8 @@
 Source: libfido2
 Section: libs
 Priority: optional
-Maintainer: Debian Authentication Maintainers <[e-mail]>
+Maintainer: Ubuntu Developers <[e-mail]>
+XSBC-Original-Maintainer: Debian Authentication Maintainers <[e-mail]>
 Uploaders:
 	Colin Watson <[e-mail]>,
 	nicoo <[e-mail]>
diff --git a/debian/rules b/debian/rules
index 234840d..ab96da1 100755
--- a/debian/rules
+++ b/debian/rules
@@ -11,3 +11,25 @@ override_dh_auto_configure:
 
 override_dh_missing:
 	dh_missing --fail-missing
+
+override_dh_auto_test:
+	# regress/ tests are only included when the build type is set to Debug, so
+	# we build it again in a separate directory as we don't want a Debug build
+	# in the shipped packages
+	mkdir good-case
+	echo "Running regression tests"
+	cd good-case; cmake -DCMAKE_BUILD_TYPE=Debug ..; make
+	echo "SUCCESS: regression tests passed"
+	# the way the tests are run, by just calling the built binary in a
+	# post-build hook, makes them super silent. The fact that a binary is even
+	# being called after the build is not shown. To be sure we really ran the
+	# tests, let's do it one more time but with an injected failure
+	echo "Injecting a failure and running regression tests again"
+	sed -r -i 's,exit\(0\);,assert(1 == 0); exit(0); /* force failure */,' regress/cred.c
+	# if the next grep fails, then the sed above didn't make any changes, and
+	# we should bail as the "force failure" case isn't valid anymore
+	grep "force failure" -q regress/cred.c
+	mkdir bad-case
+	cd bad-case; cmake -DCMAKE_BUILD_TYPE=Debug ..; \
+		make && { echo "ERROR: Expected regression test failure did not happen"; exit 1; } \
+		|| echo "SUCCESS: the expected failure happened"