Repository navigation
Refactor EST and SCEP protocol engine - #62
Conversation
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #62
Scan targets checked: wolfcert-src, wolfcert-bugs
Not reviewed (no scan target covers them): AGENTS.md, README.md, docs/ARCHITECTURE.md, docs/MIGRATING-FROM-WOLFSCEP.md, examples/enroll_cryptocb.c, examples/enroll_est.c, examples/enroll_scep.c, src/client.c, src/errors.c, src/http.c, src/internal.h, tests/integration/est_client_cases.h, zephyr/tests/wolfcert_est/src/main.c
Coverage: 15 of 22 in-scope changed file(s) opened by the reviewer; not opened: tests/integration/test_est_mldsa_roundtrip.c, tests/integration/test_est_pending_roundtrip.c, tests/integration/test_est_roundtrip.c, tests/integration/test_est_tls_roundtrip.c, tests/integration/test_scep_get_cert.c, tests/integration/test_scep_poll_roundtrip.c, wolfcert/types.h
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.
Review tier: Lite
e6ad4b0 to
808cd8e
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #62
Scan targets checked: wolfcert-src, wolfcert-bugs
Not reviewed (no scan target covers them): AGENTS.md, README.md, docs/ARCHITECTURE.md, docs/MIGRATING-FROM-WOLFSCEP.md, examples/enroll_cryptocb.c, examples/enroll_est.c, examples/enroll_scep.c, src/client.c, src/errors.c, src/http.c, src/internal.h, tests/integration/est_client_cases.h, zephyr/tests/wolfcert_est/src/main.c
Coverage: 2 of 3 in-scope changed file(s) opened by the reviewer; not opened: wolfcert/scep.h
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
Thanks, folding the one-shots onto the session path removes a lot of drift. A few things below, mostly around error-path contracts that changed along the way: the csrattrs output no longer being cleared on failure, the empty 405/501 csrattrs case, and the PHA flag now reaching the one-shots.
On your two questions: the WolfCertEncoding argument works for me, and it lines up with wolfcert_client_get_ca. Could wolfcert_scep_get_next_ca_cert (and its session forms) take it too? It's still PEM-only while its input and ca_bundle want DER. An unknown encoding value should probably return WOLFCERT_ERR_BAD_ARG rather than fall back to PEM.
Dropping wolfcert_http_request() in a follow-up is fine with me, as long as it lands before the next release so integrators get one API break rather than two. The HTTP and transport tests use it heavily, so they'd need a small helper, and ARCHITECTURE.md still mentions it.
- est_finish() parses every EST response; 401/403 returns WOLFCERT_ERR_AUTH from every operation, and csrattrs reads an empty body as no attributes only on 200, 204, 404, 405 or 501; est.h, types.h and ARCHITECTURE.md say so. - est_session_call() runs every session call; est_one_shot() opens a session, makes one call and closes it for every one-shot. - est_enroll_finish(), fill_common(), post_enroll_ex(), est_result_flatten() and est_check_cfg() are removed. - New session calls: get_cacerts_enc and get_csr_attrs with _nb forms, and simple_reenroll_ex with an _nb_ex form. - One-shots read proto_opts.est.allow_post_handshake_auth too; types.h no longer limits it to the session API. - test_est_pha_roundtrip reenrolls over PHA through the session call in place of a raw TLS client. - wolfcert_http_session_set_close() makes an HTTP session send Connection: close; wolfcert_http_request() and est_one_shot() call it. - est_session_free_request() frees the request URL and body once the HTTP call succeeds, before est_finish() parses the response. - types.h: allow_post_handshake_auth applies to every EST call. - A session reenroll without a client identity returns WOLFCERT_ERR_BAD_ARG before it sends. - wolfcert_est_session_open() hands its URL to the HTTP session, and http.h says a base_url path is ignored.
…calls - The EST and SCEP enroll calls returning a WolfCertBuffer are removed; the result-returning calls take the plain names, and EST's _nb_ex calls become _nb. - wolfcert_client_enroll() and wolfcert_client_reenroll() map a PENDING result to WOLFCERT_ERR_PENDING, as client.h now states. - The CLI, examples, headers, docs and tests use the result form. - test_est_mtls_roundtrip: a matching renewal under approval is PENDING, then issued on its re-POST, and wolfcert_client_reenroll() returns WOLFCERT_ERR_PENDING. - errors.h and wolfcert_strerror() describe WOLFCERT_ERR_PENDING as a pending EST enrollment from the client calls.
- wolfcert_est_session_open_async() is removed; any session takes blocking and _nb calls, and a blocking call while a request is in flight gets WOLFCERT_ERR_BAD_ARG. - est_call_clear() zeroes an enroll result or a buffer output only when a call starts a request, and est.h says a resume passes it back unfreed; est_finish() clears an empty csrattrs result. - wolfcert_est_get_cacerts_enc() moves to est.h, and wolfcert_est_simple_reenroll() rejects an empty current_cert. - wolfcert_client_fetch_meta() calls wolfcert_client_auto_csrattrs().
- scep_session_call() runs every session call from a ScepCall, and scep_one_shot() every one-shot; run_pki_op(), do_scep_round_trip() and fill_common() are removed. - scep_session_begin_pki_op() builds every PKIOperation; scep_prepare() and scep_finish() read the request state from the session. - New session calls with _nb forms: get_ca_caps, get_ca_cert_enc, get_next_ca_cert and get_cert. Every call accepts a NULL caps. - wolfcert_scep_session_open_async() and the mode checks are removed; a blocking call while a request is in flight gets WOLFCERT_ERR_BAD_ARG, on EST and SCEP sessions alike. - A non-PrintableString GetCertInitial transactionID gets WOLFCERT_ERR_BAD_ARG before the dial. - scep_one_shot() sends Connection: close. - scep_session_free_request() frees the request URL and pkiMessage once the HTTP call succeeds, before scep_session_finish(). - scep_call_clear() also clears a buffer output. - scep_one_shot() builds the request before it connects: session open is split into scep_session_new() and scep_session_connect(), and the send step into scep_session_send(). - The SCEP session hands its URL to the HTTP session, and wolfcert_http_url_origin() is removed. - A blocking SCEP call uses the caller's CA bundle and signer cert; an _nb request keeps owned copies. - rsa_key_to_der() allocates the exact DER size, and scep_session_begin_pki_op() converts the key just before it signs the pkiMessage.
- The EST cacerts and SCEP GetCACert and GetNextCACert one-shot and session calls take a WolfCertEncoding; the _enc names and PEM-only forms are removed. An unknown value returns WOLFCERT_ERR_BAD_ARG. - est_call_check() holds the EST argument check, run before the dial and when a session call starts a request. Both sessions keep the encoding of the request in flight and refuse a resume with another. - est.h, scep.h, AGENTS.md, README.md, ARCHITECTURE.md and MIGRATING-FROM-WOLFSCEP.md describe blocking and _nb calls on one session type. - test_est and test_scep_async_roundtrip drop cases another test covers.
808cd8e to
311ade8
Compare
|
Thanks for the review. On the two answers:
|
Background
After #59, EST and SCEP still built and parsed each request twice, once for one-shots and once for sessions, and the copies had drifted: a session
/cacertsignored DER,/cacertsand/csrattrsreturnedERR_HTTPon 401/403, and an empty 500 on/csrattrsread as "no attributes". There were two session types (_open/_open_async), sessions were missing several operations, and each enroll and get-CA call existed twice (buffer /_ex, PEM /_enc).Fix (
src/est/est_client.c,src/scep/scep_client.c)est_session_call()/scep_session_call()build, send and parse every request; the public blocking and_nbcalls are one-line wrappers.Connection: close. A SCEP one-shot builds and signs its request before it connects._nbcalls; an_nbresume must match the request in flight (operation, output, encoding).*_session_open_asyncremoved*_session_open_excalls take their nameswolfcert_client_*returnWOLFCERT_ERR_PENDING_enccalls folded into aWolfCertEncodingargument, GetNextCACert includedWOLFCERT_ENCODING_PEMfor the old behaviour; an unknown value returnsBAD_ARGWOLFCERT_ERR_AUTHfrom every EST call; an empty/csrattrsmeans no attributes on 200, 204, 404, 405 or 501ERR_HTTP, and an empty 500 read as OKallow_post_handshake_authWOLFCERT_ERR_UNSUPPORTEDwithout PHA in wolfSSLVerification
-Werror.make checkand theno-des3build, and builds and runs the Zephyrqemu_x86suites.Not in this PR
wolfcert_http_request()from the public API, in a follow-up PR before the next release; upgrade notes go with that release.ARCHITECTURE.mdsection 4.3 has a SCEP sample for now.