Run the API tests in the TPM CI job - #1165
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the TPM CI workflow to actually compile and run wolfSSH’s API test suite (tests/api.test) under a --enable-tpm build, ensuring TPM-guarded tests can gate merges.
Changes:
- Add a gated
make check TESTS=tests/api.teststep to run API tests in a single selected matrix cell. - Add a failure-only step to print
tests/api.log. - Archive
wolfssh/tests/api.logas a workflow artifact.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
013464b to
51e3eff
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1165
No scan targets match the changed files in this PR. Review skipped.
ejohnstown
left a comment
There was a problem hiding this comment.
Also, going forward, please be more descriptive in your commit messages. The commit messages are the real history that moves around. This commit is doing something non-obvious enough (why TESTS= is overridden, why one matrix cell, why the grep guard exists) that the reasoning belongs in the commit body, not only on the PR page.
|
|
||
| # Builds every check_PROGRAM -- no other job compiles wolfSSH's tests | ||
| # with TPM support -- but runs only the API suite; the rest fail under | ||
| # --enable-tpm. One cell: these tests ignore the simulator and host key. |
There was a problem hiding this comment.
"the rest fail under --enable-tpm" is not true, and I disproved it on both platforms. Only kex.test fails. unit.test, regress.test and testsuite.test all pass under --enable-tpm --enable-certs — 4/4 on Linux against a CI-faithful wolfSSL, same on macOS. A future reader hits that comment while wondering why coverage is so narrow and gets a wrong answer, which is the kind of thing that keeps a restriction in place long after its reason has evaporated. Suggested replacement:
# Builds every check_PROGRAM -- no other job compiles wolfSSH's tests
# with TPM support -- but runs only the API suite; kex.test fails under
# --enable-tpm. One cell: these tests ignore the simulator and host key.There was a problem hiding this comment.
You're right on the exit codes, and I've fixed the comment. I reproduced
your result against a CI-faithful stack: --enable-tpm --enable-certs
gives PASS for api, unit, regress and testsuite, FAIL for kex alone.
Where I'd push back is on reading those passes as coverage:
testsuite.testexecutes nothing here. Its log is one line with no
output, andnmfinds zeroEchoTest/SftpTestsymbols. Themain()
body is entirely#ifdef WOLFSSH_SHELL/#ifdef WOLFSSH_SFTP
(testsuite.c:257-270) and this job defines neither. Rebuild it as
--enable-all --enable-tpmso the body compiles in and it FAILs, with
the same TPM key error as kex.unit.testandregress.testare unaffected by the define. I built
the tree twice against the same wolfSSL, with and without
--enable-tpm, and diffed the logs: byte-identical. Both are already
covered by the jobs that runmake check.
So the narrow run isn't caution -- api.test holds the only TPM-specific
tests in the tree (grep -i tpm: 28 hits there, zero in the other five).
That reason holds in any config, so the comment now says it:
# Builds every check_PROGRAM -- no other job compiles wolfSSH's tests
# with TPM support -- but runs only api.test, the one suite with TPM
# tests. One cell: these tests ignore the simulator and host key.kex.test is the one exclusion that costs coverage -- three handshakes in
this build, two of them ML-KEM. It dies at client.c:893, which aborts
when -K is absent in a TPM build even though kex.test passes no -i
and would take the built-in key-buffer path. Narrowing that guard would
let kex run on a software key, still no TPM coverage, so I'd rather pair
it with a real client-side TPM auth test in a follow-up -- nothing covers
that path today.
Commit message expanded as well, agreed.
- No job compiled a wolfSSH test binary with WOLFSSH_TPM defined, so a test guarded on it compiled out everywhere and could not gate a merge. This job enables TPM but only builds; the jobs that run make check do not enable it. - Add a make check step. automake's check-am builds every check_PROGRAM regardless of the TESTS override, so this is the only job that compiles wolfSSH's tests with TPM support. - Override TESTS to run only tests/api.test, the one suite with TPM-specific tests. kex.test also aborts in the example client, which demands -K in a TPM build. - Restrict it to one matrix cell. The 2x2x2 matrix varies the simulator and the host key, neither of which these tests touch. - Assert -DWOLFSSH_TPM in AM_CPPFLAGS first. A build without it compiles the guarded tests out and still exits 0, a hollow pass. - Dump tests/api.log on failure and archive it.
51e3eff to
ef9d83c
Compare
Problem
No CI job compiles a wolfSSH test binary with
WOLFSSH_TPMdefined, so any test guarded on it silently compiles out everywhere and cannot gate a merge.--enable-tpmmake checktpm-ssh.ymlmake+make installsanitizer.yml--enable-allexcludes TPM)The intersection is empty. Found while writing #1164, whose regression test is unreachable by CI for exactly this reason.
Fix (
.github/workflows/tpm-ssh.yml)Adds a
make check TESTS=tests/api.teststep afterBuild wolfSSH:check_PROGRAMs still compile —check-ambuilds them regardless of theTESTSoverride. That is the point: this is the only job compiling wolfSSH's tests with TPM support.api.testruns. It holds the only TPM-specific tests in the tree:grep -i tpmfinds 28 hits intests/api.cand zero in the other five test sources.ecc/ibmswtpm2/raw). The 2×2×2 matrix varies the simulator and the host key, neither of which these tests touch.-DWOLFSSH_TPMcompiles the guarded tests out and still exits 0, a hollow pass. The step grepsAM_CPPFLAGSfor the define, prints what it found, and aborts if absent.tests/api.logis dumped on failure and added to the archived artifacts.Verification
Against a scratch prefix reproducing CI's library layout (wolfSSL with this job's exact flags, wolfTPM rebuilt against it):
PASS: tests/api.test, and all five test binaries compile under--enable-tpm --enable-certs.--enable-tpm, the guard fails and the step aborts.unit.testandregress.testgive byte-identical output with and without the define (diffed);testsuite.testexits 0 having executed nothing, its body being entirely#ifdef WOLFSSH_SHELL/WOLFSSH_SFTP;kex.testfails;auth.testis not built.Not in this PR
kex.testfails because the example client aborts with "You must specify a password for the TPM key". Two separate defects sit behind that:-Kguard is too broad. The client aborts whenevertpmKeyAuth == NULLin a TPM build, butkex.testpasses no-i, soClientSetPrivateKey()takes the built-in key-buffer path wheretpmKeyAuthis unused. Fixing only this would letkex.testrun on a software key, adding no TPM coverage.#elifinClientSetPrivateKey()routes every-ithroughwolfSSH_TPM_InitKey(), leaving no path towolfSSH_ReadKey_file().Both point at the same gap: nothing anywhere exercises a client user-auth key held in the TPM. That deserves a purpose-built test rather than a retrofit of the KEX sweep, so it is left for a follow-up.