[1] Refactor the swtpm_cert tests and fix some more issues in swtpm_cert#1123
Open
stefanberger wants to merge 5 commits into
Open
[1] Refactor the swtpm_cert tests and fix some more issues in swtpm_cert#1123stefanberger wants to merge 5 commits into
stefanberger wants to merge 5 commits into
Conversation
stefanberger
force-pushed
the
stefanberger/refactor_tests_and_fix_swtpm_cert
branch
from
25affb8 to
bdffc09
Compare
stefanberger
changed the title
stefanberger
force-pushed
the
stefanberger/refactor_tests_and_fix_swtpm_cert
branch
5 times, most recently
from
fcbe04d to
0139177
Compare
stefanberger
force-pushed
the
stefanberger/refactor_tests_and_fix_swtpm_cert
branch
from
0139177 to
91d8324
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCI runner OS updated to ubuntu-26.04; libcrypto pkg-config tightened to >= 3.5. TPM IAK SAN ASN.1 tags removed and static ASN.1 table/C write paths adjusted; ek-cert.c X509 time/result handling changed. Test suite refactored: static fixtures removed, new helper scripts added, and wrappers now generate keys/certs at runtime. ChangesOpenSSL Update and ASN.1 Schema Corrections
Test Infrastructure Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 9: Replace all occurrences of the unsupported runner label "runs-on:
ubuntu-26.04" with a supported label such as "runs-on: ubuntu-24.04" (there are
four occurrences referenced by the diff); update each "runs-on" line in the CI
workflow to use "ubuntu-24.04" (or another valid GitHub-hosted runner) so
actionlint no longer reports an unknown runner.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81cea28d-0721-43a4-89cd-0146d0285360
⛔ Files ignored due to path filters (8)
tests/data/ecprivek.pemis excluded by!**/*.pemtests/data/ecpubek.pemis excluded by!**/*.pemtests/data/issuercert.pemis excluded by!**/*.pemtests/data/pubek.pemis excluded by!**/*.pemtests/data/signkey-encrypted.pemis excluded by!**/*.pemtests/data/signkey.pemis excluded by!**/*.pemtests/data/swtpm-localca-rootca-cert.pemis excluded by!**/*.pemtests/data/swtpm-localca-rootca-privkey.pemis excluded by!**/*.pem
📒 Files selected for processing (11)
.github/workflows/ci.ymlconfigure.acsrc/swtpm_cert/ek-cert.csrc/swtpm_cert/tpm.asnsrc/swtpm_cert/tpm_asn1.htests/Makefile.amtests/_test_swtpm_certtests/_test_tpm2_swtpm_certtests/test_swtpm_certtests/test_tpm2_swtpm_certtests/test_tpm2_swtpm_cert_ecc
💤 Files with no reviewable changes (1)
- tests/test_tpm2_swtpm_cert_ecc
stefanberger
force-pushed
the
stefanberger/refactor_tests_and_fix_swtpm_cert
branch
from
91d8324 to
919c2fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tests/_test_tpm2_swtpm_cert (1)
12-16: 💤 Low valueDefine
cleanupfunction before thetrapstatement.The trap on line 12 references the
cleanupfunction before it's defined on lines 13-16. While this works in bash (the function is resolved when the trap fires, not when defined), it's unconventional and can confuse readers.Suggested reorder
-trap "cleanup" SIGTERM EXIT function cleanup() { rm -f "${cert}" } +trap "cleanup" SIGTERM EXIT🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/_test_tpm2_swtpm_cert` around lines 12 - 16, Move the cleanup function definition (function cleanup() { rm -f "${cert}"; }) above the trap invocation so the trap "cleanup" SIGTERM EXIT references an already-declared symbol; keep the trap arguments unchanged (SIGTERM EXIT) and preserve the "${cert}" use inside cleanup to retain original behavior and quoting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/swtpm_cert/ek-cert.c`:
- Around line 1592-1594: The CHECK_OSSL_NULLPTR call using X509_time_adj_ex
passes a format string that incorrectly uses the unsigned specifier "%lu" for
the variable days (which is declared as long); update the format specifier to
"%ld" in the CHECK_OSSL_NULLPTR invocation (referencing X509_time_adj_ex,
asn1_time, days, now) so the signed long is logged correctly.
In `@tests/_test_swtpm_cert`:
- Line 61: Replace use of echo -en for feeding password material into process
substitution with printf '%s' to preserve exact bytes; find occurrences like the
--signkey-pwd file:<(echo -en "${PARAM_PASSWORD}") (and the second occurrence
around the other instance) and change them to use printf '%s'
"${PARAM_PASSWORD}" in the process substitution so backslashes and special bytes
are not reinterpreted.
- Around line 20-32: The test currently only issues warnings in function
check_cert_size when the certificate file is missing or its byte size (computed
via size=$(stat -c%s "${cert}" 2>/dev/null)) does not match the expected value
(exp); change check_cert_size to treat these conditions as failures by printing
the same diagnostic and then failing the test (e.g., exit 1 or return non-zero)
so CI fails on mismatch or missing cert. Ensure you still log "Warning:
Certificate file has unexpected size." and the "Expected: $exp; found: $size"
details before exiting, and handle the case where stat returns empty (missing
file) as a failure as well.
In `@tests/_test_tpm2_swtpm_cert`:
- Around line 152-170: The test invokes SWTPM_CERT with PARAM_ECPUBKEY (an EC
public key) but the final success message prints "RSA key", which is misleading;
update the success output in the block that calls SWTPM_CERT/check_cert
(referencing SWTPM_CERT, PARAM_ECPUBKEY, check_cert, PARAM_CERT_SIZES, and the
echo "Test ${TC}: OK (RSA key)") to reflect EC/ECC (e.g., "EC key" or "ECC key")
so the message matches the key type used.
- Around line 70-81: The COMMON array contains a duplicated flag --tpm2
(appearing twice); edit the COMMON variable definition and remove the redundant
--tpm2 entry so the array only includes a single --tpm2 flag (look for the
COMMON=( ... ) block that includes --signkey "${PARAM_SIGNKEY}", --issuercert
"${PARAM_ISSUERCERT}", and --out-cert "${cert}" and delete the extra --tpm2
occurrence).
---
Nitpick comments:
In `@tests/_test_tpm2_swtpm_cert`:
- Around line 12-16: Move the cleanup function definition (function cleanup() {
rm -f "${cert}"; }) above the trap invocation so the trap "cleanup" SIGTERM EXIT
references an already-declared symbol; keep the trap arguments unchanged
(SIGTERM EXIT) and preserve the "${cert}" use inside cleanup to retain original
behavior and quoting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4bebd59b-1fe5-4d3e-9ea9-f3c94b4e5f23
⛔ Files ignored due to path filters (8)
tests/data/ecprivek.pemis excluded by!**/*.pemtests/data/ecpubek.pemis excluded by!**/*.pemtests/data/issuercert.pemis excluded by!**/*.pemtests/data/pubek.pemis excluded by!**/*.pemtests/data/signkey-encrypted.pemis excluded by!**/*.pemtests/data/signkey.pemis excluded by!**/*.pemtests/data/swtpm-localca-rootca-cert.pemis excluded by!**/*.pemtests/data/swtpm-localca-rootca-privkey.pemis excluded by!**/*.pem
📒 Files selected for processing (11)
.github/workflows/ci.ymlconfigure.acsrc/swtpm_cert/ek-cert.csrc/swtpm_cert/tpm.asnsrc/swtpm_cert/tpm_asn1.htests/Makefile.amtests/_test_swtpm_certtests/_test_tpm2_swtpm_certtests/test_swtpm_certtests/test_tpm2_swtpm_certtests/test_tpm2_swtpm_cert_ecc
💤 Files with no reviewable changes (1)
- tests/test_tpm2_swtpm_cert_ecc
stefanberger
force-pushed
the
stefanberger/refactor_tests_and_fix_swtpm_cert
branch
from
919c2fe to
ffb95f2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/_test_tpm2_swtpm_cert (1)
18-34: 💤 Low valueConsider failing on size mismatch for consistent test behavior.
Similar to
_test_swtpm_cert, thecheck_cert_sizefunction only warns when the DER certificate size falls outside the expected range. While thecheck_certfunction handles content verification failures properly, silent size mismatches could mask regressions.Since this uses a range format (
lo-hi) and EC signature sizes can legitimately vary, this may be intentional. If so, consider adding a comment explaining why warnings are acceptable here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/_test_tpm2_swtpm_cert` around lines 18 - 34, The check_cert_size function currently only prints a warning when the DER cert size is outside the expected lo-hi range; update it for consistent test behavior by making it fail the test on mismatch: inside check_cert_size (function name) after computing size, replace the warning-only branch with a deterministic failure (e.g., log the same messages then exit nonzero or return nonzero) so callers see a test failure instead of a silent warning; if variable EC signature sizes require allowing ranges, alternatively add a short comment above check_cert_size explaining why a warning is acceptable and list the expected variability, but prefer making the function return nonzero/exit 1 to ensure failures are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/_test_tpm2_swtpm_cert`:
- Around line 18-34: The check_cert_size function currently only prints a
warning when the DER cert size is outside the expected lo-hi range; update it
for consistent test behavior by making it fail the test on mismatch: inside
check_cert_size (function name) after computing size, replace the warning-only
branch with a deterministic failure (e.g., log the same messages then exit
nonzero or return nonzero) so callers see a test failure instead of a silent
warning; if variable EC signature sizes require allowing ranges, alternatively
add a short comment above check_cert_size explaining why a warning is acceptable
and list the expected variability, but prefer making the function return
nonzero/exit 1 to ensure failures are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57018ddf-b3d4-4c23-ba05-c3a3b5b4530d
⛔ Files ignored due to path filters (8)
tests/data/ecprivek.pemis excluded by!**/*.pemtests/data/ecpubek.pemis excluded by!**/*.pemtests/data/issuercert.pemis excluded by!**/*.pemtests/data/pubek.pemis excluded by!**/*.pemtests/data/signkey-encrypted.pemis excluded by!**/*.pemtests/data/signkey.pemis excluded by!**/*.pemtests/data/swtpm-localca-rootca-cert.pemis excluded by!**/*.pemtests/data/swtpm-localca-rootca-privkey.pemis excluded by!**/*.pem
📒 Files selected for processing (11)
.github/workflows/ci.ymlconfigure.acsrc/swtpm_cert/ek-cert.csrc/swtpm_cert/tpm.asnsrc/swtpm_cert/tpm_asn1.htests/Makefile.amtests/_test_swtpm_certtests/_test_tpm2_swtpm_certtests/test_swtpm_certtests/test_tpm2_swtpm_certtests/test_tpm2_swtpm_cert_ecc
💤 Files with no reviewable changes (1)
- tests/test_tpm2_swtpm_cert_ecc
stefanberger
force-pushed
the
stefanberger/refactor_tests_and_fix_swtpm_cert
branch
from
ffb95f2 to
6707c66
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/_test_tpm2_swtpm_cert (1)
18-34: ⚡ Quick winSize check only warns, allowing silent regressions.
Similar to the TPM1 helper,
check_cert_sizeonly emits a warning when DER certificate size falls outside the expected range. Consider making this a hard failure to catch output regressions in CI.Proposed fix
function check_cert_size() { local cert="$1" local exp="$2" local size lo hi lo=$(cut -d"-" -f1 <<< "${exp}") hi=$(cut -d"-" -f2 <<< "${exp}") # Check size of DER cert size=$(openssl x509 -in "${cert}" -outform der | wc -c) if [ "${size}" -lt "${lo}" ] || [ "${size}" -gt "${hi}" ]; then - echo "Warning: DER Certificate has unexpected size." - echo " Expected: $exp; found: $size" + echo "Error: DER Certificate has unexpected size." + echo " Expected: $exp; found: $size" + exit 1 fi }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/_test_tpm2_swtpm_cert` around lines 18 - 34, The check_cert_size function currently only prints warnings when the DER certificate size is outside the expected range; change it to fail the test instead by returning a non-zero status (or exiting) so CI catches regressions: update check_cert_size to print the same messages to stderr and then call return 1 (or exit 1 if intended to stop the script) when the size is out of bounds, ensuring callers see the failing status; keep function name check_cert_size and the same size calculation logic unchanged.tests/test_swtpm_cert (1)
25-52: 💤 Low valueChained openssl commands capture only the last error message.
When multiple openssl commands are chained with
||, only the last failed command's output is captured in$msg. If an earlier command fails,$msgmay contain output from a subsequent (possibly successful) command or be misleading.Consider capturing each command's output separately or appending to
$msg:Alternative approach
-if ! msg=$(openssl genrsa -out "${RSAPRIVKEY}" 2432 2>&1) || - ! msg=$(openssl rsa -in "${RSAPRIVKEY}" -pubout -out "${RSAPUBKEY}" 2>&1) || +msg="" +if ! out=$(openssl genrsa -out "${RSAPRIVKEY}" 2432 2>&1); then msg+="$out"; fi +if [ -z "$msg" ] && ! out=$(openssl rsa -in "${RSAPRIVKEY}" -pubout -out "${RSAPUBKEY}" 2>&1); then msg+="$out"; fi ...Or simply run commands sequentially with
set -eand let the first failure exit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_swtpm_cert` around lines 25 - 52, The chained openssl pipeline only preserves the last command's output in $msg, so change the block to run each openssl command sequentially and capture each command's stderr into a temporary variable (or append to $msg) and exit immediately on failure; target the commands that produce RSAPRIVKEY, RSAPUBKEY, CAKEY/CACERT, RSA3072ENCRYPTED_PRIVKEY/RSA3072ENCRYPTED_PUBKEY and ISSUERCERT_RSA3072ENCRYPTED_PRIVKEY, e.g. run each openssl invocation separately, check its exit status, and if non-zero copy its captured output into $msg and exit 1 (or enable set -e at top and let the first failing command terminate while redirecting stderr to a per-command log), so you always report the exact failing command and its error text.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/_test_tpm2_swtpm_cert`:
- Around line 18-34: The check_cert_size function currently only prints warnings
when the DER certificate size is outside the expected range; change it to fail
the test instead by returning a non-zero status (or exiting) so CI catches
regressions: update check_cert_size to print the same messages to stderr and
then call return 1 (or exit 1 if intended to stop the script) when the size is
out of bounds, ensuring callers see the failing status; keep function name
check_cert_size and the same size calculation logic unchanged.
In `@tests/test_swtpm_cert`:
- Around line 25-52: The chained openssl pipeline only preserves the last
command's output in $msg, so change the block to run each openssl command
sequentially and capture each command's stderr into a temporary variable (or
append to $msg) and exit immediately on failure; target the commands that
produce RSAPRIVKEY, RSAPUBKEY, CAKEY/CACERT,
RSA3072ENCRYPTED_PRIVKEY/RSA3072ENCRYPTED_PUBKEY and
ISSUERCERT_RSA3072ENCRYPTED_PRIVKEY, e.g. run each openssl invocation
separately, check its exit status, and if non-zero copy its captured output into
$msg and exit 1 (or enable set -e at top and let the first failing command
terminate while redirecting stderr to a per-command log), so you always report
the exact failing command and its error text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9b450d3-83c9-466f-908b-6431d471e61e
⛔ Files ignored due to path filters (8)
tests/data/ecprivek.pemis excluded by!**/*.pemtests/data/ecpubek.pemis excluded by!**/*.pemtests/data/issuercert.pemis excluded by!**/*.pemtests/data/pubek.pemis excluded by!**/*.pemtests/data/signkey-encrypted.pemis excluded by!**/*.pemtests/data/signkey.pemis excluded by!**/*.pemtests/data/swtpm-localca-rootca-cert.pemis excluded by!**/*.pemtests/data/swtpm-localca-rootca-privkey.pemis excluded by!**/*.pem
📒 Files selected for processing (6)
tests/Makefile.amtests/_test_swtpm_certtests/_test_tpm2_swtpm_certtests/test_swtpm_certtests/test_tpm2_swtpm_certtests/test_tpm2_swtpm_cert_ecc
💤 Files with no reviewable changes (1)
- tests/test_tpm2_swtpm_cert_ecc
stefanberger
force-pushed
the
stefanberger/refactor_tests_and_fix_swtpm_cert
branch
from
6707c66 to
ed48b1c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/_test_swtpm_cert`:
- Line 61: Replace unsafe printf usage that can interpret % sequences in
passwords: locate the here-document substitutions using printf
"${PARAM_PASSWORD}" (e.g., the --signkey-pwd file:<(printf "${PARAM_PASSWORD}")
invocation and the other similar occurrence) and change them to use a fixed
format string printf '%s' "${PARAM_PASSWORD}" so the password bytes are passed
verbatim to swtpm-certificate.
In `@tests/_test_tpm2_swtpm_cert`:
- Around line 18-45: The test currently only logs a warning in check_cert_size()
and check_cert() ignores that result, so DER size regressions don't fail the
test; modify check_cert_size() (the function that computes lo/hi and size) to
return a non-zero status (or exit 1) when size is outside the expected range
instead of only echoing a warning, and update check_cert() to call
check_cert_size and propagate its failure (exit or return non-zero) so the test
suite fails on size regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01d60282-b926-4691-92eb-e7d3ee715a55
⛔ Files ignored due to path filters (8)
tests/data/ecprivek.pemis excluded by!**/*.pemtests/data/ecpubek.pemis excluded by!**/*.pemtests/data/issuercert.pemis excluded by!**/*.pemtests/data/pubek.pemis excluded by!**/*.pemtests/data/signkey-encrypted.pemis excluded by!**/*.pemtests/data/signkey.pemis excluded by!**/*.pemtests/data/swtpm-localca-rootca-cert.pemis excluded by!**/*.pemtests/data/swtpm-localca-rootca-privkey.pemis excluded by!**/*.pem
📒 Files selected for processing (6)
tests/Makefile.amtests/_test_swtpm_certtests/_test_tpm2_swtpm_certtests/test_swtpm_certtests/test_tpm2_swtpm_certtests/test_tpm2_swtpm_cert_ecc
💤 Files with no reviewable changes (1)
- tests/test_tpm2_swtpm_cert_ecc
stefanberger
force-pushed
the
stefanberger/refactor_tests_and_fix_swtpm_cert
branch
7 times, most recently
from
90abd43 to
8e1dffb
Compare
…pport Require that OpenSSL's libcrypto >= v3.5 is available since ML-KEM and ML-DSA support was added in this version. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Since swtpm now needs OpenSSL >= v3.5, upgrade the requirement for Ubuntu to 26.04. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
X509_time_adj_ex will return a NULL pointer if the days parameter is too far in the future. Therefore adjust the error message. Also avoid a memory leak when the return value was NULL. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
The IAK/IDevID certificate did not pass certificate chain verification due to malformed ASN.1 in the SAN. Fix the ASN.1 that is put into the SAN to have proper nesting of sequences. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Refactor the certificate creation tests to - create all needed keys and certs using openssl CLI tool - accept input parameters passed to test script - grep for more expected data in the created certificates - verify the created certificate with the intermediate CA - test signing with a secp521r1 key Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
stefanberger
force-pushed
the
stefanberger/refactor_tests_and_fix_swtpm_cert
branch
from
8e1dffb to
2218e61
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
See patch descriptions.
This now also requires OpenSSL 3.5 since we want to be able to grep for "Endorsement Key Certificate" in a certificate dump.
Summary by CodeRabbit
Dependencies
Bug Fixes
Tests
Chores