certs: fix SIGPIPE race in the shipped-key fingerprint check - #11
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The nonportable EXIT trap condition can make the credential test fail immediately under /bin/sh on Ubuntu.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes intermittent shipped-key fingerprint failures caused by a SIGPIPE/pipefail race and improves test isolation and CI diagnostics.
Changes:
- Uses a temporary file for public-key fingerprinting.
- Adds credential-test logging and vault cleanup.
- Improves CI matrix handling, artifact naming, and action versions.
File summaries
| File | Summary |
|---|---|
tests/unit_tests.c |
Cleans stale vault state before testing. |
tests/credential-options.test |
Adds tracing, logs, and failure preservation. |
certs/gen-certs.sh |
Removes the fingerprinting pipeline race. |
.gitignore |
Ignores preserved failure artifacts. |
.github/workflows/test-pull.yml |
Disables matrix fail-fast and labels configurations. |
.github/workflows/test-nightly.yml |
Disables matrix fail-fast and labels configurations. |
.github/workflows/sanitizer.yml |
Updates actions and diagnostic uploads. |
.github/workflows/make-test.yml |
Adds configuration-specific artifacts and logs. |
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
aidangarske
approved these changes
Sep 11, 2026
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
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.
The nightly workflow has failed on all but two nights since 2026-08-13, always on
tests/credential-options.test, always with no diagnostic output. The failing job set differs every night, which made it look configuration-dependent; it is not.certs/gen-certs.shruns underset -euo pipefail, andreject_shipped_key()fingerprinted a key withopenssl pkey ... | openssl dgst | awk. When the reader exits before the writer's output lands,openssl pkeydies of SIGPIPE andpipefailturns that into a fingerprint failure, so the check reports "could not be fingerprinted" instead of rejecting the shipped key. It now writes the DER public key to a file and digests the file, so no pipeline has a reader that can exit early. The four hardcoded fingerprints are unchanged - the file and pipe forms produce identical digests.Also in this PR:
tests/credential-options.testtraces to stderr, logs each check to a file instead of/dev/null, and preserves its temporary directory on failure. Previously the only record of a failure wasFAIL ... (exit status: 1).vault_test()intests/unit_tests.cnow unlinksvault.binfirst, matchingvault_reopen_test()andvault_corrupt_test(). It was inheriting items and encryption mode from whichever build ran last in the tree.fail-fast: falseon every matrix, unique per-configuration failure-log artifact names,tests/*.logadded to the uploads, andactions/checkout/actions/upload-artifactmoved to@v5.Introduced by PR #10 (
d9874b9). Not present in any tagged release - the most recent tag is v1.1 from 2021, so onlymasteris affected.The ASan and UBSan jobs were failing on this same test, not on a sanitizer finding; neither job log contained any sanitizer diagnostic.
Test status
No hardware involved. Verified on x86-64 Linux against wolfSSL master built with the CI configure line.
tests/credential-options.testunder parallel load: 17 failures in 100 iterations before the fix, 0 in 900 after.make checkclean in all six CI configurations, and under both-fsanitize=addressand-fsanitize=undefined -fno-sanitize-recover=allwith the workflow's sanitizer options.make distplus the release credential generation check from the build workflow.Scope
src/keymanager.cand theLoadPasswordFile()validation semantics are unchanged; that code is deterministic and was not implicated.