Add OpenSSH certificate user authentication#1060
Conversation
2576e03 to
1c40cc1
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1060
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
1c40cc1 to
c00d111
Compare
aidangarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 4 total — 4 posted, 0 skipped
Posted findings
- [High] RSA certificate guards assume rsa-sha2-256 is always available —
src/internal.c:15861-15876 - [Medium] OpenSSH RSA certificate CA signatures bypass SHA-1 soft-disable policy —
src/ossh.c:1013-1025 - [Medium] OSSH certificate API test assumes RSA and ECDSA are enabled —
tests/api.c:1579-1598 - [Medium] New sshd OSSH cert test cannot be selected or excluded —
apps/wolfsshd/test/run_all_sshd_tests.sh:5-14
Review generated by Skoll.
| keySig->sigId = matchId; | ||
| keySig->sigName = IdToName(matchId); | ||
| keySig->sigNameSz = (word32)WSTRLEN(keySig->sigName); | ||
| #ifdef WOLFSSH_OSSH_CERTS |
There was a problem hiding this comment.
🟠 [High] RSA certificate guards assume rsa-sha2-256 is always available
🚫 BLOCK bug
The new OpenSSH RSA certificate path can reference cannedKeyAlgoOsshRsaSha2_256CertName even when WOLFSSH_NO_RSA_SHA2_256 is defined. internal.h allows WOLFSSH_NO_RSA_SHA2_256 to be set by NO_SHA256 while rsa-sha2-512 remains available, so an --enable-ossh-certs build with SHA-256 disabled and SHA-512 enabled fails to compile on this fallback. The same guard mismatch also advertises/maps rsa-sha2-512-cert-v01@openssh.com while parts of the cert handling gate ID_OSSH_CERT_RSA on SHA2-256, leaving SHA512-only builds inconsistent.
Recommendation: Use one consistent RSA-cert availability guard based on RSA plus at least one RSA SHA2 signature algorithm. Only reference the SHA2-256 cert name under #ifndef WOLFSSH_NO_RSA_SHA2_256, choose SHA2-512 when SHA2-256 is disabled, and apply the same guard to cannedKeyAlgoNames, NameIdMap, cannedKeyAlgoClient, OsshCertBaseId, and OsshRsaCertSigId. Add a compile configuration covering WOLFSSH_OSSH_CERTS with WOLFSSH_NO_RSA_SHA2_256 and SHA2-512 still enabled.
| &idx); | ||
| } | ||
|
|
||
| /* select hash from the signature algorithm name */ |
There was a problem hiding this comment.
🟡 [Medium] OpenSSH RSA certificate CA signatures bypass SHA-1 soft-disable policy
💡 SUGGEST
The PR introduces OpenSSH certificate verification and accepts an RSA CA certificate signature whose signature algorithm string is ssh-rsa, mapping it to WC_HASH_TYPE_SHA. Data Flow: remote SSH userauth public-key packet -> certificate blob parsed by DoUserAuthRequestPublicKey -> OsshCertParse extracts the attacker-supplied signature algorithm from the certificate signature field -> OsshCertVerifySignature dispatches to OsshVerifyRsa -> SHA-1 RSA verification is accepted -> the daemon later trusts the embedded CA key via TrustedUserCAKeys. This is inconsistent with the existing user-auth algorithm list, where plain ID_SSH_RSA is only advertised when WOLFSSH_NO_SHA1_SOFT_DISABLE is defined. As a result, builds that otherwise soft-disable SHA-1 RSA can still authenticate OpenSSH certificates signed by a trusted RSA CA using SHA-1. A malicious or compromised certificate issuer, or an attacker able to exploit SHA-1 collision weaknesses in a CA-signing workflow, gets a weaker authentication path than the build policy intends. This issue is introduced by the PR's new RSA OpenSSH certificate verifier; no existing test asserts that SHA-1 OSSH CA signatures are rejected under the soft-disable configuration.
Recommendation: Apply the same SHA-1 soft-disable policy used by normal public-key authentication, or reject ssh-rsa CA signatures entirely by default. For example gate the ssh-rsa branch under #ifdef WOLFSSH_NO_SHA1_SOFT_DISABLE. Also add a negative OpenSSH certificate test that signs an RSA-CA certificate with ssh-rsa and verifies rejection when SHA-1 is soft-disabled.
| /* Parse, verify the CA signature, and validate the options of each committed | ||
| * certificate vector; then flip the final signature byte and confirm the | ||
| * verification fails while the parse still succeeds. */ | ||
| static void test_wolfSSH_OsshCert_valid(void) |
There was a problem hiding this comment.
🟡 [Medium] OSSH certificate API test assumes RSA and ECDSA are enabled
💡 SUGGEST test
The new certificate verification test is guarded only by WOLFSSH_OSSH_CERTS and !WOLFSSH_NO_ED25519, but it always verifies vectors signed by Ed25519, RSA, and ECDSA CAs. Builds that enable OSSH certs and Ed25519 while disabling RSA or ECDSA will compile out the corresponding verifier and then fail this test, even though that algorithm-disabled configuration is otherwise valid.
Recommendation: Build the vector list conditionally: include the Ed25519 CA vector under Ed25519, the RSA CA vector under RSA support, and the ECDSA CA vector under ECDSA plus the needed curve support. Apply the same per-algorithm guards to the malformed verifier caTypes list.
| @@ -176,6 +176,14 @@ else | |||
| printf "Shutting down test wolfSSHd\n" | |||
There was a problem hiding this comment.
🟡 [Medium] New sshd OSSH cert test cannot be selected or excluded
💡 SUGGEST test
sshd_ossh_cert_test.sh is invoked manually at the end of the local-host path, but it is not added to the test_cases array used by --match validation and --exclude. As a result, --match sshd_ossh_cert_test.sh is rejected as unknown, and --exclude sshd_ossh_cert_test.sh does not suppress the new test.
Recommendation: Add sshd_ossh_cert_test.sh to the selectable test list, or add a separate extra-test list that participates in --match and --exclude validation before running the self-contained local-host test.
Add OpenSSH certificate user authentication
Adds support for authenticating users with OpenSSH certificates
(
*-cert-v01@openssh.com) in wolfSSHd, behind a new--enable-ossh-certsconfigure option (also enabled by
--enable-all).Motivation
OpenSSH certificates let an operator authorize users by trusting a single CA
(via
TrustedUserCAKeys) instead of distributing every user's public key intoauthorized_keys. This brings wolfSSHd in line with stock OpenSSHsshdforcertificate-based user auth.
What's implemented (Unix)
Library (
src/ossh.c,src/internal.c) — portable, no platform deps:certificate (RSA incl.
rsa-sha2-256/512, ECDSA P-256/P-384/P-521, Ed25519).force-commandandsource-address; tolerate unknown extensions.against it (proving possession of the certified private key), using the
on-the-wire signed length so it works with the reconstructed key.
caKey,principals, validity,source-address,force-command) are handed to the user-auth callback for enforcement.Daemon (
apps/wolfsshd/auth.c,wolfsshd.c) —CheckPublicKeyUnixenforces,fail-closed and in order:
TrustedUserCAKeys.principal-less certificate is rejected, matching OpenSSH
sshd).valid_after/valid_before.force-commandoverrides therequested command for shell, exec, SFTP, and SCP sessions
(
internal-sftpstill permits SFTP).Security model
its embedded CA) and that the client holds the certified private key. It
does not decide trust — that is the application callback's responsibility
(check
caKeyagainst a trust store, bind the principal, enforcevalidity/source-address). This mirrors how regular public-key auth delegates
the
authorized_keyscheck.Configuration
./configure --enable-ossh-certs --enable-sshd # or --enable-allPlatform support
on the Windows threaded path. Follow-up work is planned in two PRs
(per-connection cert-state relocation, then Windows enforcement + a Windows CI
cert-auth job).
Testing
tests/api.c— certificate parse, CA-signature verify, andcritical-option/extension handling (self-contained binary vectors).
tests/unit.c— Ed25519 public-key parse and the user-signaturewire-length path (wire-derived length equals the legacy field-sum).
apps/wolfsshd/test/test_configuration.c— direct unit tests for theenforcement helpers (principal binding incl. empty-list rejection, validity
window, source-address CIDR/prefix matching).
apps/wolfsshd/test/sshd_ossh_cert_test.sh— end-to-end against both thewolfSSH example client and the system OpenSSH client: valid cert (Ed25519/RSA/
ECDSA CAs and user keys), untrusted CA, wrong principal, empty principal,
expired cert, unknown critical option, source-address match/deny,
force-command,
internal-sftp, and SFTP/SCP force-command gating.--enable-allcompiles the option in; the e2e runs viarun_all_sshd_tests.sh).Known limitations
!entry) is not implemented; a negated entryfails the whole list closed (denies) and logs the offending entry.
wolfSSH_CTX_UseOsshCert_bufferreturns
WS_UNIMPLEMENTED_E.Notes for reviewers
WS_UserAuthData_PublicKeyare appended after theexisting members so the default (
WOLFSSH_OSSH_CERTSdisabled) struct layoutis unchanged; enabling the option changes the layout, so the app and library
must be built with the same setting.
src/ossh.cis added to the Windows MSVC project (wolfssh.vcxproj) becausethe OpenSSH key decoders it defines are referenced unconditionally.