From e4aa8731216d0493f5f7c8dd172bb54237fa492c Mon Sep 17 00:00:00 2001
From: Sebastian Ott <174621899+VSSOtt@users.noreply.github.com>
Date: Fri, 17 Jul 2026 10:34:30 +0200
Subject: [PATCH 1/8] Support PKCS#11 certificates in Windows ssh-agent
Bring the Windows agent implementation in line with upstream support for
associated-certs-v00@openssh.com.
Match supplied certificates to PKCS#11-backed RSA and ECDSA keys,
persist and reload certificate identities, and support certificate-only
loading. Enable the existing -C option in the Windows ssh-add argument
parser.
Persist certificate identities separately from their underlying keys
and reconstruct their PKCS#11 signing state after an agent restart.
Allow individual certificate deletion while preserving provider-wide
removal behavior.
Keep the existing provider allowlist, remote-provider restrictions, and
encrypted PIN storage intact. Reject malformed, duplicate, truncated,
and oversized certificate constraints.
Add Windows unit and Pester coverage for RSA and ECDSA certificates,
certificate-only loading, unmatched certificates, deletion, provider
removal, and service restart.
---
contrib/win32/openssh/ssh-agent.vcxproj | 2 +
.../openssh/unittest-win32compat.vcxproj | 10 +
contrib/win32/win32compat/pkcs11-cert.c | 157 +++++++
contrib/win32/win32compat/pkcs11-cert.h | 27 ++
.../win32compat/ssh-agent/keyagent-request.c | 389 +++++++++++++-----
regress/pesterTests/KeyUtils.Tests.ps1 | 93 ++++-
regress/pesterTests/README.md | 16 +
.../unittests/win32compat/pkcs11_cert_tests.c | 269 ++++++++++++
regress/unittests/win32compat/tests.c | 1 +
regress/unittests/win32compat/tests.h | 1 +
ssh-add.c | 2 +-
ssh-pkcs11-client.c | 45 ++
12 files changed, 912 insertions(+), 100 deletions(-)
create mode 100644 contrib/win32/win32compat/pkcs11-cert.c
create mode 100644 contrib/win32/win32compat/pkcs11-cert.h
create mode 100644 regress/unittests/win32compat/pkcs11_cert_tests.c
diff --git a/contrib/win32/openssh/ssh-agent.vcxproj b/contrib/win32/openssh/ssh-agent.vcxproj
index cc6e9b5e4813..8a5b1e6e13e3 100644
--- a/contrib/win32/openssh/ssh-agent.vcxproj
+++ b/contrib/win32/openssh/ssh-agent.vcxproj
@@ -416,12 +416,14 @@
+
+
diff --git a/contrib/win32/openssh/unittest-win32compat.vcxproj b/contrib/win32/openssh/unittest-win32compat.vcxproj
index bbc3ed9b73f4..b52de4bfd072 100644
--- a/contrib/win32/openssh/unittest-win32compat.vcxproj
+++ b/contrib/win32/openssh/unittest-win32compat.vcxproj
@@ -36,6 +36,15 @@
+
+ true
+
+
+ true
+
+
+ true
+
true
@@ -65,6 +74,7 @@
+
diff --git a/contrib/win32/win32compat/pkcs11-cert.c b/contrib/win32/win32compat/pkcs11-cert.c
new file mode 100644
index 000000000000..e88aa5eab7f7
--- /dev/null
+++ b/contrib/win32/win32compat/pkcs11-cert.c
@@ -0,0 +1,157 @@
+/*
+ * Copyright (c) 2026 Sebastian Ott. All rights reserved.
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include "includes.h"
+
+#include "authfd.h"
+#include "digest.h"
+#include "log.h"
+#include "sshbuf.h"
+#include "ssherr.h"
+#include "sshkey.h"
+#include "xmalloc.h"
+
+#include "pkcs11-cert.h"
+
+char *
+pkcs11_identity_name(const struct sshkey *key, const u_char *blob,
+ size_t blob_len)
+{
+ u_char digest[SSH_DIGEST_MAX_LENGTH];
+ char *name;
+ size_t i, digest_len;
+
+ if (!sshkey_is_cert(key))
+ return sshkey_fingerprint(key, SSH_FP_HASH_DEFAULT,
+ SSH_FP_DEFAULT);
+ digest_len = ssh_digest_bytes(SSH_DIGEST_SHA256);
+ if (ssh_digest_memory(SSH_DIGEST_SHA256, blob, blob_len, digest,
+ sizeof(digest)) != 0)
+ return NULL;
+ name = xmalloc(5 + digest_len * 2 + 1);
+ memcpy(name, "cert-", 5);
+ for (i = 0; i < digest_len; i++)
+ snprintf(name + 5 + i * 2, 3, "%02x", digest[i]);
+ return name;
+}
+
+void
+free_pkcs11_certs(struct sshkey **certs, size_t ncerts)
+{
+ size_t i;
+
+ for (i = 0; i < ncerts; i++)
+ sshkey_free(certs[i]);
+ free(certs);
+}
+
+int
+parse_pkcs11_add_constraints(struct sshbuf *m, int *cert_onlyp,
+ struct sshkey ***certsp, size_t *ncertsp)
+{
+ struct sshbuf *b = NULL;
+ struct sshkey *key = NULL;
+ char *ext_name = NULL;
+ u_char ctype, value;
+ u_int seconds;
+ int r, seen = 0, seen_lifetime = 0, seen_confirm = 0;
+ int seen_destinations = 0;
+
+ if (m == NULL || cert_onlyp == NULL || certsp == NULL ||
+ ncertsp == NULL || *certsp != NULL || *ncertsp != 0)
+ return SSH_ERR_INVALID_ARGUMENT;
+ *cert_onlyp = 0;
+
+ while (sshbuf_len(m) != 0) {
+ if ((r = sshbuf_get_u8(m, &ctype)) != 0)
+ goto out;
+ if (ctype == SSH_AGENT_CONSTRAIN_LIFETIME) {
+ if (seen_lifetime ||
+ (r = sshbuf_get_u32(m, &seconds)) != 0) {
+ r = SSH_ERR_INVALID_FORMAT;
+ goto out;
+ }
+ seen_lifetime = 1;
+ continue;
+ }
+ if (ctype == SSH_AGENT_CONSTRAIN_CONFIRM) {
+ if (seen_confirm) {
+ r = SSH_ERR_INVALID_FORMAT;
+ goto out;
+ }
+ seen_confirm = 1;
+ continue;
+ }
+ if (ctype != SSH_AGENT_CONSTRAIN_EXTENSION) {
+ error_f("unsupported smartcard constraint %u", ctype);
+ r = SSH_ERR_FEATURE_UNSUPPORTED;
+ goto out;
+ }
+ if ((r = sshbuf_get_cstring(m, &ext_name, NULL)) != 0)
+ goto out;
+ if (strcmp(ext_name,
+ "restrict-destination-v00@openssh.com") == 0) {
+ if (seen_destinations || sshbuf_skip_string(m) != 0) {
+ r = SSH_ERR_INVALID_FORMAT;
+ goto out;
+ }
+ seen_destinations = 1;
+ free(ext_name);
+ ext_name = NULL;
+ continue;
+ }
+ if (strcmp(ext_name,
+ "associated-certs-v00@openssh.com") != 0) {
+ error_f("unsupported smartcard constraint \"%s\"",
+ ext_name);
+ r = SSH_ERR_FEATURE_UNSUPPORTED;
+ goto out;
+ }
+ if (seen) {
+ error_f("%s already set", ext_name);
+ r = SSH_ERR_INVALID_FORMAT;
+ goto out;
+ }
+ seen = 1;
+ if ((r = sshbuf_get_u8(m, &value)) != 0 ||
+ (r = sshbuf_froms(m, &b)) != 0)
+ goto out;
+ *cert_onlyp = value != 0;
+ while (sshbuf_len(b) != 0) {
+ if (*ncertsp >= AGENT_MAX_EXT_CERTS) {
+ error_f("too many %s constraints", ext_name);
+ r = SSH_ERR_INVALID_FORMAT;
+ goto out;
+ }
+ if ((r = sshkey_froms(b, &key)) != 0)
+ goto out;
+ *certsp = xrecallocarray(*certsp, *ncertsp,
+ *ncertsp + 1, sizeof(**certsp));
+ (*certsp)[(*ncertsp)++] = key;
+ key = NULL;
+ }
+ sshbuf_free(b);
+ b = NULL;
+ free(ext_name);
+ ext_name = NULL;
+ }
+ r = 0;
+ out:
+ sshkey_free(key);
+ sshbuf_free(b);
+ free(ext_name);
+ return r;
+}
diff --git a/contrib/win32/win32compat/pkcs11-cert.h b/contrib/win32/win32compat/pkcs11-cert.h
new file mode 100644
index 000000000000..42f0c750e8d6
--- /dev/null
+++ b/contrib/win32/win32compat/pkcs11-cert.h
@@ -0,0 +1,27 @@
+/*
+ * Copyright (c) 2026 Sebastian Ott. All rights reserved.
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#pragma once
+
+#include "sshbuf.h"
+#include "sshkey.h"
+
+#define AGENT_MAX_EXT_CERTS 1024
+
+char *pkcs11_identity_name(const struct sshkey *, const u_char *, size_t);
+int parse_pkcs11_add_constraints(struct sshbuf *, int *,
+ struct sshkey ***, size_t *);
+void free_pkcs11_certs(struct sshkey **, size_t);
diff --git a/contrib/win32/win32compat/ssh-agent/keyagent-request.c b/contrib/win32/win32compat/ssh-agent/keyagent-request.c
index b9b4b036e19b..05f4c61c6995 100644
--- a/contrib/win32/win32compat/ssh-agent/keyagent-request.c
+++ b/contrib/win32/win32compat/ssh-agent/keyagent-request.c
@@ -36,6 +36,7 @@
#include
#ifdef ENABLE_PKCS11
#include "ssh-pkcs11.h"
+#include "pkcs11-cert.h"
#endif
#include "xmalloc.h"
@@ -365,6 +366,205 @@ process_add_identity(struct sshbuf* request, struct sshbuf* response, struct age
return r;
}
+#ifdef ENABLE_PKCS11
+static int
+store_pkcs11_identity(HKEY user_root, const struct sshkey *key,
+ const char *provider)
+{
+ SECURITY_ATTRIBUTES sa = { 0, NULL, 0 };
+ HKEY reg = NULL, sub = NULL;
+ u_char *blob = NULL;
+ size_t blob_len;
+ char *thumbprint = NULL;
+ int success = 0;
+
+ sa.nLength = sizeof(sa);
+ if (!ConvertStringSecurityDescriptorToSecurityDescriptorW(REG_KEY_SDDL,
+ SDDL_REVISION_1, &sa.lpSecurityDescriptor, &sa.nLength) ||
+ sshkey_to_blob(key, &blob, &blob_len) != 0 ||
+ blob_len == 0 || blob_len > MAX_MESSAGE_SIZE ||
+ (thumbprint = pkcs11_identity_name(key, blob, blob_len)) == NULL ||
+ RegCreateKeyExW(user_root, SSH_KEYS_ROOT, 0, NULL, 0,
+ KEY_WRITE | KEY_WOW64_64KEY, &sa, ®, NULL) != ERROR_SUCCESS ||
+ RegCreateKeyExA(reg, thumbprint, 0, NULL, 0,
+ KEY_WRITE | KEY_WOW64_64KEY, &sa, &sub, NULL) != ERROR_SUCCESS ||
+ RegSetValueExW(sub, NULL, 0, REG_BINARY, blob,
+ (DWORD)blob_len) != ERROR_SUCCESS ||
+ RegSetValueExW(sub, L"pub", 0, REG_BINARY, blob,
+ (DWORD)blob_len) != ERROR_SUCCESS ||
+ RegSetValueExW(sub, L"type", 0, REG_DWORD,
+ (const BYTE *)&key->type, sizeof(key->type)) != ERROR_SUCCESS ||
+ RegSetValueExW(sub, L"comment", 0, REG_BINARY,
+ (const BYTE *)provider, (DWORD)strlen(provider)) != ERROR_SUCCESS) {
+ error_f("failed to persist PKCS11 identity");
+ goto out;
+ }
+ success = 1;
+ out:
+ if (sub != NULL) {
+ RegCloseKey(sub);
+ sub = NULL;
+ }
+ if (!success && reg != NULL && thumbprint != NULL)
+ RegDeleteTreeA(reg, thumbprint);
+ if (reg != NULL)
+ RegCloseKey(reg);
+ if (sa.lpSecurityDescriptor != NULL)
+ LocalFree(sa.lpSecurityDescriptor);
+ free(thumbprint);
+ free(blob);
+ return success ? 0 : -1;
+}
+
+static int
+store_pkcs11_provider(HKEY user_root, struct agent_connection *con,
+ const char *provider, const char *pin, size_t pin_len)
+{
+ SECURITY_ATTRIBUTES sa = { 0, NULL, 0 };
+ HKEY reg = NULL, sub = NULL;
+ char *epin = NULL;
+ DWORD epin_len = 0;
+ int success = 0;
+
+ sa.nLength = sizeof(sa);
+ if (!ConvertStringSecurityDescriptorToSecurityDescriptorW(REG_KEY_SDDL,
+ SDDL_REVISION_1, &sa.lpSecurityDescriptor, &sa.nLength) ||
+ convert_blob(con, pin, (DWORD)pin_len, &epin, &epin_len, TRUE) != 0 ||
+ RegCreateKeyExW(user_root, SSH_PKCS11_PROVIDERS_ROOT, 0, NULL, 0,
+ KEY_WRITE | KEY_WOW64_64KEY, &sa, ®, NULL) != ERROR_SUCCESS ||
+ RegCreateKeyExA(reg, provider, 0, NULL, 0,
+ KEY_WRITE | KEY_WOW64_64KEY, &sa, &sub, NULL) != ERROR_SUCCESS ||
+ RegSetValueExW(sub, L"provider", 0, REG_BINARY,
+ (const BYTE *)provider, (DWORD)strlen(provider)) != ERROR_SUCCESS ||
+ RegSetValueExW(sub, L"pin", 0, REG_BINARY, (const BYTE *)epin,
+ epin_len) != ERROR_SUCCESS) {
+ error_f("failed to persist PKCS11 provider");
+ goto out;
+ }
+ success = 1;
+ out:
+ if (epin != NULL) {
+ SecureZeroMemory(epin, epin_len);
+ free(epin);
+ }
+ if (sub != NULL) {
+ RegCloseKey(sub);
+ sub = NULL;
+ }
+ if (!success && reg != NULL)
+ RegDeleteTreeA(reg, provider);
+ if (reg != NULL)
+ RegCloseKey(reg);
+ if (sa.lpSecurityDescriptor != NULL)
+ LocalFree(sa.lpSecurityDescriptor);
+ return success ? 0 : -1;
+}
+
+static int
+load_pkcs11_identities(HKEY user_root, const char *provider,
+ struct sshkey **token_keys, int nkeys)
+{
+ HKEY root = NULL, sub = NULL;
+ wchar_t sub_name[MAX_KEY_LENGTH];
+ DWORD sub_name_len, blob_len, comment_len;
+ u_char *blob = NULL;
+ char *comment = NULL;
+ struct sshkey *registered = NULL, *cert = NULL;
+ u_char *plain_added = NULL;
+ size_t provider_len = strlen(provider);
+ int i, index = 0, loaded = 0;
+ LSTATUS status;
+
+ if (nkeys > 0)
+ plain_added = xcalloc((size_t)nkeys, sizeof(*plain_added));
+ status = RegOpenKeyExW(user_root, SSH_KEYS_ROOT, 0,
+ KEY_ENUMERATE_SUB_KEYS | KEY_QUERY_VALUE | KEY_WOW64_64KEY, &root);
+ if (status == ERROR_FILE_NOT_FOUND)
+ goto out;
+ if (status != ERROR_SUCCESS) {
+ error_f("failed to open persisted identities: %ld", status);
+ loaded = -1;
+ goto out;
+ }
+ for (;;) {
+ sub_name_len = MAX_KEY_LENGTH;
+ if (sub != NULL) {
+ RegCloseKey(sub);
+ sub = NULL;
+ }
+ status = RegEnumKeyExW(root, index++, sub_name, &sub_name_len,
+ NULL, NULL, NULL, NULL);
+ if (status == ERROR_NO_MORE_ITEMS)
+ break;
+ if (status != ERROR_SUCCESS)
+ continue;
+ if (RegOpenKeyExW(root, sub_name, 0,
+ KEY_QUERY_VALUE | KEY_WOW64_64KEY, &sub) != ERROR_SUCCESS ||
+ RegQueryValueExW(sub, L"pub", NULL, NULL, NULL,
+ &blob_len) != ERROR_SUCCESS ||
+ RegQueryValueExW(sub, L"comment", NULL, NULL, NULL,
+ &comment_len) != ERROR_SUCCESS ||
+ blob_len == 0 || blob_len > MAX_MESSAGE_SIZE ||
+ comment_len != provider_len)
+ continue;
+ free(blob);
+ free(comment);
+ blob = xmalloc(blob_len);
+ comment = xmalloc((size_t)comment_len + 1);
+ if (RegQueryValueExW(sub, L"pub", NULL, NULL, blob,
+ &blob_len) != ERROR_SUCCESS ||
+ RegQueryValueExW(sub, L"comment", NULL, NULL,
+ (BYTE *)comment, &comment_len) != ERROR_SUCCESS)
+ continue;
+ comment[comment_len] = '\0';
+ if (memcmp(comment, provider, provider_len) != 0)
+ continue;
+ sshkey_free(registered);
+ registered = NULL;
+ if (sshkey_from_blob(blob, blob_len, ®istered) != 0)
+ continue;
+ for (i = 0; i < nkeys; i++) {
+ if (token_keys[i] == NULL)
+ continue;
+ if (sshkey_is_cert(registered)) {
+ if (!sshkey_equal_public(token_keys[i], registered))
+ continue;
+ if (pkcs11_make_cert(token_keys[i], registered,
+ &cert) != 0)
+ continue;
+ add_key(cert, (char *)provider);
+ cert = NULL;
+ loaded++;
+ break;
+ }
+ if (!plain_added[i] &&
+ sshkey_equal(token_keys[i], registered)) {
+ plain_added[i] = 1;
+ break;
+ }
+ }
+ }
+ for (i = 0; i < nkeys; i++) {
+ if (!plain_added[i] || token_keys[i] == NULL)
+ continue;
+ add_key(token_keys[i], (char *)provider);
+ token_keys[i] = NULL;
+ loaded++;
+ }
+ out:
+ sshkey_free(cert);
+ sshkey_free(registered);
+ free(plain_added);
+ free(comment);
+ free(blob);
+ if (sub != NULL)
+ RegCloseKey(sub);
+ if (root != NULL)
+ RegCloseKey(root);
+ return loaded;
+}
+#endif /* ENABLE_PKCS11 */
+
static int sign_blob(const struct sshkey *pubkey, u_char ** sig, size_t *siglen,
const u_char *blob, size_t blen, u_int flags, struct agent_connection* con)
{
@@ -454,7 +654,7 @@ process_sign_request(struct sshbuf* request, struct sshbuf* response, struct age
struct sshkey *key = NULL;
#ifdef ENABLE_PKCS11
- int i, count = 0, index = 0;;
+ int i, count = 0, index = 0, loaded = 0;
wchar_t sub_name[MAX_KEY_LENGTH];
DWORD sub_name_len = MAX_KEY_LENGTH;
DWORD pin_len, epin_len, provider_len;
@@ -497,10 +697,16 @@ process_sign_request(struct sshbuf* request, struct sshbuf* response, struct age
pin = npin;
pin[pin_len] = '\0';
count = pkcs11_add_provider(provider, pin, &keys, NULL);
- for (i = 0; i < count; i++) {
- add_key(keys[i], provider);
- }
+ if (count <= 0)
+ goto done;
+ loaded = load_pkcs11_identities(user_root, provider,
+ keys, count);
+ for (i = 0; i < count; i++)
+ sshkey_free(keys[i]);
free(keys);
+ keys = NULL;
+ if (loaded < 0)
+ goto done;
if (provider)
free(provider);
if (pin) {
@@ -553,6 +759,11 @@ process_sign_request(struct sshbuf* request, struct sshbuf* response, struct age
if (signature)
free(signature);
#ifdef ENABLE_PKCS11
+ if (keys != NULL) {
+ for (i = 0; i < count; i++)
+ sshkey_free(keys[i]);
+ free(keys);
+ }
del_all_keys();
pkcs11_terminate();
if (provider)
@@ -590,7 +801,12 @@ process_remove_key(struct sshbuf* request, struct sshbuf* response, struct agent
goto done;
}
- if ((thumbprint = sshkey_fingerprint(key, SSH_FP_HASH_DEFAULT, SSH_FP_DEFAULT)) == NULL ||
+ if ((thumbprint =
+#ifdef ENABLE_PKCS11
+ pkcs11_identity_name(key, (const u_char *)blob, blen)) == NULL ||
+#else
+ sshkey_fingerprint(key, SSH_FP_HASH_DEFAULT, SSH_FP_DEFAULT)) == NULL ||
+#endif
get_user_root(con, &user_root) != 0 ||
RegOpenKeyExW(user_root, SSH_KEYS_ROOT, 0,
DELETE | KEY_ENUMERATE_SUB_KEYS | KEY_QUERY_VALUE | KEY_WOW64_64KEY, &root) != 0 ||
@@ -641,28 +857,34 @@ process_remove_all(struct sshbuf* request, struct sshbuf* response, struct agent
}
#ifdef ENABLE_PKCS11
-int process_add_smartcard_key(struct sshbuf* request, struct sshbuf* response, struct agent_connection* con)
+int
+process_add_smartcard_key(struct sshbuf *request, struct sshbuf *response,
+ struct agent_connection *con)
{
- char *provider = NULL, *pin = NULL, canonical_provider[PATH_MAX];
- int i, count = 0, r = 0, request_invalid = 0, success = 0;
- struct sshkey **keys = NULL;
- struct sshkey* key = NULL;
- size_t pubkey_blob_len, provider_len, pin_len, epin_len;
- u_char *pubkey_blob = NULL;
- char *thumbprint = NULL;
- char *epin = NULL;
- HKEY reg = 0, sub = 0, user_root = 0;
- SECURITY_ATTRIBUTES sa = { 0, NULL, 0 };
+ char *provider = NULL, *pin = NULL, canonical_provider[PATH_MAX] = { 0 };
+ char allowed_provider[PATH_MAX];
+ int i, j, count = 0, r = 0, request_invalid = 0, success = 0;
+ int cert_only = 0, identities_stored = 0;
+ struct sshkey **keys = NULL, **certs = NULL, *cert = NULL;
+ size_t pin_len = 0, ncerts = 0;
+ HKEY user_root = NULL;
pkcs11_init(0);
- if ((r = sshbuf_get_cstring(request, &provider, &provider_len)) != 0 ||
- (r = sshbuf_get_cstring(request, &pin, &pin_len)) != 0 ||
- pin_len > 256) {
+ if ((r = sshbuf_get_cstring(request, &provider, NULL)) != 0 ||
+ (r = sshbuf_get_cstring(request, &pin, &pin_len)) != 0 ||
+ pin_len > 256) {
error("add smartcard request is invalid");
request_invalid = 1;
goto done;
}
+ if (sshbuf_len(request) != 0 &&
+ parse_pkcs11_add_constraints(request, &cert_only, &certs,
+ &ncerts) != 0) {
+ error("add smartcard constraints are invalid");
+ request_invalid = 1;
+ goto done;
+ }
if (con->nsession_ids != 0 && !remote_add_provider) {
verbose("failed PKCS#11 add of \"%.100s\": remote addition of "
@@ -677,76 +899,66 @@ int process_add_smartcard_key(struct sshbuf* request, struct sshbuf* response, s
goto done;
}
- to_lower_case(provider);
- verbose("provider realpath: \"%.100s\"", provider);
+ /* Remove the leading slash from the canonical Windows drive path. */
+ if (canonical_provider[0] == '/')
+ memmove(canonical_provider, canonical_provider + 1,
+ strlen(canonical_provider));
+ strcpy_s(allowed_provider, sizeof(allowed_provider), canonical_provider);
+ for (i = 0; allowed_provider[i] != '\0'; i++) {
+ if (allowed_provider[i] == '/')
+ allowed_provider[i] = '\\';
+ }
+ to_lower_case(allowed_provider);
+ verbose("provider realpath: \"%.100s\"", canonical_provider);
verbose("allowed provider paths: \"%.100s\"", allowed_providers);
- if (match_pattern_list(provider, allowed_providers, 1) != 1) {
+ if (match_pattern_list(allowed_provider, allowed_providers, 1) != 1) {
verbose("refusing PKCS#11 add of \"%.100s\": "
- "provider not allowed", provider);
+ "provider not allowed", canonical_provider);
goto done;
}
- // Remove 'drive root' if exists
- if (canonical_provider[0] == '/')
- memmove(canonical_provider, canonical_provider + 1, strlen(canonical_provider));
-
count = pkcs11_add_provider(canonical_provider, pin, &keys, NULL);
if (count <= 0) {
- error_f("failed to add key to store. count:%d", count);
+ error_f("failed to load provider keys: count:%d", count);
goto done;
}
- // If HKCU registry already has the provider then remove the provider and associated keys.
- // This allows customers to add new keys.
- if (get_user_root(con, &user_root) != 0 ||
- is_reg_sub_key_exists(user_root, SSH_PKCS11_PROVIDERS_ROOT, canonical_provider)) {
- remove_matching_subkeys_from_registry(user_root, SSH_KEYS_ROOT, L"comment", canonical_provider);
- remove_matching_subkeys_from_registry(user_root, SSH_PKCS11_PROVIDERS_ROOT, L"provider", canonical_provider);
+ /* Replace the provider and all of its persisted identities. */
+ if (get_user_root(con, &user_root) != 0)
+ goto done;
+ if (is_reg_sub_key_exists(user_root, SSH_PKCS11_PROVIDERS_ROOT,
+ canonical_provider)) {
+ remove_matching_subkeys_from_registry(user_root, SSH_KEYS_ROOT,
+ L"comment", canonical_provider);
+ remove_matching_subkeys_from_registry(user_root,
+ SSH_PKCS11_PROVIDERS_ROOT, L"provider", canonical_provider);
}
for (i = 0; i < count; i++) {
- key = keys[i];
- if (sa.lpSecurityDescriptor)
- LocalFree(sa.lpSecurityDescriptor);
- if (reg) {
- RegCloseKey(reg);
- reg = NULL;
- }
- if (sub) {
- RegCloseKey(sub);
- sub = NULL;
+ for (j = 0; j < (int)ncerts; j++) {
+ if (!sshkey_is_cert(certs[j]) ||
+ !sshkey_equal_public(keys[i], certs[j]))
+ continue;
+ if (pkcs11_make_cert(keys[i], certs[j], &cert) != 0)
+ continue;
+ if (store_pkcs11_identity(user_root, cert,
+ canonical_provider) != 0)
+ goto done;
+ sshkey_free(cert);
+ cert = NULL;
+ identities_stored++;
}
- memset(&sa, 0, sizeof(SECURITY_ATTRIBUTES));
- sa.nLength = sizeof(sa);
- if ((!ConvertStringSecurityDescriptorToSecurityDescriptorW(REG_KEY_SDDL, SDDL_REVISION_1, &sa.lpSecurityDescriptor, &sa.nLength)) ||
- sshkey_to_blob(key, &pubkey_blob, &pubkey_blob_len) != 0 ||
- ((thumbprint = sshkey_fingerprint(key, SSH_FP_HASH_DEFAULT, SSH_FP_DEFAULT)) == NULL) ||
- RegCreateKeyExW(user_root, SSH_KEYS_ROOT, 0, 0, 0, KEY_WRITE | KEY_WOW64_64KEY, &sa, ®, NULL) != 0 ||
- RegCreateKeyExA(reg, thumbprint, 0, 0, 0, KEY_WRITE | KEY_WOW64_64KEY, &sa, &sub, NULL) != 0 ||
- RegSetValueExW(sub, NULL, 0, REG_BINARY, pubkey_blob, (DWORD)pubkey_blob_len) != 0 ||
- RegSetValueExW(sub, L"pub", 0, REG_BINARY, pubkey_blob, (DWORD)pubkey_blob_len) != 0 ||
- RegSetValueExW(sub, L"type", 0, REG_DWORD, (BYTE*)&key->type, 4) != 0 ||
- RegSetValueExW(sub, L"comment", 0, REG_BINARY, canonical_provider, (DWORD)strlen(canonical_provider)) != 0) {
- error_f("failed to add key to store");
+ if (!cert_only && store_pkcs11_identity(user_root, keys[i],
+ canonical_provider) == 0)
+ identities_stored++;
+ else if (!cert_only)
goto done;
- }
}
- debug("added smartcard keys to store");
-
- memset(&sa, 0, sizeof(SECURITY_ATTRIBUTES));
- sa.nLength = sizeof(sa);
- if ((!ConvertStringSecurityDescriptorToSecurityDescriptorW(REG_KEY_SDDL, SDDL_REVISION_1, &sa.lpSecurityDescriptor, &sa.nLength)) ||
- convert_blob(con, pin, (DWORD)pin_len, &epin, (DWORD*)&epin_len, 1) != 0 ||
- RegCreateKeyExW(user_root, SSH_PKCS11_PROVIDERS_ROOT, 0, 0, 0, KEY_WRITE | KEY_WOW64_64KEY, &sa, ®, NULL) != 0 ||
- RegCreateKeyExA(reg, canonical_provider, 0, 0, 0, KEY_WRITE | KEY_WOW64_64KEY, &sa, &sub, NULL) != 0 ||
- RegSetValueExW(sub, L"provider", 0, REG_BINARY, canonical_provider, (DWORD)strlen(canonical_provider)) != 0 ||
- RegSetValueExW(sub, L"pin", 0, REG_BINARY, epin, (DWORD)epin_len) != 0) {
- error("failed to add pkcs11 provider to store");
+ if (identities_stored == 0 || store_pkcs11_provider(user_root, con,
+ canonical_provider, pin, pin_len) != 0)
goto done;
- }
-
- debug("added pkcs11 provider to store");
+ debug("added PKCS11 provider and identities to store");
success = 1;
done:
r = 0;
@@ -755,44 +967,27 @@ int process_add_smartcard_key(struct sshbuf* request, struct sshbuf* response, s
else if (sshbuf_put_u8(response, success ? SSH_AGENT_SUCCESS : SSH_AGENT_FAILURE) != 0)
r = -1;
- /* delete created reg keys if not succeeded*/
- if ((success == 0) && reg) {
- if (thumbprint)
- RegDeleteKeyExA(reg, thumbprint, KEY_WOW64_64KEY, 0);
- if (canonical_provider)
- RegDeleteKeyExA(reg, canonical_provider, KEY_WOW64_64KEY, 0);
+ if (!success && user_root != NULL && canonical_provider[0] != '\0') {
+ remove_matching_subkeys_from_registry(user_root, SSH_KEYS_ROOT,
+ L"comment", canonical_provider);
+ remove_matching_subkeys_from_registry(user_root,
+ SSH_PKCS11_PROVIDERS_ROOT, L"provider", canonical_provider);
}
pkcs11_terminate();
- if (sa.lpSecurityDescriptor)
- LocalFree(sa.lpSecurityDescriptor);
+ sshkey_free(cert);
for (i = 0; i < count; i++)
sshkey_free(keys[i]);
- if (keys)
- free(keys);
- if (thumbprint)
- free(thumbprint);
- if (pubkey_blob)
- free(pubkey_blob);
- if (provider)
- free(provider);
- if (allowed_providers)
- free(allowed_providers);
+ free(keys);
+ free_pkcs11_certs(certs, ncerts);
+ free(provider);
if (pin) {
SecureZeroMemory(pin, (DWORD)pin_len);
free(pin);
}
- if (epin) {
- SecureZeroMemory(epin, (DWORD)epin_len);
- free(epin);
- }
if (user_root)
RegCloseKey(user_root);
- if (reg)
- RegCloseKey(reg);
- if (sub)
- RegCloseKey(sub);
return r;
}
diff --git a/regress/pesterTests/KeyUtils.Tests.ps1 b/regress/pesterTests/KeyUtils.Tests.ps1
index 5881f509d57f..1194d81e2fa9 100644
--- a/regress/pesterTests/KeyUtils.Tests.ps1
+++ b/regress/pesterTests/KeyUtils.Tests.ps1
@@ -301,10 +301,15 @@ Describe "E2E scenarios for ssh key management" -Tags "CI" {
}
It "$tC.$tI - ssh-add - pkcs11 library (if available)" {
- $pkcs11Path = "C:\\Program Files\\OpenSC Project\\OpenSC\\pkcs11\\opensc-pkcs11.dll"
+ $pkcs11Path = $env:OPENSSH_TEST_PKCS11_PROVIDER
+ if (-not $pkcs11Path) {
+ $pkcs11Path = "C:\\Program Files\\OpenSC Project\\OpenSC\\pkcs11\\opensc-pkcs11.dll"
+ }
if (Test-Path $pkcs11Path) {
#set up SSH_ASKPASS
- Add-PasswordSetting -Pass $pkcs11Pin
+ $testPin = $env:OPENSSH_TEST_PKCS11_PIN
+ if (-not $testPin) { $testPin = $pkcs11Pin }
+ Add-PasswordSetting -Pass $testPin
ssh-add -s "$pkcs11Path"
$LASTEXITCODE | Should Be 0
@@ -326,6 +331,90 @@ Describe "E2E scenarios for ssh key management" -Tags "CI" {
Write-Host "skipping pkcs11 test because provider not found"
}
}
+
+ It "$tC.$tI - ssh-add - pkcs11 certificates (if configured)" {
+ $pkcs11Path = $env:OPENSSH_TEST_PKCS11_PROVIDER
+ $publicKeyPaths = @($env:OPENSSH_TEST_PKCS11_PUBLIC_KEYS -split ';' |
+ Where-Object { $_ })
+ if (-not $pkcs11Path -or -not (Test-Path $pkcs11Path) -or
+ $publicKeyPaths.Count -eq 0) {
+ Write-Host "skipping pkcs11 certificate test because provider and public keys are not configured"
+ return
+ }
+
+ foreach ($publicKeyPath in $publicKeyPaths) {
+ Test-Path $publicKeyPath | Should Be $true
+ }
+ $testPin = $env:OPENSSH_TEST_PKCS11_PIN
+ if (-not $testPin) { $testPin = $pkcs11Pin }
+ Add-PasswordSetting -Pass $testPin
+
+ $ca = Join-Path $testDir "pkcs11-ca"
+ Remove-Item "$ca*" -Force -ErrorAction SilentlyContinue
+ & ssh-keygen -q -t ed25519 -N '""' -f $ca
+ $LASTEXITCODE | Should Be 0
+
+ $certPaths = @()
+ $copiedPublicKeyPaths = @()
+ $serial = 1
+ foreach ($publicKeyPath in $publicKeyPaths) {
+ $copiedPublicKeyPath = Join-Path $testDir "pkcs11-$serial.pub"
+ Copy-Item $publicKeyPath $copiedPublicKeyPath -Force
+ & ssh-keygen -q -s $ca -I "pkcs11-$serial" -n $env:USERNAME `
+ -z $serial $copiedPublicKeyPath
+ $LASTEXITCODE | Should Be 0
+ $copiedPublicKeyPaths += $copiedPublicKeyPath
+ $certPaths += $copiedPublicKeyPath.Replace(".pub", "-cert.pub")
+ $serial++
+ }
+ & ssh-keygen -q -s $ca -I "pkcs11-unmatched" -n $env:USERNAME `
+ -z 999 "$ca.pub"
+ $LASTEXITCODE | Should Be 0
+ $unmatchedCertPath = "$ca-cert.pub"
+ $associatedCertPaths = $certPaths + $unmatchedCertPath
+
+ $addArguments = @("-s", $pkcs11Path) + $associatedCertPaths
+ & ssh-add @addArguments
+ $LASTEXITCODE | Should Be 0
+ $allKeys = @(ssh-add -L)
+ foreach ($keyPath in $copiedPublicKeyPaths + $certPaths) {
+ $keyBlob = (Get-Content $keyPath).Split(' ')[1]
+ @($allKeys | Where-Object { $_.Contains($keyBlob) }).Count | Should Be 1
+ & ssh-add -T $keyPath
+ $LASTEXITCODE | Should Be 0
+ }
+
+ Restart-Service ssh-agent
+ WaitForStatus -ServiceName ssh-agent -Status "Running"
+ foreach ($certPath in $certPaths) {
+ & ssh-add -T $certPath
+ $LASTEXITCODE | Should Be 0
+ }
+ & ssh-add -d $certPaths[0]
+ $LASTEXITCODE | Should Be 0
+ $deletedKeyBlob = (Get-Content $certPaths[0]).Split(' ')[1]
+ @((ssh-add -L) | Where-Object { $_.Contains($deletedKeyBlob) }).Count |
+ Should Be 0
+
+ ssh-add -D
+ $LASTEXITCODE | Should Be 0
+ $addArguments = @("-s", $pkcs11Path, "-C") + $associatedCertPaths
+ & ssh-add @addArguments
+ $LASTEXITCODE | Should Be 0
+ $allKeys = @(ssh-add -L)
+ $allKeys.Count | Should Be $certPaths.Count
+ foreach ($certPath in $certPaths) {
+ $keyBlob = (Get-Content $certPath).Split(' ')[1]
+ @($allKeys | Where-Object { $_.Contains($keyBlob) }).Count | Should Be 1
+ & ssh-add -T $certPath
+ $LASTEXITCODE | Should Be 0
+ }
+
+ & ssh-add -e $pkcs11Path
+ $LASTEXITCODE | Should Be 0
+ @(ssh-add -L) -match "The agent has no identities." | Should Be $true
+ Remove-PasswordSetting
+ }
}
Context "$tC ssh-keygen known_hosts operations" {
diff --git a/regress/pesterTests/README.md b/regress/pesterTests/README.md
index afd636e76c6d..4d9de131c5b3 100644
--- a/regress/pesterTests/README.md
+++ b/regress/pesterTests/README.md
@@ -64,3 +64,19 @@ Follow these simple steps for test case indexing
AfterAll{$tC++}
```
- Prefix any test out file with $tC.$tI. You may use pre-created $stderrFile, $stdoutFile, $logFile for this purpose
+
+#### PKCS#11 certificate tests
+
+The PKCS#11 certificate scenario in `KeyUtils.Tests.ps1` is enabled when the
+following environment variables are set before running the E2E tests:
+
+* `OPENSSH_TEST_PKCS11_PROVIDER`: absolute path to a PKCS#11 provider DLL.
+* `OPENSSH_TEST_PKCS11_PIN`: token PIN.
+* `OPENSSH_TEST_PKCS11_PUBLIC_KEYS`: semicolon-separated public-key files whose
+ corresponding private keys are present on the token.
+
+The test creates short-lived OpenSSH certificates for the supplied public
+keys. It verifies plain and certificate identities, signing, agent service
+restart, individual certificate deletion, cert-only loading, an unmatched
+certificate, and provider removal. Never use production token credentials in
+CI.
diff --git a/regress/unittests/win32compat/pkcs11_cert_tests.c b/regress/unittests/win32compat/pkcs11_cert_tests.c
new file mode 100644
index 000000000000..f0e1cfb74de9
--- /dev/null
+++ b/regress/unittests/win32compat/pkcs11_cert_tests.c
@@ -0,0 +1,269 @@
+/*
+ * Copyright (c) 2026 Sebastian Ott. All rights reserved.
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include "includes.h"
+
+#include "authfd.h"
+#include "sshbuf.h"
+#include "ssherr.h"
+#include "sshkey.h"
+#include "xmalloc.h"
+
+#include "contrib/win32/win32compat/pkcs11-cert.h"
+#include "../test_helper/test_helper.h"
+#include "tests.h"
+
+#define TEST_CERT \
+ "ecdsa-sha2-nistp256-cert-v01@openssh.com " \
+ "AAAAKGVjZHNhLXNoYTItbmlzdHAyNTYtY2VydC12MDFAb3BlbnNzaC5jb20AAAAg" \
+ "OtFRnMigkGliaYfPmX5IidVWfV3tRH6lqRXv0l8bvKoAAAAIbmlzdHAyNTYAAABB" \
+ "BAxZW5ZDq1vcnSlYbTPvQGN3PbGgRO0ht5Rcd/JwWr5AAw2iPY4d/5Lxvybfb6" \
+ "ZttqsKJJUwhg38wpF5CCmlpQcAAAAAAAAABwAAAAIAAAAGanVsaXVzAAAAEgAAAA" \
+ "Vob3N0MQAAAAVob3N0MgAAAAA2jAHwAAAAAE0eYHAAAAAAAAAAAAAAAAAAAABoAA" \
+ "AAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBAxZW5ZDq1vcnS" \
+ "lYbTPvQGN3PbGgRO0ht5Rcd/JwWr5AAw2iPY4d/5Lxvybfb6ZttqsKJJUwhg38w" \
+ "pF5CCmlpQcAAABkAAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAABJAAAAIHbxGwTnu" \
+ "e7KxhHXGFvRcxBnekhQ3Qx84vV/Vs4oVCrpAAAAIQC7vk2+d14aS7td7kVXLQn3" \
+ "92oALjEBzMZoDvT1vT/zOA== test"
+
+static struct sshkey *
+load_test_cert(void)
+{
+ struct sshkey *key = NULL;
+ char *line = NULL, *cp;
+
+ line = xstrdup(TEST_CERT);
+ cp = line;
+ key = sshkey_new(KEY_UNSPEC);
+ if (key == NULL || sshkey_read(key, &cp) != 0) {
+ sshkey_free(key);
+ key = NULL;
+ }
+ free(line);
+ return key;
+}
+
+static int
+put_associated_certs(struct sshbuf *m, int cert_only,
+ struct sshkey *cert, size_t ncerts)
+{
+ struct sshbuf *b = NULL;
+ size_t i;
+ int r;
+
+ if ((b = sshbuf_new()) == NULL)
+ return SSH_ERR_ALLOC_FAIL;
+ for (i = 0; i < ncerts; i++) {
+ if ((r = sshkey_puts(cert, b)) != 0)
+ goto out;
+ }
+ if ((r = sshbuf_put_u8(m, SSH_AGENT_CONSTRAIN_EXTENSION)) != 0 ||
+ (r = sshbuf_put_cstring(m,
+ "associated-certs-v00@openssh.com")) != 0 ||
+ (r = sshbuf_put_u8(m, cert_only != 0)) != 0 ||
+ (r = sshbuf_put_stringb(m, b)) != 0)
+ goto out;
+ r = 0;
+ out:
+ sshbuf_free(b);
+ return r;
+}
+
+static void
+test_pkcs11_cert_constraints_valid(void)
+{
+ struct sshbuf *m = NULL;
+ struct sshkey *cert = NULL, **certs = NULL;
+ size_t ncerts = 0;
+ int cert_only = 0, r;
+
+ TEST_START("PKCS11 associated certificate constraint");
+ ASSERT_PTR_NE(cert = load_test_cert(), NULL);
+ ASSERT_PTR_NE(m = sshbuf_new(), NULL);
+ ASSERT_INT_EQ(put_associated_certs(m, 1, cert, 1), 0);
+ ASSERT_INT_EQ(r = parse_pkcs11_add_constraints(m, &cert_only,
+ &certs, &ncerts), 0);
+ ASSERT_INT_EQ(cert_only, 1);
+ ASSERT_SIZE_T_EQ(ncerts, 1);
+ ASSERT_INT_EQ(sshkey_equal(cert, certs[0]), 1);
+ free_pkcs11_certs(certs, ncerts);
+ sshkey_free(cert);
+ sshbuf_free(m);
+ TEST_DONE();
+}
+
+static void
+test_pkcs11_cert_constraints_compatible(void)
+{
+ struct sshbuf *m = NULL, *destinations = NULL;
+ struct sshkey *cert = NULL, **certs = NULL;
+ size_t ncerts = 0;
+ int cert_only = 0;
+
+ TEST_START("PKCS11 certificate with existing constraints");
+ ASSERT_PTR_NE(cert = load_test_cert(), NULL);
+ ASSERT_PTR_NE(m = sshbuf_new(), NULL);
+ ASSERT_PTR_NE(destinations = sshbuf_new(), NULL);
+ ASSERT_INT_EQ(sshbuf_put_u8(m, SSH_AGENT_CONSTRAIN_LIFETIME), 0);
+ ASSERT_INT_EQ(sshbuf_put_u32(m, 60), 0);
+ ASSERT_INT_EQ(sshbuf_put_u8(m, SSH_AGENT_CONSTRAIN_CONFIRM), 0);
+ ASSERT_INT_EQ(sshbuf_put_u8(m, SSH_AGENT_CONSTRAIN_EXTENSION), 0);
+ ASSERT_INT_EQ(sshbuf_put_cstring(m,
+ "restrict-destination-v00@openssh.com"), 0);
+ ASSERT_INT_EQ(sshbuf_put_stringb(m, destinations), 0);
+ ASSERT_INT_EQ(put_associated_certs(m, 1, cert, 1), 0);
+ ASSERT_INT_EQ(parse_pkcs11_add_constraints(m, &cert_only,
+ &certs, &ncerts), 0);
+ ASSERT_INT_EQ(cert_only, 1);
+ ASSERT_SIZE_T_EQ(ncerts, 1);
+ free_pkcs11_certs(certs, ncerts);
+ sshkey_free(cert);
+ sshbuf_free(destinations);
+ sshbuf_free(m);
+ TEST_DONE();
+}
+
+static void
+test_pkcs11_cert_identity_name(void)
+{
+ struct sshkey *cert = NULL, *plain = NULL;
+ u_char *cert_blob = NULL, *plain_blob = NULL;
+ size_t cert_blob_len = 0, plain_blob_len = 0;
+ char *cert_name = NULL, *cert_name_again = NULL, *plain_name = NULL;
+
+ TEST_START("distinct PKCS11 certificate registry identity");
+ ASSERT_PTR_NE(cert = load_test_cert(), NULL);
+ ASSERT_INT_EQ(sshkey_from_private(cert, &plain), 0);
+ ASSERT_INT_EQ(sshkey_drop_cert(plain), 0);
+ ASSERT_INT_EQ(sshkey_to_blob(cert, &cert_blob, &cert_blob_len), 0);
+ ASSERT_INT_EQ(sshkey_to_blob(plain, &plain_blob, &plain_blob_len), 0);
+ ASSERT_PTR_NE(cert_name = pkcs11_identity_name(cert, cert_blob,
+ cert_blob_len), NULL);
+ ASSERT_PTR_NE(cert_name_again = pkcs11_identity_name(cert, cert_blob,
+ cert_blob_len), NULL);
+ ASSERT_PTR_NE(plain_name = pkcs11_identity_name(plain, plain_blob,
+ plain_blob_len), NULL);
+ ASSERT_INT_EQ(strncmp(cert_name, "cert-", 5), 0);
+ ASSERT_STRING_EQ(cert_name, cert_name_again);
+ ASSERT_STRING_NE(cert_name, plain_name);
+ free(plain_name);
+ free(cert_name_again);
+ free(cert_name);
+ free(plain_blob);
+ free(cert_blob);
+ sshkey_free(plain);
+ sshkey_free(cert);
+ TEST_DONE();
+}
+
+static void
+test_pkcs11_cert_constraints_duplicate(void)
+{
+ struct sshbuf *m = NULL;
+ struct sshkey *cert = NULL, **certs = NULL;
+ size_t ncerts = 0;
+ int cert_only = 0;
+
+ TEST_START("duplicate PKCS11 certificate constraint");
+ ASSERT_PTR_NE(cert = load_test_cert(), NULL);
+ ASSERT_PTR_NE(m = sshbuf_new(), NULL);
+ ASSERT_INT_EQ(put_associated_certs(m, 0, cert, 1), 0);
+ ASSERT_INT_EQ(put_associated_certs(m, 0, cert, 1), 0);
+ ASSERT_INT_NE(parse_pkcs11_add_constraints(m, &cert_only,
+ &certs, &ncerts), 0);
+ free_pkcs11_certs(certs, ncerts);
+ sshkey_free(cert);
+ sshbuf_free(m);
+ TEST_DONE();
+}
+
+static void
+test_pkcs11_cert_constraints_truncated(void)
+{
+ struct sshbuf *m = NULL;
+ struct sshkey **certs = NULL;
+ size_t ncerts = 0;
+ int cert_only = 0;
+
+ TEST_START("truncated PKCS11 certificate constraint");
+ ASSERT_PTR_NE(m = sshbuf_new(), NULL);
+ ASSERT_INT_EQ(sshbuf_put_u8(m, SSH_AGENT_CONSTRAIN_EXTENSION), 0);
+ ASSERT_INT_EQ(sshbuf_put_cstring(m,
+ "associated-certs-v00@openssh.com"), 0);
+ ASSERT_INT_NE(parse_pkcs11_add_constraints(m, &cert_only,
+ &certs, &ncerts), 0);
+ free_pkcs11_certs(certs, ncerts);
+ sshbuf_free(m);
+ TEST_DONE();
+}
+
+static void
+test_pkcs11_cert_constraints_malformed(void)
+{
+ struct sshbuf *m = NULL, *b = NULL;
+ struct sshkey **certs = NULL;
+ size_t ncerts = 0;
+ int cert_only = 0;
+
+ TEST_START("malformed PKCS11 certificate constraint");
+ ASSERT_PTR_NE(m = sshbuf_new(), NULL);
+ ASSERT_PTR_NE(b = sshbuf_new(), NULL);
+ ASSERT_INT_EQ(sshbuf_put_string(b, "bad", 3), 0);
+ ASSERT_INT_EQ(sshbuf_put_u8(m, SSH_AGENT_CONSTRAIN_EXTENSION), 0);
+ ASSERT_INT_EQ(sshbuf_put_cstring(m,
+ "associated-certs-v00@openssh.com"), 0);
+ ASSERT_INT_EQ(sshbuf_put_u8(m, 0), 0);
+ ASSERT_INT_EQ(sshbuf_put_stringb(m, b), 0);
+ ASSERT_INT_NE(parse_pkcs11_add_constraints(m, &cert_only,
+ &certs, &ncerts), 0);
+ free_pkcs11_certs(certs, ncerts);
+ sshbuf_free(b);
+ sshbuf_free(m);
+ TEST_DONE();
+}
+
+static void
+test_pkcs11_cert_constraints_oversized(void)
+{
+ struct sshbuf *m = NULL;
+ struct sshkey *cert = NULL, **certs = NULL;
+ size_t ncerts = 0;
+ int cert_only = 0;
+
+ TEST_START("oversized PKCS11 certificate constraint");
+ ASSERT_PTR_NE(cert = load_test_cert(), NULL);
+ ASSERT_PTR_NE(m = sshbuf_new(), NULL);
+ ASSERT_INT_EQ(put_associated_certs(m, 0, cert,
+ AGENT_MAX_EXT_CERTS + 1), 0);
+ ASSERT_INT_NE(parse_pkcs11_add_constraints(m, &cert_only,
+ &certs, &ncerts), 0);
+ free_pkcs11_certs(certs, ncerts);
+ sshkey_free(cert);
+ sshbuf_free(m);
+ TEST_DONE();
+}
+
+void
+pkcs11_cert_tests(void)
+{
+ test_pkcs11_cert_constraints_valid();
+ test_pkcs11_cert_constraints_compatible();
+ test_pkcs11_cert_identity_name();
+ test_pkcs11_cert_constraints_duplicate();
+ test_pkcs11_cert_constraints_truncated();
+ test_pkcs11_cert_constraints_malformed();
+ test_pkcs11_cert_constraints_oversized();
+}
diff --git a/regress/unittests/win32compat/tests.c b/regress/unittests/win32compat/tests.c
index ae27730dc757..bb110e6f4de8 100644
--- a/regress/unittests/win32compat/tests.c
+++ b/regress/unittests/win32compat/tests.c
@@ -19,6 +19,7 @@ tests()
{
_set_abort_behavior(0, 1);
log_init(NULL, 7, 2, 0);
+ pkcs11_cert_tests();
signal_tests();
socket_tests();
file_tests();
diff --git a/regress/unittests/win32compat/tests.h b/regress/unittests/win32compat/tests.h
index 580cf063f859..64e06f09f0f8 100644
--- a/regress/unittests/win32compat/tests.h
+++ b/regress/unittests/win32compat/tests.h
@@ -3,6 +3,7 @@ void signal_tests();
void socket_tests();
void file_tests();
void miscellaneous_tests();
+void pkcs11_cert_tests(void);
char *dup_str(char *inStr);
void delete_dir_recursive(char *full_dir_path);
diff --git a/ssh-add.c b/ssh-add.c
index b90ae87b8455..137af7e5ce7e 100644
--- a/ssh-add.c
+++ b/ssh-add.c
@@ -855,7 +855,7 @@ main(int argc, char **argv)
skprovider = getenv("SSH_SK_PROVIDER");
#ifdef WINDOWS
- while ((ch = getopt(argc, argv, "vkKlLcdDTxXE:e:M:m:qs:S:t:")) != -1) {
+ while ((ch = getopt(argc, argv, "vkKlLCcdDTxXE:e:M:m:qs:S:t:")) != -1) {
#else
while ((ch = getopt(argc, argv, "vkKlLCcdDTxXE:e:h:H:M:m:qs:S:t:")) != -1) {
#endif
diff --git a/ssh-pkcs11-client.c b/ssh-pkcs11-client.c
index b2f8e0f1e400..052e6cbbf58f 100644
--- a/ssh-pkcs11-client.c
+++ b/ssh-pkcs11-client.c
@@ -662,6 +662,51 @@ wrap_key(struct helper *helper, struct sshkey *k)
#ifdef WINDOWS
+/*
+ * Make a private PKCS#11-backed certificate by grafting a previously-loaded
+ * PKCS#11 private key and a public certificate key.
+ */
+int
+pkcs11_make_cert(const struct sshkey *priv,
+ const struct sshkey *certpub, struct sshkey **certprivp)
+{
+ struct sshkey *ret = NULL;
+ int r;
+
+ if (certprivp == NULL)
+ return SSH_ERR_INVALID_ARGUMENT;
+ *certprivp = NULL;
+ debug3_f("private key type %s cert type %s", sshkey_type(priv),
+ sshkey_type(certpub));
+ if (!sshkey_is_cert(certpub) || sshkey_is_cert(priv) ||
+ !sshkey_equal_public(priv, certpub)) {
+ error_f("private key %s doesn't match cert %s",
+ sshkey_type(priv), sshkey_type(certpub));
+ return SSH_ERR_INVALID_ARGUMENT;
+ }
+ if (priv->type != KEY_RSA
+#if defined(OPENSSL_HAS_ECC) && defined(HAVE_EC_KEY_METHOD_NEW)
+ && priv->type != KEY_ECDSA
+#endif
+ ) {
+ error_f("unsupported PKCS11 key type %s", sshkey_type(priv));
+ return SSH_ERR_KEY_TYPE_UNKNOWN;
+ }
+ if ((r = sshkey_from_private(priv, &ret)) != 0)
+ goto out;
+ wrap_key(ret);
+ ret->flags |= SSHKEY_FLAG_EXT;
+ if ((r = sshkey_to_certified(ret)) != 0 ||
+ (r = sshkey_cert_copy(certpub, ret)) != 0)
+ goto out;
+ *certprivp = ret;
+ ret = NULL;
+ r = 0;
+ out:
+ sshkey_free(ret);
+ return r;
+}
+
static int
pkcs11_start_helper_methods(void)
{
From dfacf57fd66682105a03548af3fe2720a49cc5ab Mon Sep 17 00:00:00 2001
From: Sebastian Ott <174621899+VSSOtt@users.noreply.github.com>
Date: Fri, 17 Jul 2026 13:54:00 +0200
Subject: [PATCH 2/8] Preserve PKCS#11 identities on provider add
Treat repeated provider additions as additive and idempotent because the Windows service persists providers and identities across client and service lifetimes. Unlike the in-memory Unix agent, this allows certificates to be added in separate requests without replacing earlier identities.
Roll back only identities created by the failed request and preserve all previously persisted state.
---
.../win32compat/ssh-agent/keyagent-request.c | 98 ++++++++++++++-----
regress/pesterTests/CommonUtils.psm1 | 2 +-
regress/pesterTests/KeyUtils.Tests.ps1 | 81 ++++++++++++---
regress/pesterTests/README.md | 3 +-
4 files changed, 143 insertions(+), 41 deletions(-)
diff --git a/contrib/win32/win32compat/ssh-agent/keyagent-request.c b/contrib/win32/win32compat/ssh-agent/keyagent-request.c
index 05f4c61c6995..721423a9742f 100644
--- a/contrib/win32/win32compat/ssh-agent/keyagent-request.c
+++ b/contrib/win32/win32compat/ssh-agent/keyagent-request.c
@@ -369,15 +369,19 @@ process_add_identity(struct sshbuf* request, struct sshbuf* response, struct age
#ifdef ENABLE_PKCS11
static int
store_pkcs11_identity(HKEY user_root, const struct sshkey *key,
- const char *provider)
+ const char *provider, char **created_identityp)
{
SECURITY_ATTRIBUTES sa = { 0, NULL, 0 };
HKEY reg = NULL, sub = NULL;
u_char *blob = NULL;
size_t blob_len;
char *thumbprint = NULL;
+ DWORD disposition = 0;
int success = 0;
+ if (created_identityp == NULL)
+ return -1;
+ *created_identityp = NULL;
sa.nLength = sizeof(sa);
if (!ConvertStringSecurityDescriptorToSecurityDescriptorW(REG_KEY_SDDL,
SDDL_REVISION_1, &sa.lpSecurityDescriptor, &sa.nLength) ||
@@ -387,8 +391,16 @@ store_pkcs11_identity(HKEY user_root, const struct sshkey *key,
RegCreateKeyExW(user_root, SSH_KEYS_ROOT, 0, NULL, 0,
KEY_WRITE | KEY_WOW64_64KEY, &sa, ®, NULL) != ERROR_SUCCESS ||
RegCreateKeyExA(reg, thumbprint, 0, NULL, 0,
- KEY_WRITE | KEY_WOW64_64KEY, &sa, &sub, NULL) != ERROR_SUCCESS ||
- RegSetValueExW(sub, NULL, 0, REG_BINARY, blob,
+ KEY_WRITE | KEY_WOW64_64KEY, &sa, &sub,
+ &disposition) != ERROR_SUCCESS) {
+ error_f("failed to persist PKCS11 identity");
+ goto out;
+ }
+ if (disposition == REG_OPENED_EXISTING_KEY) {
+ success = 1;
+ goto out;
+ }
+ if (RegSetValueExW(sub, NULL, 0, REG_BINARY, blob,
(DWORD)blob_len) != ERROR_SUCCESS ||
RegSetValueExW(sub, L"pub", 0, REG_BINARY, blob,
(DWORD)blob_len) != ERROR_SUCCESS ||
@@ -399,13 +411,15 @@ store_pkcs11_identity(HKEY user_root, const struct sshkey *key,
error_f("failed to persist PKCS11 identity");
goto out;
}
+ *created_identityp = xstrdup(thumbprint);
success = 1;
out:
if (sub != NULL) {
RegCloseKey(sub);
sub = NULL;
}
- if (!success && reg != NULL && thumbprint != NULL)
+ if (!success && disposition == REG_CREATED_NEW_KEY && reg != NULL &&
+ thumbprint != NULL)
RegDeleteTreeA(reg, thumbprint);
if (reg != NULL)
RegCloseKey(reg);
@@ -424,6 +438,7 @@ store_pkcs11_provider(HKEY user_root, struct agent_connection *con,
HKEY reg = NULL, sub = NULL;
char *epin = NULL;
DWORD epin_len = 0;
+ DWORD disposition = 0;
int success = 0;
sa.nLength = sizeof(sa);
@@ -433,7 +448,8 @@ store_pkcs11_provider(HKEY user_root, struct agent_connection *con,
RegCreateKeyExW(user_root, SSH_PKCS11_PROVIDERS_ROOT, 0, NULL, 0,
KEY_WRITE | KEY_WOW64_64KEY, &sa, ®, NULL) != ERROR_SUCCESS ||
RegCreateKeyExA(reg, provider, 0, NULL, 0,
- KEY_WRITE | KEY_WOW64_64KEY, &sa, &sub, NULL) != ERROR_SUCCESS ||
+ KEY_WRITE | KEY_WOW64_64KEY, &sa, &sub,
+ &disposition) != ERROR_SUCCESS ||
RegSetValueExW(sub, L"provider", 0, REG_BINARY,
(const BYTE *)provider, (DWORD)strlen(provider)) != ERROR_SUCCESS ||
RegSetValueExW(sub, L"pin", 0, REG_BINARY, (const BYTE *)epin,
@@ -451,7 +467,7 @@ store_pkcs11_provider(HKEY user_root, struct agent_connection *con,
RegCloseKey(sub);
sub = NULL;
}
- if (!success && reg != NULL)
+ if (!success && disposition == REG_CREATED_NEW_KEY && reg != NULL)
RegDeleteTreeA(reg, provider);
if (reg != NULL)
RegCloseKey(reg);
@@ -460,6 +476,28 @@ store_pkcs11_provider(HKEY user_root, struct agent_connection *con,
return success ? 0 : -1;
}
+static void
+rollback_pkcs11_identities(HKEY user_root, char **identities,
+ size_t nidentities)
+{
+ HKEY reg = NULL;
+ size_t i;
+
+ if (nidentities == 0)
+ return;
+ if (RegOpenKeyExW(user_root, SSH_KEYS_ROOT, 0,
+ DELETE | KEY_ENUMERATE_SUB_KEYS | KEY_WOW64_64KEY,
+ ®) != ERROR_SUCCESS) {
+ error_f("failed to open PKCS11 identities for rollback");
+ return;
+ }
+ for (i = 0; i < nidentities; i++) {
+ if (RegDeleteTreeA(reg, identities[i]) != ERROR_SUCCESS)
+ error_f("failed to roll back PKCS11 identity");
+ }
+ RegCloseKey(reg);
+}
+
static int
load_pkcs11_identities(HKEY user_root, const char *provider,
struct sshkey **token_keys, int nkeys)
@@ -862,11 +900,12 @@ process_add_smartcard_key(struct sshbuf *request, struct sshbuf *response,
struct agent_connection *con)
{
char *provider = NULL, *pin = NULL, canonical_provider[PATH_MAX] = { 0 };
- char allowed_provider[PATH_MAX];
+ char allowed_provider[PATH_MAX], *created_identity = NULL;
+ char **created_identities = NULL;
int i, j, count = 0, r = 0, request_invalid = 0, success = 0;
int cert_only = 0, identities_stored = 0;
struct sshkey **keys = NULL, **certs = NULL, *cert = NULL;
- size_t pin_len = 0, ncerts = 0;
+ size_t k, pin_len = 0, ncerts = 0, ncreated_identities = 0;
HKEY user_root = NULL;
pkcs11_init(0);
@@ -923,16 +962,8 @@ process_add_smartcard_key(struct sshbuf *request, struct sshbuf *response,
goto done;
}
- /* Replace the provider and all of its persisted identities. */
if (get_user_root(con, &user_root) != 0)
goto done;
- if (is_reg_sub_key_exists(user_root, SSH_PKCS11_PROVIDERS_ROOT,
- canonical_provider)) {
- remove_matching_subkeys_from_registry(user_root, SSH_KEYS_ROOT,
- L"comment", canonical_provider);
- remove_matching_subkeys_from_registry(user_root,
- SSH_PKCS11_PROVIDERS_ROOT, L"provider", canonical_provider);
- }
for (i = 0; i < count; i++) {
for (j = 0; j < (int)ncerts; j++) {
@@ -942,16 +973,32 @@ process_add_smartcard_key(struct sshbuf *request, struct sshbuf *response,
if (pkcs11_make_cert(keys[i], certs[j], &cert) != 0)
continue;
if (store_pkcs11_identity(user_root, cert,
- canonical_provider) != 0)
+ canonical_provider, &created_identity) != 0)
goto done;
+ if (created_identity != NULL) {
+ created_identities = xrecallocarray(created_identities,
+ ncreated_identities, ncreated_identities + 1,
+ sizeof(*created_identities));
+ created_identities[ncreated_identities++] =
+ created_identity;
+ created_identity = NULL;
+ }
sshkey_free(cert);
cert = NULL;
identities_stored++;
}
if (!cert_only && store_pkcs11_identity(user_root, keys[i],
- canonical_provider) == 0)
+ canonical_provider, &created_identity) == 0) {
+ if (created_identity != NULL) {
+ created_identities = xrecallocarray(created_identities,
+ ncreated_identities, ncreated_identities + 1,
+ sizeof(*created_identities));
+ created_identities[ncreated_identities++] =
+ created_identity;
+ created_identity = NULL;
+ }
identities_stored++;
- else if (!cert_only)
+ } else if (!cert_only)
goto done;
}
@@ -967,16 +1014,17 @@ process_add_smartcard_key(struct sshbuf *request, struct sshbuf *response,
else if (sshbuf_put_u8(response, success ? SSH_AGENT_SUCCESS : SSH_AGENT_FAILURE) != 0)
r = -1;
- if (!success && user_root != NULL && canonical_provider[0] != '\0') {
- remove_matching_subkeys_from_registry(user_root, SSH_KEYS_ROOT,
- L"comment", canonical_provider);
- remove_matching_subkeys_from_registry(user_root,
- SSH_PKCS11_PROVIDERS_ROOT, L"provider", canonical_provider);
- }
+ if (!success && user_root != NULL)
+ rollback_pkcs11_identities(user_root, created_identities,
+ ncreated_identities);
pkcs11_terminate();
sshkey_free(cert);
+ free(created_identity);
+ for (k = 0; k < ncreated_identities; k++)
+ free(created_identities[k]);
+ free(created_identities);
for (i = 0; i < count; i++)
sshkey_free(keys[i]);
free(keys);
diff --git a/regress/pesterTests/CommonUtils.psm1 b/regress/pesterTests/CommonUtils.psm1
index 67c696e99dbf..a4222185025a 100644
--- a/regress/pesterTests/CommonUtils.psm1
+++ b/regress/pesterTests/CommonUtils.psm1
@@ -67,7 +67,7 @@ function Set-FilePermission
function Add-PasswordSetting
{
param([string] $pass)
- if ($IsWindows) {
+ if ($IsWindows -or $env:OS -eq "Windows_NT") {
if (-not($env:DISPLAY)) {$env:DISPLAY = 1}
$askpass_util = Join-Path $PSScriptRoot "utilities\askpass_util\askpass_util.exe"
$env:SSH_ASKPASS=$askpass_util
diff --git a/regress/pesterTests/KeyUtils.Tests.ps1 b/regress/pesterTests/KeyUtils.Tests.ps1
index 1194d81e2fa9..69827d6f4318 100644
--- a/regress/pesterTests/KeyUtils.Tests.ps1
+++ b/regress/pesterTests/KeyUtils.Tests.ps1
@@ -215,6 +215,7 @@ Describe "E2E scenarios for ssh key management" -Tags "CI" {
}
}
AfterAll{$tC++}
+ AfterEach { Remove-PasswordSetting }
# Executing ssh-agent will start agent service
# This is to support typical Unix scenarios where
@@ -310,11 +311,9 @@ Describe "E2E scenarios for ssh key management" -Tags "CI" {
$testPin = $env:OPENSSH_TEST_PKCS11_PIN
if (-not $testPin) { $testPin = $pkcs11Pin }
Add-PasswordSetting -Pass $testPin
-
+ $env:SSH_ASKPASS_REQUIRE = "force"
ssh-add -s "$pkcs11Path"
$LASTEXITCODE | Should Be 0
- #remove SSH_ASKPASS
- Remove-PasswordSetting
#ensure added keys are listed
$allkeys = ssh-add -L
@@ -347,11 +346,9 @@ Describe "E2E scenarios for ssh key management" -Tags "CI" {
}
$testPin = $env:OPENSSH_TEST_PKCS11_PIN
if (-not $testPin) { $testPin = $pkcs11Pin }
- Add-PasswordSetting -Pass $testPin
-
$ca = Join-Path $testDir "pkcs11-ca"
Remove-Item "$ca*" -Force -ErrorAction SilentlyContinue
- & ssh-keygen -q -t ed25519 -N '""' -f $ca
+ & ssh-keygen -q -t ed25519 -N $keypassphrase -f $ca
$LASTEXITCODE | Should Be 0
$certPaths = @()
@@ -360,19 +357,29 @@ Describe "E2E scenarios for ssh key management" -Tags "CI" {
foreach ($publicKeyPath in $publicKeyPaths) {
$copiedPublicKeyPath = Join-Path $testDir "pkcs11-$serial.pub"
Copy-Item $publicKeyPath $copiedPublicKeyPath -Force
- & ssh-keygen -q -s $ca -I "pkcs11-$serial" -n $env:USERNAME `
- -z $serial $copiedPublicKeyPath
+ & ssh-keygen -q -s $ca -P $keypassphrase -I "pkcs11-$serial" `
+ -n $env:USERNAME -z $serial $copiedPublicKeyPath
$LASTEXITCODE | Should Be 0
$copiedPublicKeyPaths += $copiedPublicKeyPath
$certPaths += $copiedPublicKeyPath.Replace(".pub", "-cert.pub")
$serial++
}
- & ssh-keygen -q -s $ca -I "pkcs11-unmatched" -n $env:USERNAME `
- -z 999 "$ca.pub"
+ & ssh-keygen -q -s $ca -P $keypassphrase -I "pkcs11-unmatched" `
+ -n $env:USERNAME -z 999 "$ca.pub"
$LASTEXITCODE | Should Be 0
$unmatchedCertPath = "$ca-cert.pub"
$associatedCertPaths = $certPaths + $unmatchedCertPath
+ $sequentialPublicKeyPath = Join-Path $testDir "pkcs11-sequential.pub"
+ Copy-Item $publicKeyPaths[0] $sequentialPublicKeyPath -Force
+ & ssh-keygen -q -s $ca -P $keypassphrase -I "pkcs11-sequential" `
+ -n $env:USERNAME -z 1000 $sequentialPublicKeyPath
+ $LASTEXITCODE | Should Be 0
+ $sequentialCertPath = $sequentialPublicKeyPath.Replace(".pub", "-cert.pub")
+
+ Add-PasswordSetting -Pass $testPin
+ $env:SSH_ASKPASS_REQUIRE = "force"
+
$addArguments = @("-s", $pkcs11Path) + $associatedCertPaths
& ssh-add @addArguments
$LASTEXITCODE | Should Be 0
@@ -398,22 +405,68 @@ Describe "E2E scenarios for ssh key management" -Tags "CI" {
ssh-add -D
$LASTEXITCODE | Should Be 0
- $addArguments = @("-s", $pkcs11Path, "-C") + $associatedCertPaths
+
+ # Separate additions for the same token key must merge certificates.
+ $addArguments = @("-s", $pkcs11Path, "-C", $certPaths[0])
+ & ssh-add @addArguments
+ $LASTEXITCODE | Should Be 0
+ $addArguments = @("-s", $pkcs11Path, "-C", $sequentialCertPath)
& ssh-add @addArguments
$LASTEXITCODE | Should Be 0
$allKeys = @(ssh-add -L)
- $allKeys.Count | Should Be $certPaths.Count
- foreach ($certPath in $certPaths) {
+ foreach ($certPath in @($certPaths[0], $sequentialCertPath)) {
$keyBlob = (Get-Content $certPath).Split(' ')[1]
@($allKeys | Where-Object { $_.Contains($keyBlob) }).Count | Should Be 1
& ssh-add -T $certPath
$LASTEXITCODE | Should Be 0
}
+ # Re-adding an exact certificate is successful and idempotent.
+ & ssh-add @addArguments
+ $LASTEXITCODE | Should Be 0
+ $sequentialCertBlob = (Get-Content $sequentialCertPath).Split(' ')[1]
+ @((ssh-add -L) | Where-Object { $_.Contains($sequentialCertBlob) }).Count |
+ Should Be 1
+
+ ssh-add -D
+ $LASTEXITCODE | Should Be 0
+
+ # cert-only applies to this request and must preserve plain identities.
+ & ssh-add -s $pkcs11Path
+ $LASTEXITCODE | Should Be 0
+ & ssh-add -s $pkcs11Path -C $certPaths[0]
+ $LASTEXITCODE | Should Be 0
+ $allKeys = @(ssh-add -L)
+ foreach ($keyPath in $copiedPublicKeyPaths + $certPaths[0]) {
+ $keyBlob = (Get-Content $keyPath).Split(' ')[1]
+ @($allKeys | Where-Object { $_.Contains($keyBlob) }).Count | Should Be 1
+ }
+
+ # A failed unmatched add must not change existing persisted identities.
+ $identitiesBefore = @(ssh-add -L | Sort-Object)
+ & ssh-add -s $pkcs11Path -C $unmatchedCertPath
+ $LASTEXITCODE | Should Not Be 0
+ $identitiesAfter = @(ssh-add -L | Sort-Object)
+ @(Compare-Object $identitiesBefore $identitiesAfter).Count | Should Be 0
+
+ Restart-Service ssh-agent
+ WaitForStatus -ServiceName ssh-agent -Status "Running"
+ foreach ($keyPath in $copiedPublicKeyPaths + $certPaths[0]) {
+ & ssh-add -T $keyPath
+ $LASTEXITCODE | Should Be 0
+ }
+
+ & ssh-add -d $certPaths[0]
+ $LASTEXITCODE | Should Be 0
+ foreach ($keyPath in $copiedPublicKeyPaths) {
+ $keyBlob = (Get-Content $keyPath).Split(' ')[1]
+ @((ssh-add -L) | Where-Object { $_.Contains($keyBlob) }).Count |
+ Should Be 1
+ }
+
& ssh-add -e $pkcs11Path
$LASTEXITCODE | Should Be 0
@(ssh-add -L) -match "The agent has no identities." | Should Be $true
- Remove-PasswordSetting
}
}
diff --git a/regress/pesterTests/README.md b/regress/pesterTests/README.md
index 4d9de131c5b3..b0885e3249fa 100644
--- a/regress/pesterTests/README.md
+++ b/regress/pesterTests/README.md
@@ -79,4 +79,5 @@ The test creates short-lived OpenSSH certificates for the supplied public
keys. It verifies plain and certificate identities, signing, agent service
restart, individual certificate deletion, cert-only loading, an unmatched
certificate, and provider removal. Never use production token credentials in
-CI.
+CI. PKCS#11 PIN input is forced through the test askpass helper so an
+unattended run cannot block on an interactive prompt.
From fea4434e03fc590d951f1d9aac91f0f8d3645a83 Mon Sep 17 00:00:00 2001
From: Sebastian Ott <174621899+VSSOtt@users.noreply.github.com>
Date: Fri, 17 Jul 2026 15:10:09 +0200
Subject: [PATCH 3/8] Restore software certificate deletion
Try the regular fingerprint-backed Registry entry before falling back to the PKCS#11 certificate identity name. This preserves individual removal for both software-backed and token-backed certificates when PKCS#11 support is enabled.
Add Windows regression coverage for RSA and ECDSA software certificates.
---
.../win32compat/ssh-agent/keyagent-request.c | 74 +++++++++++++++++--
regress/pesterTests/KeyUtils.Tests.ps1 | 50 +++++++++++++
2 files changed, 116 insertions(+), 8 deletions(-)
diff --git a/contrib/win32/win32compat/ssh-agent/keyagent-request.c b/contrib/win32/win32compat/ssh-agent/keyagent-request.c
index 721423a9742f..099a790baf15 100644
--- a/contrib/win32/win32compat/ssh-agent/keyagent-request.c
+++ b/contrib/win32/win32compat/ssh-agent/keyagent-request.c
@@ -824,14 +824,59 @@ process_sign_request(struct sshbuf* request, struct sshbuf* response, struct age
return r;
}
+static LSTATUS
+delete_matching_identity(HKEY root, const char *name, const u_char *blob,
+ size_t blob_len)
+{
+ HKEY sub = NULL;
+ u_char *stored_blob = NULL;
+ DWORD stored_blob_len = 0;
+ LSTATUS status;
+
+ status = RegOpenKeyExA(root, name, 0,
+ KEY_QUERY_VALUE | KEY_WOW64_64KEY, &sub);
+ if (status != ERROR_SUCCESS)
+ return status;
+ status = RegQueryValueExW(sub, L"pub", NULL, NULL, NULL,
+ &stored_blob_len);
+ if (status != ERROR_SUCCESS)
+ goto out;
+ if (stored_blob_len > MAX_MESSAGE_SIZE) {
+ status = ERROR_INVALID_DATA;
+ goto out;
+ }
+ stored_blob = xmalloc(stored_blob_len == 0 ? 1 : stored_blob_len);
+ status = RegQueryValueExW(sub, L"pub", NULL, NULL, stored_blob,
+ &stored_blob_len);
+ if (status != ERROR_SUCCESS)
+ goto out;
+ if (stored_blob_len != blob_len ||
+ memcmp(stored_blob, blob, blob_len) != 0) {
+ status = ERROR_FILE_NOT_FOUND;
+ goto out;
+ }
+ RegCloseKey(sub);
+ sub = NULL;
+ status = RegDeleteTreeA(root, name);
+ out:
+ free(stored_blob);
+ if (sub != NULL)
+ RegCloseKey(sub);
+ return status;
+}
+
int
process_remove_key(struct sshbuf* request, struct sshbuf* response, struct agent_connection* con)
{
HKEY user_root = 0, root = 0;
char *blob, *thumbprint = NULL;
+#ifdef ENABLE_PKCS11
+ char *pkcs11_name = NULL;
+#endif
size_t blen;
int r = 0, success = 0, request_invalid = 0;
struct sshkey *key = NULL;
+ LSTATUS status;
if (sshbuf_get_string_direct(request, &blob, &blen) != 0 ||
sshkey_from_blob(blob, blen, &key) != 0) {
@@ -839,16 +884,26 @@ process_remove_key(struct sshbuf* request, struct sshbuf* response, struct agent
goto done;
}
- if ((thumbprint =
-#ifdef ENABLE_PKCS11
- pkcs11_identity_name(key, (const u_char *)blob, blen)) == NULL ||
-#else
- sshkey_fingerprint(key, SSH_FP_HASH_DEFAULT, SSH_FP_DEFAULT)) == NULL ||
-#endif
+ if ((thumbprint = sshkey_fingerprint(key, SSH_FP_HASH_DEFAULT,
+ SSH_FP_DEFAULT)) == NULL ||
get_user_root(con, &user_root) != 0 ||
RegOpenKeyExW(user_root, SSH_KEYS_ROOT, 0,
- DELETE | KEY_ENUMERATE_SUB_KEYS | KEY_QUERY_VALUE | KEY_WOW64_64KEY, &root) != 0 ||
- RegDeleteTreeA(root, thumbprint) != 0)
+ DELETE | KEY_ENUMERATE_SUB_KEYS | KEY_QUERY_VALUE |
+ KEY_WOW64_64KEY, &root) != 0)
+ goto done;
+ status = delete_matching_identity(root, thumbprint,
+ (const u_char *)blob, blen);
+#ifdef ENABLE_PKCS11
+ if (status == ERROR_FILE_NOT_FOUND && sshkey_is_cert(key)) {
+ pkcs11_name = pkcs11_identity_name(key,
+ (const u_char *)blob, blen);
+ if (pkcs11_name == NULL)
+ goto done;
+ status = delete_matching_identity(root, pkcs11_name,
+ (const u_char *)blob, blen);
+ }
+#endif
+ if (status != ERROR_SUCCESS)
goto done;
success = 1;
done:
@@ -866,6 +921,9 @@ process_remove_key(struct sshbuf* request, struct sshbuf* response, struct agent
RegCloseKey(root);
if (thumbprint)
free(thumbprint);
+#ifdef ENABLE_PKCS11
+ free(pkcs11_name);
+#endif
return r;
}
int
diff --git a/regress/pesterTests/KeyUtils.Tests.ps1 b/regress/pesterTests/KeyUtils.Tests.ps1
index 69827d6f4318..6385b80d3cd2 100644
--- a/regress/pesterTests/KeyUtils.Tests.ps1
+++ b/regress/pesterTests/KeyUtils.Tests.ps1
@@ -301,6 +301,56 @@ Describe "E2E scenarios for ssh key management" -Tags "CI" {
ValidateRegistryACL -count $allkeys.count
}
+ It "$tC.$tI - ssh-add - remove software certificates" {
+ if ($NoLibreSSL) {
+ Write-Host "skipping software certificate removal test without LibreSSL"
+ return
+ }
+
+ $ca = Join-Path $testDir "software-cert-ca"
+ $nullFile = Join-Path $testDir "$tC.$tI.nullfile"
+ $null > $nullFile
+ Remove-Item "$ca*" -Force -ErrorAction SilentlyContinue
+ & ssh-keygen -q -t ed25519 -N $keypassphrase -f $ca
+ $LASTEXITCODE | Should Be 0
+
+ try {
+ ssh-add -D
+ $LASTEXITCODE | Should Be 0
+ Add-PasswordSetting -Pass $keypassphrase
+ $env:SSH_ASKPASS_REQUIRE = "force"
+
+ foreach ($type in @("rsa", "ecdsa")) {
+ $keyPath = Join-Path $testDir "id_$type"
+ & ssh-keygen -q -s $ca -P $keypassphrase `
+ -I "software-$type" -n $env:USERNAME "$keyPath.pub"
+ $LASTEXITCODE | Should Be 0
+ $certPath = "$keyPath-cert.pub"
+
+ cmd /c "ssh-add `"$keyPath`" < `"$nullFile`""
+ $LASTEXITCODE | Should Be 0
+ & ssh-add -T $certPath
+ $LASTEXITCODE | Should Be 0
+
+ $certBlob = (Get-Content $certPath).Split(' ')[1]
+ @((ssh-add -L) | Where-Object { $_.Contains($certBlob) }).Count |
+ Should Be 1
+ & ssh-add -d $certPath
+ $LASTEXITCODE | Should Be 0
+ @((ssh-add -L) | Where-Object { $_.Contains($certBlob) }).Count |
+ Should Be 0
+ }
+ }
+ finally {
+ ssh-add -D | Out-Null
+ Remove-Item "$ca*" -Force -ErrorAction SilentlyContinue
+ foreach ($type in @("rsa", "ecdsa")) {
+ Remove-Item (Join-Path $testDir "id_$type-cert.pub") `
+ -Force -ErrorAction SilentlyContinue
+ }
+ }
+ }
+
It "$tC.$tI - ssh-add - pkcs11 library (if available)" {
$pkcs11Path = $env:OPENSSH_TEST_PKCS11_PROVIDER
if (-not $pkcs11Path) {
From a9c25bfcd2d86337a85272d58b7ffc5e70f3afb7 Mon Sep 17 00:00:00 2001
From: Sebastian Ott <174621899+VSSOtt@users.noreply.github.com>
Date: Fri, 17 Jul 2026 15:16:20 +0200
Subject: [PATCH 4/8] Keep unrelated PKCS#11 providers from blocking signatures
Continue past persisted providers that cannot be loaded while rebuilding the transient PKCS#11 key set for a sign request. The requested identity still fails when its backing key is unavailable, while software keys and identities from other providers remain usable.
Cover signing with a stale provider record in the Windows PKCS#11 E2E tests.
---
.../win32compat/ssh-agent/keyagent-request.c | 89 +++++++++--------
regress/pesterTests/KeyUtils.Tests.ps1 | 98 +++++++++++++++++++
2 files changed, 147 insertions(+), 40 deletions(-)
diff --git a/contrib/win32/win32compat/ssh-agent/keyagent-request.c b/contrib/win32/win32compat/ssh-agent/keyagent-request.c
index 099a790baf15..e9ad1f33ea2e 100644
--- a/contrib/win32/win32compat/ssh-agent/keyagent-request.c
+++ b/contrib/win32/win32compat/ssh-agent/keyagent-request.c
@@ -601,6 +601,32 @@ load_pkcs11_identities(HKEY user_root, const char *provider,
RegCloseKey(root);
return loaded;
}
+
+static void
+free_pkcs11_sign_provider(char **providerp, char **pinp, DWORD pin_len,
+ char **epinp, DWORD epin_len, struct sshkey ***keysp, int nkeys)
+{
+ int i;
+
+ if (*keysp != NULL) {
+ for (i = 0; i < nkeys; i++)
+ sshkey_free((*keysp)[i]);
+ free(*keysp);
+ *keysp = NULL;
+ }
+ free(*providerp);
+ *providerp = NULL;
+ if (*pinp != NULL) {
+ SecureZeroMemory(*pinp, pin_len);
+ free(*pinp);
+ *pinp = NULL;
+ }
+ if (*epinp != NULL) {
+ SecureZeroMemory(*epinp, epin_len);
+ free(*epinp);
+ *epinp = NULL;
+ }
+}
#endif /* ENABLE_PKCS11 */
static int sign_blob(const struct sshkey *pubkey, u_char ** sig, size_t *siglen,
@@ -692,10 +718,11 @@ process_sign_request(struct sshbuf* request, struct sshbuf* response, struct age
struct sshkey *key = NULL;
#ifdef ENABLE_PKCS11
- int i, count = 0, index = 0, loaded = 0;
+ int count = 0, index = 0, loaded = 0;
wchar_t sub_name[MAX_KEY_LENGTH];
DWORD sub_name_len = MAX_KEY_LENGTH;
- DWORD pin_len, epin_len, provider_len;
+ DWORD pin_len = 0, epin_len = 0, provider_len = 0;
+ DWORD epin_alloc_len = 0;
char *pin = NULL, *npin = NULL, *epin = NULL, *provider = NULL;
HKEY root = 0, sub = 0, user_root = 0;
struct sshkey **keys = NULL;
@@ -713,6 +740,7 @@ process_sign_request(struct sshbuf* request, struct sshbuf* response, struct age
while (1) {
sub_name_len = MAX_KEY_LENGTH;
+ pin_len = epin_len = provider_len = 0;
if (sub) {
RegCloseKey(sub);
sub = NULL;
@@ -721,43 +749,37 @@ process_sign_request(struct sshbuf* request, struct sshbuf* response, struct age
if (RegOpenKeyExW(root, sub_name, 0, KEY_QUERY_VALUE | KEY_WOW64_64KEY, &sub) == 0 &&
RegQueryValueExW(sub, L"provider", 0, NULL, NULL, &provider_len) == 0 &&
RegQueryValueExW(sub, L"pin", 0, NULL, NULL, &epin_len) == 0) {
- if ((epin = malloc(epin_len + 1)) == NULL ||
+ epin_alloc_len = epin_len;
+ if ((epin = malloc(epin_alloc_len + 1)) == NULL ||
(provider = malloc(provider_len + 1)) == NULL ||
RegQueryValueExW(sub, L"provider", 0, NULL, provider, &provider_len) != 0 ||
- RegQueryValueExW(sub, L"pin", 0, NULL, epin, &epin_len) != 0)
- goto done;
+ RegQueryValueExW(sub, L"pin", 0, NULL, epin, &epin_len) != 0) {
+ free_pkcs11_sign_provider(&provider, &pin, pin_len,
+ &epin, epin_alloc_len, &keys, count);
+ continue;
+ }
provider[provider_len] = '\0';
epin[epin_len] = '\0';
if (convert_blob(con, epin, epin_len, &pin, &pin_len, 0) != 0 ||
(npin = realloc(pin, pin_len + 1)) == NULL) {
- goto done;
+ free_pkcs11_sign_provider(&provider, &pin, pin_len,
+ &epin, epin_len, &keys, count);
+ continue;
}
pin = npin;
pin[pin_len] = '\0';
count = pkcs11_add_provider(provider, pin, &keys, NULL);
- if (count <= 0)
- goto done;
+ if (count <= 0) {
+ free_pkcs11_sign_provider(&provider, &pin, pin_len,
+ &epin, epin_len, &keys, count);
+ continue;
+ }
loaded = load_pkcs11_identities(user_root, provider,
keys, count);
- for (i = 0; i < count; i++)
- sshkey_free(keys[i]);
- free(keys);
- keys = NULL;
+ free_pkcs11_sign_provider(&provider, &pin, pin_len,
+ &epin, epin_len, &keys, count);
if (loaded < 0)
goto done;
- if (provider)
- free(provider);
- if (pin) {
- SecureZeroMemory(pin, (DWORD)pin_len);
- free(pin);
- }
- if (epin) {
- SecureZeroMemory(epin, (DWORD)epin_len);
- free(epin);
- }
- provider = NULL;
- pin = NULL;
- epin = NULL;
}
}
else
@@ -797,23 +819,10 @@ process_sign_request(struct sshbuf* request, struct sshbuf* response, struct age
if (signature)
free(signature);
#ifdef ENABLE_PKCS11
- if (keys != NULL) {
- for (i = 0; i < count; i++)
- sshkey_free(keys[i]);
- free(keys);
- }
+ free_pkcs11_sign_provider(&provider, &pin, pin_len, &epin, epin_len,
+ &keys, count);
del_all_keys();
pkcs11_terminate();
- if (provider)
- free(provider);
- if (pin) {
- SecureZeroMemory(pin, (DWORD)pin_len);
- free(pin);
- }
- if (epin) {
- SecureZeroMemory(epin, (DWORD)epin_len);
- free(epin);
- }
if (user_root)
RegCloseKey(user_root);
if (root)
diff --git a/regress/pesterTests/KeyUtils.Tests.ps1 b/regress/pesterTests/KeyUtils.Tests.ps1
index 6385b80d3cd2..5296ab77263a 100644
--- a/regress/pesterTests/KeyUtils.Tests.ps1
+++ b/regress/pesterTests/KeyUtils.Tests.ps1
@@ -518,6 +518,104 @@ Describe "E2E scenarios for ssh key management" -Tags "CI" {
$LASTEXITCODE | Should Be 0
@(ssh-add -L) -match "The agent has no identities." | Should Be $true
}
+
+ It "$tC.$tI - ssh-add - stale pkcs11 provider isolation (if configured)" {
+ $pkcs11Path = $env:OPENSSH_TEST_PKCS11_PROVIDER
+ $publicKeyPaths = @($env:OPENSSH_TEST_PKCS11_PUBLIC_KEYS -split ';' |
+ Where-Object { $_ })
+ if (-not $pkcs11Path -or -not (Test-Path $pkcs11Path) -or
+ $publicKeyPaths.Count -eq 0) {
+ Write-Host "skipping stale provider test because provider and public keys are not configured"
+ return
+ }
+
+ $testPin = $env:OPENSSH_TEST_PKCS11_PIN
+ if (-not $testPin) { $testPin = $pkcs11Pin }
+ $softwareKeyPath = Join-Path $testDir "id_rsa"
+ $unavailableKeyPath = Join-Path $testDir "id_ecdsa.pub"
+ $nullFile = Join-Path $testDir "$tC.$tI.nullfile"
+ $null > $nullFile
+ $providerRoot = $null
+ $validProviderKey = $null
+ $staleProviderKey = $null
+ $staleProviderPath = Join-Path $testDir `
+ "nonexistent\openssh-stale-provider.dll"
+ $staleProvider = [IO.Path]::GetFullPath($staleProviderPath).Replace(
+ '\', '/')
+ $corruptProviderPath = Join-Path $testDir `
+ "nonexistent\openssh-corrupt-provider.dll"
+ $corruptProvider = [IO.Path]::GetFullPath(
+ $corruptProviderPath).Replace('\', '/')
+
+ try {
+ ssh-add -D
+ $LASTEXITCODE | Should Be 0
+
+ Add-PasswordSetting -Pass $keypassphrase
+ $env:SSH_ASKPASS_REQUIRE = "force"
+ cmd /c "ssh-add `"$softwareKeyPath`" < `"$nullFile`""
+ $LASTEXITCODE | Should Be 0
+ Remove-PasswordSetting
+
+ Add-PasswordSetting -Pass $testPin
+ $env:SSH_ASKPASS_REQUIRE = "force"
+ & ssh-add -s $pkcs11Path
+ $LASTEXITCODE | Should Be 0
+ & ssh-add -T $softwareKeyPath
+ $LASTEXITCODE | Should Be 0
+ & ssh-add -T $publicKeyPaths[0]
+ $LASTEXITCODE | Should Be 0
+
+ $providerRootPath = "$currentUserSid\Software\OpenSSH\Agent\PKCS11_Providers"
+ $providerRoot = [Microsoft.Win32.Registry]::Users.OpenSubKey(
+ $providerRootPath, $true)
+ $providerRoot | Should Not Be $null
+ $canonicalProvider = [IO.Path]::GetFullPath($pkcs11Path).Replace('\', '/')
+ $validProviderKey = $providerRoot.OpenSubKey($canonicalProvider)
+ $validProviderKey | Should Not Be $null
+ $encryptedPin = $validProviderKey.GetValue("pin")
+ $encryptedPin -is [byte[]] | Should Be $true
+
+ $staleProviderKey = $providerRoot.CreateSubKey($staleProvider)
+ $staleProviderKey.SetValue("provider",
+ [Text.Encoding]::UTF8.GetBytes($staleProvider),
+ [Microsoft.Win32.RegistryValueKind]::Binary)
+ $staleProviderKey.SetValue("pin", $encryptedPin,
+ [Microsoft.Win32.RegistryValueKind]::Binary)
+
+ & ssh-add -T $softwareKeyPath
+ $LASTEXITCODE | Should Be 0
+ & ssh-add -T $publicKeyPaths[0]
+ $LASTEXITCODE | Should Be 0
+ & ssh-add -T $unavailableKeyPath
+ $LASTEXITCODE | Should Not Be 0
+
+ $staleProviderKey.Dispose()
+ $staleProviderKey = $providerRoot.CreateSubKey($corruptProvider)
+ $staleProviderKey.SetValue("provider",
+ [Text.Encoding]::UTF8.GetBytes($corruptProvider),
+ [Microsoft.Win32.RegistryValueKind]::Binary)
+ $staleProviderKey.SetValue("pin",
+ [Text.Encoding]::UTF8.GetBytes("invalid encrypted pin"),
+ [Microsoft.Win32.RegistryValueKind]::Binary)
+
+ & ssh-add -T $softwareKeyPath
+ $LASTEXITCODE | Should Be 0
+ & ssh-add -T $publicKeyPaths[0]
+ $LASTEXITCODE | Should Be 0
+ }
+ finally {
+ if ($staleProviderKey) { $staleProviderKey.Dispose() }
+ if ($validProviderKey) { $validProviderKey.Dispose() }
+ if ($providerRoot) {
+ $providerRoot.DeleteSubKeyTree($staleProvider, $false)
+ $providerRoot.DeleteSubKeyTree($corruptProvider, $false)
+ $providerRoot.Dispose()
+ }
+ ssh-add -D | Out-Null
+ Remove-PasswordSetting
+ }
+ }
}
Context "$tC ssh-keygen known_hosts operations" {
From 133e9f5bf062679060b9a3f26e2274027f057a1e Mon Sep 17 00:00:00 2001
From: Sebastian Ott <174621899+VSSOtt@users.noreply.github.com>
Date: Fri, 17 Jul 2026 15:37:15 +0200
Subject: [PATCH 5/8] Use PKCS#11 labels for Windows agent identities
Persist the PKCS#11 provider association separately from the user-visible identity comment and use the label returned by the provider. Empty labels continue to fall back to the canonical provider path.
Keep legacy Registry entries readable and removable, migrate their metadata on a repeated add, and preserve labels across merged certificate additions and service restarts.
---
contrib/win32/win32compat/pkcs11-cert.c | 6 +
contrib/win32/win32compat/pkcs11-cert.h | 1 +
.../win32compat/ssh-agent/keyagent-request.c | 318 ++++++++++++++----
regress/pesterTests/KeyUtils.Tests.ps1 | 135 ++++++++
regress/pesterTests/README.md | 3 +
.../unittests/win32compat/pkcs11_cert_tests.c | 14 +
6 files changed, 418 insertions(+), 59 deletions(-)
diff --git a/contrib/win32/win32compat/pkcs11-cert.c b/contrib/win32/win32compat/pkcs11-cert.c
index e88aa5eab7f7..ff789e7b96b8 100644
--- a/contrib/win32/win32compat/pkcs11-cert.c
+++ b/contrib/win32/win32compat/pkcs11-cert.c
@@ -48,6 +48,12 @@ pkcs11_identity_name(const struct sshkey *key, const u_char *blob,
return name;
}
+const char *
+pkcs11_identity_comment(const char *provider, const char *label)
+{
+ return label == NULL || *label == '\0' ? provider : label;
+}
+
void
free_pkcs11_certs(struct sshkey **certs, size_t ncerts)
{
diff --git a/contrib/win32/win32compat/pkcs11-cert.h b/contrib/win32/win32compat/pkcs11-cert.h
index 42f0c750e8d6..c6019672cc61 100644
--- a/contrib/win32/win32compat/pkcs11-cert.h
+++ b/contrib/win32/win32compat/pkcs11-cert.h
@@ -22,6 +22,7 @@
#define AGENT_MAX_EXT_CERTS 1024
char *pkcs11_identity_name(const struct sshkey *, const u_char *, size_t);
+const char *pkcs11_identity_comment(const char *, const char *);
int parse_pkcs11_add_constraints(struct sshbuf *, int *,
struct sshkey ***, size_t *);
void free_pkcs11_certs(struct sshkey **, size_t);
diff --git a/contrib/win32/win32compat/ssh-agent/keyagent-request.c b/contrib/win32/win32compat/ssh-agent/keyagent-request.c
index e9ad1f33ea2e..b98d34cbc82d 100644
--- a/contrib/win32/win32compat/ssh-agent/keyagent-request.c
+++ b/contrib/win32/win32compat/ssh-agent/keyagent-request.c
@@ -367,21 +367,99 @@ process_add_identity(struct sshbuf* request, struct sshbuf* response, struct age
}
#ifdef ENABLE_PKCS11
+struct pkcs11_identity_change {
+ char *name;
+ int created;
+ int had_provider;
+ DWORD provider_type;
+ u_char *provider;
+ DWORD provider_len;
+ int had_comment;
+ DWORD comment_type;
+ u_char *comment;
+ DWORD comment_len;
+};
+
+static void
+free_pkcs11_identity_change(struct pkcs11_identity_change *change)
+{
+ if (change == NULL)
+ return;
+ free(change->name);
+ free(change->provider);
+ free(change->comment);
+ free(change);
+}
+
+static int
+read_optional_reg_value(HKEY key, const wchar_t *name, int *presentp,
+ DWORD *typep, u_char **datap, DWORD *lenp)
+{
+ LSTATUS status;
+
+ *presentp = 0;
+ *datap = NULL;
+ *lenp = 0;
+ status = RegQueryValueExW(key, name, NULL, typep, NULL, lenp);
+ if (status == ERROR_FILE_NOT_FOUND)
+ return 0;
+ if (status != ERROR_SUCCESS || *lenp > MAX_MESSAGE_SIZE)
+ return -1;
+ *datap = xmalloc(*lenp == 0 ? 1 : *lenp);
+ if (RegQueryValueExW(key, name, NULL, typep, *datap,
+ lenp) != ERROR_SUCCESS) {
+ free(*datap);
+ *datap = NULL;
+ return -1;
+ }
+ *presentp = 1;
+ return 0;
+}
+
+static int
+restore_optional_reg_value(HKEY key, const wchar_t *name, int present,
+ DWORD type, const u_char *data, DWORD len)
+{
+ LSTATUS status;
+
+ if (present)
+ return RegSetValueExW(key, name, 0, type, data, len) ==
+ ERROR_SUCCESS ? 0 : -1;
+ status = RegDeleteValueW(key, name);
+ return status == ERROR_SUCCESS || status == ERROR_FILE_NOT_FOUND ? 0 : -1;
+}
+
+static int
+restore_pkcs11_identity_metadata(HKEY key,
+ const struct pkcs11_identity_change *change)
+{
+ int r1, r2;
+
+ r1 = restore_optional_reg_value(key, L"provider",
+ change->had_provider, change->provider_type, change->provider,
+ change->provider_len);
+ r2 = restore_optional_reg_value(key, L"comment", change->had_comment,
+ change->comment_type, change->comment, change->comment_len);
+ return r1 == 0 && r2 == 0 ? 0 : -1;
+}
+
static int
store_pkcs11_identity(HKEY user_root, const struct sshkey *key,
- const char *provider, char **created_identityp)
+ const char *provider, const char *comment,
+ struct pkcs11_identity_change **changep)
{
SECURITY_ATTRIBUTES sa = { 0, NULL, 0 };
HKEY reg = NULL, sub = NULL;
u_char *blob = NULL;
size_t blob_len;
char *thumbprint = NULL;
+ struct pkcs11_identity_change *change = NULL;
DWORD disposition = 0;
int success = 0;
- if (created_identityp == NULL)
+ if (changep == NULL || provider == NULL || comment == NULL)
return -1;
- *created_identityp = NULL;
+ *changep = NULL;
sa.nLength = sizeof(sa);
if (!ConvertStringSecurityDescriptorToSecurityDescriptorW(REG_KEY_SDDL,
SDDL_REVISION_1, &sa.lpSecurityDescriptor, &sa.nLength) ||
@@ -391,27 +469,54 @@ store_pkcs11_identity(HKEY user_root, const struct sshkey *key,
RegCreateKeyExW(user_root, SSH_KEYS_ROOT, 0, NULL, 0,
KEY_WRITE | KEY_WOW64_64KEY, &sa, ®, NULL) != ERROR_SUCCESS ||
RegCreateKeyExA(reg, thumbprint, 0, NULL, 0,
- KEY_WRITE | KEY_WOW64_64KEY, &sa, &sub,
+ KEY_WRITE | KEY_QUERY_VALUE | KEY_WOW64_64KEY, &sa, &sub,
&disposition) != ERROR_SUCCESS) {
error_f("failed to persist PKCS11 identity");
goto out;
}
+ change = xcalloc(1, sizeof(*change));
+ change->name = xstrdup(thumbprint);
if (disposition == REG_OPENED_EXISTING_KEY) {
- success = 1;
- goto out;
- }
- if (RegSetValueExW(sub, NULL, 0, REG_BINARY, blob,
- (DWORD)blob_len) != ERROR_SUCCESS ||
- RegSetValueExW(sub, L"pub", 0, REG_BINARY, blob,
- (DWORD)blob_len) != ERROR_SUCCESS ||
- RegSetValueExW(sub, L"type", 0, REG_DWORD,
- (const BYTE *)&key->type, sizeof(key->type)) != ERROR_SUCCESS ||
- RegSetValueExW(sub, L"comment", 0, REG_BINARY,
- (const BYTE *)provider, (DWORD)strlen(provider)) != ERROR_SUCCESS) {
- error_f("failed to persist PKCS11 identity");
- goto out;
+ if (read_optional_reg_value(sub, L"provider",
+ &change->had_provider, &change->provider_type,
+ &change->provider, &change->provider_len) != 0 ||
+ read_optional_reg_value(sub, L"comment",
+ &change->had_comment, &change->comment_type,
+ &change->comment, &change->comment_len) != 0) {
+ error_f("failed to read PKCS11 identity metadata");
+ goto out;
+ }
+ if (RegSetValueExW(sub, L"provider", 0, REG_BINARY,
+ (const BYTE *)provider, (DWORD)strlen(provider)) !=
+ ERROR_SUCCESS ||
+ RegSetValueExW(sub, L"comment", 0, REG_BINARY,
+ (const BYTE *)comment, (DWORD)strlen(comment)) !=
+ ERROR_SUCCESS) {
+ error_f("failed to update PKCS11 identity metadata");
+ if (restore_pkcs11_identity_metadata(sub, change) != 0)
+ error_f("failed to restore PKCS11 identity metadata");
+ goto out;
+ }
+ } else {
+ change->created = 1;
+ if (RegSetValueExW(sub, NULL, 0, REG_BINARY, blob,
+ (DWORD)blob_len) != ERROR_SUCCESS ||
+ RegSetValueExW(sub, L"pub", 0, REG_BINARY, blob,
+ (DWORD)blob_len) != ERROR_SUCCESS ||
+ RegSetValueExW(sub, L"type", 0, REG_DWORD,
+ (const BYTE *)&key->type, sizeof(key->type)) != ERROR_SUCCESS ||
+ RegSetValueExW(sub, L"provider", 0, REG_BINARY,
+ (const BYTE *)provider, (DWORD)strlen(provider)) !=
+ ERROR_SUCCESS ||
+ RegSetValueExW(sub, L"comment", 0, REG_BINARY,
+ (const BYTE *)comment, (DWORD)strlen(comment)) !=
+ ERROR_SUCCESS) {
+ error_f("failed to persist PKCS11 identity");
+ goto out;
+ }
}
- *created_identityp = xstrdup(thumbprint);
+ *changep = change;
+ change = NULL;
success = 1;
out:
if (sub != NULL) {
@@ -425,6 +530,7 @@ store_pkcs11_identity(HKEY user_root, const struct sshkey *key,
RegCloseKey(reg);
if (sa.lpSecurityDescriptor != NULL)
LocalFree(sa.lpSecurityDescriptor);
+ free_pkcs11_identity_change(change);
free(thumbprint);
free(blob);
return success ? 0 : -1;
@@ -477,10 +583,11 @@ store_pkcs11_provider(HKEY user_root, struct agent_connection *con,
}
static void
-rollback_pkcs11_identities(HKEY user_root, char **identities,
+rollback_pkcs11_identities(HKEY user_root,
+ struct pkcs11_identity_change **changes,
size_t nidentities)
{
- HKEY reg = NULL;
+ HKEY reg = NULL, sub = NULL;
size_t i;
if (nidentities == 0)
@@ -491,26 +598,99 @@ rollback_pkcs11_identities(HKEY user_root, char **identities,
error_f("failed to open PKCS11 identities for rollback");
return;
}
- for (i = 0; i < nidentities; i++) {
- if (RegDeleteTreeA(reg, identities[i]) != ERROR_SUCCESS)
- error_f("failed to roll back PKCS11 identity");
+ for (i = nidentities; i > 0; i--) {
+ if (changes[i - 1]->created) {
+ if (RegDeleteTreeA(reg, changes[i - 1]->name) !=
+ ERROR_SUCCESS)
+ error_f("failed to roll back PKCS11 identity");
+ continue;
+ }
+ if (RegOpenKeyExA(reg, changes[i - 1]->name, 0,
+ KEY_SET_VALUE | KEY_WOW64_64KEY, &sub) != ERROR_SUCCESS ||
+ restore_pkcs11_identity_metadata(sub, changes[i - 1]) != 0)
+ error_f("failed to roll back PKCS11 identity metadata");
+ if (sub != NULL) {
+ RegCloseKey(sub);
+ sub = NULL;
+ }
}
RegCloseKey(reg);
}
+static int
+remove_pkcs11_identities(HKEY user_root, const char *provider)
+{
+ HKEY root = NULL, sub = NULL;
+ wchar_t sub_name[MAX_KEY_LENGTH];
+ DWORD sub_name_len, type, data_len;
+ u_char *data = NULL;
+ size_t provider_len = strlen(provider);
+ int index = 0, present, remove;
+ LSTATUS status;
+
+ status = RegOpenKeyExW(user_root, SSH_KEYS_ROOT, 0,
+ DELETE | KEY_ENUMERATE_SUB_KEYS | KEY_WOW64_64KEY, &root);
+ if (status == ERROR_FILE_NOT_FOUND)
+ return 0;
+ if (status != ERROR_SUCCESS)
+ return -1;
+ for (;;) {
+ sub_name_len = MAX_KEY_LENGTH;
+ status = RegEnumKeyExW(root, index, sub_name, &sub_name_len,
+ NULL, NULL, NULL, NULL);
+ if (status == ERROR_NO_MORE_ITEMS)
+ break;
+ if (status != ERROR_SUCCESS) {
+ index++;
+ continue;
+ }
+ if (RegOpenKeyExW(root, sub_name, 0,
+ KEY_QUERY_VALUE | KEY_WOW64_64KEY, &sub) != ERROR_SUCCESS) {
+ index++;
+ continue;
+ }
+ free(data);
+ data = NULL;
+ if (read_optional_reg_value(sub, L"provider", &present, &type,
+ &data, &data_len) != 0 ||
+ (!present && read_optional_reg_value(sub, L"comment",
+ &present, &type, &data, &data_len) != 0)) {
+ RegCloseKey(sub);
+ sub = NULL;
+ index++;
+ continue;
+ }
+ remove = present && data_len == provider_len &&
+ memcmp(data, provider, provider_len) == 0;
+ RegCloseKey(sub);
+ sub = NULL;
+ if (remove) {
+ if (RegDeleteTreeW(root, sub_name) != ERROR_SUCCESS) {
+ RegCloseKey(root);
+ free(data);
+ return -1;
+ }
+ } else
+ index++;
+ }
+ RegCloseKey(root);
+ free(data);
+ return 0;
+}
+
static int
load_pkcs11_identities(HKEY user_root, const char *provider,
struct sshkey **token_keys, int nkeys)
{
HKEY root = NULL, sub = NULL;
wchar_t sub_name[MAX_KEY_LENGTH];
- DWORD sub_name_len, blob_len, comment_len;
+ DWORD sub_name_len, blob_len, comment_len, association_len;
u_char *blob = NULL;
- char *comment = NULL;
+ char *comment = NULL, *association = NULL;
struct sshkey *registered = NULL, *cert = NULL;
u_char *plain_added = NULL;
size_t provider_len = strlen(provider);
- int i, index = 0, loaded = 0;
+ int i, index = 0, legacy, loaded = 0;
LSTATUS status;
if (nkeys > 0)
@@ -543,19 +723,37 @@ load_pkcs11_identities(HKEY user_root, const char *provider,
RegQueryValueExW(sub, L"comment", NULL, NULL, NULL,
&comment_len) != ERROR_SUCCESS ||
blob_len == 0 || blob_len > MAX_MESSAGE_SIZE ||
- comment_len != provider_len)
+ comment_len > MAX_MESSAGE_SIZE)
+ continue;
+ status = RegQueryValueExW(sub, L"provider", NULL, NULL, NULL,
+ &association_len);
+ if (status == ERROR_FILE_NOT_FOUND) {
+ legacy = 1;
+ association_len = comment_len;
+ } else if (status == ERROR_SUCCESS &&
+ association_len <= MAX_MESSAGE_SIZE)
+ legacy = 0;
+ else
continue;
free(blob);
free(comment);
+ free(association);
blob = xmalloc(blob_len);
comment = xmalloc((size_t)comment_len + 1);
+ association = xmalloc((size_t)association_len + 1);
if (RegQueryValueExW(sub, L"pub", NULL, NULL, blob,
&blob_len) != ERROR_SUCCESS ||
RegQueryValueExW(sub, L"comment", NULL, NULL,
- (BYTE *)comment, &comment_len) != ERROR_SUCCESS)
+ (BYTE *)comment, &comment_len) != ERROR_SUCCESS ||
+ (!legacy && RegQueryValueExW(sub, L"provider", NULL, NULL,
+ (BYTE *)association, &association_len) != ERROR_SUCCESS))
continue;
comment[comment_len] = '\0';
- if (memcmp(comment, provider, provider_len) != 0)
+ if (legacy)
+ memcpy(association, comment, comment_len);
+ association[association_len] = '\0';
+ if (association_len != provider_len ||
+ memcmp(association, provider, provider_len) != 0)
continue;
sshkey_free(registered);
registered = NULL;
@@ -593,6 +791,7 @@ load_pkcs11_identities(HKEY user_root, const char *provider,
sshkey_free(cert);
sshkey_free(registered);
free(plain_added);
+ free(association);
free(comment);
free(blob);
if (sub != NULL)
@@ -967,12 +1166,14 @@ process_add_smartcard_key(struct sshbuf *request, struct sshbuf *response,
struct agent_connection *con)
{
char *provider = NULL, *pin = NULL, canonical_provider[PATH_MAX] = { 0 };
- char allowed_provider[PATH_MAX], *created_identity = NULL;
- char **created_identities = NULL;
+ char allowed_provider[PATH_MAX], **labels = NULL;
+ const char *comment;
int i, j, count = 0, r = 0, request_invalid = 0, success = 0;
int cert_only = 0, identities_stored = 0;
struct sshkey **keys = NULL, **certs = NULL, *cert = NULL;
- size_t k, pin_len = 0, ncerts = 0, ncreated_identities = 0;
+ struct pkcs11_identity_change *identity_change = NULL;
+ struct pkcs11_identity_change **identity_changes = NULL;
+ size_t k, pin_len = 0, ncerts = 0, nidentity_changes = 0;
HKEY user_root = NULL;
pkcs11_init(0);
@@ -1023,7 +1224,7 @@ process_add_smartcard_key(struct sshbuf *request, struct sshbuf *response,
goto done;
}
- count = pkcs11_add_provider(canonical_provider, pin, &keys, NULL);
+ count = pkcs11_add_provider(canonical_provider, pin, &keys, &labels);
if (count <= 0) {
error_f("failed to load provider keys: count:%d", count);
goto done;
@@ -1033,6 +1234,7 @@ process_add_smartcard_key(struct sshbuf *request, struct sshbuf *response,
goto done;
for (i = 0; i < count; i++) {
+ comment = pkcs11_identity_comment(canonical_provider, labels[i]);
for (j = 0; j < (int)ncerts; j++) {
if (!sshkey_is_cert(certs[j]) ||
!sshkey_equal_public(keys[i], certs[j]))
@@ -1040,30 +1242,24 @@ process_add_smartcard_key(struct sshbuf *request, struct sshbuf *response,
if (pkcs11_make_cert(keys[i], certs[j], &cert) != 0)
continue;
if (store_pkcs11_identity(user_root, cert,
- canonical_provider, &created_identity) != 0)
+ canonical_provider, comment, &identity_change) != 0)
goto done;
- if (created_identity != NULL) {
- created_identities = xrecallocarray(created_identities,
- ncreated_identities, ncreated_identities + 1,
- sizeof(*created_identities));
- created_identities[ncreated_identities++] =
- created_identity;
- created_identity = NULL;
- }
+ identity_changes = xrecallocarray(identity_changes,
+ nidentity_changes, nidentity_changes + 1,
+ sizeof(*identity_changes));
+ identity_changes[nidentity_changes++] = identity_change;
+ identity_change = NULL;
sshkey_free(cert);
cert = NULL;
identities_stored++;
}
if (!cert_only && store_pkcs11_identity(user_root, keys[i],
- canonical_provider, &created_identity) == 0) {
- if (created_identity != NULL) {
- created_identities = xrecallocarray(created_identities,
- ncreated_identities, ncreated_identities + 1,
- sizeof(*created_identities));
- created_identities[ncreated_identities++] =
- created_identity;
- created_identity = NULL;
- }
+ canonical_provider, comment, &identity_change) == 0) {
+ identity_changes = xrecallocarray(identity_changes,
+ nidentity_changes, nidentity_changes + 1,
+ sizeof(*identity_changes));
+ identity_changes[nidentity_changes++] = identity_change;
+ identity_change = NULL;
identities_stored++;
} else if (!cert_only)
goto done;
@@ -1082,19 +1278,22 @@ process_add_smartcard_key(struct sshbuf *request, struct sshbuf *response,
r = -1;
if (!success && user_root != NULL)
- rollback_pkcs11_identities(user_root, created_identities,
- ncreated_identities);
+ rollback_pkcs11_identities(user_root, identity_changes,
+ nidentity_changes);
pkcs11_terminate();
sshkey_free(cert);
- free(created_identity);
- for (k = 0; k < ncreated_identities; k++)
- free(created_identities[k]);
- free(created_identities);
+ free_pkcs11_identity_change(identity_change);
+ for (k = 0; k < nidentity_changes; k++)
+ free_pkcs11_identity_change(identity_changes[k]);
+ free(identity_changes);
for (i = 0; i < count; i++)
sshkey_free(keys[i]);
free(keys);
+ for (i = 0; i < count; i++)
+ free(labels[i]);
+ free(labels);
free_pkcs11_certs(certs, ncerts);
free(provider);
if (pin) {
@@ -1134,8 +1333,9 @@ int process_remove_smartcard_key(struct sshbuf* request, struct sshbuf* response
!is_reg_sub_key_exists(user_root, SSH_PKCS11_PROVIDERS_ROOT, canonical_provider))
goto done;
- if (remove_matching_subkeys_from_registry(user_root, SSH_KEYS_ROOT, L"comment", canonical_provider) != 0 ||
- remove_matching_subkeys_from_registry(user_root, SSH_PKCS11_PROVIDERS_ROOT, L"provider", canonical_provider) != 0) {
+ if (remove_pkcs11_identities(user_root, canonical_provider) != 0 ||
+ remove_matching_subkeys_from_registry(user_root,
+ SSH_PKCS11_PROVIDERS_ROOT, L"provider", canonical_provider) != 0) {
goto done;
}
diff --git a/regress/pesterTests/KeyUtils.Tests.ps1 b/regress/pesterTests/KeyUtils.Tests.ps1
index 5296ab77263a..c33b2579c793 100644
--- a/regress/pesterTests/KeyUtils.Tests.ps1
+++ b/regress/pesterTests/KeyUtils.Tests.ps1
@@ -394,6 +394,44 @@ Describe "E2E scenarios for ssh key management" -Tags "CI" {
foreach ($publicKeyPath in $publicKeyPaths) {
Test-Path $publicKeyPath | Should Be $true
}
+ Test-Path Env:OPENSSH_TEST_PKCS11_LABELS | Should Be $true
+ $pkcs11Labels = @($env:OPENSSH_TEST_PKCS11_LABELS.Split(
+ [char[]]@(';'), [StringSplitOptions]::None))
+ $pkcs11Labels.Count | Should Be $publicKeyPaths.Count
+ $canonicalProvider = [IO.Path]::GetFullPath($pkcs11Path).Replace('\', '/')
+ $expectedComments = @($pkcs11Labels | ForEach-Object {
+ if ($_) { $_ } else { $canonicalProvider }
+ })
+
+ function Assert-Pkcs11IdentityComments {
+ param([string[]]$KeyPaths, [string[]]$Comments)
+
+ $longListing = @(ssh-add -L)
+ $shortListing = @(ssh-add -l)
+ $KeyPaths.Count | Should Be $Comments.Count
+ $fingerprints = @($KeyPaths | ForEach-Object {
+ ((ssh-keygen -lf $_) -split ' ')[1]
+ })
+ for ($index = 0; $index -lt $KeyPaths.Count; $index++) {
+ $keyBlob = (Get-Content $KeyPaths[$index]).Split(' ')[1]
+ $longEntry = @($longListing | Where-Object {
+ $_.Contains($keyBlob)
+ })
+ $longEntry.Count | Should Be 1
+ ($longEntry[0] -split ' ', 3)[2] | Should Be $Comments[$index]
+
+ $fingerprint = $fingerprints[$index]
+ $shortEntry = @($shortListing | Where-Object {
+ $_.Contains(" $fingerprint ")
+ })
+ $shortEntry.Count | Should Be @($fingerprints |
+ Where-Object { $_ -eq $fingerprint }).Count
+ foreach ($entry in $shortEntry) {
+ $entry | Should Match (" " +
+ [regex]::Escape($Comments[$index]) + " \([^)]+\)$")
+ }
+ }
+ }
$testPin = $env:OPENSSH_TEST_PKCS11_PIN
if (-not $testPin) { $testPin = $pkcs11Pin }
$ca = Join-Path $testDir "pkcs11-ca"
@@ -440,6 +478,9 @@ Describe "E2E scenarios for ssh key management" -Tags "CI" {
& ssh-add -T $keyPath
$LASTEXITCODE | Should Be 0
}
+ Assert-Pkcs11IdentityComments `
+ ($copiedPublicKeyPaths + $certPaths) `
+ ($expectedComments + $expectedComments)
Restart-Service ssh-agent
WaitForStatus -ServiceName ssh-agent -Status "Running"
@@ -447,6 +488,9 @@ Describe "E2E scenarios for ssh key management" -Tags "CI" {
& ssh-add -T $certPath
$LASTEXITCODE | Should Be 0
}
+ Assert-Pkcs11IdentityComments `
+ ($copiedPublicKeyPaths + $certPaths) `
+ ($expectedComments + $expectedComments)
& ssh-add -d $certPaths[0]
$LASTEXITCODE | Should Be 0
$deletedKeyBlob = (Get-Content $certPaths[0]).Split(' ')[1]
@@ -477,6 +521,9 @@ Describe "E2E scenarios for ssh key management" -Tags "CI" {
$sequentialCertBlob = (Get-Content $sequentialCertPath).Split(' ')[1]
@((ssh-add -L) | Where-Object { $_.Contains($sequentialCertBlob) }).Count |
Should Be 1
+ Assert-Pkcs11IdentityComments `
+ @($certPaths[0], $sequentialCertPath) `
+ @($expectedComments[0], $expectedComments[0])
ssh-add -D
$LASTEXITCODE | Should Be 0
@@ -491,6 +538,9 @@ Describe "E2E scenarios for ssh key management" -Tags "CI" {
$keyBlob = (Get-Content $keyPath).Split(' ')[1]
@($allKeys | Where-Object { $_.Contains($keyBlob) }).Count | Should Be 1
}
+ Assert-Pkcs11IdentityComments `
+ ($copiedPublicKeyPaths + $certPaths[0]) `
+ ($expectedComments + $expectedComments[0])
# A failed unmatched add must not change existing persisted identities.
$identitiesBefore = @(ssh-add -L | Sort-Object)
@@ -505,6 +555,9 @@ Describe "E2E scenarios for ssh key management" -Tags "CI" {
& ssh-add -T $keyPath
$LASTEXITCODE | Should Be 0
}
+ Assert-Pkcs11IdentityComments `
+ ($copiedPublicKeyPaths + $certPaths[0]) `
+ ($expectedComments + $expectedComments[0])
& ssh-add -d $certPaths[0]
$LASTEXITCODE | Should Be 0
@@ -514,6 +567,88 @@ Describe "E2E scenarios for ssh key management" -Tags "CI" {
Should Be 1
}
+ # Legacy identities used comment for their provider association.
+ $identityRootPath = "$currentUserSid\Software\OpenSSH\Agent\Keys"
+ $identityRoot = [Microsoft.Win32.Registry]::Users.OpenSubKey(
+ $identityRootPath, $true)
+ $identityRoot | Should Not Be $null
+ $plainBlob = (Get-Content $copiedPublicKeyPaths[0]).Split(' ')[1]
+ $identityKey = $null
+ foreach ($identityName in $identityRoot.GetSubKeyNames()) {
+ $candidate = $identityRoot.OpenSubKey($identityName, $true)
+ $storedBlob = $candidate.GetValue("pub")
+ if ($storedBlob -is [byte[]] -and
+ [Convert]::ToBase64String($storedBlob) -eq $plainBlob) {
+ $identityKey = $candidate
+ break
+ }
+ $candidate.Dispose()
+ }
+ $identityKey | Should Not Be $null
+ $providerBytes = [Text.Encoding]::UTF8.GetBytes($canonicalProvider)
+ $identityKey.DeleteValue("provider", $false)
+ $identityKey.SetValue("comment", $providerBytes,
+ [Microsoft.Win32.RegistryValueKind]::Binary)
+
+ Restart-Service ssh-agent
+ WaitForStatus -ServiceName ssh-agent -Status "Running"
+ Assert-Pkcs11IdentityComments @($copiedPublicKeyPaths[0]) `
+ @($canonicalProvider)
+ & ssh-add -T $copiedPublicKeyPaths[0]
+ $LASTEXITCODE | Should Be 0
+ $identityKey.GetValue("provider", $null) | Should Be $null
+ [Text.Encoding]::UTF8.GetString($identityKey.GetValue("comment")) |
+ Should Be $canonicalProvider
+
+ # A failed provider update must roll legacy metadata back.
+ $providerRootPath = "$currentUserSid\Software\OpenSSH\Agent\PKCS11_Providers"
+ $providerRoot = [Microsoft.Win32.Registry]::Users.OpenSubKey(
+ $providerRootPath, $true)
+ $providerRoot | Should Not Be $null
+ $providerKey = $providerRoot.OpenSubKey($canonicalProvider,
+ [Microsoft.Win32.RegistryKeyPermissionCheck]::ReadWriteSubTree,
+ [Security.AccessControl.RegistryRights]::FullControl)
+ $providerKey | Should Not Be $null
+ $blockedAcl = $providerKey.GetAccessControl()
+ $denySetValue = New-Object `
+ System.Security.AccessControl.RegistryAccessRule(
+ $systemSid,
+ [System.Security.AccessControl.RegistryRights]::SetValue,
+ [System.Security.AccessControl.AccessControlType]::Deny)
+ $blockedAcl.AddAccessRule($denySetValue) | Out-Null
+ try {
+ $providerKey.SetAccessControl($blockedAcl)
+ & ssh-add -s $pkcs11Path
+ $LASTEXITCODE | Should Not Be 0
+ $identityKey.GetValue("provider", $null) | Should Be $null
+ [Text.Encoding]::UTF8.GetString(
+ $identityKey.GetValue("comment")) |
+ Should Be $canonicalProvider
+ }
+ finally {
+ $blockedAcl.RemoveAccessRuleSpecific($denySetValue)
+ $providerKey.SetAccessControl($blockedAcl)
+ }
+
+ # Re-adding migrates metadata without replacing the key entry.
+ & ssh-add -s $pkcs11Path
+ $LASTEXITCODE | Should Be 0
+ [Text.Encoding]::UTF8.GetString($identityKey.GetValue("provider")) |
+ Should Be $canonicalProvider
+ [Text.Encoding]::UTF8.GetString($identityKey.GetValue("comment")) |
+ Should Be $expectedComments[0]
+ Assert-Pkcs11IdentityComments @($copiedPublicKeyPaths[0]) `
+ @($expectedComments[0])
+
+ # Provider removal accepts both migrated and legacy identities.
+ $identityKey.DeleteValue("provider", $false)
+ $identityKey.SetValue("comment", $providerBytes,
+ [Microsoft.Win32.RegistryValueKind]::Binary)
+ $providerKey.Dispose()
+ $providerRoot.Dispose()
+ $identityKey.Dispose()
+ $identityRoot.Dispose()
+
& ssh-add -e $pkcs11Path
$LASTEXITCODE | Should Be 0
@(ssh-add -L) -match "The agent has no identities." | Should Be $true
diff --git a/regress/pesterTests/README.md b/regress/pesterTests/README.md
index b0885e3249fa..cd9ac12d0871 100644
--- a/regress/pesterTests/README.md
+++ b/regress/pesterTests/README.md
@@ -74,6 +74,9 @@ following environment variables are set before running the E2E tests:
* `OPENSSH_TEST_PKCS11_PIN`: token PIN.
* `OPENSSH_TEST_PKCS11_PUBLIC_KEYS`: semicolon-separated public-key files whose
corresponding private keys are present on the token.
+* `OPENSSH_TEST_PKCS11_LABELS`: semicolon-separated labels corresponding
+ positionally to `OPENSSH_TEST_PKCS11_PUBLIC_KEYS`. An empty item expects the
+ canonical provider path fallback.
The test creates short-lived OpenSSH certificates for the supplied public
keys. It verifies plain and certificate identities, signing, agent service
diff --git a/regress/unittests/win32compat/pkcs11_cert_tests.c b/regress/unittests/win32compat/pkcs11_cert_tests.c
index f0e1cfb74de9..c863d7e7d51a 100644
--- a/regress/unittests/win32compat/pkcs11_cert_tests.c
+++ b/regress/unittests/win32compat/pkcs11_cert_tests.c
@@ -169,6 +169,19 @@ test_pkcs11_cert_identity_name(void)
TEST_DONE();
}
+static void
+test_pkcs11_identity_comment(void)
+{
+ const char *provider = "C:/provider.dll";
+
+ TEST_START("PKCS11 identity comment fallback");
+ ASSERT_STRING_EQ(pkcs11_identity_comment(provider, "token label"),
+ "token label");
+ ASSERT_STRING_EQ(pkcs11_identity_comment(provider, ""), provider);
+ ASSERT_STRING_EQ(pkcs11_identity_comment(provider, NULL), provider);
+ TEST_DONE();
+}
+
static void
test_pkcs11_cert_constraints_duplicate(void)
{
@@ -262,6 +275,7 @@ pkcs11_cert_tests(void)
test_pkcs11_cert_constraints_valid();
test_pkcs11_cert_constraints_compatible();
test_pkcs11_cert_identity_name();
+ test_pkcs11_identity_comment();
test_pkcs11_cert_constraints_duplicate();
test_pkcs11_cert_constraints_truncated();
test_pkcs11_cert_constraints_malformed();
From 07a27ac33689fc020a2723c48697eaf3b046ceea Mon Sep 17 00:00:00 2001
From: Sebastian Ott <174621899+VSSOtt@users.noreply.github.com>
Date: Fri, 17 Jul 2026 18:25:10 +0200
Subject: [PATCH 6/8] Harden persisted PKCS#11 provider reload
Skip persisted provider records with invalid or oversized Registry metadata while rebuilding the transient signing key set. This keeps corrupt records from causing unbounded allocations or blocking unrelated identities.
Free Windows PKCS#11 provider list entries during termination and extend the stale-provider regression to cover oversized encrypted PIN data.
---
.../win32compat/ssh-agent/keyagent-request.c | 6 +++++-
regress/pesterTests/KeyUtils.Tests.ps1 | 18 ++++++++++++++++++
ssh-pkcs11-client.c | 2 ++
3 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/contrib/win32/win32compat/ssh-agent/keyagent-request.c b/contrib/win32/win32compat/ssh-agent/keyagent-request.c
index b98d34cbc82d..c66ad9e74efe 100644
--- a/contrib/win32/win32compat/ssh-agent/keyagent-request.c
+++ b/contrib/win32/win32compat/ssh-agent/keyagent-request.c
@@ -940,6 +940,7 @@ process_sign_request(struct sshbuf* request, struct sshbuf* response, struct age
while (1) {
sub_name_len = MAX_KEY_LENGTH;
pin_len = epin_len = provider_len = 0;
+ epin_alloc_len = 0;
if (sub) {
RegCloseKey(sub);
sub = NULL;
@@ -948,6 +949,9 @@ process_sign_request(struct sshbuf* request, struct sshbuf* response, struct age
if (RegOpenKeyExW(root, sub_name, 0, KEY_QUERY_VALUE | KEY_WOW64_64KEY, &sub) == 0 &&
RegQueryValueExW(sub, L"provider", 0, NULL, NULL, &provider_len) == 0 &&
RegQueryValueExW(sub, L"pin", 0, NULL, NULL, &epin_len) == 0) {
+ if (provider_len == 0 || provider_len >= PATH_MAX ||
+ epin_len == 0 || epin_len > MAX_MESSAGE_SIZE)
+ continue;
epin_alloc_len = epin_len;
if ((epin = malloc(epin_alloc_len + 1)) == NULL ||
(provider = malloc(provider_len + 1)) == NULL ||
@@ -1018,7 +1022,7 @@ process_sign_request(struct sshbuf* request, struct sshbuf* response, struct age
if (signature)
free(signature);
#ifdef ENABLE_PKCS11
- free_pkcs11_sign_provider(&provider, &pin, pin_len, &epin, epin_len,
+ free_pkcs11_sign_provider(&provider, &pin, pin_len, &epin, epin_alloc_len,
&keys, count);
del_all_keys();
pkcs11_terminate();
diff --git a/regress/pesterTests/KeyUtils.Tests.ps1 b/regress/pesterTests/KeyUtils.Tests.ps1
index c33b2579c793..14dc13fb24d9 100644
--- a/regress/pesterTests/KeyUtils.Tests.ps1
+++ b/regress/pesterTests/KeyUtils.Tests.ps1
@@ -681,6 +681,10 @@ Describe "E2E scenarios for ssh key management" -Tags "CI" {
"nonexistent\openssh-corrupt-provider.dll"
$corruptProvider = [IO.Path]::GetFullPath(
$corruptProviderPath).Replace('\', '/')
+ $oversizedProviderPath = Join-Path $testDir `
+ "nonexistent\openssh-oversized-provider.dll"
+ $oversizedProvider = [IO.Path]::GetFullPath(
+ $oversizedProviderPath).Replace('\', '/')
try {
ssh-add -D
@@ -738,6 +742,19 @@ Describe "E2E scenarios for ssh key management" -Tags "CI" {
$LASTEXITCODE | Should Be 0
& ssh-add -T $publicKeyPaths[0]
$LASTEXITCODE | Should Be 0
+
+ $staleProviderKey.Dispose()
+ $staleProviderKey = $providerRoot.CreateSubKey($oversizedProvider)
+ $staleProviderKey.SetValue("provider",
+ [Text.Encoding]::UTF8.GetBytes($oversizedProvider),
+ [Microsoft.Win32.RegistryValueKind]::Binary)
+ $staleProviderKey.SetValue("pin", (New-Object byte[] 11000),
+ [Microsoft.Win32.RegistryValueKind]::Binary)
+
+ & ssh-add -T $softwareKeyPath
+ $LASTEXITCODE | Should Be 0
+ & ssh-add -T $publicKeyPaths[0]
+ $LASTEXITCODE | Should Be 0
}
finally {
if ($staleProviderKey) { $staleProviderKey.Dispose() }
@@ -745,6 +762,7 @@ Describe "E2E scenarios for ssh key management" -Tags "CI" {
if ($providerRoot) {
$providerRoot.DeleteSubKeyTree($staleProvider, $false)
$providerRoot.DeleteSubKeyTree($corruptProvider, $false)
+ $providerRoot.DeleteSubKeyTree($oversizedProvider, $false)
$providerRoot.Dispose()
}
ssh-add -D | Out-Null
diff --git a/ssh-pkcs11-client.c b/ssh-pkcs11-client.c
index 052e6cbbf58f..5c8a42378c2e 100644
--- a/ssh-pkcs11-client.c
+++ b/ssh-pkcs11-client.c
@@ -386,6 +386,8 @@ pkcs11_terminate(void)
// Send message to helper to gracefully unload providers
pkcs11_del_provider(p->name);
TAILQ_REMOVE(&pkcs11_providers, p, next);
+ free(p->name);
+ free(p);
}
if (pid != -1) {
From c5ae397a293e94f4ee541f227bc03a6a3717ae24 Mon Sep 17 00:00:00 2001
From: Sebastian Ott <174621899+VSSOtt@users.noreply.github.com>
Date: Fri, 17 Jul 2026 19:31:26 +0200
Subject: [PATCH 7/8] Harden Windows PKCS#11 registry helpers
Keep SECURITY_ATTRIBUTES.nLength stable when converting Registry security descriptors by using a separate descriptor-size variable. Also wipe the full encrypted PIN allocation during provider reload cleanup.
---
.../win32/win32compat/ssh-agent/keyagent-request.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/contrib/win32/win32compat/ssh-agent/keyagent-request.c b/contrib/win32/win32compat/ssh-agent/keyagent-request.c
index c66ad9e74efe..4e3088b16371 100644
--- a/contrib/win32/win32compat/ssh-agent/keyagent-request.c
+++ b/contrib/win32/win32compat/ssh-agent/keyagent-request.c
@@ -455,6 +455,7 @@ store_pkcs11_identity(HKEY user_root, const struct sshkey *key,
char *thumbprint = NULL;
struct pkcs11_identity_change *change = NULL;
DWORD disposition = 0;
+ ULONG sd_len = 0;
int success = 0;
if (changep == NULL || provider == NULL || comment == NULL)
@@ -462,7 +463,7 @@ store_pkcs11_identity(HKEY user_root, const struct sshkey *key,
*changep = NULL;
sa.nLength = sizeof(sa);
if (!ConvertStringSecurityDescriptorToSecurityDescriptorW(REG_KEY_SDDL,
- SDDL_REVISION_1, &sa.lpSecurityDescriptor, &sa.nLength) ||
+ SDDL_REVISION_1, &sa.lpSecurityDescriptor, &sd_len) ||
sshkey_to_blob(key, &blob, &blob_len) != 0 ||
blob_len == 0 || blob_len > MAX_MESSAGE_SIZE ||
(thumbprint = pkcs11_identity_name(key, blob, blob_len)) == NULL ||
@@ -545,11 +546,12 @@ store_pkcs11_provider(HKEY user_root, struct agent_connection *con,
char *epin = NULL;
DWORD epin_len = 0;
DWORD disposition = 0;
+ ULONG sd_len = 0;
int success = 0;
sa.nLength = sizeof(sa);
if (!ConvertStringSecurityDescriptorToSecurityDescriptorW(REG_KEY_SDDL,
- SDDL_REVISION_1, &sa.lpSecurityDescriptor, &sa.nLength) ||
+ SDDL_REVISION_1, &sa.lpSecurityDescriptor, &sd_len) ||
convert_blob(con, pin, (DWORD)pin_len, &epin, &epin_len, TRUE) != 0 ||
RegCreateKeyExW(user_root, SSH_PKCS11_PROVIDERS_ROOT, 0, NULL, 0,
KEY_WRITE | KEY_WOW64_64KEY, &sa, ®, NULL) != ERROR_SUCCESS ||
@@ -966,7 +968,7 @@ process_sign_request(struct sshbuf* request, struct sshbuf* response, struct age
if (convert_blob(con, epin, epin_len, &pin, &pin_len, 0) != 0 ||
(npin = realloc(pin, pin_len + 1)) == NULL) {
free_pkcs11_sign_provider(&provider, &pin, pin_len,
- &epin, epin_len, &keys, count);
+ &epin, epin_alloc_len, &keys, count);
continue;
}
pin = npin;
@@ -974,13 +976,13 @@ process_sign_request(struct sshbuf* request, struct sshbuf* response, struct age
count = pkcs11_add_provider(provider, pin, &keys, NULL);
if (count <= 0) {
free_pkcs11_sign_provider(&provider, &pin, pin_len,
- &epin, epin_len, &keys, count);
+ &epin, epin_alloc_len, &keys, count);
continue;
}
loaded = load_pkcs11_identities(user_root, provider,
keys, count);
free_pkcs11_sign_provider(&provider, &pin, pin_len,
- &epin, epin_len, &keys, count);
+ &epin, epin_alloc_len, &keys, count);
if (loaded < 0)
goto done;
}
From 46b57e8be67e4c57b5acdfa1cf765bfb4340f2c8 Mon Sep 17 00:00:00 2001
From: Sebastian Ott <174621899+VSSOtt@users.noreply.github.com>
Date: Fri, 17 Jul 2026 19:51:55 +0200
Subject: [PATCH 8/8] Mark wrapped Windows PKCS#11 keys as external
Set SSHKEY_FLAG_EXT in the Windows wrap_key() path, matching the non-Windows helper and keeping the runtime flag with the private-operation redirection. Move the flag assignment there for consistency, so wrapped keys are marked external at their creation point instead of only in the certificate path.
Remove the now-redundant certificate-specific assignment.
---
ssh-pkcs11-client.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ssh-pkcs11-client.c b/ssh-pkcs11-client.c
index 5c8a42378c2e..b59b7e22381a 100644
--- a/ssh-pkcs11-client.c
+++ b/ssh-pkcs11-client.c
@@ -621,6 +621,7 @@ wrap_key(struct sshkey* k)
#endif /* OPENSSL_HAS_ECC && HAVE_EC_KEY_METHOD_NEW */
} else
fatal_f("unknown key type");
+ k->flags |= SSHKEY_FLAG_EXT;
}
#else
@@ -697,7 +698,6 @@ pkcs11_make_cert(const struct sshkey *priv,
if ((r = sshkey_from_private(priv, &ret)) != 0)
goto out;
wrap_key(ret);
- ret->flags |= SSHKEY_FLAG_EXT;
if ((r = sshkey_to_certified(ret)) != 0 ||
(r = sshkey_cert_copy(certpub, ret)) != 0)
goto out;