scep: answer unprocessable pkiMessages with a CertRep

[yosuke-wolfssl]

Problem

After wolfcert_scep_parse_pki_message() has verified the CMS signature and
recovered the transactionID and senderNonce, two dispatch failures in
handle_pki_op() answered with a bare text/plain HTTP 400 instead of a
pkiMessage: a pkcsPKIEnvelope that fails to decrypt for messageType 19/17,
and an unrecognized messageType. RFC 8894 section 3.3.2 returns a CertRep
FAILURE for a rejected request, and section 3.2.1.4 defines the failInfo codes
that describe why. A conforming SCEP client maps any non-200 to a generic
transport error, so the failInfo never reaches the caller. Closes f-8033.

Fix (src/scep/scep_server.c)

New send_pki_failure() builds the CertRep from the transactionID and
senderNonce already in scope, with no certificate and no envelope target.

Case	Answer
missing transactionID, senderNonce or messageType	HTTP 400
pkcsPKIEnvelope will not decrypt (19/17)	CertRep pkiStatus 2, failInfo 0 badAlg when the content cipher is unsupported, else 2 badRequest
unrecognized messageType	CertRep pkiStatus 2, failInfo 2 badRequest

The line is RFC 8894 section 3.2.1: a message omitting one of the three
mandatory attributes is not a pkiMessage, so it keeps the HTTP 400 — that guard
now covers messageType alongside the other two. Anything past it is a message
we understood but cannot act on, and gets a CertRep. Both branches log a failed send,
and the de-envelop branch keeps its own error as the return value.
They also clear keep_alive: the non-OK return is what closes the socket,
so the flag only controls what the Connection: header says — the tests cover the close, not the header.

The four existing pkiStatus 2 sites in handle_enroll() and
handle_get_cert_initial() now share the same helper, which lets
handle_get_cert_initial() drop its signer-certificate parameters.

Tests

check_malformed_dispatch() POSTs three hand-built pkiMessages — an
undecryptable payload, an unrecognized messageType, and an envelope whose
content cipher OID is unassigned — and requires a CertRep echoing the
transactionID with a fresh senderNonce, the expected failInfo, and no enveloped
messageData. A further request over a keep-alive socket requires the reply to
arrive and the connection to close. check_required_attrs() gains a
missing-messageType round and now checks the reply's senderNonce.

Verification

• Clean build under CMake and autoconf, no warnings; 27/27 ctest.
• ASan + UBSan clean.
• Negative control per branch: reverting each to send_text() fails the test.

Not in this PR

f-8033 also recommended converting the missing-messageType branch. It stays an
HTTP 400: RFC 8894 section 3.2.1 makes messageType mandatory, so such a message
is malformed rather than rejected, and no failInfo value describes it. The
Connection: mismatch on the pre-existing 400 and 500 sites, and the
transactionID being re-encoded as a PrintableString without a charset check,
are left alone — both pre-date this change.

[Copilot]

Pull request overview

Updates the SCEP server to return signed CertRep failure responses for malformed authenticated messages and adds integration tests.

Changes:

• Centralizes failure CertRep generation.
• Covers missing, undecryptable, and unknown message types.
• Adds malformed-message round-trip coverage.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Summary
src/scep/scep_server.c	Implements shared signed failure responses. Nit (3 votes): clear keep_alive before the fallback response when no transaction ID is available.
tests/integration/test_scep_roundtrip.c	Tests malformed SCEP dispatch responses.

Suppressed comments (1)

src/scep/scep_server.c:664

• The parser does not require a senderNonce, so a signed request can reach this helper with a valid transactionID but snonce == NULL/zero length. This still emits an HTTP 200 CertRep without recipientNonce (the builder omits it when the pointer is NULL), which violates RFC 8894 §3.2.1 and cannot be matched by a client. Treat a missing senderNonce like a missing transactionID and use the HTTP 400 fallback, or reject it before building the CertRep.

    if (tid == NULL || tid_len == 0) {
        send_text(s, fd, 400, "Bad Request", "text/plain", "");
        return WOLFCERT_ERR_PROTOCOL;
    }

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #23

Scan targets checked: wolfcert-bugs, wolfcert-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #23

Scan targets checked: wolfcert-bugs, wolfcert-src

No new issues found in the changed files. ✅

[Frauschi]

The refactor half of this is good - one send_pki_failure() for the four existing pkiStatus 2 sites, and handle_get_cert_initial() losing its signer-certificate parameters. My concern is the other half: converting the mt == NULL, de-envelop-failure and unknown-messageType branches from 400 to a signed CertRep.

Two things to settle first.

1. RFC 8894 doesn't require it. 3.2 makes transactionID, messageType and a fresh senderNonce mandatory in every message, so a message missing one isn't a pkiMessage at all; 3.3's "request is granted / request is rejected -> CertRep" language is about a request the CA can reason about. On malformed input the RFC is silent - there is no MUST here in either direction.

2. On the mt == NULL path the CertRep can't conform anyway. recipientNonce is copied from the request's senderNonce, and the parser leaves senderNonce NULL both when the peer omitted it and when it sent one we declined to decode (scep_msg.c:718 continues past any attribute failing its length checks). send_pki_failure() guards tid but passes snonce through, so we emit a CertRep with no recipientNonce - which our own client rejects unconditionally (scep_client.c:882). The failInfo never reaches the caller, which is the outcome this change exists to prevent.

The reference implementations all split it the other way. smallstep/scep - the protocol library behind both micromdm/scep and step-ca, the two peers our interop tests drive - rejects a missing transactionID, a missing or unknown messageType and a missing senderNonce inside ParsePKIMessage, before any dispatch, and its Fail() hard-wires RecipientNonce(msg.SenderNonce) so it structurally cannot build one without a senderNonce. Both servers turn a parse or decrypt failure into an HTTP error and keep the signed FAILURE CertRep for decisions on a well-formed, decrypted message. OpenXPKI draws the same line. There is a cost argument too: these branches sit before deenvelop on an unauthenticated request, so each malformed POST buys an RNG init and a CA private-key signature, and #22 deliberately keeps its guards ahead of the RSA decrypt for that reason.

This also collides with #22 directly. #22 inserts its transactionID/senderNonce guards at old line 792, between the mt == NULL block here and the deenvelop call - a textual conflict, and its guards land after this PR's mt == NULL branch, so they don't cover it and its snonce guard doesn't fix the missing recipientNonce above. My comment on #22 argued for 400 by citing :815 and :828 as the file's convention, and those are exactly the two sites this PR converts. Both positions can't stand.

Suggestion: keep the send_pki_failure() extraction and the four existing sites, drop the three new conversions, and settle the 400-vs-CertRep question once across both PRs. I'm open to being argued out of it on the de-envelop branch specifically, since tid and snonce genuinely are in hand there - though the badAlg/badRequest comment below is a reason it's awkward as a CertRep too.

The rest is per-site: one build-config break in the new test, and some smaller things.

[Frauschi]

This is the only unguarded AES-128 use in the file. SCEP_SRV_ENC_OID (scep_server.c:52-57) falls back to DES3b when WOLFSSL_AES_128/HAVE_AES_CBC are absent, and every other AES-CBC use here sits behind WOLFCERT_TEST_HAVE_CIPHER_OVERRIDE (:266) or an equivalent guard. On such a build wolfcert_scep_envelop() fails before the loop, all four rounds are skipped, and the REQUIRE() in main() takes the whole integration test down even though the server is correct.

Only round 2 needs a decryptable envelope and the server de-envelops whatever OID arrives, so a file-level TEST_SCEP_ENC_OID chosen the same way the server chooses SCEP_SRV_ENC_OID covers it.

[yosuke-wolfssl]

Fixed, and it applied to check_required_attrs too, so the guard covers both helpers and both call sites:

#if defined(HAVE_AES_CBC) && defined(WOLFSSL_AES_128)

Note it names WOLFSSL_AES_128 rather than (AES_128 || AES_256) — both helpers hardcode AES128CBCb, so the wider condition would let an AES-256-only build through and fail exactly as before.

The trade-off is that both tests now compile out on a 3DES-only build instead of adapting to it, which is the config where the server's DES3b fallback runs. A file-level TEST_SCEP_ENC_OID picked the way the server picks SCEP_SRV_ENC_OID would keep the coverage; happy to switch if you prefer that.

[Frauschi]

Keep the guard. The coverage it costs is a config we don't build in CI, and TEST_SCEP_ENC_OID would buy that back at the price of a second place where the test has to track how the server picks its cipher. I forced the guard off and the file still compiles clean - raw_http_req() survives via raw_http_status() - so nothing else was leaning on those two helpers.

One thing to add: both groups now vanish with no notice, so a 3DES-only build reports a clean full run having skipped them. test_est_mldsa_roundtrip.c:59 prints a SKIP (...) line for exactly this and is worth matching.

[yosuke-wolfssl]

Keeping the guard then, and the notice is in, matching test_est_mldsa_roundtrip.c:59:

    printf("SKIP required-attrs and malformed-dispatch "
           "(wolfSSL built without AES-128-CBC)\n");

So a 3DES-only build now says which two groups it skipped rather than reporting a clean run.

[Frauschi]

The helper clears keep_alive on its 400 fallback but leaves it to the caller on the CertRep path, and the callers are split: handle_pki_op (:802, :816, :832) and handle_enroll (:688, :696) set it, while handle_enroll's queue-full and handle_get_cert_initial's badCertId branches deliberately do not. Every combination is coherent today, but split ownership means a future caller that returns non-OK without the assignment emits Connection: keep-alive on a socket we then close.

The parse-failure branch five lines up (:796) already does exactly that, as do issue_and_reply()'s "Bad CSR" 400 and the 500 sites. All pre-existing, but this PR introduces the invariant and leaves its siblings out of it. Setting keep_alive = 0 inside send_text() for status >= 400 would make it unforgettable. Note also that nothing asserts the header - WolfCertHttpResponse exposes none - so a branch answering with a CertRep and keep_alive still 1 would pass the suite.

[yosuke-wolfssl]

Not changed, and I do not think send_text() can carry it: these failure replies go out as HTTP 200 CertReps through send_bin, so a "status >= 400" rule inside send_text never sees them. The call-site assignment is the only thing that makes that header truthful.

I did measure it. On the same rejection over a keep-alive socket: with the assignment the reply says Connection: close, without it Connection: keep-alive — and the server closes either way, because the non-OK return breaks the serve loop. So the flag is redundant for closing and load-bearing for the header, which is a distinction worth a comment; I have not added one yet.

There is now a test that sends over a keep-alive socket and requires the reply plus a clean close. It does not assert the header itself — raw_http_req discards headers — so that gap is real, as you said.

The pre-existing siblings (parse-failure 400, "Bad CSR", the 500s) are untouched. Happy to take them in a follow-up.

[Frauschi]

Agreed on the mechanism - send_text() can't carry it, since these go out as HTTP 200 through send_bin(). But the round you added doesn't cover the assignment, and I think the PR body currently reads as if it does.

I deleted s->keep_alive = 0 from each new branch in turn and rebuilt: 27/27 green both times. The reason is the one you measured - src/server.c:351 breaks the keep-alive loop on any non-OK serve_fd() return, so the socket closes whether or not the flag is clear, and raw_http_req() discards headers. So the round asserts the return-code path; the header stays untested, exactly as you said in your last paragraph.

Nothing to change in the code. Two small things: the PR body's "Both branches clear keep_alive so the Connection: header matches the close" is worth rewording so it doesn't imply the new test proves it, and the comment you mentioned but haven't added is worth adding - "redundant for closing, load-bearing for the header" is precisely the thing a future editor would delete.

If you want it actually pinned, raw_http_req() returning the Connection: value alongside the status would do it in a couple of lines and would also cover the pre-existing 400/500 siblings once you get to them. Happy for that to be the follow-up rather than this PR.

[yosuke-wolfssl]

Both done. The comment is on each branch — "Closed by the non-OK return; the flag is for the header" — which is the sentence a future editor needs before deleting the line. The PR body no longer implies the new round covers the assignment; it now says the tests cover the close and not the header.

Taking the raw_http_req() change as the follow-up, as you suggest.

[Frauschi]

The parser takes the transactionID attribute's inner value whatever its tag, so a peer can send an OCTET STRING or a UTF8String of arbitrary bytes and we echo them back inside a PrintableString - enc_printable_n() (scep_msg.c:100) does no charset check. The result isn't valid DER and a strict client decoder rejects it.

The encoder is pre-existing; what changes here is reachability. Before, these three cases got a text/plain 400 and the transactionID was never re-encoded; now it is, on a message that has passed nothing but the CMS signature check. If these paths stay as CertReps, worth validating the recovered transactionID against the PrintableString repertoire and taking the 400 fallback when it doesn't conform.

[yosuke-wolfssl]

Acknowledged, not fixed. enc_printable_n() writes tag 0x13 with no charset check, and #22's guard tests that the transactionID is present, not what bytes it holds, so an OCTET STRING or UTF8String value still round-trips into a PrintableString.

It reaches the two surviving branches the same way it already reaches the enroll and badCertId replies, so this is a pre-existing encoder issue that this PR widens rather than introduces. I would rather fix it once for every site than only for these — validate against the PrintableString repertoire in the encoder, or reject at parse. Want it in this PR, or as a follow-up?

[Frauschi]

Follow-up, and fix it in the encoder rather than at these sites - your instinct is right.

Confirming it still stands at 76a0273b, since the reachability is what changed: I sent a transactionID of 00 FF 7F 40 5F 24 21 0A on the unrecognized-messageType path and got it back echoed under tag 0x13. The parser unwraps by length and never looks at the inner tag (scep_msg.c:788-820), and enc_printable_n() writes 0x13 unconditionally.

Per-site validation would leave the enroll and badCertId replies broken anyway, so one check in enc_printable_n() - or a repertoire check at parse, which also stops us storing it - covers every site at once. Not a blocker here.

[Frauschi]

Confirming the scope for that follow-up, because there is a second value the repertoire check would catch and a narrow fix would miss: a messageType with an embedded NUL dispatches on its prefix.

scep_msg.c:862 copies vlen bytes and appends its own terminator, and dispatch is strcmp(mt, "19"), so a value of 31 39 00 78 ("19\0x") compares equal to "19". Probed against 74643ed7 by patching enc_printable() to map '@' to a NUL and sending the control round of check_required_attrs as "19@x": it passed the control's own assertions - 200, pkiStatus 0, enveloped messageData - so the server issued a certificate for a messageType that is not 19.

NUL is not in the PrintableString repertoire either, so one check at parse closes this and the tag hole together, whereas a check that only inspects what the encoder writes would leave it. Pre-existing, not a blocker here, and this PR does not widen it - "19\0x" reached handle_enroll before the change too. Just worth pinning to the follow-up so it does not get written narrowly.

It also means the new mt[0] == '\0' guard covers the empty case but not this one, so the four attributes are not quite treated the same way yet.

[Frauschi]

Heads up on sequencing rather than a new round of review: #21 is merged and #22 is close, so this will need a rebase. The conflict with #22 is real but purely positional, five hunks where you both add a hand-built-pkiMessage helper and its main() call at the same point in tests/integration/test_scep_roundtrip.c. No design collision: #22 POSTs over a raw socket, this PR uses wolfcert_http_request().

Two things change here once #22 lands.

1. I have asked scep: require transactionID and senderNonce before dispatch #22 to move its required-attribute guard above the mt == NULL check. With that in, send_pki_failure() is only ever reached with a non-NULL transactionID and senderNonce, so it needs no senderNonce guard of its own and its tid == NULL fallback (:661) becomes belt and braces. Worth waiting for rather than adding a second copy of the check here. The hole is live today: I added a fifth round to check_malformed_dispatch() with a transactionID, no senderNonce and no messageType, and got

   PROBE tid-but-no-senderNonce: status=200 msgType=3 pkiStatus=2 recipientNonce=ABSENT
   

   a signed CertRep with no recipientNonce, which our own client rejects at scep_client.c:882. All four current rounds pass a senderNonce, so nothing catches it.

2. scep: require transactionID and senderNonce before dispatch #22's raw_http_req() reads to EOF before reporting a status, which is the tool the thread on scep_server.c:656 says is missing. It distinguishes "answered and closed" from "answered and held open", so a branch replying with a CertRep while keep_alive is still 1 becomes assertable.

The seven open threads from 4 Sep still stand on their own; this is only about ordering.

[yosuke-wolfssl]

Thank you so much @Frauschi for reviewing my PRs.
I reworked and replied inline comments you posted.
Regarding to RFC conformance, I've conceded the messageType case and kept the other two, on a line taken from the RFC rather than from taste:

• A message missing a mandatory attribute is not a pkiMessage. Section 3.2.1 requires transactionID, messageType and a fresh senderNonce in every message, so mt == NULL is now folded into scep: require transactionID and senderNonce before dispatch #22's guard and answers the same HTTP 400 as a missing transactionID or senderNonce. Your point stands and the separate branch is gone.
• A message we understood but cannot act on gets a CertRep. Section 3.2.1.4 defines badAlg as "Unrecognised or unsupported algorithm" and section 3.3.2.2 makes failInfo the vehicle for it; an HTTP 400 carries no failInfo, so answering that way makes badAlg unreachable in practice. For an unrecognized messageType the message satisfies every 3.2.1 MUST — it just names a transaction we do not implement, which is badRequest verbatim ("Transaction not permitted or supported").

On conformance: since #22 landed with its guard above the messageType check, both remaining branches run with a validated transactionID and senderNonce, so every CertRep they emit carries a recipientNonce. That was the strongest version of your objection and it is now structurally impossible.

On cost, you are right and I have not fixed it: a malformed POST that has already passed CMS signature verification now buys a CA signature. It is bounded — the peer must produce a validly signed CMS first — but the asymmetry is real. If you would rather narrow it further, the de-envelop branch is the one worth keeping and I would drop the unrecognized-messageType conversion too; please say the word.

f-8033 recommended converting all three; the PR body records that one was declined and why.

[Frauschi]

The rework does what it says. I rebuilt at 76a0273b (rebases clean on 7c256ed): CMake build clean, 27/27 ctest, ASan+UBSan clean, leaks reports 0.

Rather than take the negative controls on trust I re-ran them as mutations against test_scep_roundtrip. Five of seven are caught, and the two that are not are the same one:

Mutation	Result
de-envelop branch back to send_text() 400	caught
unrecognized-messageType branch back to 400	caught
failInfo forced to "2"	caught
mt removed from the mandatory-attribute guard	caught
reply senderNonce frozen	caught
s->keep_alive = 0 deleted from the de-envelop branch	not caught, 27/27 green
s->keep_alive = 0 deleted from the unrecognized-messageType branch	not caught, 27/27 green

Details in the thread on scep_server.c:656. The badAlg derivation checks out - wolfcert_map_wc_err() (src/internal.c:355) routes ALGO_ID_E and NOT_COMPILED_IN to WOLFCERT_ERR_UNSUPPORTED, so reading failInfo straight off rc is sound and needs nothing propagated out of wolfcert_scep_deenvelop().

Conceding the mt == NULL branch was the right call and the guard is the right place for it. One new thing on that line, below. I've resolved the threads that are simply done and replied only where something is still open.

[Frauschi]

This checks mt == NULL where its three siblings on the same line check NULL or zero length. A messageType attribute that is present but carries a zero-length value parses to mt = "": scep_msg.c:861 allocates vlen + 1 and writes s[0] = '\0', so the pointer is non-NULL. It passes the guard, matches none of the strcmps, and lands on the unrecognized-messageType CertRep.

Probed against this head, adding a round to check_required_attrs with .message_type = "" and everything else present:

PROBE empty-messageType: status=200        <- signed CertRep, pkiStatus 2

A zero-length transactionID or senderNonce gets the 400, so messageType is the odd one out, and the line the PR draws ("a message omitting one of the three mandatory attributes is not a pkiMessage") reads as if it should cover this too - an attribute with no value is functionally an absent one.

    if (tid == NULL || tid_len == 0 || snonce == NULL || snonce_len == 0 ||
            mt == NULL || mt[0] == '\0') {

[yosuke-wolfssl]

Good catch, and reproduced before fixing: a zero-length messageType parses to "" via the vlen + 1 allocation at scep_msg.c:862, so it passed the guard and came back as a 200 CertRep. Fixed as you wrote it:

    if (tid == NULL || tid_len == 0 || snonce == NULL || snonce_len == 0 ||
            mt == NULL || mt[0] == '\0') {

All four attributes are now treated the same way, which is what the PR body already claimed. check_required_attrs gains a zero-length-messageType round alongside the absent one, and reverting the guard to mt == NULL alone fails it.

[yosuke-wolfssl]

Thanks @Frauschi ,
I reworked and posted replies in each thread.
Could you review this again ?

[Frauschi]

Approving.

All three threads from the last round are closed, and I checked each rather than reading it:

• empty messageType: reverting the guard to mt == NULL alone fails with required-attrs round 5 (status 200), so the new round genuinely pins it.
• keep_alive: the comment and the reworded body are exactly what was missing. Taking the raw_http_req() header assertion as the follow-up, along with the pre-existing 400/500 siblings.
• SKIP notice: forced both guards to #if 0 and rebuilt - no warnings, the line prints, the binary still passes.

27/27 ctest locally plus an ASan+UBSan build of test_scep_roundtrip, clean. CI green across all 19 jobs.

Two non-blocking leftovers, neither worth another round:

• The embedded-NUL messageType, in the thread on scep_server.c:661 - a scoping note for the PrintableString follow-up, not new work here.
• The commit message says check_required_attrs "gains a round for a message carrying no messageType"; it now gains two, absent and zero-length. Not holding the merge for it.

Nits for whenever you are next in the file: the else if (i == 4 || i == 5) body is byte-identical to the control else body, and the loop header comment still says each round omits a required attribute though rounds 1, 3 and 5 send empty values instead.