Load every PEM block when adding root CAs, not just the first - #1149
Load every PEM block when adding root CAs, not just the first#1149yosuke-wolfssl wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR fixes CA bundle loading so wolfSSH_CTX_AddRootCert_*() processes all PEM certificate blocks in a buffer/file instead of only the first, and adds tests covering multi-block and malformed-bundle behavior.
Changes:
- Add
LoadRootCaPemBuffer()to iterate through PEM blocks and load each CA into the cert manager. - Update
wolfSSH_ProcessBuffer()to use the bundle-walking loader for PEM CA buffers. - Add an API test to validate multi-CA bundles, malformed blocks, and interstitial text behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
wolfssh/ssh.h |
Clarifies API behavior for multi-PEM inputs (leaf-only vs CA bundle loading). |
tests/api.c |
Adds bundle-focused tests and a helper to build multi-block PEM buffers in memory. |
src/internal.c |
Implements PEM bundle walking for CA loading and wires it into PEM CA processing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
48d0971 to
b114e26
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1149
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 5
5 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
b114e26 to
60b08ba
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1149
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
60b08ba to
91eb466
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1149
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 9
9 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
91eb466 to
03c09ff
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1149
Scan targets checked: wolfssh-bugs, wolfssh-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.
2f905c6 to
5c11e36
Compare
|
Hi @ejohnstown , |
- wolfSSH_ProcessBuffer() hands a PEM BUFTYPE_CA buffer to a new LoadRootCaPemBuffer(), which walks every block, decodes each with wc_PemToDer(), and gives the DER to wolfSSH_CERTMAN_LoadRootCA_buffer(). - A block that will not decode, or that the cert manager refuses, is logged and skipped. The call returns WS_PARSE_E when no block loaded, and WS_BAD_FILE_E when the buffer holds no block at all. - wc_PemGetHeaderFooter() supplies the CERTIFICATE and TRUSTED CERTIFICATE headers, and a new FindInBuffer() seeks the next one without stopping at an embedded NUL. - LoadRootCaPemBuffer() takes its EncryptedInfo from the heap under WOLFSSH_SMALL_STACK. - internal.h defines WOLFSSH_HAVE_TRUSTED_CERT_PEM for wolfSSL 5.8.0 and later, along with the WOLFSSL_V5_8_0 constant it tests. - SniffCertForm() reports the trusted certificate form through a new out parameter. UseCertFile() takes that form for BUFTYPE_CA and answers WS_BAD_FILETYPE_E otherwise, as does wolfSSH_ReadCert_buffer(). - ssh.h records what each buffer entry point reads and when it fails. - tests/api.c adds test_wolfSSH_CTX_AddRootCert_bundle(), test_wolfSSH_CTX_AddRootCert_file_trusted(), and test_wolfSSH_ReadCert_buffer_trusted().
5c11e36 to
3957f62
Compare
Problem
The certificate readers added in #1140 call
wc_CertPemToDer()once, whichdecodes the first PEM block and stops. For a host certificate that is correct.
For
wolfSSH_CTX_AddRootCert_file()/_buffer()it is a trap: a CA bundleinstalls exactly one CA and still returns
WS_SUCCESS, so trust the callerbelieves is installed is not. Reachable today —
examples/echoserverhands awhole
-a caCertfile to the buffer form, as does wolfsshd'sUserCAKeysFile.Separately,
AddRootCert_file()rejected CA files written in OpenSSL'strusted-certificate form, which the buffer entry point could already read.
Fix (
src/internal.c,src/ssh.c)LoadRootCaPemBuffer()walks every block in a PEM CA buffer:wc_PemGetHeaderFooter()supplies theCERTIFICATEandTRUSTED CERTIFICATEheaders, so text between blocks is stepped over and whicheverheader leads types the block.
wc_PemToDer()decodes each block;info.consumedgives its length, thesame mechanism wolfSSL's own chain loader uses.
and skipped, matching
ProcessChainBuffer(). The call fails only when thebuffer leaves no CA installed.
SniffCertForm()now names the trusted form, soAddRootCert_file()takes it.
UseCert_*andReadCert_*still decline it: the trust settingsit carries mean nothing in a certificate presented to a peer.
Behavior change
WS_SUCCESS, CA never installedWS_SUCCESS[ca, undecodable]WS_SUCCESS, block unreadWS_SUCCESS, block logged[undecodable, ca]WS_PARSE_E, nothing installedWS_SUCCESSWS_PARSE_EWS_PARSE_EWS_PARSE_EWS_BAD_FILE_EWS_BAD_FILETYPE_EWS_SUCCESSNo signatures change;
UseCert_*,ReadCert_*and the DER paths are untouched.Tests
test_wolfSSH_CTX_AddRootCert_bundle(),test_wolfSSH_CTX_AddRootCert_file_trusted()andtest_wolfSSH_ReadCert_buffer_trusted()intests/api.ccover multi-blockbundles, a bad block leading and trailing, interstitial text, an embedded NUL,
the trusted form through both entry points, and the all-fail / no-block error
codes. Accept cases assert the CA verifies a leaf, not just the return code.
Verification
api.testpasses; clean under-Werroracross 6 GCC configurations,including
--enable-smallstack.-DWOLFSSH_SMALL_STACKrun so the heap pathis the one exercised.
trusted detection removed the file test fails.
openssl x509 -trustout -addtrustCA (certificate plus 24 bytes oftrust settings) loads and verifies a leaf.