Skip to content

Commit

Permalink
Update Agent To Differentiate Key Types
Browse files Browse the repository at this point in the history
- Adjusted registry entires used by the SSH agent to include a SSH key type in the registry key name.
  • Loading branch information
NoMoreFood committed Feb 23, 2019
1 parent 68ad673 commit d7b6489
Showing 1 changed file with 28 additions and 2 deletions.
30 changes: 28 additions & 2 deletions contrib/win32/win32compat/ssh-agent/keyagent-request.c
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,26 @@ process_unsupported_request(struct sshbuf* request, struct sshbuf* response, str
return r;
}

char *
get_key_subkey_name(const struct sshkey * key)
{
const char * key_type_string = sshkey_type(key);
char * key_fingerprint = sshkey_fingerprint(key, SSH_FP_HASH_DEFAULT, SSH_FP_DEFAULT);
char * key_subkey_name = NULL;

/* concatenate the hash with the key type */
if (key_type_string != NULL && key_fingerprint != NULL) {
const size_t key_subkey_size = strlen(key_fingerprint) + 1 + strlen(key_type_string) + 1;
key_subkey_name = malloc(key_subkey_size);
sprintf_s(key_subkey_name, key_subkey_size, "%s:%s", key_fingerprint, key_type_string);
}

if (key_fingerprint)
free(key_fingerprint);

return key_subkey_name;
}

int
process_add_identity(struct sshbuf* request, struct sshbuf* response, struct agent_connection* con)
{
Expand Down Expand Up @@ -154,7 +174,7 @@ process_add_identity(struct sshbuf* request, struct sshbuf* response, struct age
if ((!ConvertStringSecurityDescriptorToSecurityDescriptorW(REG_KEY_SDDL, SDDL_REVISION_1, &sa.lpSecurityDescriptor, &sa.nLength)) ||
sshkey_to_blob(key, &pubkey_blob, &pubkey_blob_len) != 0 ||
convert_blob(con, blob, blob_len, &eblob, &eblob_len, 1) != 0 ||
((thumbprint = sshkey_fingerprint(key, SSH_FP_HASH_DEFAULT, SSH_FP_DEFAULT)) == NULL) ||
((thumbprint = get_key_subkey_name(key)) == NULL) ||
get_user_root(con, &user_root) != 0 ||
RegCreateKeyExW(user_root, SSH_KEYS_ROOT, 0, 0, 0, KEY_WRITE | KEY_WOW64_64KEY, &sa, &reg, NULL) != 0 ||
RegCreateKeyExA(reg, thumbprint, 0, 0, 0, KEY_WRITE | KEY_WOW64_64KEY, &sa, &sub, NULL) != 0 ||
Expand All @@ -179,6 +199,12 @@ process_add_identity(struct sshbuf* request, struct sshbuf* response, struct age
if ((success == 0) && reg && thumbprint)
RegDeleteKeyExA(reg, thumbprint, KEY_WOW64_64KEY, 0);

/* delete previous version of thumbprint keys without type */
if (thumbprint) {
*strrchr(thumbprint, ':') = '\0';
RegDeleteKeyExA(reg, thumbprint, KEY_WOW64_64KEY, 0);
}

if (eblob)
free(eblob);
if (sa.lpSecurityDescriptor)
Expand Down Expand Up @@ -318,7 +344,7 @@ 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 = get_key_subkey_name(key)) == 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 ||
Expand Down

3 comments on commit d7b6489

@manojampalam
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @NoMoreFood.

A few comments:

  • Just tried out Unix's version of ssh-agent. It seems that it can store multiple certs associated with the same user's public key.
  • With your changes, we are storing the same private key multiple times.
  • These changes would also break existing keys stored in Windows ssh-agent.

To address these, here's my proposal:

  • Keep the public key thumbprint the root "registry key"
  • store certificates as sub nodes identified by certificate identity, under the public key node
    Something like this
    image

Can you also please add test cases in regress\pesterTests\KeyUtils.Tests.ps1

@NoMoreFood
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have looked the Unix implementation more closely. Topically, I like your proposal and will investigate further.

@tomtastic
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance this could be resurrected as it's still a problem in 2024.

Please sign in to comment.