Skip to content

cli: pin the SCEP GetCACert response - #17

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:mainfrom
yosuke-wolfssl:fix/f_8046
Open

cli: pin the SCEP GetCACert response#17
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:mainfrom
yosuke-wolfssl:fix/f_8046

Conversation

@yosuke-wolfssl

@yosuke-wolfssl yosuke-wolfssl commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Problem

The reference CLI never authenticated the SCEP CA it enrolled against. cmd_enroll fetched GetCACert over 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. Since wolfcert_scep_verify_rep_signer accepts any signer whose SPKI matches any bundle member, an active attacker answering GetCACert with their own RSA certificate both decrypts the CSR with its challengePassword and returns a certificate the client accepts. wolfcert_scep_verify_ca_fingerprint was written for exactly this bootstrap and had zero callers in cli/. 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.

  • A matching certificate becomes the only trust anchor. It is the envelope recipient and the sole CertRep anchor; the bundle-widening fetch is skipped. No match fails closed.
  • getcacerts prints each certificate's SHA-256, so the value can be read once over a trusted path and pinned thereafter.
  • getnextca resolves the current CA through the same pin, closing the circularity its own comment documented.
  • Unpinned SCEP warns, whatever the transport.
  • getnextca wants the CA's own fingerprint, not the RA's — RFC 8894 section 4.6.1 has the current CA sign the roll-over, while a CertRep in an RA deployment is signed by the RA. getcacerts prints both, so one trusted read covers it.
wolfcert-client getcacerts --proto scep --url http://ca.example/scep
# getcacerts: certificate 0 is sha256:3F:A1:...
wolfcert-client enroll --proto scep --url http://ca.example/scep \
    --ca-fingerprint sha256:3F:A1:... --key-type rsa:2048 --subject "CN=dev"

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 starts wolfcert-server, reads the fingerprint from getcacerts, and runs enroll and getnextca against a mismatched and a matching pin. The group skips itself when the server was not built, so no new build-file registration is needed.

Not in this PR

Deferred to follow-up issues, all raised in review:

  • A multi-certificate GetCACert fixture, so pin selection is exercised on the split CA/RA shape. Needs new test infrastructure.
  • wolfcert_scep_get_ca_caps's discarded return in scep_enroll — a distinct issue from this one; the pin cannot address it, and the README now documents the exposure.
  • A failed XMALLOC in wolfcert_scep_parse_pki_message surfacing as WOLFCERT_ERR_AUTH rather than a memory error.
  • FIXTURES_REQUIRED on the ctest registration.

Verification

  • 26/26, build clean under the project's warning set.
  • Negative controls: disabling the enforcement branch in scep_enroll or cmd_getnextca makes the matching mismatched-pin case pass.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Aug 19, 2026
Copilot AI lite review requested due to automatic review settings August 19, 2026 01:29

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 getcacerts to support operator pin acquisition; apply the same pin resolution to getnextca.
  • 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.

Comment thread tests/integration/cli_proto_scoping.sh Outdated

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cli/wolfcert_client.c Outdated
Comment thread cli/wolfcert_client.c
Comment thread cli/wolfcert_client.c Outdated
Comment thread cli/wolfcert_client.c

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cli/wolfcert_client.c
Comment thread cli/wolfcert_client.c
Comment thread cli/wolfcert_client.c
Comment thread cli/wolfcert_client.c

@Frauschi Frauschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cli/wolfcert_client.c
}
else if (find_pinned_cert(ca_pem.data, ca_pem.len, &pin, &pinned,
NULL) != 0) {
fprintf(stderr, "getnextca: the GetCACert response does not match "

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-fingerprint help 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 getcacerts prints one line per served certificate, so both values come out of a single trusted read.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took the message and the README sentence; skipped the help line, since the failure now explains itself where it matters.

The message went on the get_next_ca_cert failure below, not here: pinning the RA matches, so this refusal never fires — the failure is WOLFCERT_ERR_AUTH from the roll-over verify. That code is a clean discriminator on this path (map_wc_err has no ERR_AUTH case, http.c has no site), but it covers three conditions, not just a wrong pin, so the wording says the signer "could not be bound to the pinned certificate" and then gives your 4.6.1 hint.

One of those three is a failed XMALLOC at scep_msg.c:654 letting the parse return WOLFCERT_OK with a NULL signer — OOM as an auth error, on the enroll path too. Filing separately.

Comment thread tests/integration/cli_proto_scoping.sh Outdated
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, deferring to a follow-up issue rather than this PR. A separate multi-scan review flagged the same gap as its only Medium, so it is being filed rather than dropped.

It gates two other things: the counter fix on print_ca_fingerprints has no in-tree test until it exists, and neither does the new getnextca hint.

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" \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with a shell rep(). Four call sites, not three — GOOD_FP is the fourth. The poll probes sleep 0.1 and falls back to whole seconds over the same ten-second budget.

Ran both scripts under a missing seq and a fractional-rejecting sleep, and two details differ: the expect_parses cases fail loudly rather than passing quietly, and the sleep gap busy-spins to completion rather than skipping the group — it shrinks the startup budget ~10x, so a skip is likelier under load, not certain. Fix is unchanged either way.

Comment thread tests/integration/cli_proto_scoping.sh Outdated

# 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))"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Reworded to say what it checks, and the comment now states the stale-mismatch path needs more than one certificate.

Also made it non-vacuous: it passed on any output lacking "fingerprint mismatch", including a success. It now fails if enroll against an unreachable port returns 0. Moving it into the live-server group waits on the fixture.

*) 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking the degenerate-PKCS#7 suggestion into the same follow-up. It needs new test infrastructure, which is more than the rest of this round combined.

Worth noting one option that does not work: a unit test on the selection logic. pem_cert_at and find_pinned_cert are static in cli/wolfcert_client.c, so that would need them moved into the library first.

Comment thread README.md
`--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`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a scoping paragraph: the pin covers GetCACert alone, GetCACaps is unauthenticated, AES can be stripped to force 3DES, and --content-cipher aes128|aes256 is the answer on an untrusted path.

Left the SHA-1 claim out — pick_hash_oid (scep_client.c:370) returns only SHA512h/SHA384h/SHA256h, so stripping caps drops SHA-512/384 to SHA-256 and no further. The AES to 3DES half is real.

Checking the get_ca_caps return is a separate finding, filed on its own.

Comment thread cli/wolfcert_client.c
*count = 0;

for (;;) {
rc = pem_cert_at(pem, pem_len, i, &der);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: print_ca_fingerprints counts decoding certificates, and find_pinned_cert's doc now says "certificates that decode". Verified on a [good, unparseable, good] bundle — 0/1 with the fix, 0/2 without.

One wrinkle in the example: pem_cert_at returns -1 only when wc_PemToDer fails, and that call just base64-decodes. A block decoding to NOTAVALIDCERTIFICATE is counted and printed as a certificate; reaching the skip path needs broken base64, not a malformed certificate. Cosmetic — the pin matches by hash — but noted.

- --ca-fingerprint takes [sha256:|sha1:|sha512:]HEX, and is refused
  under EST. parse_ca_fingerprint decodes it into a CaPin;
  scep_pin_setup resolves it, and notes that an unpinned run trusts
  the served CA.
- pem_cert_at, find_pinned_cert, der_to_pem and print_ca_fingerprints
  select, re-encode and report the certificates of a GetCACert
  response, numbered over the ones that decode.
- With a pin, scep_enroll makes the match the envelope recipient and
  the only CertRep anchor and skips the bundle fetch; cmd_getcacerts
  writes only the match; cmd_getnextca resolves the current CA through
  the pin, and on WOLFCERT_ERR_AUTH names the CA as the signer of the
  roll-over.
- cli_proto_scoping.sh adds the option scoping, the argument
  rejections, and a wolfcert-server group covering a mismatched and a
  matching pin, with a shell repeat helper and a whole-second sleep
  fallback; tests/CMakeLists.txt and Makefile.am describe the
  registration.
- README.md and docs/MIGRATING-FROM-WOLFSCEP.md document the flag,
  which certificate getnextca wants pinned, and that the pin leaves
  GetCACaps unauthenticated.

Issue: F-8046

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #17

Scan targets checked: wolfcert-bugs, wolfcert-src

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

@yosuke-wolfssl

Copy link
Copy Markdown
Contributor Author

Hello @Frauschi ,
I reworked on it. Can you review this again ?
Please note that some issues are not addressed here because they are pre-existing gaps.
Especially we need new infrastructure for tests of the split CA/RA shape, so I'll take it as follow-up.
They are described in PR body honestly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants