Use of Windows certificate store for authentication by JacobBarthelmeh · Pull Request #900 · wolfSSL/wolfssh

JacobBarthelmeh · 2026-03-25T12:38:01Z

No description provided.

Copilot

Pull request overview

This PR adds Windows Certificate Store integration to wolfSSH so host keys and client authentication keys can be sourced from the Windows cert store (including CI coverage on Windows).

Changes:

Add a Windows-only API to load a private key by locating a certificate in the Windows Certificate Store, and use CNG to sign during SSH handshakes/auth.
Extend cert manager plumbing and wolfsshd configuration to support system/user CA loading and cert-store-based host keys.
Update Windows build projects and add a GitHub Actions workflow to exercise file-vs-store interop permutations.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
wolfssh/test.h	Prefer wolfCrypt Base16 when available; otherwise keep local Base16 decode helper.
wolfssh/ssh.h	Add `wolfSSH_CTX_UsePrivateKey_fromStore()` Windows-only public API.
wolfssh/internal.h	Add CTX private-key metadata for cert-store backed keys and internal helper prototypes.
wolfssh/certman.h	Expose cert-manager setter and Windows cert-store spec parser API.
src/ssh.c	Implement loading a CTX private key from the Windows Certificate Store.
src/internal.c	Add cert-store signing path (CNG) and cert-derived RSA public-key extraction for KEX/auth flows.
src/certman.c	Implement `wolfSSH_SetCertManager()` and `wolfSSH_ParseCertStoreSpec()`.
ide/winvs/wolfsshd/wolfsshd.vcxproj	Link against `crypt32`/`ncrypt` for cert-store features.
ide/winvs/wolfssh/wolfssh.vcxproj	Link against `crypt32`/`ncrypt` for cert-store features.
ide/winvs/wolfsftp-client/wolfsftp-client.vcxproj	Link against `crypt32`/`ncrypt` for cert-store features.
ide/winvs/unit-test/unit-test.vcxproj	Link against `crypt32`/`ncrypt` for cert-store features; normalize XML header.
ide/winvs/echoserver/echoserver.vcxproj	Link against `crypt32`/`ncrypt` for cert-store features.
ide/winvs/client/client.vcxproj	Link against `crypt32`/`ncrypt` for cert-store features.
ide/winvs/api-test/api-test.vcxproj	Link against `crypt32`/`ncrypt` for cert-store features; normalize XML header.
examples/sftpclient/sftpclient.c	Add `-W store:subject:flags` support for client key from Windows cert store.
examples/echoserver/echoserver.c	Add `-W` support for server host key from Windows cert store; skip key-file root search when using store.
examples/client/common.h	Declare helper functions for cert-store key loading/auth setup.
examples/client/common.c	Implement cert-store key loading wrapper + auth globals setup for x509v3 publickey auth.
apps/wolfsshd/wolfsshd.c	Add host-key-from-store support and optional system/user CA store loading into wolfSSH cert manager.
apps/wolfsshd/configuration.h	Add config getters for host-key store and Windows user-CA store options.
apps/wolfsshd/configuration.c	Add parsing/storage for new config directives and defaults.
.github/workflows/windows-cert-store-test.yml	Add Windows CI workflow to validate store/file combinations.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 21 out of 22 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 22 out of 23 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #900

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.

Copilot

Pull request overview

Copilot reviewed 22 out of 23 changed files in this pull request and generated 8 comments.

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #900

Scan targets checked: wolfssh-bugs, wolfssh-src

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

This review was generated automatically by Fenrir. Findings are non-blocking.

…t store.

add Windows cert store test case make windows cert feature default disabled and simplify macro guard additional unit tests, advertise x509 and pubkey, use CN to match username, build check for WOLFSSL_SYS_CA_CERTS, fix for CM ref count additional build test, uniform enum name, fail on unkown cert store ecc curve, tie in of loading whole cert store for sys CA's

…nged default from MY to required to be set enable SHA1 with windows cert store test case expand test cases, adjust to authorized key file, minor dead code adjustments add more documentation, refactor duplicate code sections, clean up test cases, more adjustments to logging spamming protections

Copilot

🔵 Needs a closer look

It introduces and wires up a new Windows credential-loading path across core auth, daemon config, examples, and build systems, which is high-impact and warrants final human review despite only one concrete nit found here.

Review details

Files reviewed: 36/36 changed files
Comments generated: 1
Review effort level: Lite

…yStoreSubject, HostKeyStoreFlags

aidangarske

Skoll Multi-Scan Review

Modes: review + review-security

Overall recommendation: REQUEST_CHANGES
Findings: 5 total — 5 posted, 0 skipped
5 finding(s) posted as inline comments (see file-level comments below)

Posted findings

[High] [review-security] Default Windows builds accept certificates restricted to non-SSH purposes — apps/wolfsshd/wolfsshd.c:696-702
[Medium] [review] Parse -W as an option before skipping the root search — examples/echoserver/echoserver.c:3178-3182
[Medium] [review] Exercise the non-FPKI Windows CN-binding path — apps/wolfsshd/auth.c:2836-2856
[Medium] [review] Do not turn arbitrary certificate-parser failures into skips — tests/unit.c:18409-18416
[Medium] [review] Distinguish certificate-store enumeration failure from end of store — apps/wolfsshd/wolfsshd.c:739-744

Review generated by Skoll

aidangarske · 2026-09-02T22:10:30Z

+            "[SSHD] LOCAL_MACHINE is the safer location for trust anchors.");
+    }
+
+#ifdef WOLFSSH_NO_FPKI


Default Windows builds accept certificates restricted to non-SSH purposes · Security

BEFORE this PR, wolfsshd had no Windows user-CA-store trust-source path. AFTER, LoadUserCACertsFromStore() imports CA certificates from the configured store and explicitly permits authentication without FPKI, merely warning that TLS-server, S/MIME, or code-signing certificates may be accepted. An attacker-supplied chain reaches wolfSSH_CERTMAN_VerifyCerts_buffer(), while the WOLFSSH_NO_FPKI path omits application-purpose, EKU, and key-usage enforcement. The new Windows authentication branch then checks only that the leaf subject CN matches the requested username before creating the user's token. Consequently, anyone holding a valid certificate and private key issued by a configured CA can authenticate as the matching Windows account even when that certificate is explicitly restricted to serverAuth, S/MIME, code signing, or lacks digitalSignature usage. This affects the supplied Visual Studio configuration, which defines WOLFSSH_NO_FPKI. Existing tests use a certificate without an explicit incompatible EKU and therefore do not cover this bypass (RFC 6187 section 2.2 requires digitalSignature when KeyUsage is present and restricts EKU-bearing certificates to their indicated purposes).

Fix: Enforce SSH-appropriate leaf usage independently of FPKI: require digitalSignature whenever KeyUsage is present and, whenever EKU is present, require id-kp-secureShellClient or an explicitly configured equivalent. Retain FPKI as an additional profile check. Add Windows integration tests proving that valid matching-CN chains containing serverAuth-only or codeSigning-only leaves are rejected.

I believe that this issue is pre-existing in wolfSSH master branch. The fix widens the scope some of this PR (will add more lines to it) but it is a good fix to have for all code paths making use of X509 certs without FPKI enabled.

This wolfSSL PR (wolfSSL/wolfssl#11361) adds some of the EKU logic needed for a cleaner bit check instead of the added manual parsing of the ASN1 syntax used for now. Later wolfSSH will be able to conditionally remove the parsing based on version of wolfSSL linked with.

aidangarske · 2026-09-02T22:10:30Z

+    int i;
+    const char* spec;
+
+    for (i = 1; i < argc; i++) {


Parse -W as an option before skipping the root search · bug

The PR's preliminary scan treats every argument beginning with -W as the certificate-store option. This includes values consumed by preceding options and tokens following --. For example, a valid option value such as -a -WolfCA.pem makes EchoserverUsingCertStore() return true even though getopt will not process a -W option. wolfSSH_Echoserver() consequently skips ChangeToWolfSshRoot(), after which relative certificate and default host-key paths are resolved from an unexpected directory. The SFTP client avoids this ambiguity by using mygetopt() for its preliminary scan.

Fix: Use the same option string as the main parser, scan with mygetopt(), and restore myoptind afterward. Add coverage for an argument value beginning with -W and for --.

Had seen this previously, was trying to avoid having the options set as a macro. It seemed a bit clunky in the examples to do it that way and changing the location of ChangeToWolfSshRoot() teases out a Mac OS / clang bug that leads to a segfault. I'll update the echoserver though to have the macro set for options.

aidangarske · 2026-09-02T22:10:30Z

-                    if (upnRealmUnchecked) {
-                        wolfSSH_Log(WS_LOG_WARN, "[SSHD] AuthorizedUPNDomains "
-                            "not set; certificate UPN domain is not checked");
+                #else


Exercise the non-FPKI Windows CN-binding path · test

The PR adds a distinct authentication path that binds certificates to Windows accounts through the subject CN when WOLFSSL_FPKI is absent. The new Windows workflow enables OPENSSL_ALL, which enables FPKI, and explicitly notes that only the UPN branch is exercised. Consequently, the newly introduced CN extraction, length comparison, and case-insensitive account matching can regress without any functional test detecting it.

Fix: Add a Windows build configuration without OPENSSL_ALL/WOLFSSL_FPKI and test a matching CN, a case-differing matching account name, and a mismatched CN.

Will expand the test coverage.

aidangarske · 2026-09-02T22:10:30Z

+    /* matching curve accepted; a failure here means the chain itself did
+     * not verify under this build's profile, so skip the curve vectors
+     * rather than fail on unrelated policy */
+    if (result == 0) {


Do not turn arbitrary certificate-parser failures into skips · test

The new control vector converts every failure from the matching P-256 ParseECCPubKeyCert() path into a positive skip result. The test harness then ignores all positive results. A regression in the newly changed certificate-chain parser, curve binding, or root verification can therefore make this test print SKIPPED while CI remains green, preventing the subsequent mismatch vector from running as well.

Fix: Skip only for an explicitly identified unsupported build profile or unavailable fixture. Treat unexpected parser and verification errors as test failures, or provide a fixture valid under every profile in which the test is compiled.

Now only WS_CERT_PROFILE_E should skip.

aidangarske · 2026-09-02T22:10:30Z

+        return WS_FATAL_ERROR;
+    }
+
+    /* Passing the previous context frees it and advances the enumeration. */


Distinguish certificate-store enumeration failure from end of store · bug

The PR's new loader treats any NULL result from CertEnumCertificatesInStore() as successful completion. Microsoft's API contract requires checking GetLastError() because NULL represents both normal exhaustion and enumeration errors. If an error occurs after at least one CA was loaded, loaded > 0 makes this function return success and report the partial trust-store load as complete, silently omitting the remaining configured authorities.

Fix: Capture GetLastError() immediately after enumeration returns NULL. Accept only the documented end-of-enumeration statuses such as CRYPT_E_NOT_FOUND and, where applicable, ERROR_NO_MORE_FILES; otherwise fail the load and avoid reporting partial success.

Fixed to check GetLastError()

…tEnumCertificatesInStore, add extended key usage type check if present, fix for EKU checks with X509 use and add test case

JacobBarthelmeh self-assigned this Mar 25, 2026

Copilot AI review requested due to automatic review settings March 25, 2026 12:38

Copilot started reviewing on behalf of JacobBarthelmeh March 25, 2026 12:39 View session

JacobBarthelmeh force-pushed the winSysCerts branch from bd1396d to ca912b0 Compare March 25, 2026 12:43

Copilot AI reviewed Mar 25, 2026

View reviewed changes

Comment thread src/internal.c Outdated

Comment thread src/certman.c Outdated

Comment thread src/certman.c Outdated

Comment thread wolfssh/internal.h Outdated

Comment thread src/ssh.c Outdated

Comment thread src/ssh.c Outdated

Comment thread src/ssh.c Outdated

Comment thread src/internal.c

Copilot AI review requested due to automatic review settings April 14, 2026 03:24

Copilot started reviewing on behalf of JacobBarthelmeh April 14, 2026 03:27 View session

Copilot AI reviewed Apr 14, 2026

View reviewed changes

Comment thread wolfssh/certman.h

Comment thread src/ssh.c Outdated

Comment thread src/ssh.c

Comment thread apps/wolfsshd/wolfsshd.c Outdated

Comment thread apps/wolfsshd/configuration.c

JacobBarthelmeh mentioned this pull request Apr 14, 2026

add windows certificate store documentation wolfSSL/documentation#261

Open

Copilot AI review requested due to automatic review settings April 14, 2026 16:30

JacobBarthelmeh force-pushed the winSysCerts branch from 66c8726 to b545c32 Compare April 14, 2026 16:30

Copilot AI reviewed Apr 14, 2026

View reviewed changes

Copilot started reviewing on behalf of JacobBarthelmeh April 14, 2026 17:09 View session

ejohnstown mentioned this pull request Jun 12, 2026

Add support for loading user CA certs from a configurable Windows cert store. #688

Closed

ejohnstown marked this pull request as draft June 25, 2026 17:23

JacobBarthelmeh force-pushed the winSysCerts branch from b545c32 to de28863 Compare July 2, 2026 05:38

JacobBarthelmeh requested a review from wolfSSL-Fenrir-bot July 2, 2026 05:39

JacobBarthelmeh force-pushed the winSysCerts branch from de28863 to 6b8a1ca Compare July 2, 2026 05:46

wolfSSL-Fenrir-bot reviewed Jul 2, 2026

View reviewed changes

JacobBarthelmeh requested review from Copilot and wolfSSL-Fenrir-bot July 2, 2026 07:21

Copilot started reviewing on behalf of JacobBarthelmeh July 2, 2026 07:22 View session

Copilot AI reviewed Jul 2, 2026

View reviewed changes

Comment thread src/internal.c Outdated

Comment thread src/ssh.c Outdated

Comment thread src/ssh.c

Comment thread wolfssh/internal.h Outdated

Comment thread src/internal.c

Comment thread src/internal.c

Comment thread src/internal.c Outdated

Comment thread src/internal.c Outdated

wolfSSL-Fenrir-bot reviewed Jul 2, 2026

View reviewed changes

JacobBarthelmeh marked this pull request as ready for review July 2, 2026 22:27

JacobBarthelmeh assigned wolfSSL-Bot and unassigned JacobBarthelmeh Jul 2, 2026

JacobBarthelmeh force-pushed the winSysCerts branch from 4d116bd to 7b7aff5 Compare July 6, 2026 22:39

ejohnstown requested changes Jul 6, 2026

View reviewed changes

Comment thread src/internal.c Outdated

Comment thread src/internal.c Outdated

JacobBarthelmeh marked this pull request as draft July 7, 2026 05:11

JacobBarthelmeh marked this pull request as draft August 4, 2026 05:30

JacobBarthelmeh force-pushed the winSysCerts branch from 18ca6b5 to cca116e Compare August 5, 2026 03:46

JacobBarthelmeh assigned JacobBarthelmeh and unassigned aidangarske Aug 5, 2026

JacobBarthelmeh force-pushed the winSysCerts branch 8 times, most recently from 05e4add to e7aa41e Compare August 24, 2026 15:06

JacobBarthelmeh force-pushed the winSysCerts branch from 16ff5e5 to 61985d5 Compare August 27, 2026 01:24

ejohnstown mentioned this pull request Aug 27, 2026

wolfsshd: drain the shell channel before closing the child's stdin #1212

Merged

JacobBarthelmeh force-pushed the winSysCerts branch 2 times, most recently from 8cd9b4b to 52a9f9e Compare September 2, 2026 17:19

JacobBarthelmeh and others added 7 commits September 2, 2026 11:26

add TrustedSystemCAKeys sshd option for system CA load

9b9b287

add macro guard for system ca certs load

d9f44b7

Add support for loading user CA certs from a configurable Windows cer…

4ba856d

…t store.

Prefix wolfSSH specific options with wolfSSH_.

84cd679

use proper free for cert store version, fix echoserver on mac os

263cfa3

JacobBarthelmeh force-pushed the winSysCerts branch from 52a9f9e to 263cfa3 Compare September 2, 2026 17:28

JacobBarthelmeh requested review from wolfSSL-Fenrir-bot and a lite review from Copilot September 2, 2026 17:33

Copilot AI reviewed Sep 2, 2026

View reviewed changes

Comment thread wolfssh/test.h

use wolfSSH_ prefix on wolfSSH specific config's HostKeyStore, HostKe…

f46ac8f

…yStoreSubject, HostKeyStoreFlags

aidangarske requested changes Sep 2, 2026

View reviewed changes

expand CI test for non-fpki build, fix check for error state with Cer…

a0dc870

…tEnumCertificatesInStore, add extended key usage type check if present, fix for EKU checks with X509 use and add test case

Uh oh!

Conversation

JacobBarthelmeh commented Mar 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #900

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #900

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Uh oh!

aidangarske left a comment

Choose a reason for hiding this comment

Skoll Multi-Scan Review

Posted findings

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JacobBarthelmeh Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

JacobBarthelmeh Sep 2, 2026 •

edited

Loading