cli: pin the SCEP GetCACert response - #17
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the reference CLI’s SCEP flow by adding CA fingerprint pinning so the client can authenticate the GetCACert response out-of-band and avoid trusting attacker-supplied CA/RA material over an untrusted transport.
Changes:
- Add
--ca-fingerprint(SCEP-only) and enforce pin validation before network access; use only the pinned cert as the envelope recipient and CertRep trust anchor. - Print SHA-256 fingerprints in
getcacertsto support operator pin acquisition; apply the same pin resolution togetnextca. - Extend the existing CLI integration test to cover option scoping, argument validation, and end-to-end pinning against
wolfcert-server; update docs accordingly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cli/wolfcert_client.c | Implements --ca-fingerprint parsing/enforcement and pins SCEP enroll/getnextca trust decisions to the matched cert. |
| tests/integration/cli_proto_scoping.sh | Adds argument-validation cases and an end-to-end pinning group using the in-tree test server. |
| README.md | Documents why/when to pin SCEP GetCACert and how to obtain/use the fingerprint via the CLI. |
| docs/MIGRATING-FROM-WOLFSCEP.md | Updates migration guidance to mention the new CLI fingerprint pinning option. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
2804065 to
e451137
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #17
Scan targets checked: wolfcert-bugs, wolfcert-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
e451137 to
d6fc617
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #17
Scan targets checked: wolfcert-bugs, wolfcert-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
- --ca-fingerprint takes [sha256:|sha1:|sha512:]HEX. parse_ca_fingerprint decodes it into a CaPin, ignoring ':' and ' ' between hex digits, and refuses malformed hex, an unnamed digest, a length that disagrees with the named digest, and, through NO_SHA and WOLFSSL_SHA512, a digest wolfSSL was not built with. scep_pin_setup resolves it for the SCEP commands and, when it is absent, notes that the served CA is trusted unverified. check_proto_only_opts refuses the option under EST. - pem_cert_at converts the idx-th PEM certificate of a GetCACert response to DER, find_pinned_cert returns the one matching the pin and clears the mismatches recorded for those it rejected first, der_to_pem re-encodes a match, and print_ca_fingerprints writes each certificate's SHA-256 to stderr. - scep_enroll carries the SCEP arm of cmd_enroll. With a pin the matched certificate is the envelope recipient and the only CertRep trust anchor and the bundle is not fetched; unpinned, the bundle remains the trust set. cmd_getcacerts reports the fingerprints, fails on a pin that matches nothing, and writes only the matched certificate. cmd_getnextca resolves the current CA from the pinned certificate. - cli_proto_scoping.sh covers the scoping and the argument rejections, and starts wolfcert-server for a group that reads the fingerprint from getcacerts, then runs enroll and getnextca with a mismatched and a matching pin. - README.md documents fetching and pinning the fingerprint; docs/MIGRATING-FROM-WOLFSCEP.md names the flag beside the API. Issue: F-8046
d6fc617 to
386b2ff
Compare
Frauschi
left a comment
There was a problem hiding this comment.
Read through the pinning change. The trust bootstrap itself looks right - find_pinned_cert selects, frees the rejected certificates and fails closed, and making the pinned certificate the sole envelope recipient and CertRep anchor is the correct call. What is below is one design question plus coverage and documentation gaps; nothing blocking.
The one worth a decision is getnextca. Pinning the RA for enroll, as the README now tells you to, makes getnextca fail with a message that reads like a broken pin, because the roll-over is signed by the CA. I think the narrowing is right and the answer is to say so rather than widen the anchor back - details inline.
The recurring theme in the rest is that wolfcert-server serves a single CA certificate, so the multi-certificate half of the pin selection never runs, and the case that claims to guard the stale-error path never reaches it.
ctest registration comment for cli_proto_scoping is now false — the test starts a network server (tests/CMakeLists.txt:117-118)
The comment above add_test(NAME cli_proto_scoping ...) still reads "Every case fails before any network access, so no server is involved". The new pinning group binds 18088/18188/18288/18388 and forks wolfcert-server, so that is no longer true - worth fixing, since that comment is what the next person uses to decide whether this test is safe to run in a sandbox or in parallel.
The registration also declares no dependency or fixture on the wolfcert-server target and passes only $<TARGET_FILE:wolfcert-client>, relying on the script deriving its sibling. That works under both CMake and the automake AM_TESTS_ENVIRONMENT, so nothing is broken, but a build dependency (or FIXTURES_REQUIRED) would stop the group silently skipping itself when ctest is driven without a full build.
| } | ||
| else if (find_pinned_cert(ca_pem.data, ca_pem.len, &pin, &pinned, | ||
| NULL) != 0) { | ||
| fprintf(stderr, "getnextca: the GetCACert response does not match " |
There was a problem hiding this comment.
Narrowing the getnextca anchor to the pinned certificate is right in itself, but it collides with the RA guidance this PR adds to the README. GetNextCACert is signed by the current CA (RFC 8894 section 4.6.1) while a CertRep in an RA deployment is signed by the RA, so someone who follows the README and pins the RA hits this refusal and reads it as a broken pin rather than a scope mismatch.
There is no real conflict at runtime - enroll and getnextca are separate invocations, so you just pass the CA's fingerprint here - but nothing in the error, the help, or the README says so. I would keep the narrowing and make the failure self-explaining. Widening the anchor back to the whole bundle once the pin matches would be worse: GetCACert is an unsigned degenerate PKCS#7, so a pin proves one member is genuine, not that an on-path attacker did not append their own certificate alongside it - and this is the one command whose job is installing a new trust anchor.
Concretely, three small changes:
- a second line on this refusal, e.g.
getnextca: the roll-over is signed by the CA, not the RA that signs a CertRep - pin the CA's fingerprint here; - one line in the
--ca-fingerprinthelp block noting that getnextca wants the CA's own fingerprint, not the RA's; - a sentence in README.md next to the existing RA paragraph, pointing out that
getcacertsprints one line per served certificate, so both values come out of a single trusted read.
| expect_parses sha512 "sha512:$(printf '0%.0s' $(seq 1 128))" | ||
| expect_parses sha1 "sha1:$(printf '0%.0s' $(seq 1 40))" | ||
|
|
||
| # Accepting a fingerprint must not leave a mismatch in the library's error |
There was a problem hiding this comment.
Every new case here pins index 0 of a one-certificate response, because wolfcert-server serves a single CA certificate. That leaves the whole interesting half of the new code unexercised: pem_cert_at's index advance for idx > 0, find_pinned_cert's keep-the-first-match-and-free-the-rest branch, the n_certs > 1 warning in scep_enroll, and the wolfcert_clear_error() whose only job is suppressing a stale mismatch recorded for certificates rejected before the match.
A multi-certificate response is the RFC 8894 section 4.2 shape a real CA serves, and the split CA/RA deployment is where pinning matters most - so this is the configuration the suite should cover first, not last.
| expect_reject "sha256 --ca-fingerprint of the wrong length" "needs 32 bytes" \ | ||
| getcacerts --proto scep --url "$SCEP_URL" \ | ||
| --ca-fingerprint sha256:0000000000000000000000000000000000000000 | ||
| expect_reject "over-long --ca-fingerprint" "longer than any supported digest" \ |
There was a problem hiding this comment.
seq and fractional sleep are not POSIX, and the failure mode here is a false alarm about the parser. Without seq, $(seq 1 200) expands to nothing, printf '0%.0s' emits nothing, and this passes --ca-fingerprint sha512: - which the CLI rejects for the wrong reason (sha512 needs 64 bytes, got 0), so expect_reject reports FAIL: over-long --ca-fingerprint rejected, but not for the expected reason. The same substitution quietly degrades the two expect_parses cases into empty-argument checks that pass for the wrong reason, and a sleep that rejects 0.1 burns all 100 poll iterations with no delay and skips the whole pinning group.
A shell-only repeat helper covers the three call sites:
rep() { i=0; while [ $i -lt "$2" ]; do printf '%s' "$1"; i=$((i+1)); done; }
and a sleep 1 fallback (or one probe at startup that exits 77) makes a platform gap read as a skip rather than a parser regression.
|
|
||
| # Accepting a fingerprint must not leave a mismatch in the library's error | ||
| # state, where an unrelated later failure would report it as the detail. | ||
| GOOD_FP="sha256:$(printf '1%.0s' $(seq 1 64))" |
There was a problem hiding this comment.
This case cannot fail. SCEP_URL is http://127.0.0.1:1/scep, so the CLI dies in wolfcert_scep_get_ca_cert on connection refused and never enters find_pinned_cert - no fingerprint is ever compared, no mismatch is ever recorded, and fingerprint mismatch cannot appear whatever the implementation does. Deleting the wolfcert_clear_error() this is meant to protect leaves it green. What it actually proves is that a well-formed sha256 argument parses, which expect_parses already covers.
Reaching the real path needs a pin that matches only after an earlier certificate in the same bundle was rejected, plus a later stage that fails - so this case wants to move into the live-server group once a two-certificate response exists. Until then, at least reword the label so it does not claim coverage it has not got.
| *) echo "ok accepting a --ca-fingerprint leaves no stale error" ;; | ||
| esac | ||
|
|
||
| # The pinning itself, end to end against the in-tree test server. wolfcert-server |
There was a problem hiding this comment.
The end-to-end group runs against wolfcert-server, which serves exactly one certificate, so the pin-selection loop never does anything non-trivial here: no iterating past a non-match, no wc_FreeDer of a rejected certificate, no index other than 0. This is the split CA/RA layout the README singles out as the tricky case, and the one where an indexing or free-ordering mistake would surface.
A fixture that serves two certificates needs no wolfcert-server change - a degenerate PKCS#7 cert bag is enough:
openssl crl2pkcs7 -nocrl -certfile ra.pem -certfile ca.pem \
-outform DER -out bundle.p7b
served as application/x-x509-ca-ra-cert from ?operation=GetCACert. Then assert that pinning the second certificate's fingerprint makes getcacerts write that certificate and not the first, and that enroll prints the pinned 1 of 2 notice. A --scep-serve-ra flag on the test server would work too, if you would rather keep it in-tree.
| `--trust PEMFILE` (or a caller-supplied trust anchor). Without it the client | ||
| refuses rather than hand its credentials and CSR to an unverified server. | ||
|
|
||
| SCEP instead authenticates the CA by fingerprint. Whoever answers `GetCACert` |
There was a problem hiding this comment.
Worth scoping this paragraph. scep_enroll still calls wolfcert_scep_get_ca_caps over the same plaintext transport and discards the return value, so an on-path attacker who cannot defeat the pin can still strip SHA-256/SHA-512 and AES and force the pkiMessage down to a SHA-1 signature with 3DES content encryption.
The pin itself holds - the content key is still RSA-wrapped to the pinned certificate, so the CSR and its challengePassword stay confidential, and the CertRep signer check is SPKI-based and caps-independent. But "SCEP instead authenticates the CA by fingerprint" reads as a claim about the whole exchange, and a reader may conclude that a pin removes the need for --content-cipher on an untrusted path. One sentence saying otherwise would fix it; checking the get_ca_caps return would also make a suppressed GetCACaps distinguishable from a genuinely legacy CA.
| *count = 0; | ||
|
|
||
| for (;;) { | ||
| rc = pem_cert_at(pem, pem_len, i, &der); |
There was a problem hiding this comment.
These two loops disagree on what the index means. find_pinned_cert increments *count only for blocks that decode, while print_ca_fingerprints labels its output with the PEM block index i and simply skips a block that fails. So for a [good, malformed, good] response getcacerts prints certificate 0 and certificate 2, while enroll reports pinned 1 of 2 served certificates - and the operator is meant to match one message against the other.
No memory or correctness consequence, and the pin match itself is unaffected. Simplest fix is a separate emitted counter in print_ca_fingerprints so both helpers count decodable certificates.
Problem
The reference CLI never authenticated the SCEP CA it enrolled against.
cmd_enrollfetchedGetCACertover the same (usually plaintext) transport it was about to trust, made the first certificate the CSR envelope recipient, and made the whole response the CertRep trust set. Sincewolfcert_scep_verify_rep_signeraccepts any signer whose SPKI matches any bundle member, an active attacker answeringGetCACertwith their own RSA certificate both decrypts the CSR with itschallengePasswordand returns a certificate the client accepts.wolfcert_scep_verify_ca_fingerprintwas written for exactly this bootstrap and had zero callers incli/. A caller using the library API correctly is unaffected.Fix (
cli/wolfcert_client.c)Adds
--ca-fingerprint [sha256:|sha1:|sha512:]HEX, rejected under EST and validated before any network access.getcacertsprints each certificate's SHA-256, so the value can be read once over a trusted path and pinned thereafter.getnextcaresolves the current CA through the same pin, closing the circularity its own comment documented.Closes
f-8046.Tests
tests/integration/cli_proto_scoping.sh, the repo's existing CLI test, gains the option scoping and argument rejections plus a group that startswolfcert-server, reads the fingerprint fromgetcacerts, and runsenrollandgetnextcaagainst a mismatched and a matching pin. The group skips itself when the server was not built, so no new build-file registration is needed.Verification
scep_enrollorcmd_getnextcamakes the matching mismatched-pin case pass.