support SSH2_AGENTC_ADD_ID_CONSTRAINED

[ddrown]

PR Summary

support SSH2_AGENTC_ADD_ID_CONSTRAINED by treating it as SSH2_AGENTC_ADD_IDENTITY

PR Context

SSH2_AGENTC_ADD_ID_CONSTRAINED is needed to support adding U2F/Fido2 ssh keys to the agent from WSL ssh-add and KeePassXC
ref PowerShell/Win32-OpenSSH#1961

this returns SSH_AGENT_FAILURE on unsupported constraint types, such as:

• SSH_AGENT_CONSTRAIN_LIFETIME
• SSH_AGENT_CONSTRAIN_CONFIRM
• SSH_AGENT_CONSTRAIN_MAXSIGN

and also returns SSH_AGENT_FAILURE on unsupported constrain extensions, such as:
"restrict-destination-v00@openssh.com"

it accepts and ignores constrain extension "sk-provider@openssh.com"

[ghost]

All CLA requirements met.

[bugficks]

See PowerShell/Win32-OpenSSH#1915 (comment)
!defined(WINDOWS) compile time check at #L946 should be removed as well I think.

[splooge]

Would love to see this working. I'm trying to use an ed_25519-sk key from a yubikey, but need to ssh-add it to the agent, which fails.

With the recent cloudflare discount for yubikeys I imagine this will soon become a hot topic.

[ccy]

Any chance to merge this fix into coming release?

[tgauth]

it accepts and ignores constrain extension "sk-provider@openssh.com"

@ddrown apologies for the delay in reviewing this.

Based on https://github.com/openssh/openssh-portable/blob/master/ssh-agent.c#L1274, would ignoring the constrain extension "sk-provider@openssh.com" deviate from upstream behavior without informing the user? I think this could cause confusion, if I am understanding the flow correctly.

[ddrown]

Based on https://github.com/openssh/openssh-portable/blob/master/ssh-agent.c#L1274, would ignoring the constrain extension sk-provider@openssh.com deviate from upstream behavior without informing the user? I think this could cause confusion, if I am understanding the flow correctly.

As far as I can tell, openssh ssh-agent doesn't actually do anything with this constraint currently, aside from rejecting constraints that it doesn't know about. I believe this patch matches the current unix ssh-agent behavior

[tgauth]

Based on https://github.com/openssh/openssh-portable/blob/master/ssh-agent.c#L1274, would ignoring the constrain extension sk-provider@openssh.com deviate from upstream behavior without informing the user? I think this could cause confusion, if I am understanding the flow correctly.

As far as I can tell, openssh ssh-agent doesn't actually do anything with this constraint currently, aside from rejecting constraints that it doesn't know about. I believe this patch matches the current unix ssh-agent behavior

The agent code diverges from upstream, but looks like both use the provider for signing - case SSH2_AGENTC_SIGN_REQUEST.
For upstream, it starts with https://github.com/openssh/openssh-portable/blob/master/ssh-agent.c#L826
For us, it starts with https://github.com/PowerShell/openssh-portable/blob/latestw_all/contrib/win32/win32compat/ssh-agent/keyagent-request.c#L465

Seems as though we will always set the provider to "internal" - https://github.com/PowerShell/openssh-portable/blob/latestw_all/contrib/win32/win32compat/ssh-agent/keyagent-request.c#L347

So, in a sense, I agree that it's ok to not store the sk_provider, but I have a few more questions:

1. What is the sk_provider value sent by Fedora and KeePassXC?
2. Do we need to account for a Windows scenario where the sk_provider is not internal?

I would like to confirm if there needs to be an additional check in the agent to confirm that the sk_provider is "internal", a debug message that the internal provider will be used for signing regardless, or that it ultimately will not matter to the user.

@anmenaga is more familiar with this code - would like to get his input!

[ddrown]

As far as I can tell, openssh ssh-agent doesn't actually do anything with this constraint currently, aside from rejecting constraints that it doesn't know about. I believe this patch matches the current unix ssh-agent behavior

The agent code diverges from upstream, but looks like both use the provider for signing - case SSH2_AGENTC_SIGN_REQUEST. For upstream, it starts with https://github.com/openssh/openssh-portable/blob/master/ssh-agent.c#L826

Ah ok, looking at that code again, it calls sshkey_sign and passes id->sk_provider
https://github.com/openssh/openssh-portable/blob/c6011129cafe4c411f6ef670a4cf271314708eb8/sshkey.c#L2099-L2107

Which passes it on to either sshsk_sign (which is what should happen for the keys relevant to this PR) or impl->funcs->sign

sshsk_sign renames it to provider_path, and passes that to sshsk_open:
https://github.com/openssh/openssh-portable/blob/b36b162be5e6206f12b734222b7bc517c13a6bc8/ssh-sk.c#L673

If provider_path is not "internal", it tries to open the file as a library:
https://github.com/openssh/openssh-portable/blob/b36b162be5e6206f12b734222b7bc517c13a6bc8/ssh-sk.c#L136

And then tries to get the sk_sign identifier from that library:
https://github.com/openssh/openssh-portable/blob/b36b162be5e6206f12b734222b7bc517c13a6bc8/ssh-sk.c#L160

sk_sign is then called to sign the message:
https://github.com/openssh/openssh-portable/blob/b36b162be5e6206f12b734222b7bc517c13a6bc8/ssh-sk.c#L683

For us, it starts with https://github.com/PowerShell/openssh-portable/blob/latestw_all/contrib/win32/win32compat/ssh-agent/keyagent-request.c#L465

Seems as though we will always set the provider to "internal" - https://github.com/PowerShell/openssh-portable/blob/latestw_all/contrib/win32/win32compat/ssh-agent/keyagent-request.c#L347

agreed

So, in a sense, I agree that it's ok to not store the sk_provider, but I have a few more questions:

1. What is the sk_provider value sent by Fedora and KeePassXC?

Fedora ssh-add sends internal by default unless you've passed in something else via the -S flag or used the SSH_SK_PROVIDER environment variable:
https://github.com/openssh/openssh-portable/blob/master/ssh-add.c#L823
https://github.com/openssh/openssh-portable/blob/master/ssh-add.c#L823
https://github.com/openssh/openssh-portable/blob/master/ssh-add.c#L937

strace output:

$ strace -s255 ssh-add .ssh/u2fkey
...
connect(3, {sa_family=AF_UNIX, sun_path="[...]agent.socket"}, 110) = 0
...
write(3, "[....]sk-provider@openssh.com\0\0\0\10internal", 233) = 233

KeePassXC is similar: https://github.com/keepassxreboot/keepassxc/blob/ba1bbd3b52a4260bddd6c501d0447d98d8c5ec4d/src/sshagent/SSHAgent.cpp#L131

2. Do we need to account for a Windows scenario where the sk_provider is not internal?

I assume most cases are covered by using the internal signature system. That's at least true for the U2F/Fido/WebAuthN keys

I would like to confirm if there needs to be an additional check in the agent to confirm that the sk_provider is "internal", a debug message that the internal provider will be used for signing regardless, or that it ultimately will not matter to the user.

A debug message would be handy for when someone is trying to do something that won't work.

@anmenaga is more familiar with this code - would like to get his input!

[anmenaga]

I'd recommend adding that debug message like @tgauth described.

[ddrown]

After changing it to reject non-internal sk_providers and logging a debug message:

testing "internal" still works:

$ strace -s258 ssh-add
...
write(3, [...]sk-provider@openssh.com\0\0\0\10internal", 233) = 233
...
Identity added: /home/ddrown/.ssh/id_ecdsa_sk (somu)

testing "something-other-than-internal" fails:

$ strace -s258 ssh-add -S something-other-than-internal
...
write(3, "[...]sk-provider@openssh.com\0\0\0\35something-other-than-internal", 254) = 254
...
Could not add identity "/home/ddrown/.ssh/id_ecdsa_sk": agent refused operation

And the debug logs:

> .\ssh-agent.exe -d
...
debug1: parse_key_constraint_extension: constraint ext sk-provider@openssh.com
parse_key_constraint_extension: unsupported sk-provider: something-other-than-internal

[tgauth]

LGTM - thanks @ddrown!