Use of Windows certificate store for authentication#900
Use of Windows certificate store for authentication#900JacobBarthelmeh wants to merge 14 commits into
Conversation
bd1396d to
ca912b0
Compare
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
66c8726 to
b545c32
Compare
There was a problem hiding this comment.
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.
b545c32 to
de28863
Compare
de28863 to
6b8a1ca
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
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.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
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.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
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.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
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.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #900
Scan targets checked: wolfssh-src
Failed targets: wolfssh-bugs
aidangarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 5 total — 5 posted, 0 skipped
Posted findings
- [High] Store-backed host keys never negotiate RFC6187 certificates —
src/ssh.c:2896-3012 - [High] Imported cert manager can dangle when up_ref fails —
src/certman.c:100-115 - [High] System CA store can become a Windows SSH user-auth trust root without identity binding —
apps/wolfsshd/wolfsshd.c:567-607 - [High] CA-store config directives are accepted but ignored when unsupported —
apps/wolfsshd/wolfsshd.c:618-627 - [Medium] Core cert-store parsing and state transitions lack unit coverage —
src/certman.c:630-717
Review generated by Skoll.
…rname, build check for WOLFSSL_SYS_CA_CERTS, fix for CM ref count
…cc curve, tie in of loading whole cert store for sys CA's
| if (WSTRCMP(flagsStr, "CURRENT_USER") == 0) { | ||
| *dwFlags = CERT_SYSTEM_STORE_CURRENT_USER; | ||
| } | ||
| else if (WSTRCMP(flagsStr, "LOCAL_MACHINE") == 0) { | ||
| *dwFlags = CERT_SYSTEM_STORE_LOCAL_MACHINE; | ||
| } | ||
| else { | ||
| *dwFlags = (word32)atoi(flagsStr); | ||
| } |
| } else if (WSTRCMP(hostKeyStoreFlags, "LOCAL_MACHINE") == 0) { | ||
| dwFlags = CERT_SYSTEM_STORE_LOCAL_MACHINE; | ||
| } else { | ||
| dwFlags = (word32)atoi(hostKeyStoreFlags); | ||
| } |
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #900
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
| } | ||
|
|
||
| #ifdef WOLFSSL_FPKI | ||
| #if defined(WOLFSSL_FPKI) || defined(_WIN32) |
There was a problem hiding this comment.
🔵 [Low] Outer _WIN32 guard vs inner WIN32 guard mismatch in cert auth path · Incorrect bitwise vs logical operators
The outer block is guarded by defined(_WIN32) (line 1543) but the SetupUserTokenWin call inside is guarded by #ifdef WIN32 (line 1642). On a Windows toolchain where _WIN32 is defined but WIN32 is not, the CN check succeeds yet the token-setup branch is skipped, causing cert auth to fall to the unconditional rejection path.
Fix: Change the outer guard to use WIN32 for consistency with the existing inner guard, or change the inner guard to _WIN32.
No description provided.