From b9cf244080772ae1cec8969b3b2a39fb3287d864 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 22 Oct 2026 11:10:14 +0900 Subject: [PATCH 1/7] feat(ssh): support FIDO security keys through SecurityKeyProvider bssh did not list the sk-* key types, so the regress harness never generated sk-* host keys and the two sk-*-cert-v01@openssh.com phases of hostkey-agent could not pass. Listing them makes every regress test add sk-* IdentityFile lines, so user authentication with authenticator-hosted keys had to work too. This follows OpenSSH 10.3p1 built without its internal provider. SecurityKeyProvider becomes a runtime option (library path, internal, none, or $VAR; default $SSH_SK_PROVIDER, printed raw by -G). src/ssh/security_key holds the SecurityKeyProvider trait that #331 will plug internal into, $VAR resolution, the dlopen binding to sk-api 0x000a0000 with every provider pointer validated and the response freed as ssh-sk.c does, and the sk-* signature encoder. The provider is loaded in process instead of through an ssh-sk-helper style subprocess: bssh has no pledge sandbox to keep the middleware out of, and a re-exec helper would add a private protocol without changing what runs as the user. Provider calls run on a blocking thread, the touch notice is printed when stderr is a terminal, and a PIN is prompted once outside BatchMode. A provider that cannot be opened is reported before the identity is offered and authentication continues; a sk_sign failure after the server asked for the signature ends the transport, like LocalSignerFailed. Static musl builds compile and name the dlopen limitation. sk-* identities sign through a russh Signer for plain keys and user certificates; agent-held keys keep using the agent. -Q and the default HostKeyAlgorithms and PubkeyAcceptedAlgorithms match the reference order except for the webauthn-sk-* pair, which bssh cannot verify. A regression test keeps a certificate-only HostKeyAlgorithms list from being widened. Validation: hostkey-agent, keytype, limit-keytype and agent pass locally with the regress sk-dummy provider; connect, proxy-connect and hostkey-rotate still pass. Unit tests cover $VAR expansion, the internal diagnostic, load failures, signature encoding, the algorithm lists and the signer, and each fails when its behavior is reverted. Refs #328 --- Cargo.lock | 11 + Cargo.toml | 3 + docs/architecture/ssh-client.md | 2 + docs/architecture/ssh-config-parser.md | 1 + docs/man/bssh.1 | 9 +- docs/openssh-compatibility-status.md | 13 +- src/app/query.rs | 139 ++++-- src/ssh/auth.rs | 3 + src/ssh/mod.rs | 1 + src/ssh/security_key/external.rs | 424 ++++++++++++++++++ src/ssh/security_key/mod.rs | 376 ++++++++++++++++ src/ssh/security_key/signature.rs | 147 ++++++ src/ssh/ssh_config/dump.rs | 6 +- src/ssh/ssh_config/dump_tests.rs | 38 ++ .../parser/options/authentication.rs | 11 + src/ssh/ssh_config/parser/options/mod.rs | 4 +- src/ssh/ssh_config/parser/options/support.rs | 12 +- src/ssh/ssh_config/resolver.rs | 3 + src/ssh/ssh_config/types.rs | 3 + src/ssh/tokio_client/algorithms.rs | 101 ++++- src/ssh/tokio_client/auth_policy.rs | 4 + src/ssh/tokio_client/authentication.rs | 140 +++++- src/ssh/tokio_client/connection.rs | 1 + src/ssh/tokio_client/connection_tests.rs | 48 ++ src/ssh/tokio_client/error.rs | 19 + src/ssh/tokio_client/mod.rs | 1 + src/ssh/tokio_client/security_key_signer.rs | 411 +++++++++++++++++ tests/openssh-regress/selection.tsv | 8 +- 28 files changed, 1849 insertions(+), 90 deletions(-) create mode 100644 src/ssh/security_key/external.rs create mode 100644 src/ssh/security_key/mod.rs create mode 100644 src/ssh/security_key/signature.rs create mode 100644 src/ssh/tokio_client/security_key_signer.rs diff --git a/Cargo.lock b/Cargo.lock index 8796a899c..8800da23c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -409,6 +409,7 @@ dependencies = [ "insta", "ipnetwork", "libc", + "libloading", "lru", "mockall", "mockito", @@ -2305,6 +2306,16 @@ version = "0.2.189" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3eaf3ede3fee6db1a4c2ee091bf8a8b4dccdc6d17f656fb07896ee72867612f2" +[[package]] +name = "libloading" +version = "0.8.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d7c4b02199fee7c5d21a5ae7d8cfa79a6ef5bb2fc834d6e9058e89c825efdc55" +dependencies = [ + "cfg-if", + "windows-link", +] + [[package]] name = "libm" version = "0.2.16" diff --git a/Cargo.toml b/Cargo.toml index 462588a45..54c2d118f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -75,6 +75,9 @@ hmac = "0.13.0" sha2 = "0.11.0" hickory-resolver = { version = "0.26.1", features = ["dnssec-ring"] } libc = "0.2" +# sk-api (FIDO security key) middleware libraries are dlopened in process; +# see src/ssh/security_key for why no ssh-sk-helper style subprocess exists. +libloading = "0.8" ipnetwork = "0.21" bcrypt = "0.19" # `argon2` 0.5 uses `crypto-common` 0.1.7, which deliberately pins diff --git a/docs/architecture/ssh-client.md b/docs/architecture/ssh-client.md index 329b54eee..3ddbff06f 100644 --- a/docs/architecture/ssh-client.md +++ b/docs/architecture/ssh-client.md @@ -15,6 +15,8 @@ **Tokio Client Module Structure :** - `tokio_client/connection.rs` - Connection management (293 lines) - `tokio_client/authentication.rs` - Authentication methods (378 lines) +- `tokio_client/security_key_signer.rs` - russh `Signer` for FIDO `sk-*` identities: resolves `SecurityKeyProvider`, calls the provider on a blocking thread, prints the touch notice and prompts once for a PIN +- `ssh/security_key/` - provider interface (`SecurityKeyProvider` trait, `$VAR` resolution, `internal`/`none` handling), the `sk-api.h` `dlopen` binding (`external.rs`), and the `sk-*` signature blob encoder (`signature.rs`) - `tokio_client/channel_manager.rs` - Channel operations (230 lines) - `tokio_client/file_transfer.rs` - SFTP file operations (285 lines) diff --git a/docs/architecture/ssh-config-parser.md b/docs/architecture/ssh-config-parser.md index e9a24cdbd..e17beb027 100644 --- a/docs/architecture/ssh-config-parser.md +++ b/docs/architecture/ssh-config-parser.md @@ -207,6 +207,7 @@ bssh supports 40+ SSH configuration directives organized into categories: - `ConnectTimeout` - Connection timeout in seconds - `ServerAliveInterval` - Keepalive interval - `ServerAliveCountMax` - Keepalive retry count +- `SecurityKeyProvider` - Library implementing OpenSSH's `sk-api.h` for `sk-*` identities, `internal`, `none`, or a `$VAR` reference (default `$SSH_SK_PROVIDER`). Stored verbatim in `security_key_provider`; `-G` prints the raw value and expansion happens in `src/ssh/tokio_client/security_key_signer.rs` when an `sk-*` identity is used. - `Compression` - Advertise transport compression to the server (yes/no, default: no). `yes` offers eager `zlib` ahead of `none`; `no`/unset offers only `none`. `zlib@openssh.com` is never advertised because russh's delayed-zlib codec desyncs the flate2 stream a few packets after activation; wiring is in `SshConnectionConfig::to_russh_config` (`src/ssh/tokio_client/connection.rs`). **Authentication Options:** diff --git a/docs/man/bssh.1 b/docs/man/bssh.1 index 0f71682f7..4a9d84171 100644 --- a/docs/man/bssh.1 +++ b/docs/man/bssh.1 @@ -122,7 +122,7 @@ Example: -J jump1@proxy1,jump2@proxy2 .TP .BR \-Q " " \fIquery_option\fR -Query SSH configuration options (cipher, kex, mac, key, protocol-version, help) +Query SSH configuration options (cipher, cipher-auth, mac, kex, compression, key, key-plain, key-cert, key-sig, protocol-version, help). The key lists include the FIDO types sk-ssh-ed25519@openssh.com and sk-ecdsa-sha2-nistp256@openssh.com in OpenSSH's order; webauthn-sk-ecdsa-sha2-nistp256@openssh.com is not listed because bssh does not verify it. .TP .BR \-4 ", " \-\-ipv4 @@ -1096,6 +1096,13 @@ Special values: Example: .I IdentityAgent ~/.1password/agent.sock +.TP +.B SecurityKeyProvider +Specifies the library used to access FIDO authenticator-hosted keys (sk-ssh-ed25519@openssh.com and sk-ecdsa-sha2-nistp256@openssh.com identity files, as enrolled by ssh-keygen -t ed25519-sk or ecdsa-sk). The value is a path to a shared library implementing OpenSSH's sk-api (for example libsk-libfido2.so), or a name starting with $ that selects an environment variable holding that path. The default is $SSH_SK_PROVIDER; when the variable is unset, or the value is none, sk-* identities are skipped. The value internal (OpenSSH's built-in USB HID provider) is not available in this build and is reported before the next identity is tried. The library is loaded with dlopen(3); statically linked musl builds cannot load it and say so. When a key requires user presence and standard error is a terminal, bssh prints a confirmation notice while waiting for the touch; when the provider asks for a PIN and BatchMode is off, bssh prompts for it once. Keys held by an agent keep signing through the agent. +.br +Example: +.I SecurityKeyProvider /usr/lib/libsk-libfido2.so + .TP .B PubkeyAcceptedAlgorithms Specifies the signature algorithms that will be used for public key authentication. diff --git a/docs/openssh-compatibility-status.md b/docs/openssh-compatibility-status.md index db9241e0c..28780a059 100644 --- a/docs/openssh-compatibility-status.md +++ b/docs/openssh-compatibility-status.md @@ -1,6 +1,6 @@ # OpenSSH compatibility status and remaining contracts -Full OpenSSH command compatibility is the target, not a claim about the current release. A regression score, accepted option, or working bssh-to-bssh control socket does not establish a complete replacement. Residual work after [#314](https://github.com/lablup/bssh/issues/314) is tracked in [#327](https://github.com/lablup/bssh/issues/327) (destination-constrained agent keys) and [#328](https://github.com/lablup/bssh/issues/328) (FIDO host-key phases of `hostkey-agent`); [#281](https://github.com/lablup/bssh/issues/281) was closed in favor of #314. +Full OpenSSH command compatibility is the target, not a claim about the current release. A regression score, accepted option, or working bssh-to-bssh control socket does not establish a complete replacement. Residual work after [#314](https://github.com/lablup/bssh/issues/314) is tracked in [#327](https://github.com/lablup/bssh/issues/327) (destination-constrained agent keys); [#328](https://github.com/lablup/bssh/issues/328) added FIDO security-key support, and [#331](https://github.com/lablup/bssh/issues/331) tracks the built-in `internal` provider. [#281](https://github.com/lablup/bssh/issues/281) was closed in favor of #314. ## Measurement boundary @@ -20,7 +20,7 @@ This ledger records contracts still requiring implementation or verification, no | Percent/environment | Directive-specific tokens and environment expansion, trusted `Match exec`, explicitly permitted shell `LocalCommand` | Historical `percent` fails on both platforms. Shell semantics must retain `PermitLocalCommand` gating and untrusted-input boundaries. | | Rekey | Actual byte/time-triggered exchange and truthful KEX/NEWKEYS diagnostics | `rekey` passes on Linux (114.3 s candidate, 114.5 s reference) after the client stopped padding every connection by 0.6 s; the manifest minimum is 180 s. macOS remains environmental (the reference client also exceeds the limit there). | | Agent | Destination-constrained identities (`session-bind@openssh.com`) | Every `agent-restrict` phase that does not need a constrained key passes on Linux; the eight constrained phases fail until russh exposes the binding tuple. Tracked in #327. | -| Host certificates | FIDO (`sk-*`) host keys and certificates | `hostkey-agent` passes every plain and non-FIDO certificate phase and `multiple hostkeys`; the two `sk-*-cert-v01@openssh.com` phases fail because bssh does not advertise FIDO host-key types. Tracked in #328. | +| Security keys | Built-in (`SecurityKeyProvider internal`) USB HID provider, `webauthn-sk-*` signatures, resident-key and enrollment tooling | `hostkey-agent`, `keytype`, `limit-keytype` and `agent` pass on Linux with the regress `sk-dummy` middleware. `internal` is refused with a diagnostic until #331; `webauthn-sk-ecdsa-sha2-nistp256@openssh.com` is not advertised because bssh cannot verify it; `bssh-keygen` cannot enroll or list resident keys. | | Multiplexing | OpenSSH mux-v4 local protocol, raw `-O proxy`, complete channel reporting, allocated remote-port replies | `multiplex` passes on Linux (72.1 s) including TCP and Unix-domain `-O forward`/`-O cancel`, when the regress directory path fits `sockaddr_un`. Control sockets still use framed JSON; passenger reuse is not OpenSSH-master interoperability. | | Jump hosts | CLI/config parsing, SSH URI hops, `none`, multihop transport/authentication | Historical `proxyjump` fails on both platforms. Parser tests alone do not establish transport behavior. | | Remote copy | Remote-to-remote copy invocation, transport selection, integrity and exit status | Historical Ubuntu `scp3` fails; macOS environmental. | @@ -37,6 +37,10 @@ Unix-domain forwarding (`-L`/`-R` with a socket path on either side, including ` Host certificates are accepted only after full validation: a `@cert-authority` line (in a known_hosts file or in the `KnownHostsCommand` output, which receives the certificate as `%K`/`%t` and runs once per check) naming the host (or `HostKeyAlias`) with the signing CA key, host certificate type, a principal matching the host name (principals may be case-sensitive wildcard patterns, as in OpenSSH, and are read only after the CA signature verifies), no critical options, a CA signature algorithm permitted by `CASignatureAlgorithms`, the validity window, a verified signature, and no revocation by `@revoked` lines in either source or by any `RevokedHostKeys` file (plain key lists; a KRL or unreadable file anywhere in the list fails closed). On a non-default port a CA line for the bare host name also matches, as OpenSSH's portless retry does. Without a matching CA the certified key is checked as a plain host key, but only if `HostKeyAlgorithms` permits its plain type, so a certificate-only list never falls back to known_hosts or first-use recording; a pinned key (`ServerCheckMethod::PublicKey`/`PublicKeyFile`) never consults CA lines; with a matching CA any other validation failure refuses the connection instead of downgrading, which is stricter than OpenSSH. `HostKeyAlgorithms` accepts certificate names and advertises the certificate form of every default algorithm; russh always sends certificate names ahead of plain names, so a mixed list cannot interleave them, and a certificate-only list yields no plain algorithm at all. `@cert-authority` host fields follow OpenSSH matching, including hashed (`|1|`) entries and `!pattern` negations that exclude a host from the CA's scope. A `RevokedHostKeys` path that cannot be expanded refuses every host key instead of disabling revocation, and `RevokedHostKeys` applies to certificates under `StrictHostKeyChecking=no` as it does to plain keys. +## Security keys + +Authenticator-hosted (`sk-ssh-ed25519@openssh.com`, `sk-ecdsa-sha2-nistp256@openssh.com`) keys follow OpenSSH 10.3p1 built without `ENABLE_SK_INTERNAL`. `SecurityKeyProvider` defaults to `$SSH_SK_PROVIDER`; a `$`-prefixed value names an environment variable, an unset variable or `none` disables `sk-*` identities (logged at debug level, as `ssh` does), and `internal` prints a diagnostic and moves on to the next identity until #331 supplies the built-in provider. An external provider is a shared library implementing `sk-api.h` version `0x000a0000` (`sk_api_version`, `sk_enroll`, `sk_sign`, `sk_load_resident_keys`); bssh loads it in process with `dlopen` and calls `sk_sign` on a blocking thread, printing `Confirm user presence for key ...` when the key requires a touch and stderr is a terminal, and prompting once for a PIN when the provider reports `SSH_SK_ERR_PIN_REQUIRED` outside `BatchMode`. OpenSSH runs the same calls in `ssh-sk-helper` to keep the middleware out of its `pledge`d client; bssh has no such sandbox, so a helper would add a private protocol without changing what runs as the user. A provider that cannot be opened (missing file, no `sk_api_version`, wrong major version) is reported before the key is offered and authentication continues; a failure inside `sk_sign` after the server has asked for the signature ends the transport, because russh is then waiting for that signature. Static musl release builds cannot `dlopen`; they build, and report that limitation when an external provider is configured. Host keys and host certificates of the `sk-*` types are verified by ssh-key's `sk` verifiers (`SHA256(application) || flags || counter || SHA256(message)`), with no flag policy on the client side, as in OpenSSH. Agent-held `sk-*` keys keep signing through the agent. `-Q key-plain`, `-Q key`, `-Q key-cert`, `-Q key-sig`, `HostKeyAlgorithms` and `PubkeyAcceptedAlgorithms` list the `sk-*` names in the reference order; the only difference from the reference `ssh -Q key-sig` and `ssh -G` is the absent `webauthn-sk-ecdsa-sha2-nistp256@openssh.com` pair. + ## ssh_config integration Each first-wave directive in #281 has a runtime consumer and behavior/policy test. Registry tests in `src/ssh/ssh_config/parser/options/support.rs` enforce one classification per keyword/alias. `canonical_unimplemented_and_unknown_diagnostics_use_real_source_and_log_file` checks named, source-located, once-only warnings. These checks prevent silent discard, not all semantic incompatibility. @@ -45,7 +49,7 @@ Named-regress acceptance is separate: historical results include candidate failu ## Previously excluded scope -[#275](https://github.com/lablup/bssh/issues/275) excluded GSSAPI, PKCS#11/FIDO middleware, X11, tun forwarding, KRL generation, CA signing, sshd configuration tests and PuTTY/Dropbear/Twisted conch/ssh.com interoperability. Manifest rows preserve reasons. `cert-userkey`, named by #281, is permanently skipped for CA signing; it must not be marked passed. +[#275](https://github.com/lablup/bssh/issues/275) excluded GSSAPI, PKCS#11 and FIDO middleware, X11, tun forwarding, KRL generation, CA signing, sshd configuration tests and PuTTY/Dropbear/Twisted conch/ssh.com interoperability. #328 reversed the FIDO half of that exclusion: `sk-*` keys are supported through an external `SecurityKeyProvider` library, matching OpenSSH 10.3p1 built without its internal provider. PKCS#11 stays excluded and its manifest rows preserve that reason. `cert-userkey`, named by #281, is permanently skipped for CA signing; it must not be marked passed. These exclusions bounded the original score-gated epic, not full compatibility. Revisit them explicitly for a broader replacement contract. Distinguish server-only tooling and third-party interoperability from the desired `ssh(1)` surface instead of silently declaring them implemented. Unsupported configuration requires explicit diagnostics until runtime support exists. @@ -61,7 +65,8 @@ The following focused checks were performed on a Linux aarch64 host while closin - `rekey`: pinned harness passes in 114.3 s (reference client 114.5 s on the same host, 105 s of it sleeping); the manifest minimum was raised to 180 s with those numbers. - `multiplex`: passes in 72.1 s through the short OBJ path, including the TCP `forward` phase, Unix-domain `forward`, and the TCP and Unix `-O forward`/`-O cancel` phases. -- `hostkey-agent`: every plain key type, every non-FIDO certificate type, and `multiple hostkeys` pass; only the two `sk-*` certificate phases fail (#328). An explicit certificate-only `HostKeyAlgorithms` restriction is now honored instead of being widened to the default certificate list. +- `hostkey-agent`: every phase passes, including the two `sk-*` certificate phases that #328 added (the regress tree supplies `sk-dummy.so` through `SSH_SK_PROVIDER`). An explicit certificate-only `HostKeyAlgorithms` restriction is honored instead of being widened to the default certificate list; `certificate_only_host_key_algorithms_are_never_widened` in `connection_tests.rs` keeps it that way. +- `keytype`, `limit-keytype` and `agent`: pass with the `sk-*` key types active. `keytype` signs user authentication with `ed25519-sk` and `ecdsa-sk` identity files through the provider. - `agent-restrict`: through the short OBJ path, every phase passes except the eight that need `session-bind@openssh.com` (#327), including the certificate hosts `host_d` and `host_e` in the `w/o agent` and unrestricted phases. The earlier macOS checkpoints from the first #314 follow-up are kept below for history. diff --git a/src/app/query.rs b/src/app/query.rs index 8a068d949..4291d372e 100644 --- a/src/app/query.rs +++ b/src/app/query.rs @@ -16,31 +16,66 @@ use bssh::diagnosticln as eprintln; -/// Plain host/user key types bssh can verify and authenticate with. -const PLAIN_KEY_TYPES: &[&str] = &[ - "ssh-ed25519", - "ecdsa-sha2-nistp256", - "ecdsa-sha2-nistp384", - "ecdsa-sha2-nistp521", - "ssh-rsa", +/// Key types bssh can verify and authenticate with, as `(plain, certificate)` +/// name pairs in the order the reference `ssh -Q key` prints them. The +/// FIDO `sk-*` certificate names do not derive from their plain names by +/// suffixing, so the table spells both out. +const KEY_TYPES: &[(&str, &str)] = &[ + ("ssh-ed25519", "ssh-ed25519-cert-v01@openssh.com"), + ( + "sk-ssh-ed25519@openssh.com", + "sk-ssh-ed25519-cert-v01@openssh.com", + ), + ( + "ecdsa-sha2-nistp256", + "ecdsa-sha2-nistp256-cert-v01@openssh.com", + ), + ( + "ecdsa-sha2-nistp384", + "ecdsa-sha2-nistp384-cert-v01@openssh.com", + ), + ( + "ecdsa-sha2-nistp521", + "ecdsa-sha2-nistp521-cert-v01@openssh.com", + ), + ( + "sk-ecdsa-sha2-nistp256@openssh.com", + "sk-ecdsa-sha2-nistp256-cert-v01@openssh.com", + ), + ("ssh-rsa", "ssh-rsa-cert-v01@openssh.com"), ]; -/// Signature algorithm names, which add the SHA-2 RSA variants. -const SIGNATURE_KEY_TYPES: &[&str] = &[ - "ssh-ed25519", - "ecdsa-sha2-nistp256", - "ecdsa-sha2-nistp384", - "ecdsa-sha2-nistp521", - "ssh-rsa", - "rsa-sha2-256", - "rsa-sha2-512", +/// Signature algorithms (`-Q key-sig`): the key types plus the SHA-2 RSA +/// variants. OpenSSH also lists `webauthn-sk-ecdsa-sha2-nistp256@openssh.com` +/// here; bssh cannot verify WebAuthn signatures and leaves it out. +const SIGNATURE_TYPES: &[(&str, &str)] = &[ + ("rsa-sha2-256", "rsa-sha2-256-cert-v01@openssh.com"), + ("rsa-sha2-512", "rsa-sha2-512-cert-v01@openssh.com"), ]; -fn certificate_names(plain: &[&str]) -> Vec { - plain +fn plain_names(pairs: &[(&'static str, &'static str)]) -> Vec<&'static str> { + pairs.iter().map(|(plain, _)| *plain).collect::>() +} + +fn certificate_names(pairs: &[(&'static str, &'static str)]) -> Vec<&'static str> { + pairs + .iter() + .map(|(_, certificate)| *certificate) + .collect::>() +} + +/// `ssh -Q key` interleaves each plain name with its certificate form. +fn interleaved_names(pairs: &[(&'static str, &'static str)]) -> Vec<&'static str> { + pairs .iter() - .map(|name| format!("{name}-cert-v01@openssh.com")) - .collect() + .flat_map(|(plain, certificate)| [*plain, *certificate]) + .collect::>() +} + +fn key_sig_names() -> Vec<&'static str> { + let mut names = interleaved_names(KEY_TYPES); + names.extend(interleaved_names(SIGNATURE_TYPES)); + names } pub fn is_supported_query(query: &str) -> bool { @@ -90,16 +125,10 @@ pub fn handle_query(query: &str) { // underlying transport currently corrupts post-auth packets. println!("none\nzlib"); } - "key-plain" => println!("{}", PLAIN_KEY_TYPES.join("\n")), - "key-cert" => println!("{}", certificate_names(PLAIN_KEY_TYPES).join("\n")), - "key" => { - println!("{}", PLAIN_KEY_TYPES.join("\n")); - println!("{}", certificate_names(PLAIN_KEY_TYPES).join("\n")); - } - "key-sig" => { - println!("{}", SIGNATURE_KEY_TYPES.join("\n")); - println!("{}", certificate_names(SIGNATURE_KEY_TYPES).join("\n")); - } + "key-plain" => println!("{}", plain_names(KEY_TYPES).join("\n")), + "key-cert" => println!("{}", certificate_names(KEY_TYPES).join("\n")), + "key" => println!("{}", interleaved_names(KEY_TYPES).join("\n")), + "key-sig" => println!("{}", key_sig_names().join("\n")), "protocol-version" => { println!("2"); } @@ -132,13 +161,55 @@ mod tests { } } + /// Reference lists from the pinned OpenSSH 10.3p1 `ssh -Q`, minus the + /// `webauthn-sk-*` signature names bssh does not verify. #[test] - fn certificate_key_names_follow_openssh_naming() { + fn key_queries_match_the_reference_client_order() { + assert!(is_supported_query("key-cert")); assert_eq!( - certificate_names(&["ssh-ed25519"]), - vec!["ssh-ed25519-cert-v01@openssh.com".to_string()] + plain_names(KEY_TYPES), + [ + "ssh-ed25519", + "sk-ssh-ed25519@openssh.com", + "ecdsa-sha2-nistp256", + "ecdsa-sha2-nistp384", + "ecdsa-sha2-nistp521", + "sk-ecdsa-sha2-nistp256@openssh.com", + "ssh-rsa", + ] ); - assert!(is_supported_query("key-cert")); + assert_eq!( + certificate_names(KEY_TYPES), + [ + "ssh-ed25519-cert-v01@openssh.com", + "sk-ssh-ed25519-cert-v01@openssh.com", + "ecdsa-sha2-nistp256-cert-v01@openssh.com", + "ecdsa-sha2-nistp384-cert-v01@openssh.com", + "ecdsa-sha2-nistp521-cert-v01@openssh.com", + "sk-ecdsa-sha2-nistp256-cert-v01@openssh.com", + "ssh-rsa-cert-v01@openssh.com", + ] + ); + assert_eq!( + &interleaved_names(KEY_TYPES)[..4], + [ + "ssh-ed25519", + "ssh-ed25519-cert-v01@openssh.com", + "sk-ssh-ed25519@openssh.com", + "sk-ssh-ed25519-cert-v01@openssh.com", + ] + ); + assert_eq!( + &key_sig_names()[14..], + [ + "rsa-sha2-256", + "rsa-sha2-256-cert-v01@openssh.com", + "rsa-sha2-512", + "rsa-sha2-512-cert-v01@openssh.com", + ] + ); + assert_eq!(key_sig_names().len(), 18); + assert!(!key_sig_names().iter().any(|name| name.contains("webauthn"))); } #[test] diff --git a/src/ssh/auth.rs b/src/ssh/auth.rs index 2127b2942..c0d86c6b0 100644 --- a/src/ssh/auth.rs +++ b/src/ssh/auth.rs @@ -559,6 +559,7 @@ impl AuthContext { accepted.clone(), !self.policy.batch_mode, use_keychain, + self.policy.security_key_provider.clone(), )); } } @@ -668,6 +669,7 @@ impl AuthContext { accepted.clone(), allow_passphrase_prompt, use_keychain, + self.policy.security_key_provider.clone(), )); } if identity.algorithm.as_ref().is_none_or(|algorithm| { @@ -678,6 +680,7 @@ impl AuthContext { accepted.clone(), allow_passphrase_prompt, use_keychain, + self.policy.security_key_provider.clone(), )); } } diff --git a/src/ssh/mod.rs b/src/ssh/mod.rs index 61c7a2bd3..534190fae 100644 --- a/src/ssh/mod.rs +++ b/src/ssh/mod.rs @@ -19,6 +19,7 @@ pub mod control; pub mod handler; pub mod known_hosts; pub mod pool; +pub mod security_key; pub mod session_policy; pub mod ssh_config; pub mod tokio_client; diff --git a/src/ssh/security_key/external.rs b/src/ssh/security_key/external.rs new file mode 100644 index 000000000..c2d30204b --- /dev/null +++ b/src/ssh/security_key/external.rs @@ -0,0 +1,424 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! `dlopen` binding to an external `sk-api.h` middleware library. +//! +//! The ABI is the pinned OpenSSH 10.3 `sk-api.h` (version `0x000a0000`). +//! Only `sk_sign` is called; `sk_enroll` and `sk_load_resident_keys` must +//! exist (as `sshsk_open` requires) but enrollment stays with `ssh-keygen`. + +use std::ffi::{CString, c_char, c_int, c_void}; +use std::path::{Path, PathBuf}; +use std::ptr; + +use libloading::{Library, Symbol}; +use zeroize::Zeroizing; + +use super::{ + SecurityKeyError, SecurityKeyProvider, SkSignRequest, SkSignResponse, check_api_version, +}; + +/// Upper bound on a single signature component the provider may hand back. +/// Real values are 32 to 64 bytes; anything larger is a corrupt response. +const MAX_COMPONENT_LEN: usize = 4096; + +/// `struct sk_sign_response` from `sk-api.h`. +#[repr(C)] +struct RawSignResponse { + flags: u8, + counter: u32, + sig_r: *mut u8, + sig_r_len: usize, + sig_s: *mut u8, + sig_s_len: usize, +} + +type SkApiVersionFn = unsafe extern "C" fn() -> u32; + +type SkSignFn = unsafe extern "C" fn( + alg: u32, + data: *const u8, + data_len: usize, + application: *const c_char, + key_handle: *const u8, + key_handle_len: usize, + flags: u8, + pin: *const c_char, + options: *mut *mut c_void, + sign_response: *mut *mut RawSignResponse, +) -> c_int; + +/// A provider library kept open for the life of the signer. +pub(super) struct ExternalProvider { + path: PathBuf, + sign: SkSignFn, + /// Declared after `sign` so the function pointer is never used once the + /// library is unmapped; both go away together. + _library: Library, +} + +impl std::fmt::Debug for ExternalProvider { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("ExternalProvider") + .field("path", &self.path) + .finish_non_exhaustive() + } +} + +impl ExternalProvider { + pub(super) fn load(path: &Path) -> Result { + // SAFETY: loading a shared object runs its initializers. The path is + // the user's own `SecurityKeyProvider` setting (or `$SSH_SK_PROVIDER`), + // the same trust OpenSSH extends when `ssh-sk-helper` dlopens it. + let library = + unsafe { Library::new(path) }.map_err(|error| SecurityKeyError::LoadFailed { + path: path.to_path_buf(), + reason: describe_load_error(&error), + })?; + let sign = bind_symbols(path, &library)?; + Ok(Self { + path: path.to_path_buf(), + sign, + _library: library, + }) + } +} + +/// Explain a failed `dlopen`, naming the static-linking limitation of musl +/// release builds instead of leaving the user with "Dynamic loading not +/// supported". +fn describe_load_error(error: &libloading::Error) -> String { + let reason = error.to_string(); + if cfg!(target_env = "musl") { + format!( + "{reason}; this bssh build is statically linked and cannot load shared libraries, so an external SecurityKeyProvider needs a dynamically linked bssh" + ) + } else { + reason + } +} + +/// Required `sk-api.h` entry points besides `sk_api_version` and `sk_sign`. +/// They are looked up, never called: `sshsk_open` refuses a library that +/// lacks any of them, and so does bssh. +const REQUIRED_OPERATIONS: [&[u8]; 2] = [b"sk_enroll\0", b"sk_load_resident_keys\0"]; + +/// The provider checks `sshsk_open` performs, with the library accesses +/// abstracted so they can be tested without a real shared object: the +/// reported version must have the supported major, and every operation +/// symbol must resolve. +fn validate_provider( + path: &Path, + version: u32, + mut has_symbol: F, +) -> Result<(), SecurityKeyError> +where + F: FnMut(&[u8]) -> Result<(), String>, +{ + tracing::debug!( + "provider {} implements version {version:#010x}", + path.display() + ); + check_api_version(path, version)?; + for name in REQUIRED_OPERATIONS.iter().chain([&&b"sk_sign\0"[..]]) { + has_symbol(name).map_err(|reason| SecurityKeyError::NotAProvider { + path: path.to_path_buf(), + reason, + })?; + } + Ok(()) +} + +/// Resolve and validate the sk-api entry points the way `sshsk_open` does. +fn bind_symbols(path: &Path, library: &Library) -> Result { + // SAFETY: the symbol is declared in sk-api.h as `uint32_t sk_api_version(void)`; + // the type annotation matches that prototype exactly. + let api_version: Symbol<'_, SkApiVersionFn> = unsafe { library.get(b"sk_api_version\0") } + .map_err(|error| SecurityKeyError::NotAProvider { + path: path.to_path_buf(), + reason: error.to_string(), + })?; + // SAFETY: calling a provider function with no arguments and no + // preconditions; it only reports a constant. + let version = unsafe { api_version() }; + validate_provider(path, version, |name| { + // SAFETY: the symbol is only looked up, never called through this + // handle, so an opaque pointer type is used on purpose. + unsafe { library.get::<*const c_void>(name) } + .map(|_| ()) + .map_err(|error| error.to_string()) + })?; + // SAFETY: the symbol is declared in sk-api.h as `int sk_sign(...)` with + // exactly the parameter list `SkSignFn` spells out, and + // `validate_provider` confirmed it resolves. + let sign: Symbol<'_, SkSignFn> = + unsafe { library.get(b"sk_sign\0") }.map_err(|error| SecurityKeyError::NotAProvider { + path: path.to_path_buf(), + reason: error.to_string(), + })?; + Ok(*sign) +} + +impl SecurityKeyProvider for ExternalProvider { + fn sign( + &self, + request: &SkSignRequest<'_>, + pin: Option<&str>, + ) -> Result { + let application = CString::new(request.application) + .map_err(|_| SecurityKeyError::InvalidRequest("application contains NUL"))?; + let pin = pin + .map(|pin| { + CString::new(pin).map_err(|_| SecurityKeyError::InvalidRequest("PIN contains NUL")) + }) + .transpose()?; + let pin_ptr = pin.as_ref().map_or(ptr::null(), |pin| pin.as_ptr()); + let mut response: *mut RawSignResponse = ptr::null_mut(); + + // SAFETY: every pointer is valid for the stated length for the whole + // call: `data` and `key_handle` borrow the request, the C strings are + // NUL-terminated and live until after the call, `options` is NULL as + // in `sshsk_sign`, and `response` is a valid out-pointer the provider + // fills with memory it allocated with malloc. + let code = unsafe { + (self.sign)( + request.algorithm.api_code(), + request.data.as_ptr(), + request.data.len(), + application.as_ptr(), + request.key_handle.as_ptr(), + request.key_handle.len(), + request.flags, + pin_ptr, + ptr::null_mut(), + &mut response, + ) + }; + if let Some(pin) = pin { + drop(Zeroizing::new(pin.into_bytes())); + } + if code != 0 { + if !response.is_null() { + // SAFETY: a non-null response was allocated by the provider + // and is owned by us from here; see `free_response`. + unsafe { free_response(response) }; + } + tracing::debug!("sk_sign failed with code {code}"); + return Err(SecurityKeyError::from_sk_code(code)); + } + if response.is_null() { + return Err(SecurityKeyError::InvalidResponse( + "provider returned success without a response", + )); + } + // SAFETY: `response` is non-null and was allocated by the provider + // for us; it is read once and then released exactly as + // `sshsk_free_sign_response` does. + let copied = unsafe { copy_response(response) }; + unsafe { free_response(response) }; + copied + } + + fn describe(&self) -> String { + self.path.display().to_string() + } +} + +/// Copy a component out of provider memory after validating pointer and +/// length together: a NULL pointer with a non-zero length, or an absurd +/// length, is a corrupt response rather than something to read. +/// +/// # Safety +/// `pointer` must either be NULL or point to `len` readable bytes. +unsafe fn copy_component( + pointer: *const u8, + len: usize, + name: &'static str, +) -> Result>, SecurityKeyError> { + if len == 0 { + return Ok(Zeroizing::new(Vec::new())); + } + if pointer.is_null() { + return Err(SecurityKeyError::InvalidResponse(name)); + } + if len > MAX_COMPONENT_LEN { + return Err(SecurityKeyError::InvalidResponse(name)); + } + // SAFETY: guaranteed by the caller contract and the checks above. + let bytes = unsafe { std::slice::from_raw_parts(pointer, len) }; + Ok(Zeroizing::new(bytes.to_vec())) +} + +/// # Safety +/// `response` must point to a `struct sk_sign_response` the provider filled. +unsafe fn copy_response( + response: *const RawSignResponse, +) -> Result { + // SAFETY: the caller guarantees `response` is a valid, initialized struct. + let raw = unsafe { &*response }; + let sig_r = unsafe { copy_component(raw.sig_r, raw.sig_r_len, "sig_r invalid") }?; + let sig_s = unsafe { copy_component(raw.sig_s, raw.sig_s_len, "sig_s invalid") }?; + Ok(SkSignResponse { + flags: raw.flags, + counter: raw.counter, + sig_r, + sig_s, + }) +} + +/// Release a response the way `sshsk_free_sign_response` does: zero and free +/// each component with the C allocator, then the struct itself. +/// +/// # Safety +/// `response` must be non-null, allocated with malloc by the provider, and +/// not used afterwards. +unsafe fn free_response(response: *mut RawSignResponse) { + // SAFETY: the caller guarantees a valid, provider-allocated struct. + let raw = unsafe { &mut *response }; + for (pointer, len) in [(raw.sig_r, raw.sig_r_len), (raw.sig_s, raw.sig_s_len)] { + if !pointer.is_null() { + // SAFETY: the provider allocated `len` bytes at `pointer` with + // malloc; zeroing then freeing matches `freezero`. + unsafe { + if len > 0 && len <= MAX_COMPONENT_LEN { + ptr::write_bytes(pointer, 0, len); + } + libc::free(pointer.cast::()); + } + } + } + // SAFETY: the struct itself came from the provider's calloc. + unsafe { + ptr::write_bytes(response, 0, 1); + libc::free(response.cast::()); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn missing_or_unloadable_libraries_report_a_load_failure() { + let directory = tempfile::tempdir().unwrap(); + let missing = directory.path().join("no-such-provider.so"); + match ExternalProvider::load(&missing).unwrap_err() { + SecurityKeyError::LoadFailed { path, reason } => { + assert_eq!(path, missing); + assert!(!reason.is_empty()); + } + other => panic!("unexpected error: {other:?}"), + } + + let garbage = directory.path().join("garbage.so"); + std::fs::write(&garbage, b"this is not an ELF or Mach-O image").unwrap(); + assert!(matches!( + ExternalProvider::load(&garbage).unwrap_err(), + SecurityKeyError::LoadFailed { .. } + )); + } + + #[cfg(unix)] + #[test] + fn a_library_without_sk_api_version_is_not_a_provider() { + let library: Library = libloading::os::unix::Library::this().into(); + let path = Path::new("/self"); + match bind_symbols(path, &library).unwrap_err() { + SecurityKeyError::NotAProvider { + path: reported, + reason, + } => { + assert_eq!(reported, path); + assert!(reason.contains("sk_api_version"), "{reason}"); + } + other => panic!("unexpected error: {other:?}"), + } + // The handle must not be closed: it is the running program. + std::mem::forget(library); + } + + #[test] + fn provider_validation_enforces_version_and_operation_symbols() { + let path = Path::new("/p.so"); + let all = |_: &[u8]| Ok(()); + assert!(validate_provider(path, super::super::SK_API_VERSION_MAJOR, all).is_ok()); + + let error = validate_provider(path, 0x0009_0000, all).unwrap_err(); + assert!(matches!( + error, + SecurityKeyError::UnsupportedVersion { + version: 0x0009_0000, + .. + } + )); + + for missing in ["sk_enroll\0", "sk_load_resident_keys\0", "sk_sign\0"] { + let error = validate_provider(path, super::super::SK_API_VERSION_MAJOR, |name| { + if name == missing.as_bytes() { + Err(format!("undefined symbol {missing}")) + } else { + Ok(()) + } + }) + .unwrap_err(); + match error { + SecurityKeyError::NotAProvider { reason, .. } => { + assert!(reason.contains(missing.trim_end_matches('\0')), "{reason}"); + } + other => panic!("unexpected error: {other:?}"), + } + } + } + + #[test] + fn musl_hint_only_appears_on_static_builds() { + let error = libloading::Error::DlOpenUnknown; + let reason = describe_load_error(&error); + assert_eq!( + reason.contains("statically linked"), + cfg!(target_env = "musl") + ); + } + + #[test] + fn response_components_are_validated_before_being_read() { + let error = unsafe { copy_component(ptr::null(), 32, "sig_r invalid") }.unwrap_err(); + assert_eq!(error, SecurityKeyError::InvalidResponse("sig_r invalid")); + let bytes = [7u8; 4]; + let error = + unsafe { copy_component(bytes.as_ptr(), MAX_COMPONENT_LEN + 1, "sig_s invalid") } + .unwrap_err(); + assert_eq!(error, SecurityKeyError::InvalidResponse("sig_s invalid")); + let copied = + unsafe { copy_component(bytes.as_ptr(), bytes.len(), "sig_r invalid") }.unwrap(); + assert_eq!(&copied[..], &bytes); + let empty = unsafe { copy_component(ptr::null(), 0, "sig_r invalid") }.unwrap(); + assert!(empty.is_empty()); + } + + /// End-to-end sign through the OpenSSH regress `sk-dummy` middleware when + /// its build is present; this is the same library the regress suite uses. + #[test] + fn sk_dummy_provider_loads_and_reports_the_supported_version() { + let path = Path::new(env!("CARGO_MANIFEST_DIR")).join( + "target/openssh-regress/openssh-portable-V_10_3_P1/regress/misc/sk-dummy/sk-dummy.so", + ); + if !path.exists() { + eprintln!("sk-dummy.so not built; skipping"); + return; + } + let provider = ExternalProvider::load(&path).unwrap(); + assert_eq!(provider.describe(), path.display().to_string()); + } +} diff --git a/src/ssh/security_key/mod.rs b/src/ssh/security_key/mod.rs new file mode 100644 index 000000000..2c40d61ee --- /dev/null +++ b/src/ssh/security_key/mod.rs @@ -0,0 +1,376 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! FIDO security-key (`sk-*`) provider interface. +//! +//! OpenSSH signs with authenticator-hosted keys through a middleware library +//! that implements `sk-api.h` (`sk_api_version`, `sk_sign`, ...). bssh loads +//! the same libraries so an identity enrolled with `ssh-keygen -t ed25519-sk` +//! works unchanged. The narrow [`SecurityKeyProvider`] trait is the seam a +//! built-in USB HID provider plugs into later: `SecurityKeyProvider internal` +//! resolves to [`ProviderSource::Internal`] and [`open_provider`] is the only +//! place that needs to learn about it. +//! +//! The provider runs in-process (`dlopen`) rather than through an +//! `ssh-sk-helper`-style subprocess. OpenSSH's helper keeps the middleware +//! out of its `pledge`d client and shares one implementation between `ssh`, +//! `ssh-agent`, and `ssh-keygen`; bssh has no such sandbox, and a re-exec +//! helper would add a private wire protocol without changing what code runs +//! as the user. Provider calls block on touch and PIN entry, so callers run +//! them on a blocking thread (see `tokio_client::security_key_signer`). + +mod external; +mod signature; + +use std::fmt; +use std::path::{Path, PathBuf}; + +pub use signature::{SkSignResponse, encode_signature}; + +/// sk-api major version bssh speaks (`SSH_SK_VERSION_MAJOR`). +pub const SK_API_VERSION_MAJOR: u32 = 0x000a_0000; +/// Mask selecting the major half of an sk-api version. +pub const SK_API_VERSION_MAJOR_MASK: u32 = 0xffff_0000; + +/// `SSH_SK_USER_PRESENCE_REQD`: the authenticator wants a touch. +pub const SK_FLAG_USER_PRESENCE_REQUIRED: u8 = 0x01; +/// `SSH_SK_USER_VERIFICATION_REQD`: the authenticator wants a PIN or biometric. +pub const SK_FLAG_USER_VERIFICATION_REQUIRED: u8 = 0x04; + +/// The ssh_config default for `SecurityKeyProvider` when no built-in provider +/// exists (`readconf.c`): a `$`-prefixed environment variable name. +pub const DEFAULT_PROVIDER_SETTING: &str = "$SSH_SK_PROVIDER"; + +/// Key algorithms the sk-api distinguishes (`SSH_SK_ECDSA`, `SSH_SK_ED25519`). +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum SkAlgorithm { + EcdsaSha2NistP256, + Ed25519, +} + +impl SkAlgorithm { + /// Numeric `alg` argument of `sk_sign`. + #[must_use] + pub const fn api_code(self) -> u32 { + match self { + Self::EcdsaSha2NistP256 => 0, + Self::Ed25519 => 1, + } + } + + /// Plain SSH key type name, which also leads the signature blob. + #[must_use] + pub const fn ssh_name(self) -> &'static str { + match self { + Self::EcdsaSha2NistP256 => "sk-ecdsa-sha2-nistp256@openssh.com", + Self::Ed25519 => "sk-ssh-ed25519@openssh.com", + } + } +} + +/// One `sk_sign` call. `data` is the raw message; the provider hashes it. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkSignRequest<'a> { + pub algorithm: SkAlgorithm, + pub data: &'a [u8], + pub application: &'a str, + pub key_handle: &'a [u8], + pub flags: u8, +} + +/// Where `SecurityKeyProvider` points after `$VAR` expansion. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ProviderSource { + /// No provider: unset environment variable or `none`. + Disabled, + /// `internal`: the built-in provider, which this build does not include. + Internal, + /// A shared library implementing `sk-api.h`. + Library(PathBuf), +} + +/// Why a provider could not be used. +#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] +pub enum SecurityKeyError { + #[error("no SecurityKeyProvider has been specified")] + NoProvider, + #[error( + "built-in security key support is not available in this build; set SecurityKeyProvider to an sk-api library" + )] + InternalUnavailable, + #[error("Provider \"{path}\" dlopen failed: {reason}")] + LoadFailed { path: PathBuf, reason: String }, + #[error("provider {path} is not an OpenSSH FIDO library ({reason})")] + NotAProvider { path: PathBuf, reason: String }, + #[error( + "Provider \"{path}\" implements unsupported version {version:#010x} (supported: {supported:#010x})" + )] + UnsupportedVersion { + path: PathBuf, + version: u32, + supported: u32, + }, + #[error("sk_sign failed: PIN required")] + PinRequired, + #[error("sk_sign failed: device not found")] + DeviceNotFound, + #[error("sk_sign failed: operation unsupported by the provider")] + Unsupported, + #[error("sk_sign failed with code {0}")] + SignFailed(i32), + #[error("sk_sign response invalid: {0}")] + InvalidResponse(&'static str), + #[error("sk_sign request invalid: {0}")] + InvalidRequest(&'static str), +} + +impl SecurityKeyError { + /// Map an sk-api error code the way `ssh-sk.c`'s `skerr_to_ssherr` does. + #[must_use] + pub fn from_sk_code(code: i32) -> Self { + match code { + -2 => Self::Unsupported, + -3 => Self::PinRequired, + -4 => Self::DeviceNotFound, + other => Self::SignFailed(other), + } + } +} + +/// A loaded sk-api middleware, or a built-in equivalent. +pub trait SecurityKeyProvider: Send + Sync { + /// Sign `request.data`. `pin` is forwarded verbatim and never logged. + fn sign( + &self, + request: &SkSignRequest<'_>, + pin: Option<&str>, + ) -> Result; + + /// Human-readable origin for diagnostics. + fn describe(&self) -> String; +} + +impl fmt::Debug for dyn SecurityKeyProvider { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("SecurityKeyProvider") + .field("source", &self.describe()) + .finish() + } +} + +/// Resolve the configured `SecurityKeyProvider` value as `ssh.c` does: a +/// leading `$` names an environment variable; an unset variable disables +/// authenticator-hosted keys; `none` disables them; `internal` selects the +/// built-in provider. `None` means the option was not set and takes +/// [`DEFAULT_PROVIDER_SETTING`]. +pub fn resolve_provider(configured: Option<&str>, env: F) -> ProviderSource +where + F: Fn(&str) -> Option, +{ + let configured = configured.unwrap_or(DEFAULT_PROVIDER_SETTING); + let value = match configured.strip_prefix('$') { + Some(name) if !name.is_empty() => match env(name) { + Some(value) => { + tracing::debug!("resolved SecurityKeyProvider {configured} => {value}"); + value + } + None => { + tracing::debug!("Authenticator provider {configured} did not resolve; disabling"); + return ProviderSource::Disabled; + } + }, + _ => configured.to_string(), + }; + if value.is_empty() || value.eq_ignore_ascii_case("none") { + ProviderSource::Disabled + } else if value.eq_ignore_ascii_case("internal") { + ProviderSource::Internal + } else { + ProviderSource::Library(PathBuf::from(value)) + } +} + +/// Resolve against the process environment. +#[must_use] +pub fn resolve_provider_from_env(configured: Option<&str>) -> ProviderSource { + resolve_provider(configured, |name| std::env::var(name).ok()) +} + +/// Open the provider a [`ProviderSource`] names, mirroring `sshsk_open`. +pub fn open_provider( + source: &ProviderSource, +) -> Result, SecurityKeyError> { + match source { + ProviderSource::Disabled => Err(SecurityKeyError::NoProvider), + ProviderSource::Internal => Err(SecurityKeyError::InternalUnavailable), + ProviderSource::Library(path) => open_library(path), + } +} + +fn open_library(path: &Path) -> Result, SecurityKeyError> { + external::ExternalProvider::load(path) + .map(|provider| Box::new(provider) as Box) +} + +/// Check a provider's reported `sk_api_version` against the version bssh +/// implements; only the major half must match. +pub fn check_api_version(path: &Path, version: u32) -> Result<(), SecurityKeyError> { + if version & SK_API_VERSION_MAJOR_MASK == SK_API_VERSION_MAJOR { + Ok(()) + } else { + Err(SecurityKeyError::UnsupportedVersion { + path: path.to_path_buf(), + version, + supported: SK_API_VERSION_MAJOR, + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn env_with(name: &'static str, value: &'static str) -> impl Fn(&str) -> Option { + move |query| (query == name).then(|| value.to_string()) + } + + fn no_env(_: &str) -> Option { + None + } + + #[test] + fn default_setting_follows_the_ssh_sk_provider_variable() { + assert_eq!( + resolve_provider(None, env_with("SSH_SK_PROVIDER", "/opt/sk.so")), + ProviderSource::Library(PathBuf::from("/opt/sk.so")) + ); + assert_eq!(resolve_provider(None, no_env), ProviderSource::Disabled); + } + + #[test] + fn dollar_prefix_expands_any_variable_and_a_bare_dollar_is_literal() { + assert_eq!( + resolve_provider(Some("$MY_SK"), env_with("MY_SK", "/lib/sk-dummy.so")), + ProviderSource::Library(PathBuf::from("/lib/sk-dummy.so")) + ); + assert_eq!( + resolve_provider(Some("$MY_SK"), no_env), + ProviderSource::Disabled + ); + assert_eq!( + resolve_provider(Some("$"), env_with("", "ignored")), + ProviderSource::Library(PathBuf::from("$")) + ); + assert_eq!( + resolve_provider(Some("$MY_SK"), env_with("MY_SK", "internal")), + ProviderSource::Internal + ); + assert_eq!( + resolve_provider(Some("$MY_SK"), env_with("MY_SK", "none")), + ProviderSource::Disabled + ); + } + + #[test] + fn literal_values_select_internal_none_or_a_library() { + assert_eq!( + resolve_provider(Some("internal"), no_env), + ProviderSource::Internal + ); + assert_eq!( + resolve_provider(Some("Internal"), no_env), + ProviderSource::Internal + ); + assert_eq!( + resolve_provider(Some("none"), no_env), + ProviderSource::Disabled + ); + assert_eq!(resolve_provider(Some(""), no_env), ProviderSource::Disabled); + assert_eq!( + resolve_provider(Some("/usr/lib/libsk-libfido2.so"), no_env), + ProviderSource::Library(PathBuf::from("/usr/lib/libsk-libfido2.so")) + ); + } + + #[test] + fn internal_provider_reports_an_explicit_unavailable_diagnostic() { + let error = open_provider(&ProviderSource::Internal).unwrap_err(); + assert_eq!(error, SecurityKeyError::InternalUnavailable); + assert!( + error + .to_string() + .contains("built-in security key support is not available") + ); + assert_eq!( + open_provider(&ProviderSource::Disabled).unwrap_err(), + SecurityKeyError::NoProvider + ); + } + + #[test] + fn api_version_check_accepts_any_minor_of_the_supported_major() { + let path = Path::new("/p.so"); + assert!(check_api_version(path, SK_API_VERSION_MAJOR).is_ok()); + assert!(check_api_version(path, SK_API_VERSION_MAJOR | 0x0001).is_ok()); + let error = check_api_version(path, 0x0009_0000).unwrap_err(); + assert_eq!( + error, + SecurityKeyError::UnsupportedVersion { + path: path.to_path_buf(), + version: 0x0009_0000, + supported: SK_API_VERSION_MAJOR, + } + ); + assert!(error.to_string().contains("0x00090000")); + assert!(error.to_string().contains("0x000a0000")); + } + + #[test] + fn sk_error_codes_map_like_skerr_to_ssherr() { + assert_eq!( + SecurityKeyError::from_sk_code(-2), + SecurityKeyError::Unsupported + ); + assert_eq!( + SecurityKeyError::from_sk_code(-3), + SecurityKeyError::PinRequired + ); + assert_eq!( + SecurityKeyError::from_sk_code(-4), + SecurityKeyError::DeviceNotFound + ); + assert_eq!( + SecurityKeyError::from_sk_code(-1), + SecurityKeyError::SignFailed(-1) + ); + assert_eq!( + SecurityKeyError::from_sk_code(-5), + SecurityKeyError::SignFailed(-5) + ); + } + + #[test] + fn algorithm_codes_and_names_follow_sk_api() { + assert_eq!(SkAlgorithm::EcdsaSha2NistP256.api_code(), 0); + assert_eq!(SkAlgorithm::Ed25519.api_code(), 1); + assert_eq!( + SkAlgorithm::Ed25519.ssh_name(), + "sk-ssh-ed25519@openssh.com" + ); + assert_eq!( + SkAlgorithm::EcdsaSha2NistP256.ssh_name(), + "sk-ecdsa-sha2-nistp256@openssh.com" + ); + } +} diff --git a/src/ssh/security_key/signature.rs b/src/ssh/security_key/signature.rs new file mode 100644 index 000000000..a96fe8796 --- /dev/null +++ b/src/ssh/security_key/signature.rs @@ -0,0 +1,147 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! SSH signature blob assembly for `sk-*` keys (`sshsk_sign` in `ssh-sk.c`). + +use russh::keys::ssh_encoding::Encode; +use russh::keys::ssh_key::Mpint; +use zeroize::Zeroizing; + +use super::{SecurityKeyError, SkAlgorithm}; + +/// The provider's `struct sk_sign_response`, copied out of C memory. +/// +/// `sig_s` is empty for Ed25519, whose whole signature travels in `sig_r`. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkSignResponse { + pub flags: u8, + pub counter: u32, + pub sig_r: Zeroizing>, + pub sig_s: Zeroizing>, +} + +/// Build the wire signature `string name || string sig || u8 flags || u32 counter`. +/// +/// For ECDSA the inner `sig` is `mpint r || mpint s`; for Ed25519 it is the +/// raw 64-byte signature. This is the blob an agent returns for the same key, +/// so russh and OpenSSH both decode it unchanged. +pub fn encode_signature( + algorithm: SkAlgorithm, + response: &SkSignResponse, +) -> Result, SecurityKeyError> { + let inner = match algorithm { + SkAlgorithm::Ed25519 => { + if response.sig_r.is_empty() { + return Err(SecurityKeyError::InvalidResponse("missing sig_r")); + } + response.sig_r.to_vec() + } + SkAlgorithm::EcdsaSha2NistP256 => { + if response.sig_r.is_empty() || response.sig_s.is_empty() { + return Err(SecurityKeyError::InvalidResponse("missing sig_r or sig_s")); + } + let mut inner = Vec::new(); + for component in [&response.sig_r, &response.sig_s] { + Mpint::from_positive_bytes(component) + .encode(&mut inner) + .map_err(|_| SecurityKeyError::InvalidResponse("malformed ECDSA component"))?; + } + inner + } + }; + let mut blob = Vec::new(); + algorithm + .ssh_name() + .encode(&mut blob) + .and_then(|()| inner.as_slice().encode(&mut blob)) + .and_then(|()| response.flags.encode(&mut blob)) + .and_then(|()| response.counter.encode(&mut blob)) + .map_err(|_| SecurityKeyError::InvalidResponse("signature encoding failed"))?; + Ok(blob) +} + +#[cfg(test)] +mod tests { + use super::*; + use russh::keys::ssh_encoding::Decode; + use russh::keys::ssh_key::{Algorithm, Signature}; + + fn response(sig_r: &[u8], sig_s: &[u8]) -> SkSignResponse { + SkSignResponse { + flags: 0x01, + counter: 0x1234_5678, + sig_r: Zeroizing::new(sig_r.to_vec()), + sig_s: Zeroizing::new(sig_s.to_vec()), + } + } + + #[test] + fn ed25519_blob_is_name_signature_flags_counter() { + let sig = [0xabu8; 64]; + let blob = encode_signature(SkAlgorithm::Ed25519, &response(&sig, &[])).unwrap(); + + let mut expected = Vec::new(); + expected.extend_from_slice(&26u32.to_be_bytes()); + expected.extend_from_slice(b"sk-ssh-ed25519@openssh.com"); + expected.extend_from_slice(&64u32.to_be_bytes()); + expected.extend_from_slice(&sig); + expected.push(0x01); + expected.extend_from_slice(&0x1234_5678u32.to_be_bytes()); + assert_eq!(blob, expected); + + let decoded = Signature::decode(&mut blob.as_slice()).unwrap(); + assert_eq!(decoded.algorithm(), Algorithm::SkEd25519); + assert_eq!(&decoded.as_bytes()[..64], &sig); + assert_eq!(&decoded.as_bytes()[64..], &[0x01, 0x12, 0x34, 0x56, 0x78]); + } + + #[test] + fn ecdsa_blob_wraps_r_and_s_as_mpints_inside_one_string() { + let mut r = [0x11u8; 32]; + r[0] = 0x80; + let s = [0x22u8; 32]; + let blob = encode_signature(SkAlgorithm::EcdsaSha2NistP256, &response(&r, &s)).unwrap(); + + let mut inner = Vec::new(); + inner.extend_from_slice(&33u32.to_be_bytes()); + inner.push(0); + inner.extend_from_slice(&r); + inner.extend_from_slice(&32u32.to_be_bytes()); + inner.extend_from_slice(&s); + let mut expected = Vec::new(); + expected.extend_from_slice(&34u32.to_be_bytes()); + expected.extend_from_slice(b"sk-ecdsa-sha2-nistp256@openssh.com"); + expected.extend_from_slice(&(inner.len() as u32).to_be_bytes()); + expected.extend_from_slice(&inner); + expected.push(0x01); + expected.extend_from_slice(&0x1234_5678u32.to_be_bytes()); + assert_eq!(blob, expected); + + let decoded = Signature::decode(&mut blob.as_slice()).unwrap(); + assert_eq!(decoded.algorithm(), Algorithm::SkEcdsaSha2NistP256); + assert_eq!(decoded.as_bytes().len(), inner.len() + 5); + } + + #[test] + fn incomplete_responses_are_rejected() { + assert_eq!( + encode_signature(SkAlgorithm::Ed25519, &response(&[], &[])).unwrap_err(), + SecurityKeyError::InvalidResponse("missing sig_r") + ); + assert_eq!( + encode_signature(SkAlgorithm::EcdsaSha2NistP256, &response(&[1; 32], &[])).unwrap_err(), + SecurityKeyError::InvalidResponse("missing sig_r or sig_s") + ); + } +} diff --git a/src/ssh/ssh_config/dump.rs b/src/ssh/ssh_config/dump.rs index 74eb6e94a..19c550616 100644 --- a/src/ssh/ssh_config/dump.rs +++ b/src/ssh/ssh_config/dump.rs @@ -213,9 +213,10 @@ pub fn render_resolved_config(original_host: &str, config: &SshHostConfig) -> Re } output.line( "securitykeyprovider", - raw_option(config, "securitykeyprovider") + config + .security_key_provider .as_deref() - .unwrap_or("$SSH_SK_PROVIDER"), + .unwrap_or(crate::ssh::security_key::DEFAULT_PROVIDER_SETTING), )?; output.line( "xauthlocation", @@ -441,7 +442,6 @@ const EMITTED_UNIMPLEMENTED: &[&str] = &[ "logverbose", "obscurekeystroketiming", "revokedhostkeys", - "securitykeyprovider", "streamlocalbindunlink", "streamlocalbindmask", "tunnel", diff --git a/src/ssh/ssh_config/dump_tests.rs b/src/ssh/ssh_config/dump_tests.rs index 3e2e61050..29ff37544 100644 --- a/src/ssh/ssh_config/dump_tests.rs +++ b/src/ssh/ssh_config/dump_tests.rs @@ -361,3 +361,41 @@ fn revoked_host_keys_list_is_dumped_on_one_line() { assert!(output.lines().any(|line| line == "revokedhostkeys none")); assert!(SshConfig::parse("Host host\n RevokedHostKeys none /x\n").is_err()); } + +#[test] +fn security_key_provider_renders_its_raw_value_and_first_match_wins() { + let config = SshConfig::parse( + "Host target\n SecurityKeyProvider $MY_SK_PROVIDER\nHost *\n SecurityKeyProvider /usr/lib/libsk-libfido2.so\n", + ) + .unwrap(); + let host = config.find_host_config("target"); + assert_eq!( + host.security_key_provider.as_deref(), + Some("$MY_SK_PROVIDER"), + "the first matching block wins and $VAR stays unexpanded for -G" + ); + assert!( + !host + .unimplemented_options + .contains_key("securitykeyprovider") + ); + let output = render_resolved_config("target", &host).unwrap(); + assert!(output.contains("securitykeyprovider $MY_SK_PROVIDER\n")); + assert_eq!( + output + .lines() + .filter(|line| line.starts_with("securitykeyprovider ")) + .count(), + 1 + ); + + let other = render_resolved_config("other", &config.find_host_config("other")).unwrap(); + assert!(other.contains("securitykeyprovider /usr/lib/libsk-libfido2.so\n")); + for value in ["internal", "none"] { + let config = + SshConfig::parse(&format!("Host *\n SecurityKeyProvider {value}\n")).unwrap(); + let output = render_resolved_config("x", &config.find_host_config("x")).unwrap(); + assert!(output.contains(&format!("securitykeyprovider {value}\n"))); + } + assert!(SshConfig::parse("Host *\n SecurityKeyProvider a b\n").is_err()); +} diff --git a/src/ssh/ssh_config/parser/options/authentication.rs b/src/ssh/ssh_config/parser/options/authentication.rs index 956be6ef9..a4d20b91d 100644 --- a/src/ssh/ssh_config/parser/options/authentication.rs +++ b/src/ssh/ssh_config/parser/options/authentication.rs @@ -59,6 +59,17 @@ pub(super) fn parse_authentication_option( } host.add_keys_to_agent = Some(value); } + "securitykeyprovider" => { + // OpenSSH stores the value verbatim: a library path, `internal`, + // `none`, or `$VAR`, which `ssh.c` expands at startup. Expansion + // happens when an sk-* identity is used so `-G` shows the raw value. + if args.len() != 1 || args[0].is_empty() { + anyhow::bail!( + "SecurityKeyProvider requires exactly one value at line {line_number}" + ); + } + host.security_key_provider = Some(args[0].clone()); + } "identityagent" => { if args.is_empty() { anyhow::bail!("IdentityAgent requires a value at line {line_number}"); diff --git a/src/ssh/ssh_config/parser/options/mod.rs b/src/ssh/ssh_config/parser/options/mod.rs index 1f0ed68c3..d5e38a759 100644 --- a/src/ssh/ssh_config/parser/options/mod.rs +++ b/src/ssh/ssh_config/parser/options/mod.rs @@ -104,6 +104,7 @@ pub fn parse_option( | "hostbasedkeytypes" | "numberofpasswordprompts" | "enablesshkeysign" + | "securitykeyprovider" | "usekeychain" => { authentication::parse_authentication_option(host, keyword, args, line_number) } @@ -190,7 +191,6 @@ pub fn parse_option( | "fallbacktorsh" | "globalknownhostsfile2" | "rhostsauthentication" - | "securitykeyprovider" | "userknownhostsfile2" | "useroaming" | "usersh" @@ -297,7 +297,7 @@ fn validate_retained_option(keyword: &str, args: &[String], line_number: usize) anyhow::bail!("Invalid obscurekeystroketiming at line {line_number}"); } } - "securitykeyprovider" | "xauthlocation" => one()?, + "xauthlocation" => one()?, "channeltimeout" | "logverbose" if args.is_empty() => { anyhow::bail!("{keyword} expects at least one value at line {line_number}"); } diff --git a/src/ssh/ssh_config/parser/options/support.rs b/src/ssh/ssh_config/parser/options/support.rs index 8e98bac5b..db8b25d04 100644 --- a/src/ssh/ssh_config/parser/options/support.rs +++ b/src/ssh/ssh_config/parser/options/support.rs @@ -242,7 +242,11 @@ pub(super) const ACCEPTED_KEYWORDS: &[(&str, &str, KeywordSupport)] = &[ "rhostsauthentication", Unimplemented, ), - ("securitykeyprovider", "securitykeyprovider", Unimplemented), + ( + "securitykeyprovider", + "securitykeyprovider", + Runtime(Authentication), + ), ("userknownhostsfile2", "userknownhostsfile2", Unimplemented), ("useroaming", "useroaming", Unimplemented), ("usersh", "usersh", Unimplemented), @@ -310,8 +314,8 @@ mod tests { use std::collections::HashSet; const ACCEPTED_SPELLING_COUNT: usize = 108; - const RUNTIME_SPELLING_COUNT: usize = 62; - const UNIMPLEMENTED_SPELLING_COUNT: usize = 46; + const RUNTIME_SPELLING_COUNT: usize = 63; + const UNIMPLEMENTED_SPELLING_COUNT: usize = 45; #[test] fn accepted_keywords_and_aliases_have_one_consistent_classification() { @@ -413,6 +417,7 @@ mod tests { ("forkafterauthentication", Session), ("sessiontype", Session), ("stdinnull", Session), + ("securitykeyprovider", Authentication), ("revokedhostkeys", HostVerification), ]; let runtime = ACCEPTED_KEYWORDS @@ -458,7 +463,6 @@ mod tests { "fallbacktorsh", "globalknownhostsfile2", "rhostsauthentication", - "securitykeyprovider", "userknownhostsfile2", "useroaming", "usersh", diff --git a/src/ssh/ssh_config/resolver.rs b/src/ssh/ssh_config/resolver.rs index 70bb2de8f..dc0ef1bbe 100644 --- a/src/ssh/ssh_config/resolver.rs +++ b/src/ssh/ssh_config/resolver.rs @@ -563,6 +563,9 @@ pub(super) fn merge_host_config(base: &mut SshHostConfig, overlay: &SshHostConfi if base.identity_agent.is_none() && overlay.identity_agent.is_some() { base.identity_agent = overlay.identity_agent.clone(); } + if base.security_key_provider.is_none() && overlay.security_key_provider.is_some() { + base.security_key_provider = overlay.security_key_provider.clone(); + } #[cfg(target_os = "macos")] if base.use_keychain.is_none() && overlay.use_keychain.is_some() { base.use_keychain = overlay.use_keychain; diff --git a/src/ssh/ssh_config/types.rs b/src/ssh/ssh_config/types.rs index 7317979d3..c6b9bd15c 100644 --- a/src/ssh/ssh_config/types.rs +++ b/src/ssh/ssh_config/types.rs @@ -186,6 +186,9 @@ pub struct SshHostConfig { pub identities_only: Option, pub add_keys_to_agent: Option, // yes/no/ask/confirm pub identity_agent: Option, // socket path or "none" + /// `SecurityKeyProvider`: sk-api library path, `internal`, `none`, or a + /// `$VAR` reference expanded at authentication time. + pub security_key_provider: Option, /// UseKeychain option (macOS only) - integrates private-key passphrases with macOS Keychain. /// This is an Apple-specific OpenSSH extension. #[cfg(target_os = "macos")] diff --git a/src/ssh/tokio_client/algorithms.rs b/src/ssh/tokio_client/algorithms.rs index 83617c008..c71586671 100644 --- a/src/ssh/tokio_client/algorithms.rs +++ b/src/ssh/tokio_client/algorithms.rs @@ -211,7 +211,7 @@ pub(crate) fn resolve_kex(values: &[String]) -> Result, String> { } fn supported_key_algorithms() -> Vec { - let mut supported = russh::Preferred::DEFAULT.key.to_vec(); + let mut supported = default_host_key_algorithms().to_vec(); for algorithm in russh::keys::key::ALL_KEY_TYPES { if !supported.contains(algorithm) { supported.push(algorithm.clone()); @@ -220,18 +220,33 @@ fn supported_key_algorithms() -> Vec { supported } -/// russh's default host-key order without the SHA-1 `ssh-rsa` signature. -/// -/// OpenSSH no longer offers `ssh-rsa` by default; bssh matches that for both -/// its client and server. `HostKeyAlgorithms +ssh-rsa` still opts back in. +/// OpenSSH's default plain host-key order (`KEX_DEFAULT_PK_ALG` in +/// `myproposal.h`): Ed25519, the ECDSA curves, the FIDO `sk-*` types, then +/// the SHA-2 RSA signatures. The SHA-1 `ssh-rsa` signature is not offered; +/// `HostKeyAlgorithms +ssh-rsa` opts back in. bssh does not verify +/// `webauthn-sk-ecdsa-sha2-nistp256@openssh.com`, so that entry is absent. pub(crate) fn default_host_key_algorithms() -> &'static [Algorithm] { static DEFAULTS: LazyLock> = LazyLock::new(|| { - russh::Preferred::DEFAULT - .key - .iter() - .filter(|algorithm| **algorithm != Algorithm::Rsa { hash: None }) - .cloned() - .collect() + vec![ + Algorithm::Ed25519, + Algorithm::Ecdsa { + curve: russh::keys::ssh_key::EcdsaCurve::NistP256, + }, + Algorithm::Ecdsa { + curve: russh::keys::ssh_key::EcdsaCurve::NistP384, + }, + Algorithm::Ecdsa { + curve: russh::keys::ssh_key::EcdsaCurve::NistP521, + }, + Algorithm::SkEd25519, + Algorithm::SkEcdsaSha2NistP256, + Algorithm::Rsa { + hash: Some(russh::keys::HashAlg::Sha512), + }, + Algorithm::Rsa { + hash: Some(russh::keys::HashAlg::Sha256), + }, + ] }); &DEFAULTS } @@ -351,8 +366,10 @@ fn public_key_names(algorithms: &[Algorithm]) -> Vec { .collect() } +/// OpenSSH's default `PubkeyAcceptedAlgorithms`: every certificate name +/// first, then every plain name, in `KEX_DEFAULT_PK_ALG` order. pub(crate) fn default_pubkey_algorithms() -> Vec { - public_key_names(default_host_key_algorithms()) + host_key_names(default_host_key_algorithms()) } pub(crate) fn resolve_pubkey_algorithms(values: &[String]) -> Result, String> { @@ -491,7 +508,7 @@ mod tests { .collect::>() ); - let nondefault = Algorithm::SkEd25519; + let nondefault = Algorithm::Dsa; assert!(!defaults.contains(&nondefault)); let prepended = resolve_host_keys(&[format!("^{}", nondefault.as_ref())]) .unwrap() @@ -587,7 +604,7 @@ mod tests { .collect::>() ); - let nondefault = Algorithm::SkEd25519.to_string(); + let nondefault = Algorithm::Dsa.to_string(); assert!(!defaults.contains(&nondefault)); let prepended = resolve_pubkey_algorithms(&[format!("^{nondefault}")]).unwrap(); assert_eq!(prepended.first(), Some(&nondefault)); @@ -601,6 +618,62 @@ mod tests { assert_eq!(&prepended[1..], defaults); } + /// `ssh -G` from the pinned OpenSSH 10.3p1 prints these lists minus the + /// `webauthn-sk-ecdsa-sha2-nistp256@openssh.com` entries bssh cannot verify. + #[test] + fn default_host_key_and_pubkey_lists_follow_openssh_order_with_sk_types() { + let expected_plain = [ + "ssh-ed25519", + "ecdsa-sha2-nistp256", + "ecdsa-sha2-nistp384", + "ecdsa-sha2-nistp521", + "sk-ssh-ed25519@openssh.com", + "sk-ecdsa-sha2-nistp256@openssh.com", + "rsa-sha2-512", + "rsa-sha2-256", + ]; + let plain = default_host_key_algorithms() + .iter() + .map(ToString::to_string) + .collect::>(); + assert_eq!(plain, expected_plain); + + let mut expected_pubkey = expected_plain + .iter() + .map(|name| match *name { + "sk-ssh-ed25519@openssh.com" => "sk-ssh-ed25519-cert-v01@openssh.com".to_string(), + "sk-ecdsa-sha2-nistp256@openssh.com" => { + "sk-ecdsa-sha2-nistp256-cert-v01@openssh.com".to_string() + } + other => format!("{other}-cert-v01@openssh.com"), + }) + .collect::>(); + expected_pubkey.extend(expected_plain.iter().map(ToString::to_string)); + assert_eq!(default_pubkey_algorithms(), expected_pubkey); + assert_eq!( + host_key_names(default_host_key_algorithms()), + expected_pubkey + ); + } + + #[test] + fn sk_wildcards_expand_to_the_supported_fido_types() { + let hosts = resolve_host_keys(&[String::from("sk-*")]).unwrap(); + assert_eq!( + hosts.keys, + vec![Algorithm::SkEd25519, Algorithm::SkEcdsaSha2NistP256] + ); + assert_eq!(hosts.certificates, hosts.keys); + let pubkey = resolve_pubkey_algorithms(&[String::from("sk-ssh-ed25519*")]).unwrap(); + assert_eq!( + pubkey, + [ + "sk-ssh-ed25519-cert-v01@openssh.com", + "sk-ssh-ed25519@openssh.com" + ] + ); + } + #[test] fn unknown_algorithm_reports_supported_values() { let error = resolve_ciphers(&[String::from("unknown-cipher")]).unwrap_err(); diff --git a/src/ssh/tokio_client/auth_policy.rs b/src/ssh/tokio_client/auth_policy.rs index 500c5ea15..e89d502ce 100644 --- a/src/ssh/tokio_client/auth_policy.rs +++ b/src/ssh/tokio_client/auth_policy.rs @@ -34,6 +34,9 @@ pub struct SshAuthenticationPolicy { pub number_of_password_prompts: u32, pub batch_mode: bool, pub pubkey_accepted_algorithms: Option>, + /// Raw `SecurityKeyProvider` value (`$VAR` unexpanded); `None` is the + /// OpenSSH default `$SSH_SK_PROVIDER`. + pub security_key_provider: Option, } impl Default for SshAuthenticationPolicy { @@ -54,6 +57,7 @@ impl Default for SshAuthenticationPolicy { number_of_password_prompts: DEFAULT_PASSWORD_PROMPTS, batch_mode: false, pubkey_accepted_algorithms: Some(super::algorithms::default_pubkey_algorithms()), + security_key_provider: None, } } } diff --git a/src/ssh/tokio_client/authentication.rs b/src/ssh/tokio_client/authentication.rs index f80a41b12..7bbccd978 100644 --- a/src/ssh/tokio_client/authentication.rs +++ b/src/ssh/tokio_client/authentication.rs @@ -51,6 +51,9 @@ pub enum AuthMethod { accepted_algorithms: Vec, allow_passphrase_prompt: bool, use_keychain: bool, + /// `SecurityKeyProvider` for `sk-*` identities; `None` is the + /// OpenSSH default (`$SSH_SK_PROVIDER`). + security_key_provider: Option, }, OpenSshCertificateFile { key_file_path: PathBuf, @@ -59,6 +62,9 @@ pub enum AuthMethod { use_keychain: bool, key_pass: Option>, accepted_algorithms: Vec, + /// `SecurityKeyProvider` for `sk-*` identities; `None` is the + /// OpenSSH default (`$SSH_SK_PROVIDER`). + security_key_provider: Option, }, PasswordPrompt { attempts: u32, @@ -136,6 +142,7 @@ impl AuthMethod { accepted_algorithms, allow_passphrase_prompt: false, use_keychain: false, + security_key_provider: None, } } @@ -152,6 +159,7 @@ impl AuthMethod { accepted_algorithms, allow_passphrase_prompt: false, use_keychain: false, + security_key_provider: None, } } @@ -160,6 +168,7 @@ impl AuthMethod { accepted_algorithms: Vec, allow_passphrase_prompt: bool, use_keychain: bool, + security_key_provider: Option, ) -> Self { Self::PrivateKeyFileWithPolicy { key_file_path: key_file_path.as_ref().to_path_buf(), @@ -167,6 +176,7 @@ impl AuthMethod { accepted_algorithms, allow_passphrase_prompt, use_keychain, + security_key_provider, } } @@ -179,6 +189,7 @@ impl AuthMethod { accepted_algorithms: Vec, allow_passphrase_prompt: bool, use_keychain: bool, + security_key_provider: Option, ) -> Self { Self::OpenSshCertificateFile { key_file_path: key_file_path.as_ref().to_path_buf(), @@ -187,6 +198,7 @@ impl AuthMethod { accepted_algorithms, allow_passphrase_prompt, use_keychain, + security_key_provider, } } @@ -615,7 +627,10 @@ fn record_key_user_interaction() { KEY_USER_INTERACTION_COUNT.fetch_add(1, std::sync::atomic::Ordering::SeqCst); } -async fn bounded_auth_prompt(prompt: &'static str, operation: F) -> Result +pub(super) async fn bounded_auth_prompt( + prompt: &'static str, + operation: F, +) -> Result where F: std::future::Future>, { @@ -627,7 +642,7 @@ where })? } -async fn serialized_bounded_auth_prompt( +pub(super) async fn serialized_bounded_auth_prompt( prompt: &'static str, operation: F, ) -> Result @@ -787,8 +802,11 @@ async fn default_hashes( async fn authenticate_private_key_with_policy( handle: &mut Handle, username: &String, - key: Arc, + key: russh::keys::PrivateKey, + key_file_path: &std::path::Path, accepted: &[String], + security_key_provider: Option<&str>, + interactive: bool, ) -> Result<(), super::Error> { let hashes = if accepted.is_empty() { default_hashes(handle, &key.algorithm()).await? @@ -802,38 +820,88 @@ async fn authenticate_private_key_with_policy( ))); } - for hash in hashes { - let result = handle - .authenticate_publickey( - username, - russh::keys::PrivateKeyWithHashAlg::new(Arc::clone(&key), hash), - ) + let public_key = key.public_key().clone(); + let signer = + LocalSigner::for_private_key(key, key_file_path, security_key_provider, interactive) .await?; - if result.success() { - return Ok(()); + match signer { + LocalSigner::Software(key) => { + for hash in hashes { + let result = handle + .authenticate_publickey( + username, + russh::keys::PrivateKeyWithHashAlg::new(Arc::clone(&key), hash), + ) + .await?; + if result.success() { + return Ok(()); + } + } + } + LocalSigner::SecurityKey(mut signer) => { + let result = handle + .authenticate_publickey_with(username, public_key, None, &mut signer) + .await?; + if result.success() { + return Ok(()); + } } } Err(super::Error::KeyAuthFailed) } +/// Signs locally held identities: software keys in process, `sk-*` keys +/// through the configured security-key provider. #[derive(Debug)] -struct LocalPrivateKeySigner { - key: Arc, +enum LocalSigner { + Software(Arc), + SecurityKey(super::security_key_signer::SecurityKeySigner), } -impl russh::Signer for LocalPrivateKeySigner { +impl LocalSigner { + /// Pick the signer for `key`; `sk-*` keys need a usable provider before + /// the identity is offered, so provider failures surface here. + async fn for_private_key( + key: russh::keys::PrivateKey, + path: &std::path::Path, + security_key_provider: Option<&str>, + interactive: bool, + ) -> Result { + match super::security_key_signer::SecurityKeyIdentity::from_private_key(&key, path) { + Some(identity) => { + let provider = super::security_key_signer::open_security_key_provider( + security_key_provider, + path, + ) + .await?; + Ok(Self::SecurityKey( + super::security_key_signer::SecurityKeySigner::new( + provider, + identity, + interactive, + ), + )) + } + None => Ok(Self::Software(Arc::new(key))), + } + } +} + +impl russh::Signer for LocalSigner { type Error = super::Error; - fn auth_sign( + async fn auth_sign( &mut self, - _key: &russh::keys::agent::AgentIdentity, + key: &russh::keys::agent::AgentIdentity, hash_alg: Option, to_sign: Vec, - ) -> impl std::future::Future, Self::Error>> + Send { - let key = Arc::clone(&self.key); - async move { - let signature = sign_local_private_key(key, hash_alg, &to_sign); - complete_local_signature(to_sign, signature) + ) -> Result, Self::Error> { + match self { + Self::Software(private_key) => { + let signature = sign_local_private_key(Arc::clone(private_key), hash_alg, &to_sign); + complete_local_signature(to_sign, signature) + } + Self::SecurityKey(signer) => signer.auth_sign(key, hash_alg, to_sign).await, } } } @@ -886,6 +954,7 @@ async fn authenticate_certificate_file( accepted: Vec, allow_passphrase_prompt: bool, use_keychain: bool, + security_key_provider: Option, ) -> Result<(), super::Error> { let key = load_policy_private_key( &key_file_path, @@ -926,7 +995,13 @@ async fn authenticate_certificate_file( ))); } - let mut signer = LocalPrivateKeySigner { key: Arc::new(key) }; + let mut signer = LocalSigner::for_private_key( + key, + &key_file_path, + security_key_provider.as_deref(), + allow_passphrase_prompt, + ) + .await?; for hash in hashes { let result = handle .authenticate_certificate_with(username, certificate.clone(), hash, &mut signer) @@ -1251,10 +1326,12 @@ pub(crate) async fn authenticate( }) } -fn is_retryable_rejection(error: &super::Error) -> bool { +pub(super) fn is_retryable_rejection(error: &super::Error) -> bool { // Agent setup fails before russh requests a signature, so the plan may // safely continue. Signer failures are intentionally absent: russh may be // waiting for `Msg::Signed`, and reusing that transport could then hang. + // `SecurityKeyUnavailable` is raised before the identity is offered; + // `SecurityKeySignFailed` comes from inside the signer and is not here. matches!( error, super::Error::KeyboardInteractiveAuthFailed @@ -1263,6 +1340,7 @@ fn is_retryable_rejection(error: &super::Error) -> bool { | super::Error::KeyInvalid(_) | super::Error::KeyPassphrasePromptDisabled { .. } | super::Error::IdentityAlgorithmExcluded(_) + | super::Error::SecurityKeyUnavailable { .. } | super::Error::PasswordWrong | super::Error::AgentConnectionFailed | super::Error::AgentRequestIdentitiesFailed @@ -1330,6 +1408,7 @@ async fn authenticate_single( accepted_algorithms, allow_passphrase_prompt, use_keychain, + security_key_provider, } => { let key = load_policy_private_key( &key_file_path, @@ -1341,8 +1420,11 @@ async fn authenticate_single( authenticate_private_key_with_policy( handle, username, - Arc::new(key), + key, + &key_file_path, &accepted_algorithms, + security_key_provider.as_deref(), + allow_passphrase_prompt, ) .await?; } @@ -1353,6 +1435,7 @@ async fn authenticate_single( accepted_algorithms, allow_passphrase_prompt, use_keychain, + security_key_provider, } => { authenticate_certificate_file( handle, @@ -1363,6 +1446,7 @@ async fn authenticate_single( accepted_algorithms, allow_passphrase_prompt, use_keychain, + security_key_provider, ) .await?; } @@ -1753,18 +1837,21 @@ mod policy_execution_tests { vec!["ssh-ed25519".into()], false, false, + None, ), AuthMethod::with_configured_key_file( malformed_path, vec!["ssh-ed25519".into()], false, false, + None, ), AuthMethod::with_configured_key_file( valid_path, vec!["ssh-ed25519".into()], false, false, + None, ), ]); @@ -1814,18 +1901,21 @@ mod policy_execution_tests { vec!["ssh-ed25519".into()], true, false, + None, ), AuthMethod::with_configured_key_file( &encrypted_path, vec!["ssh-ed25519".into()], true, false, + None, ), AuthMethod::with_configured_key_file( &malformed_path, vec!["ssh-ed25519".into()], true, false, + None, ), ]); authenticate(&mut handle, &"user".to_string(), methods) @@ -2207,12 +2297,14 @@ mod policy_execution_tests { vec!["ssh-ed25519".into()], false, false, + None, ), AuthMethod::with_configured_key_file( &rejected_path, vec!["ssh-ed25519".into()], false, false, + None, ), ]); diff --git a/src/ssh/tokio_client/connection.rs b/src/ssh/tokio_client/connection.rs index c6f1c68a8..9482d7e0f 100644 --- a/src/ssh/tokio_client/connection.rs +++ b/src/ssh/tokio_client/connection.rs @@ -572,6 +572,7 @@ impl SshConnectionConfigResolver { .resolved_pubkey_accepted_algorithms .clone() .or_else(|| SshAuthenticationPolicy::default().pubkey_accepted_algorithms), + security_key_provider: config.security_key_provider.clone(), } }) .unwrap_or_default(); diff --git a/src/ssh/tokio_client/connection_tests.rs b/src/ssh/tokio_client/connection_tests.rs index e6bb1a7d9..a293a65e4 100644 --- a/src/ssh/tokio_client/connection_tests.rs +++ b/src/ssh/tokio_client/connection_tests.rs @@ -739,6 +739,54 @@ fn configured_algorithms_reach_the_russh_transport_preferences() { ); } +/// `hostkey-agent` restricts the client to one `*-cert-v01@openssh.com` +/// name. The KEXINIT host-key list russh writes is `host_key_certificates` +/// followed by `key`, so an explicit certificate-only policy must leave the +/// plain list empty instead of falling back to the defaults. +#[test] +fn certificate_only_host_key_algorithms_are_never_widened() { + for name in [ + "sk-ssh-ed25519-cert-v01@openssh.com", + "sk-ecdsa-sha2-nistp256-cert-v01@openssh.com", + "ssh-ed25519-cert-v01@openssh.com", + ] { + let ssh_config = + SshConfig::parse(&format!("Host target\n HostKeyAlgorithms {name}\n")).unwrap(); + let config = SshConnectionConfigResolver::new() + .with_ssh_config(Some(ssh_config)) + .resolve_for_host("target") + .to_russh_config(); + assert!( + config.preferred.key.is_empty(), + "{name}: plain host keys advertised: {:?}", + config.preferred.key + ); + let certificates = config + .preferred + .host_key_certificates + .iter() + .map(|algorithm| algorithm.to_certificate_type().to_string()) + .collect::>(); + assert_eq!(certificates, [name]); + } + + let defaults = SshConnectionConfigResolver::new() + .resolve_for_host("target") + .to_russh_config(); + assert!( + defaults + .preferred + .key + .contains(&russh::keys::ssh_key::Algorithm::SkEd25519) + ); + assert!( + defaults + .preferred + .host_key_certificates + .contains(&russh::keys::ssh_key::Algorithm::SkEcdsaSha2NistP256) + ); +} + #[test] fn connection_attempts_rejects_zero() { let error = SshConfig::parse("Host target\n ConnectionAttempts 0\n").unwrap_err(); diff --git a/src/ssh/tokio_client/error.rs b/src/ssh/tokio_client/error.rs index b4b0c1928..e20d3f3e6 100644 --- a/src/ssh/tokio_client/error.rs +++ b/src/ssh/tokio_client/error.rs @@ -49,6 +49,23 @@ pub enum Error { AuthenticationPolicy(String), #[error("SSH identity algorithm is excluded by PubkeyAcceptedAlgorithms: {0}")] IdentityAlgorithmExcluded(String), + /// An authenticator-hosted (`sk-*`) identity could not be used because no + /// usable `SecurityKeyProvider` was available before it was offered; + /// authentication continues with the next identity. + #[error("authenticator-hosted key {path} skipped: {reason}")] + SecurityKeyUnavailable { + path: std::path::PathBuf, + reason: String, + }, + /// The security-key provider failed after the server requested a + /// signature; like [`Self::LocalSignerFailed`], this ends the transport. + #[error( + "security key signing failed for {path} after the server requested a signature: {reason}" + )] + SecurityKeySignFailed { + path: std::path::PathBuf, + reason: String, + }, #[error("Permission denied (password).")] PasswordWrong, #[error( @@ -302,6 +319,8 @@ impl Error { | Self::TransportIntegrity { .. } | Self::AgentAuthError(_) | Self::IdentityAlgorithmExcluded(_) + | Self::SecurityKeyUnavailable { .. } + | Self::SecurityKeySignFailed { .. } | Self::AgentConnectionFailed | Self::AgentRequestIdentitiesFailed | Self::AgentOperationTimeout { .. } diff --git a/src/ssh/tokio_client/mod.rs b/src/ssh/tokio_client/mod.rs index 68949eb40..a7227a4d6 100644 --- a/src/ssh/tokio_client/mod.rs +++ b/src/ssh/tokio_client/mod.rs @@ -29,6 +29,7 @@ mod hostkey_rotation; mod known_hosts_command; mod process_stdin; mod proxy_command; +mod security_key_signer; mod session; pub mod sftp_client; mod sshfp; diff --git a/src/ssh/tokio_client/security_key_signer.rs b/src/ssh/tokio_client/security_key_signer.rs new file mode 100644 index 000000000..3342234d7 --- /dev/null +++ b/src/ssh/tokio_client/security_key_signer.rs @@ -0,0 +1,411 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! russh [`Signer`](russh::Signer) that signs user authentication through a +//! FIDO security-key provider, following `identity_sign` in `sshconnect2.c`: +//! announce the touch when the key wants user presence, prompt for a PIN +//! once when the provider asks for one, and never block the async runtime +//! while the authenticator waits for the user. + +use std::io::IsTerminal; +use std::path::{Path, PathBuf}; +use std::sync::Arc; + +use russh::keys::ssh_encoding::Encode as _; +use russh::keys::ssh_key::HashAlg; +use russh::keys::ssh_key::private::KeypairData; +use zeroize::Zeroizing; + +use crate::ssh::security_key::{ + ProviderSource, SK_FLAG_USER_PRESENCE_REQUIRED, SecurityKeyError, SecurityKeyProvider, + SkAlgorithm, SkSignRequest, encode_signature, open_provider, resolve_provider_from_env, +}; + +/// Everything the provider needs from an `sk-*` private key file. +#[derive(Debug, Clone)] +pub(super) struct SecurityKeyIdentity { + pub(super) algorithm: SkAlgorithm, + application: String, + key_handle: Zeroizing>, + flags: u8, + fingerprint: String, + pub(super) path: PathBuf, +} + +impl SecurityKeyIdentity { + /// `Some` for authenticator-hosted keys, `None` for software keys. + pub(super) fn from_private_key(key: &russh::keys::PrivateKey, path: &Path) -> Option { + let (algorithm, application, key_handle, flags) = match key.key_data() { + KeypairData::SkEd25519(sk) => ( + SkAlgorithm::Ed25519, + sk.public().application().to_string(), + sk.key_handle().to_vec(), + sk.flags(), + ), + KeypairData::SkEcdsaSha2NistP256(sk) => ( + SkAlgorithm::EcdsaSha2NistP256, + sk.public().application().to_string(), + sk.key_handle().to_vec(), + sk.flags(), + ), + _ => return None, + }; + Some(Self { + algorithm, + application, + key_handle: Zeroizing::new(key_handle), + flags, + fingerprint: key.public_key().fingerprint(HashAlg::Sha256).to_string(), + path: path.to_path_buf(), + }) + } + + fn wants_user_presence(&self) -> bool { + self.flags & SK_FLAG_USER_PRESENCE_REQUIRED != 0 + } +} + +/// Resolve and open the configured provider for one identity. Every failure +/// is reported before the key is offered, so the caller can move on to the +/// next identity without poisoning the transport. +pub(super) async fn open_security_key_provider( + setting: Option<&str>, + path: &Path, +) -> Result, super::Error> { + let unavailable = |reason: String| super::Error::SecurityKeyUnavailable { + path: path.to_path_buf(), + reason, + }; + let source = resolve_provider_from_env(setting); + match &source { + ProviderSource::Disabled => { + tracing::debug!( + "ignoring authenticator-hosted key {} as no SecurityKeyProvider has been specified", + path.display() + ); + return Err(unavailable(SecurityKeyError::NoProvider.to_string())); + } + ProviderSource::Internal => { + let error = SecurityKeyError::InternalUnavailable; + crate::warningln!("SecurityKeyProvider internal: {error}"); + return Err(unavailable(error.to_string())); + } + ProviderSource::Library(_) => {} + } + let opened = tokio::task::spawn_blocking(move || open_provider(&source)) + .await + .map_err(super::Error::JoinError)?; + match opened { + Ok(provider) => Ok(Arc::from(provider)), + Err(error) => { + crate::warningln!("{error}"); + Err(unavailable(error.to_string())) + } + } +} + +/// Signs `to_sign` with the provider and returns it with the signature +/// appended, which is what russh expects back from a [`russh::Signer`]. +#[derive(Debug)] +pub(super) struct SecurityKeySigner { + provider: Arc, + identity: Arc, + /// `BatchMode no`: a touch notice may be printed and a PIN prompted. + interactive: bool, +} + +impl SecurityKeySigner { + pub(super) fn new( + provider: Arc, + identity: SecurityKeyIdentity, + interactive: bool, + ) -> Self { + Self { + provider, + identity: Arc::new(identity), + interactive, + } + } + + pub(super) async fn sign_blob(&self, data: Vec) -> Result, super::Error> { + let failed = |reason: String| super::Error::SecurityKeySignFailed { + path: self.identity.path.clone(), + reason, + }; + let notify = self.interactive && self.identity.wants_user_presence(); + let notify = notify && std::io::stderr().is_terminal(); + if notify { + crate::diagnosticln!( + "Confirm user presence for key {} {}", + self.identity.algorithm.ssh_name(), + self.identity.fingerprint + ); + } + let data = Arc::new(data); + let mut pin: Option> = None; + let mut retried = false; + loop { + let provider = Arc::clone(&self.provider); + let identity = Arc::clone(&self.identity); + let data = Arc::clone(&data); + let attempt_pin = pin.clone(); + let result = tokio::task::spawn_blocking(move || { + let request = SkSignRequest { + algorithm: identity.algorithm, + data: &data, + application: &identity.application, + key_handle: &identity.key_handle, + flags: identity.flags, + }; + provider.sign(&request, attempt_pin.as_deref().map(String::as_str)) + }) + .await + .map_err(super::Error::JoinError)?; + match result { + Ok(response) => { + if notify { + crate::diagnosticln!("User presence confirmed"); + } + return encode_signature(self.identity.algorithm, &response) + .map_err(|error| failed(error.to_string())); + } + Err(SecurityKeyError::PinRequired) + if !retried && pin.is_none() && self.interactive => + { + retried = true; + pin = Some(self.prompt_pin().await?); + } + Err(error) => return Err(failed(error.to_string())), + } + } + } + + async fn prompt_pin(&self) -> Result, super::Error> { + let prompt = format!( + "Enter PIN for {} key {}: ", + self.identity.algorithm.ssh_name(), + self.identity.path.display() + ); + super::authentication::serialized_bounded_auth_prompt("security key PIN", || async { + let prompt_task = tokio::task::spawn_blocking(move || { + rpassword::prompt_password(prompt).map(Zeroizing::new) + }); + prompt_task + .await + .map_err(super::Error::JoinError)? + .map_err(super::Error::IoError) + }) + .await + } +} + +impl russh::Signer for SecurityKeySigner { + type Error = super::Error; + + async fn auth_sign( + &mut self, + _key: &russh::keys::agent::AgentIdentity, + _hash_alg: Option, + to_sign: Vec, + ) -> Result, Self::Error> { + let blob = self.sign_blob(to_sign.clone()).await?; + let mut signed = to_sign; + blob.as_slice().encode(&mut signed).map_err(|error| { + super::Error::SecurityKeySignFailed { + path: self.identity.path.clone(), + reason: error.to_string(), + } + })?; + Ok(signed) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::Mutex; + + use crate::ssh::security_key::SkSignResponse; + use russh::keys::ssh_key::private::SkEd25519 as SkEd25519Private; + use russh::keys::ssh_key::public::{Ed25519PublicKey, SkEd25519 as SkEd25519Public}; + use russh::keys::ssh_key::{PrivateKey, private}; + + fn sk_ed25519_key(flags: u8) -> PrivateKey { + let public = SkEd25519Public::new(Ed25519PublicKey([9u8; 32]), "ssh:example"); + let private = SkEd25519Private::new(public, flags, vec![1, 2, 3, 4]).unwrap(); + PrivateKey::new(KeypairData::SkEd25519(private), "test").unwrap() + } + + /// `(algorithm, data, application, key_handle, flags, had_pin)` + type RecordedCall = (SkAlgorithm, Vec, String, Vec, u8, bool); + + #[derive(Debug, Default)] + struct RecordingProvider { + calls: Mutex>, + fail_until_pin: bool, + } + + impl SecurityKeyProvider for RecordingProvider { + fn sign( + &self, + request: &SkSignRequest<'_>, + pin: Option<&str>, + ) -> Result { + self.calls.lock().unwrap().push(( + request.algorithm, + request.data.to_vec(), + request.application.to_string(), + request.key_handle.to_vec(), + request.flags, + pin.is_some(), + )); + if self.fail_until_pin && pin.is_none() { + return Err(SecurityKeyError::PinRequired); + } + Ok(SkSignResponse { + flags: request.flags, + counter: 7, + sig_r: Zeroizing::new(vec![0x5a; 64]), + sig_s: Zeroizing::new(Vec::new()), + }) + } + + fn describe(&self) -> String { + "recording".to_string() + } + } + + #[test] + fn identity_is_extracted_only_from_sk_private_keys() { + let key = sk_ed25519_key(0x01); + let identity = SecurityKeyIdentity::from_private_key(&key, Path::new("/k")).unwrap(); + assert_eq!(identity.algorithm, SkAlgorithm::Ed25519); + assert_eq!(identity.application, "ssh:example"); + assert_eq!(&identity.key_handle[..], &[1, 2, 3, 4]); + assert!(identity.wants_user_presence()); + assert!(identity.fingerprint.starts_with("SHA256:")); + + let software = + PrivateKey::random(&mut rand::rng(), russh::keys::ssh_key::Algorithm::Ed25519).unwrap(); + assert!(SecurityKeyIdentity::from_private_key(&software, Path::new("/k")).is_none()); + let _ = private::KeypairData::Ed25519; + } + + #[tokio::test] + async fn signer_appends_the_provider_signature_to_the_request() { + let key = sk_ed25519_key(0x01); + let identity = SecurityKeyIdentity::from_private_key(&key, Path::new("/k")).unwrap(); + let provider = Arc::new(RecordingProvider::default()); + let mut signer = SecurityKeySigner::new(provider.clone(), identity, false); + let to_sign = vec![0xaa, 0xbb, 0xcc]; + let signed = russh::Signer::auth_sign( + &mut signer, + &key.public_key().clone().into(), + None, + to_sign.clone(), + ) + .await + .unwrap(); + assert_eq!(&signed[..3], &to_sign[..]); + let blob_len = u32::from_be_bytes(signed[3..7].try_into().unwrap()) as usize; + let blob = &signed[7..]; + assert_eq!(blob.len(), blob_len); + assert_eq!(&blob[4..30], b"sk-ssh-ed25519@openssh.com"); + assert_eq!(blob[blob.len() - 5], 0x01); + assert_eq!(&blob[blob.len() - 4..], &7u32.to_be_bytes()); + + let calls = provider.calls.lock().unwrap(); + assert_eq!(calls.len(), 1); + let (algorithm, data, application, key_handle, flags, had_pin) = &calls[0]; + assert_eq!(*algorithm, SkAlgorithm::Ed25519); + assert_eq!(data, &to_sign, "the provider hashes the raw message itself"); + assert_eq!(application, "ssh:example"); + assert_eq!(key_handle, &[1, 2, 3, 4]); + assert_eq!(*flags, 0x01); + assert!(!had_pin); + } + + #[tokio::test] + async fn pin_required_in_batch_mode_fails_without_prompting() { + let key = sk_ed25519_key(0x05); + let identity = SecurityKeyIdentity::from_private_key(&key, Path::new("/k")).unwrap(); + let provider = Arc::new(RecordingProvider { + fail_until_pin: true, + ..RecordingProvider::default() + }); + let signer = SecurityKeySigner::new(provider.clone(), identity, false); + let error = signer.sign_blob(vec![1]).await.unwrap_err(); + match error { + super::super::Error::SecurityKeySignFailed { path, reason } => { + assert_eq!(path, Path::new("/k")); + assert!(reason.contains("PIN required"), "{reason}"); + } + other => panic!("unexpected error: {other:?}"), + } + assert_eq!(provider.calls.lock().unwrap().len(), 1); + } + + #[tokio::test] + async fn provider_errors_name_the_identity() { + #[derive(Debug)] + struct Absent; + impl SecurityKeyProvider for Absent { + fn sign( + &self, + _: &SkSignRequest<'_>, + _: Option<&str>, + ) -> Result { + Err(SecurityKeyError::DeviceNotFound) + } + fn describe(&self) -> String { + "absent".to_string() + } + } + let key = sk_ed25519_key(0x01); + let identity = SecurityKeyIdentity::from_private_key(&key, Path::new("/id_sk")).unwrap(); + let signer = SecurityKeySigner::new(Arc::new(Absent), identity, true); + let error = signer.sign_blob(vec![1]).await.unwrap_err().to_string(); + assert!(error.contains("/id_sk"), "{error}"); + assert!(error.contains("device not found"), "{error}"); + } + + #[tokio::test] + async fn disabled_and_internal_providers_are_skipped_before_offering_the_key() { + let path = Path::new("/home/u/.ssh/id_ed25519_sk"); + let error = open_security_key_provider(Some("none"), path) + .await + .unwrap_err(); + assert!(matches!( + error, + super::super::Error::SecurityKeyUnavailable { .. } + )); + assert!(super::super::authentication::is_retryable_rejection(&error)); + + let error = open_security_key_provider(Some("internal"), path) + .await + .unwrap_err(); + let message = error.to_string(); + assert!( + message.contains("built-in security key support is not available"), + "{message}" + ); + assert!(message.contains("id_ed25519_sk"), "{message}"); + + let error = open_security_key_provider(Some("/nonexistent/provider.so"), path) + .await + .unwrap_err(); + assert!(error.to_string().contains("dlopen failed"), "{error}"); + assert!(super::super::authentication::is_retryable_rejection(&error)); + } +} diff --git a/tests/openssh-regress/selection.tsv b/tests/openssh-regress/selection.tsv index f1227c84a..0ac6dceda 100644 --- a/tests/openssh-regress/selection.tsv +++ b/tests/openssh-regress/selection.tsv @@ -1,9 +1,9 @@ test disposition reason timeout_seconds addrmatch run agent-getpeereid run -agent-pkcs11-cert skip Permanent candidate skip: PKCS#11 and FIDO/security-key middleware are outside the compatibility target. -agent-pkcs11-restrict skip Permanent candidate skip: PKCS#11 and FIDO/security-key middleware are outside the compatibility target. -agent-pkcs11 skip Permanent candidate skip: PKCS#11 and FIDO/security-key middleware are outside the compatibility target. +agent-pkcs11-cert skip Permanent candidate skip: PKCS#11 token middleware is outside the compatibility target (FIDO security keys are supported through SecurityKeyProvider). +agent-pkcs11-restrict skip Permanent candidate skip: PKCS#11 token middleware is outside the compatibility target (FIDO security keys are supported through SecurityKeyProvider). +agent-pkcs11 skip Permanent candidate skip: PKCS#11 token middleware is outside the compatibility target (FIDO security keys are supported through SecurityKeyProvider). agent-ptrace run agent-restrict run agent-subprocess run @@ -101,7 +101,7 @@ ssh-com-client exclude Permanent exclusion: ssh.com interoperability is outside ssh-com-keygen exclude Permanent exclusion: ssh.com interoperability is outside the compatibility target. ssh-com-sftp exclude Permanent exclusion: ssh.com interoperability is outside the compatibility target. ssh-com exclude Permanent exclusion: ssh.com interoperability is outside the compatibility target. -ssh-pkcs11 skip Permanent candidate skip: PKCS#11 and FIDO/security-key middleware are outside the compatibility target. +ssh-pkcs11 skip Permanent candidate skip: PKCS#11 token middleware is outside the compatibility target (FIDO security keys are supported through SecurityKeyProvider). ssh-tty run ssh2putty exclude Upstream conversion helper; it is not a standalone regression test. sshcfgparse run From 5618c12a176184717b3adda1fa1c31d42732a771 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 22 Oct 2026 11:20:01 +0900 Subject: [PATCH 2/7] test: keep unimplemented-option diagnostic test off SecurityKeyProvider SecurityKeyProvider now has a runtime consumer, so the compat-output test can no longer use it as the example of a parsed-but-unimplemented option. The fixture now uses VisualHostKey at the same line for the once-only unsupported diagnostic, keeps a SecurityKeyProvider line, and asserts that it produces no unsupported diagnostic. Validated with cargo test --test ssh_compat_output_test and cargo test --test ssh_config_dump_test. Refs #328 --- tests/ssh_compat_output_test.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/ssh_compat_output_test.rs b/tests/ssh_compat_output_test.rs index 5937c9e41..c6059dce1 100644 --- a/tests/ssh_compat_output_test.rs +++ b/tests/ssh_compat_output_test.rs @@ -128,7 +128,7 @@ fn canonical_unimplemented_and_unknown_diagnostics_use_real_source_and_log_file( let log = directory.path().join("bssh.log"); fs::write( &included, - "# Source: /spoofed/config:9000\nChallengeResponseAuthentication no\nKbdInteractiveAuthentication yes\nSecurityKeyProvider /usr/lib/ssh/ssh-sk-helper\nDefinitelyUnknownOption yes\n", + "# Source: /spoofed/config:9000\nChallengeResponseAuthentication no\nKbdInteractiveAuthentication yes\nVisualHostKey yes\nDefinitelyUnknownOption yes\nSecurityKeyProvider /usr/lib/ssh/ssh-sk-helper\n", ) .expect("included ssh config should be written"); #[cfg(unix)] @@ -178,7 +178,7 @@ fn canonical_unimplemented_and_unknown_diagnostics_use_real_source_and_log_file( "Unsupported SSH config option 'kbdinteractiveauthentication' at {included}:2; bssh parses this value for inspection but does not implement its runtime behavior" ); let unimplemented = format!( - "Unsupported SSH config option 'securitykeyprovider' at {included}:4; bssh parses this value for inspection but does not implement its runtime behavior" + "Unsupported SSH config option 'visualhostkey' at {included}:4; bssh parses this value for inspection but does not implement its runtime behavior" ); let unknown = format!("Unknown SSH config option 'definitelyunknownoption' at {included}:5"); let distinct_unknown = "Unknown SSH config option 'anotherunknownoption' at -o option #4"; @@ -209,6 +209,11 @@ fn canonical_unimplemented_and_unknown_diagnostics_use_real_source_and_log_file( 1, "distinct unknown diagnostic must remain independent: {diagnostics:?}" ); + // SecurityKeyProvider has a runtime consumer, so it must not be reported. + assert!( + !diagnostics.contains("securitykeyprovider"), + "implemented SecurityKeyProvider must not be reported as unsupported: {diagnostics:?}" + ); assert!(!diagnostics.contains("/spoofed/config")); } From d4a48116fd7f8e55b1f02b749249f3eacb00b7e9 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 22 Oct 2026 11:29:15 +0900 Subject: [PATCH 3/7] fix(ssh): continue after security-key signing failures A failed sk_sign (device absent, PIN required in BatchMode, provider error) after the server accepted an sk-* key ended the whole connection, while OpenSSH's sign_and_send_pubkey failure path moves on to the next identity. russh 0.64.1 blocks its session loop on Msg::Signed after an Err from the signer, but it sends nothing when the signer returns the request unchanged. The sk signer now declines that way, records the error, and authenticate_declinable drops the handle-side exchange that would otherwise wait for a reply forever. SecurityKeySignFailed is now a retryable rejection. Provider calls are serialized process-wide inside the blocking task: sk-api makes no thread-safety promise and OpenSSH never calls a provider concurrently, while bssh authenticates to many hosts at once. sk-* identity files are checked against the provider from their unencrypted public half before any passphrase prompt, and ~/.ssh/id_ed25519_sk and ~/.ssh/id_ecdsa_sk join the runtime default identities that -G already printed. New tests cover each behavior and fail when it is reverted: the next identity authenticates on the same connection after a declined signature, provider calls never overlap, an encrypted sk key without a provider is skipped before decryption, and the defaults include the FIDO key files. Refs #328 --- README.md | 2 +- docs/architecture/ssh-client.md | 2 +- docs/man/bssh.1 | 4 +- docs/openssh-compatibility-status.md | 2 +- src/ssh/auth.rs | 40 +++- src/ssh/tokio_client/authentication.rs | 225 ++++++++++++++++++-- src/ssh/tokio_client/error.rs | 10 +- src/ssh/tokio_client/security_key_signer.rs | 201 ++++++++++++++++- 8 files changed, 443 insertions(+), 43 deletions(-) diff --git a/README.md b/README.md index ba43407d7..6dbb1bf17 100644 --- a/README.md +++ b/README.md @@ -602,7 +602,7 @@ See [examples/mpi_exit_code.sh](examples/mpi_exit_code.sh) and [examples/health_ bssh supports multiple authentication methods: ### SSH Key Authentication -- **Default keys**: Automatically tries `~/.ssh/id_ed25519`, `~/.ssh/id_rsa`, `~/.ssh/id_ecdsa`, `~/.ssh/id_dsa` +- **Default keys**: Automatically tries `~/.ssh/id_ed25519`, `~/.ssh/id_ed25519_sk`, `~/.ssh/id_rsa`, `~/.ssh/id_ecdsa`, `~/.ssh/id_ecdsa_sk`, `~/.ssh/id_dsa` (the `*_sk` FIDO keys need a `SecurityKeyProvider`) - **Custom key**: Use `-i` flag to specify a key file - **Encrypted keys**: Automatically detects and prompts for passphrase diff --git a/docs/architecture/ssh-client.md b/docs/architecture/ssh-client.md index 3ddbff06f..7e3928bbd 100644 --- a/docs/architecture/ssh-client.md +++ b/docs/architecture/ssh-client.md @@ -15,7 +15,7 @@ **Tokio Client Module Structure :** - `tokio_client/connection.rs` - Connection management (293 lines) - `tokio_client/authentication.rs` - Authentication methods (378 lines) -- `tokio_client/security_key_signer.rs` - russh `Signer` for FIDO `sk-*` identities: resolves `SecurityKeyProvider`, calls the provider on a blocking thread, prints the touch notice and prompts once for a PIN +- `tokio_client/security_key_signer.rs` - russh `Signer` for FIDO `sk-*` identities: resolves `SecurityKeyProvider`, checks the provider before decrypting an `sk-*` key file, calls the provider on a blocking thread (serialized process-wide), prints the touch notice, prompts once for a PIN, and declines a failed signature so authentication continues with the next identity (`authenticate_declinable`) - `ssh/security_key/` - provider interface (`SecurityKeyProvider` trait, `$VAR` resolution, `internal`/`none` handling), the `sk-api.h` `dlopen` binding (`external.rs`), and the `sk-*` signature blob encoder (`signature.rs`) - `tokio_client/channel_manager.rs` - Channel operations (230 lines) - `tokio_client/file_transfer.rs` - SFTP file operations (285 lines) diff --git a/docs/man/bssh.1 b/docs/man/bssh.1 index 4a9d84171..76033ae2f 100644 --- a/docs/man/bssh.1 +++ b/docs/man/bssh.1 @@ -1098,7 +1098,7 @@ Example: .TP .B SecurityKeyProvider -Specifies the library used to access FIDO authenticator-hosted keys (sk-ssh-ed25519@openssh.com and sk-ecdsa-sha2-nistp256@openssh.com identity files, as enrolled by ssh-keygen -t ed25519-sk or ecdsa-sk). The value is a path to a shared library implementing OpenSSH's sk-api (for example libsk-libfido2.so), or a name starting with $ that selects an environment variable holding that path. The default is $SSH_SK_PROVIDER; when the variable is unset, or the value is none, sk-* identities are skipped. The value internal (OpenSSH's built-in USB HID provider) is not available in this build and is reported before the next identity is tried. The library is loaded with dlopen(3); statically linked musl builds cannot load it and say so. When a key requires user presence and standard error is a terminal, bssh prints a confirmation notice while waiting for the touch; when the provider asks for a PIN and BatchMode is off, bssh prompts for it once. Keys held by an agent keep signing through the agent. +Specifies the library used to access FIDO authenticator-hosted keys (sk-ssh-ed25519@openssh.com and sk-ecdsa-sha2-nistp256@openssh.com identity files, as enrolled by ssh-keygen -t ed25519-sk or ecdsa-sk). The value is a path to a shared library implementing OpenSSH's sk-api (for example libsk-libfido2.so), or a name starting with $ that selects an environment variable holding that path. The default is $SSH_SK_PROVIDER; when the variable is unset, or the value is none, sk-* identities are skipped. The value internal (OpenSSH's built-in USB HID provider) is not available in this build and is reported before the next identity is tried. The library is loaded with dlopen(3); statically linked musl builds cannot load it and say so. When a key requires user presence and standard error is a terminal, bssh prints a confirmation notice while waiting for the touch; when the provider asks for a PIN and BatchMode is off, bssh prompts for it once. If signing fails (no device, PIN required in BatchMode, provider error), bssh reports it and continues with the next identity. Keys held by an agent keep signing through the agent. .br Example: .I SecurityKeyProvider /usr/lib/libsk-libfido2.so @@ -2088,7 +2088,7 @@ Configuration file when XDG_CONFIG_HOME is set SSH known hosts file for host key verification .TP -.I ~/.ssh/id_ed25519, ~/.ssh/id_rsa, ~/.ssh/id_ecdsa, ~/.ssh/id_dsa +.I ~/.ssh/id_ed25519, ~/.ssh/id_ed25519_sk, ~/.ssh/id_rsa, ~/.ssh/id_ecdsa, ~/.ssh/id_ecdsa_sk, ~/.ssh/id_dsa Default SSH private keys (checked in order of preference). If a key is encrypted, bssh will prompt for the passphrase. diff --git a/docs/openssh-compatibility-status.md b/docs/openssh-compatibility-status.md index 28780a059..44c3dd88a 100644 --- a/docs/openssh-compatibility-status.md +++ b/docs/openssh-compatibility-status.md @@ -39,7 +39,7 @@ Host certificates are accepted only after full validation: a `@cert-authority` l ## Security keys -Authenticator-hosted (`sk-ssh-ed25519@openssh.com`, `sk-ecdsa-sha2-nistp256@openssh.com`) keys follow OpenSSH 10.3p1 built without `ENABLE_SK_INTERNAL`. `SecurityKeyProvider` defaults to `$SSH_SK_PROVIDER`; a `$`-prefixed value names an environment variable, an unset variable or `none` disables `sk-*` identities (logged at debug level, as `ssh` does), and `internal` prints a diagnostic and moves on to the next identity until #331 supplies the built-in provider. An external provider is a shared library implementing `sk-api.h` version `0x000a0000` (`sk_api_version`, `sk_enroll`, `sk_sign`, `sk_load_resident_keys`); bssh loads it in process with `dlopen` and calls `sk_sign` on a blocking thread, printing `Confirm user presence for key ...` when the key requires a touch and stderr is a terminal, and prompting once for a PIN when the provider reports `SSH_SK_ERR_PIN_REQUIRED` outside `BatchMode`. OpenSSH runs the same calls in `ssh-sk-helper` to keep the middleware out of its `pledge`d client; bssh has no such sandbox, so a helper would add a private protocol without changing what runs as the user. A provider that cannot be opened (missing file, no `sk_api_version`, wrong major version) is reported before the key is offered and authentication continues; a failure inside `sk_sign` after the server has asked for the signature ends the transport, because russh is then waiting for that signature. Static musl release builds cannot `dlopen`; they build, and report that limitation when an external provider is configured. Host keys and host certificates of the `sk-*` types are verified by ssh-key's `sk` verifiers (`SHA256(application) || flags || counter || SHA256(message)`), with no flag policy on the client side, as in OpenSSH. Agent-held `sk-*` keys keep signing through the agent. `-Q key-plain`, `-Q key`, `-Q key-cert`, `-Q key-sig`, `HostKeyAlgorithms` and `PubkeyAcceptedAlgorithms` list the `sk-*` names in the reference order; the only difference from the reference `ssh -Q key-sig` and `ssh -G` is the absent `webauthn-sk-ecdsa-sha2-nistp256@openssh.com` pair. +Authenticator-hosted (`sk-ssh-ed25519@openssh.com`, `sk-ecdsa-sha2-nistp256@openssh.com`) keys follow OpenSSH 10.3p1 built without `ENABLE_SK_INTERNAL`. `SecurityKeyProvider` defaults to `$SSH_SK_PROVIDER`; a `$`-prefixed value names an environment variable, an unset variable or `none` disables `sk-*` identities (logged at debug level, as `ssh` does), and `internal` prints a diagnostic and moves on to the next identity until #331 supplies the built-in provider. An external provider is a shared library implementing `sk-api.h` version `0x000a0000` (`sk_api_version`, `sk_enroll`, `sk_sign`, `sk_load_resident_keys`); bssh loads it in process with `dlopen` and calls `sk_sign` on a blocking thread, printing `Confirm user presence for key ...` when the key requires a touch and stderr is a terminal, and prompting once for a PIN when the provider reports `SSH_SK_ERR_PIN_REQUIRED` outside `BatchMode`. OpenSSH runs the same calls in `ssh-sk-helper` to keep the middleware out of its `pledge`d client; bssh has no such sandbox, so a helper would add a private protocol without changing what runs as the user. A provider that cannot be opened (missing file, no `sk_api_version`, wrong major version) is reported before the key is offered and authentication continues; a failure inside `sk_sign` after the server has accepted the key (device absent, PIN required in `BatchMode`, provider error) prints `security key signing failed for : ...` and authentication continues with the next identity on the same connection, as after OpenSSH's `sign_and_send_pubkey` failure: the signer hands russh the request back unsigned, russh sends nothing for it, and bssh stops waiting for the reply. Provider calls are serialized process-wide, because sk-api makes no thread-safety promise and bssh may authenticate to many hosts at once. An `sk-*` identity is checked against the provider from its unencrypted public half before any passphrase prompt, and `~/.ssh/id_ed25519_sk` and `~/.ssh/id_ecdsa_sk` are among the default identities. Static musl release builds cannot `dlopen`; they build, and report that limitation when an external provider is configured. Host keys and host certificates of the `sk-*` types are verified by ssh-key's `sk` verifiers (`SHA256(application) || flags || counter || SHA256(message)`), with no flag policy on the client side, as in OpenSSH. Agent-held `sk-*` keys keep signing through the agent. `-Q key-plain`, `-Q key`, `-Q key-cert`, `-Q key-sig`, `HostKeyAlgorithms` and `PubkeyAcceptedAlgorithms` list the `sk-*` names in the reference order; the only difference from the reference `ssh -Q key-sig` and `ssh -G` is the absent `webauthn-sk-ecdsa-sha2-nistp256@openssh.com` pair. ## ssh_config integration diff --git a/src/ssh/auth.rs b/src/ssh/auth.rs index c0d86c6b0..c5f485ea9 100644 --- a/src/ssh/auth.rs +++ b/src/ssh/auth.rs @@ -691,10 +691,19 @@ impl AuthContext { return Vec::new(); }; let ssh_dir = home_dir.join(".ssh"); - ["id_ed25519", "id_rsa", "id_ecdsa", "id_dsa"] - .into_iter() - .map(|name| ssh_dir.join(name)) - .collect() + // OpenSSH also tries the FIDO `*_sk` defaults; they are skipped before + // any prompt when no SecurityKeyProvider is usable. + [ + "id_ed25519", + "id_ed25519_sk", + "id_rsa", + "id_ecdsa", + "id_ecdsa_sk", + "id_dsa", + ] + .into_iter() + .map(|name| ssh_dir.join(name)) + .collect() } } @@ -727,6 +736,29 @@ mod tests { assert!(ctx.password.is_none()); } + #[test] + fn default_identities_include_the_fido_key_files() { + let ctx = AuthContext::new("user".to_string(), "host".to_string()).unwrap(); + let names = ctx + .default_key_paths() + .iter() + .filter_map(|path| path.file_name()?.to_str().map(str::to_string)) + .collect::>(); + if dirs::home_dir().is_none() { + assert!(names.is_empty()); + return; + } + for name in [ + "id_ed25519_sk", + "id_ecdsa_sk", + "id_ed25519", + "id_rsa", + "id_ecdsa", + ] { + assert!(names.iter().any(|found| found == name), "{name}: {names:?}"); + } + } + #[tokio::test] async fn test_auth_context_with_pre_collected_password() { let password = Arc::new(Password::new("test123".to_string()).unwrap()); diff --git a/src/ssh/tokio_client/authentication.rs b/src/ssh/tokio_client/authentication.rs index 7bbccd978..1379bcad8 100644 --- a/src/ssh/tokio_client/authentication.rs +++ b/src/ssh/tokio_client/authentication.rs @@ -805,7 +805,7 @@ async fn authenticate_private_key_with_policy( key: russh::keys::PrivateKey, key_file_path: &std::path::Path, accepted: &[String], - security_key_provider: Option<&str>, + security_key: SecurityKeySource<'_>, interactive: bool, ) -> Result<(), super::Error> { let hashes = if accepted.is_empty() { @@ -822,8 +822,7 @@ async fn authenticate_private_key_with_policy( let public_key = key.public_key().clone(); let signer = - LocalSigner::for_private_key(key, key_file_path, security_key_provider, interactive) - .await?; + LocalSigner::for_private_key(key, key_file_path, security_key, interactive).await?; match signer { LocalSigner::Software(key) => { for hash in hashes { @@ -839,9 +838,12 @@ async fn authenticate_private_key_with_policy( } } LocalSigner::SecurityKey(mut signer) => { - let result = handle - .authenticate_publickey_with(username, public_key, None, &mut signer) - .await?; + let decline = signer.decline(); + let result = super::security_key_signer::authenticate_declinable( + &decline, + handle.authenticate_publickey_with(username, public_key, None, &mut signer), + ) + .await?; if result.success() { return Ok(()); } @@ -850,6 +852,14 @@ async fn authenticate_private_key_with_policy( Err(super::Error::KeyAuthFailed) } +/// How an `sk-*` identity reaches its provider: the raw `SecurityKeyProvider` +/// setting, plus the provider `preflight_security_key_file` already opened +/// before the private key was decrypted. +struct SecurityKeySource<'a> { + setting: Option<&'a str>, + preopened: Option>, +} + /// Signs locally held identities: software keys in process, `sk-*` keys /// through the configured security-key provider. #[derive(Debug)] @@ -860,20 +870,27 @@ enum LocalSigner { impl LocalSigner { /// Pick the signer for `key`; `sk-*` keys need a usable provider before - /// the identity is offered, so provider failures surface here. + /// the identity is offered, so provider failures surface here (or + /// earlier, in `preflight_security_key_file`, which hands its provider + /// over as `preopened`). async fn for_private_key( key: russh::keys::PrivateKey, path: &std::path::Path, - security_key_provider: Option<&str>, + security_key: SecurityKeySource<'_>, interactive: bool, ) -> Result { match super::security_key_signer::SecurityKeyIdentity::from_private_key(&key, path) { Some(identity) => { - let provider = super::security_key_signer::open_security_key_provider( - security_key_provider, - path, - ) - .await?; + let provider = match security_key.preopened { + Some(provider) => provider, + None => { + super::security_key_signer::open_security_key_provider( + security_key.setting, + path, + ) + .await? + } + }; Ok(Self::SecurityKey( super::security_key_signer::SecurityKeySigner::new( provider, @@ -956,6 +973,11 @@ async fn authenticate_certificate_file( use_keychain: bool, security_key_provider: Option, ) -> Result<(), super::Error> { + let preopened_provider = super::security_key_signer::preflight_security_key_file( + security_key_provider.as_deref(), + &key_file_path, + ) + .await?; let key = load_policy_private_key( &key_file_path, key_pass, @@ -998,14 +1020,26 @@ async fn authenticate_certificate_file( let mut signer = LocalSigner::for_private_key( key, &key_file_path, - security_key_provider.as_deref(), + SecurityKeySource { + setting: security_key_provider.as_deref(), + preopened: preopened_provider, + }, allow_passphrase_prompt, ) .await?; + let decline = match &signer { + LocalSigner::SecurityKey(signer) => Some(signer.decline()), + LocalSigner::Software(_) => None, + }; for hash in hashes { - let result = handle - .authenticate_certificate_with(username, certificate.clone(), hash, &mut signer) - .await?; + let exchange = + handle.authenticate_certificate_with(username, certificate.clone(), hash, &mut signer); + let result = match &decline { + Some(decline) => { + super::security_key_signer::authenticate_declinable(decline, exchange).await + } + None => exchange.await, + }?; if result.success() { return Ok(()); } @@ -1330,8 +1364,10 @@ pub(super) fn is_retryable_rejection(error: &super::Error) -> bool { // Agent setup fails before russh requests a signature, so the plan may // safely continue. Signer failures are intentionally absent: russh may be // waiting for `Msg::Signed`, and reusing that transport could then hang. - // `SecurityKeyUnavailable` is raised before the identity is offered; - // `SecurityKeySignFailed` comes from inside the signer and is not here. + // `SecurityKeyUnavailable` is raised before the identity is offered. + // `SecurityKeySignFailed` is the exception among signer failures: the + // security-key signer declines without sending anything and + // `authenticate_declinable` releases the transport (see there). matches!( error, super::Error::KeyboardInteractiveAuthFailed @@ -1341,6 +1377,7 @@ pub(super) fn is_retryable_rejection(error: &super::Error) -> bool { | super::Error::KeyPassphrasePromptDisabled { .. } | super::Error::IdentityAlgorithmExcluded(_) | super::Error::SecurityKeyUnavailable { .. } + | super::Error::SecurityKeySignFailed { .. } | super::Error::PasswordWrong | super::Error::AgentConnectionFailed | super::Error::AgentRequestIdentitiesFailed @@ -1410,6 +1447,11 @@ async fn authenticate_single( use_keychain, security_key_provider, } => { + let preopened_provider = super::security_key_signer::preflight_security_key_file( + security_key_provider.as_deref(), + &key_file_path, + ) + .await?; let key = load_policy_private_key( &key_file_path, key_pass, @@ -1423,7 +1465,10 @@ async fn authenticate_single( key, &key_file_path, &accepted_algorithms, - security_key_provider.as_deref(), + SecurityKeySource { + setting: security_key_provider.as_deref(), + preopened: preopened_provider, + }, allow_passphrase_prompt, ) .await?; @@ -1811,6 +1856,146 @@ mod policy_execution_tests { task.abort(); } + /// A security-key signing failure after the server accepted the key + /// (device absent, PIN needed in BatchMode) must leave the transport + /// usable, so the next identity authenticates on the same connection as + /// it does after OpenSSH's `sign_and_send_pubkey` failure. + #[tokio::test] + async fn security_key_sign_failure_moves_on_to_the_next_identity() { + use crate::ssh::security_key::{ + SecurityKeyError, SecurityKeyProvider, SkSignRequest, SkSignResponse, + }; + use russh::keys::ssh_key::private::{KeypairData, SkEd25519 as SkEd25519Private}; + use russh::keys::ssh_key::public::{Ed25519PublicKey, SkEd25519 as SkEd25519Public}; + + #[derive(Debug)] + struct AbsentDevice(std::sync::atomic::AtomicUsize); + impl SecurityKeyProvider for AbsentDevice { + fn sign( + &self, + _: &SkSignRequest<'_>, + _: Option<&str>, + ) -> Result { + self.0.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + Err(SecurityKeyError::DeviceNotFound) + } + fn describe(&self) -> String { + "absent".to_string() + } + } + + let sk_public = SkEd25519Public::new(Ed25519PublicKey([7u8; 32]), "ssh:"); + let sk_private = SkEd25519Private::new(sk_public, 0x01, vec![1, 2, 3]).unwrap(); + let sk_key = + russh::keys::PrivateKey::new(KeypairData::SkEd25519(sk_private), "sk").unwrap(); + let directory = tempfile::TempDir::new().unwrap(); + let software_path = directory.path().join("software-key"); + let software = write_key(&software_path); + let attempts = Arc::new(Mutex::new(Vec::new())); + let (address, task) = start_server(ScriptedAuthServer { + attempts: Arc::clone(&attempts), + key_names: HashMap::from([ + (sk_key.public_key().key_data().clone(), "sk-key".to_string()), + ( + software.public_key().key_data().clone(), + "software-key".to_string(), + ), + ]), + accepted_key: Some("software-key".to_string()), + accepted_certificate: None, + }) + .await; + let mut handle = connect(address).await; + + let provider = Arc::new(AbsentDevice(std::sync::atomic::AtomicUsize::new(0))); + let identity = super::super::security_key_signer::SecurityKeyIdentity::from_private_key( + &sk_key, + std::path::Path::new("/id_ed25519_sk"), + ) + .unwrap(); + let mut signer = super::super::security_key_signer::SecurityKeySigner::new( + provider.clone(), + identity, + false, + ); + let decline = signer.decline(); + let outcome = tokio::time::timeout(std::time::Duration::from_secs(10), async { + let error = super::super::security_key_signer::authenticate_declinable( + &decline, + handle.authenticate_publickey_with( + "user", + sk_key.public_key().clone(), + None, + &mut signer, + ), + ) + .await + .unwrap_err(); + assert!(is_retryable_rejection(&error), "{error:?}"); + assert!(error.to_string().contains("device not found"), "{error}"); + + let methods = + AuthMethod::with_key_file_policy(&software_path, None, vec!["ssh-ed25519".into()]); + authenticate(&mut handle, &"user".to_string(), methods).await + }) + .await + .expect("the transport must stay usable after a declined security-key signature"); + outcome.unwrap(); + assert_eq!( + provider.0.load(std::sync::atomic::Ordering::SeqCst), + 1, + "the server accepted the sk key, so the provider was asked to sign" + ); + assert_eq!(attempts.lock().await.as_slice(), ["software-key"]); + task.abort(); + } + + /// An encrypted `sk-*` identity without a usable provider is skipped from + /// its public half, before any passphrase handling, as OpenSSH does. + #[tokio::test] + async fn security_key_identity_without_provider_is_skipped_before_decryption() { + use russh::keys::ssh_key::private::{KeypairData, SkEd25519 as SkEd25519Private}; + use russh::keys::ssh_key::public::{Ed25519PublicKey, SkEd25519 as SkEd25519Public}; + + let sk_public = SkEd25519Public::new(Ed25519PublicKey([3u8; 32]), "ssh:"); + let sk_private = SkEd25519Private::new(sk_public, 0x01, vec![9, 9]).unwrap(); + let encrypted = russh::keys::PrivateKey::new(KeypairData::SkEd25519(sk_private), "sk") + .unwrap() + .encrypt(&mut rand::rng(), "passphrase") + .unwrap(); + let directory = tempfile::TempDir::new().unwrap(); + let path = directory.path().join("id_ed25519_sk"); + std::fs::write( + &path, + encrypted.to_openssh(LineEnding::LF).unwrap().as_bytes(), + ) + .unwrap(); + + let (address, task) = start_server(ScriptedAuthServer { + attempts: Arc::new(Mutex::new(Vec::new())), + key_names: HashMap::new(), + accepted_key: None, + accepted_certificate: None, + }) + .await; + let mut handle = connect(address).await; + let method = AuthMethod::with_configured_key_file( + &path, + vec!["sk-ssh-ed25519@openssh.com".into()], + false, + false, + Some("none".to_string()), + ); + let error = authenticate_single(&mut handle, &"user".to_string(), method) + .await + .unwrap_err(); + assert!( + matches!(error, super::super::Error::SecurityKeyUnavailable { .. }), + "{error:?}" + ); + task.abort(); + } + #[tokio::test] async fn unavailable_cli_identities_are_retryable_before_a_valid_key() { let directory = tempfile::TempDir::new().unwrap(); diff --git a/src/ssh/tokio_client/error.rs b/src/ssh/tokio_client/error.rs index e20d3f3e6..6ff25f027 100644 --- a/src/ssh/tokio_client/error.rs +++ b/src/ssh/tokio_client/error.rs @@ -57,11 +57,11 @@ pub enum Error { path: std::path::PathBuf, reason: String, }, - /// The security-key provider failed after the server requested a - /// signature; like [`Self::LocalSignerFailed`], this ends the transport. - #[error( - "security key signing failed for {path} after the server requested a signature: {reason}" - )] + /// The security-key provider failed after the server accepted the key. + /// The signer declines without sending a signature (see + /// `security_key_signer::authenticate_declinable`), so authentication + /// continues with the next identity on the same transport. + #[error("security key signing failed for {path}: {reason}")] SecurityKeySignFailed { path: std::path::PathBuf, reason: String, diff --git a/src/ssh/tokio_client/security_key_signer.rs b/src/ssh/tokio_client/security_key_signer.rs index 3342234d7..a49842afc 100644 --- a/src/ssh/tokio_client/security_key_signer.rs +++ b/src/ssh/tokio_client/security_key_signer.rs @@ -17,10 +17,20 @@ //! announce the touch when the key wants user presence, prompt for a PIN //! once when the provider asks for one, and never block the async runtime //! while the authenticator waits for the user. +//! +//! A provider failure (device absent, PIN required in `BatchMode`, provider +//! error) skips the identity the way a failed `identity_sign` does in +//! OpenSSH: the signer hands russh the request back unsigned, which russh +//! then does not send, and [`authenticate_declinable`] stops waiting for a +//! server reply that will never come. The transport stays usable for the +//! next identity. +use std::future::Future; use std::io::IsTerminal; use std::path::{Path, PathBuf}; -use std::sync::Arc; +use std::sync::{Arc, Mutex}; + +use tokio::sync::Notify; use russh::keys::ssh_encoding::Encode as _; use russh::keys::ssh_key::HashAlg; @@ -115,6 +125,100 @@ pub(super) async fn open_security_key_provider( } } +/// Serializes every provider call in the process. sk-api makes no +/// thread-safety promise and OpenSSH never calls a provider concurrently +/// (each signature runs in its own `ssh-sk-helper`), while bssh may +/// authenticate to many hosts at once. The lock is taken inside the blocking +/// task, so a dropped authentication future cannot release it while the +/// provider is still running. +static PROVIDER_CALL_LOCK: Mutex<()> = Mutex::new(()); + +/// Records a signature the signer declined to produce. +#[derive(Debug, Default)] +pub(super) struct SignatureDecline { + error: Mutex>, + notify: Notify, +} + +impl SignatureDecline { + fn record(&self, error: super::Error) { + *self + .error + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) = Some(error); + self.notify.notify_one(); + } + + fn take(&self) -> Option { + self.error + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .take() + } +} + +/// Drive one russh public-key or certificate exchange that signs with a +/// [`SecurityKeySigner`], ending it as soon as the signer declines. +/// +/// When the signer declines, it has returned the request unsigned. russh's +/// session loop compares the returned buffer with the request it handed out +/// and, finding it unchanged, sends nothing; the `Msg::Signed` that carries +/// it is queued on russh's bounded handle channel in the same poll, because +/// authentication holds the handle exclusively and that channel is otherwise +/// empty. The handle-side future then waits for a server reply that will +/// never arrive, so it is dropped here (its receive is cancellation-safe) and +/// the recorded signing error is returned instead. The server never saw a +/// signed request, so the next identity can be offered on the same transport, +/// as OpenSSH's `sign_and_send_pubkey` failure path does. +pub(super) async fn authenticate_declinable( + decline: &SignatureDecline, + exchange: F, +) -> Result +where + F: Future>, +{ + tokio::pin!(exchange); + let result = tokio::select! { + biased; + result = &mut exchange => result, + () = decline.notify.notified() => Err(decline + .take() + .unwrap_or_else(|| super::Error::AuthenticationPolicy( + "security key signature declined".to_string(), + ))), + }; + match decline.take() { + Some(error) => Err(error), + None => result, + } +} + +/// Resolve the provider for an `sk-*` identity file before its private half +/// is decrypted, as OpenSSH skips such keys from their public half when no +/// provider is configured. The public key of an OpenSSH-format private key is +/// stored unencrypted, so this never prompts. Returns `None` for software +/// keys and for files that are not OpenSSH-format private keys (the regular +/// loader reports those). +pub(super) async fn preflight_security_key_file( + setting: Option<&str>, + path: &Path, +) -> Result>, super::Error> { + let Ok(text) = tokio::fs::read_to_string(path).await.map(Zeroizing::new) else { + return Ok(None); + }; + let Ok(key) = russh::keys::ssh_key::PrivateKey::from_openssh(text.as_bytes()) else { + return Ok(None); + }; + if !matches!( + key.algorithm(), + russh::keys::ssh_key::Algorithm::SkEd25519 + | russh::keys::ssh_key::Algorithm::SkEcdsaSha2NistP256 + ) { + return Ok(None); + } + open_security_key_provider(setting, path).await.map(Some) +} + /// Signs `to_sign` with the provider and returns it with the signature /// appended, which is what russh expects back from a [`russh::Signer`]. #[derive(Debug)] @@ -123,6 +227,7 @@ pub(super) struct SecurityKeySigner { identity: Arc, /// `BatchMode no`: a touch notice may be printed and a PIN prompted. interactive: bool, + decline: Arc, } impl SecurityKeySigner { @@ -135,9 +240,16 @@ impl SecurityKeySigner { provider, identity: Arc::new(identity), interactive, + decline: Arc::new(SignatureDecline::default()), } } + /// Where a declined signature is reported; pass it to + /// [`authenticate_declinable`] around the russh exchange. + pub(super) fn decline(&self) -> Arc { + Arc::clone(&self.decline) + } + pub(super) async fn sign_blob(&self, data: Vec) -> Result, super::Error> { let failed = |reason: String| super::Error::SecurityKeySignFailed { path: self.identity.path.clone(), @@ -168,6 +280,9 @@ impl SecurityKeySigner { key_handle: &identity.key_handle, flags: identity.flags, }; + let _serialized = PROVIDER_CALL_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); provider.sign(&request, attempt_pin.as_deref().map(String::as_str)) }) .await @@ -219,15 +334,36 @@ impl russh::Signer for SecurityKeySigner { _hash_alg: Option, to_sign: Vec, ) -> Result, Self::Error> { - let blob = self.sign_blob(to_sign.clone()).await?; - let mut signed = to_sign; - blob.as_slice().encode(&mut signed).map_err(|error| { - super::Error::SecurityKeySignFailed { - path: self.identity.path.clone(), - reason: error.to_string(), + let signed = match self.sign_blob(to_sign.clone()).await { + Ok(blob) => { + let mut signed = to_sign.clone(); + blob.as_slice() + .encode(&mut signed) + .map(|()| signed) + .map_err(|error| super::Error::SecurityKeySignFailed { + path: self.identity.path.clone(), + reason: error.to_string(), + }) } - })?; - Ok(signed) + Err(error) => Err(error), + }; + match signed { + Ok(signed) => Ok(signed), + Err(error) => { + // OpenSSH reports "sign_and_send_pubkey: signing failed for + // ..." and moves on to the next identity. + match &error { + super::Error::SecurityKeySignFailed { .. } => crate::warningln!("{error}"), + other => crate::warningln!( + "security key signing failed for {}: {other}", + self.identity.path.display() + ), + } + self.decline.record(error); + // Unchanged length: russh sends nothing for this request. + Ok(to_sign) + } + } } } @@ -380,6 +516,53 @@ mod tests { assert!(error.contains("device not found"), "{error}"); } + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn provider_calls_never_overlap_across_signers() { + use std::sync::atomic::{AtomicUsize, Ordering}; + + #[derive(Debug, Default)] + struct OverlapProbe { + in_flight: AtomicUsize, + max_in_flight: AtomicUsize, + } + impl SecurityKeyProvider for OverlapProbe { + fn sign( + &self, + request: &SkSignRequest<'_>, + _: Option<&str>, + ) -> Result { + let now = self.in_flight.fetch_add(1, Ordering::SeqCst) + 1; + self.max_in_flight.fetch_max(now, Ordering::SeqCst); + std::thread::sleep(std::time::Duration::from_millis(30)); + self.in_flight.fetch_sub(1, Ordering::SeqCst); + Ok(SkSignResponse { + flags: request.flags, + counter: 1, + sig_r: Zeroizing::new(vec![0x5a; 64]), + sig_s: Zeroizing::new(Vec::new()), + }) + } + fn describe(&self) -> String { + "overlap-probe".to_string() + } + } + + let provider = Arc::new(OverlapProbe::default()); + let key = sk_ed25519_key(0x00); + let tasks = (0..4) + .map(|_| { + let identity = + SecurityKeyIdentity::from_private_key(&key, Path::new("/k")).unwrap(); + let signer = SecurityKeySigner::new(provider.clone(), identity, false); + tokio::spawn(async move { signer.sign_blob(vec![1, 2, 3]).await }) + }) + .collect::>(); + for task in tasks { + task.await.unwrap().unwrap(); + } + assert_eq!(provider.max_in_flight.load(Ordering::SeqCst), 1); + } + #[tokio::test] async fn disabled_and_internal_providers_are_skipped_before_offering_the_key() { let path = Path::new("/home/u/.ssh/id_ed25519_sk"); From 7ca198dbb4095242a7bde382a3c7543ee2a3c9e1 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 22 Oct 2026 11:40:53 +0900 Subject: [PATCH 4/7] fix(ssh): queue security-key signers on an async lock PROVIDER_CALL_LOCK was a std mutex taken inside each spawn_blocking task. In multi-host mode every host queued behind a touch pinned its own blocking thread, so with --parallel above tokio's blocking-pool limit the pool filled up and tokio::fs and DNS work for every connection, including ones without security keys, stalled until the touches drained. A connection that hit ConnectTimeout while queued also kept its blocking task, which later called sk_sign anyway and asked the user to touch the key for a connection that no longer existed, holding the lock and pushing the remaining hosts toward their own timeouts. The lock is now a FIFO tokio mutex acquired before spawn_blocking; the owned guard moves into the blocking task, so a dropped future still cannot release it while the provider runs. A queued waiter that is cancelled leaves the queue without reaching the provider. The touch notice is printed once the identity holds the provider, on every attempt as identity_sign does, so it matches the authenticator that is actually waiting. Two new tests fail with the previous lock: a cancelled queued signer must never reach the provider, and queued signers must leave a two-thread blocking pool free for unrelated spawn_blocking work. Refs #328 --- src/ssh/tokio_client/security_key_signer.rs | 187 ++++++++++++++++++-- 1 file changed, 172 insertions(+), 15 deletions(-) diff --git a/src/ssh/tokio_client/security_key_signer.rs b/src/ssh/tokio_client/security_key_signer.rs index a49842afc..188efbd68 100644 --- a/src/ssh/tokio_client/security_key_signer.rs +++ b/src/ssh/tokio_client/security_key_signer.rs @@ -28,7 +28,7 @@ use std::future::Future; use std::io::IsTerminal; use std::path::{Path, PathBuf}; -use std::sync::{Arc, Mutex}; +use std::sync::{Arc, LazyLock, Mutex}; use tokio::sync::Notify; @@ -128,10 +128,19 @@ pub(super) async fn open_security_key_provider( /// Serializes every provider call in the process. sk-api makes no /// thread-safety promise and OpenSSH never calls a provider concurrently /// (each signature runs in its own `ssh-sk-helper`), while bssh may -/// authenticate to many hosts at once. The lock is taken inside the blocking -/// task, so a dropped authentication future cannot release it while the -/// provider is still running. -static PROVIDER_CALL_LOCK: Mutex<()> = Mutex::new(()); +/// authenticate to many hosts at once. +/// +/// The lock is an async FIFO mutex acquired before the blocking task is +/// spawned, and the owned guard moves into that task. Waiting for a touch on +/// one host therefore occupies a single blocking thread rather than one per +/// queued host, which would otherwise exhaust tokio's blocking pool and stall +/// `tokio::fs` and DNS for every connection. A connection that times out +/// while queued leaves the queue without ever reaching the provider, so the +/// user is never asked to touch the key for a dead connection. Once the call +/// has started, the guard lives in the blocking task, so a dropped +/// authentication future cannot release it while the provider is running. +static PROVIDER_CALL_LOCK: LazyLock>> = + LazyLock::new(|| Arc::new(tokio::sync::Mutex::new(()))); /// Records a signature the signer declined to produce. #[derive(Debug, Default)] @@ -257,13 +266,6 @@ impl SecurityKeySigner { }; let notify = self.interactive && self.identity.wants_user_presence(); let notify = notify && std::io::stderr().is_terminal(); - if notify { - crate::diagnosticln!( - "Confirm user presence for key {} {}", - self.identity.algorithm.ssh_name(), - self.identity.fingerprint - ); - } let data = Arc::new(data); let mut pin: Option> = None; let mut retried = false; @@ -272,7 +274,19 @@ impl SecurityKeySigner { let identity = Arc::clone(&self.identity); let data = Arc::clone(&data); let attempt_pin = pin.clone(); + let serialized = Arc::clone(&PROVIDER_CALL_LOCK).lock_owned().await; + // Announced only once this identity holds the provider, so the + // notice matches the authenticator that is actually waiting + // (`identity_sign` notifies again on the PIN retry as well). + if notify { + crate::diagnosticln!( + "Confirm user presence for key {} {}", + self.identity.algorithm.ssh_name(), + self.identity.fingerprint + ); + } let result = tokio::task::spawn_blocking(move || { + let _serialized = serialized; let request = SkSignRequest { algorithm: identity.algorithm, data: &data, @@ -280,9 +294,6 @@ impl SecurityKeySigner { key_handle: &identity.key_handle, flags: identity.flags, }; - let _serialized = PROVIDER_CALL_LOCK - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); provider.sign(&request, attempt_pin.as_deref().map(String::as_str)) }) .await @@ -563,6 +574,152 @@ mod tests { assert_eq!(provider.max_in_flight.load(Ordering::SeqCst), 1); } + /// A provider that blocks inside `sign` until released, standing in for + /// an authenticator that waits for a touch. + #[derive(Debug, Default)] + struct GatedProvider { + calls: std::sync::atomic::AtomicUsize, + open: Mutex, + opened: std::sync::Condvar, + } + + impl GatedProvider { + fn calls(&self) -> usize { + self.calls.load(std::sync::atomic::Ordering::SeqCst) + } + + fn release(&self) { + *self.open.lock().unwrap_or_else(|e| e.into_inner()) = true; + self.opened.notify_all(); + } + + async fn wait_for_calls(&self, expected: usize) { + let deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(10); + while self.calls() < expected { + assert!( + tokio::time::Instant::now() < deadline, + "provider never reached {expected} calls" + ); + tokio::time::sleep(std::time::Duration::from_millis(5)).await; + } + } + } + + impl SecurityKeyProvider for GatedProvider { + fn sign( + &self, + request: &SkSignRequest<'_>, + _: Option<&str>, + ) -> Result { + self.calls.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + let mut open = self.open.lock().unwrap_or_else(|e| e.into_inner()); + while !*open { + open = self.opened.wait(open).unwrap_or_else(|e| e.into_inner()); + } + Ok(SkSignResponse { + flags: request.flags, + counter: 1, + sig_r: Zeroizing::new(vec![0x5a; 64]), + sig_s: Zeroizing::new(Vec::new()), + }) + } + + fn describe(&self) -> String { + "gated".to_string() + } + } + + /// Releases the gate even when an assertion fails, so a blocked provider + /// thread never outlives the test. + struct ReleaseOnDrop(Arc); + + impl Drop for ReleaseOnDrop { + fn drop(&mut self) { + self.0.release(); + } + } + + fn gated_signer(provider: &Arc) -> SecurityKeySigner { + let key = sk_ed25519_key(0x00); + let identity = SecurityKeyIdentity::from_private_key(&key, Path::new("/k")).unwrap(); + SecurityKeySigner::new( + Arc::clone(provider) as Arc, + identity, + false, + ) + } + + /// A connection that gives up (ConnectTimeout) while another host holds + /// the authenticator must leave the queue: it must never reach the + /// provider afterwards and ask for a touch nobody is waiting for. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_cancelled_queued_signature_never_reaches_the_provider() { + let provider = Arc::new(GatedProvider::default()); + let _release = ReleaseOnDrop(Arc::clone(&provider)); + + let holder = gated_signer(&provider); + let holder = tokio::spawn(async move { holder.sign_blob(vec![1]).await }); + provider.wait_for_calls(1).await; + + let queued = gated_signer(&provider); + let queued = tokio::spawn(async move { queued.sign_blob(vec![2]).await }); + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + queued.abort(); + assert!(queued.await.unwrap_err().is_cancelled()); + + provider.release(); + holder.await.unwrap().unwrap(); + tokio::time::sleep(std::time::Duration::from_millis(300)).await; + assert_eq!( + provider.calls(), + 1, + "the cancelled signature was still sent to the provider" + ); + } + + /// Hosts queued behind a touch must not each pin a blocking thread: with + /// a small blocking pool, unrelated `spawn_blocking` work (tokio::fs, DNS) + /// still has to run while the authenticator waits. + #[test] + fn queued_signatures_do_not_occupy_blocking_threads() { + let runtime = tokio::runtime::Builder::new_multi_thread() + .worker_threads(2) + .max_blocking_threads(2) + .enable_all() + .build() + .unwrap(); + let provider = Arc::new(GatedProvider::default()); + let release = ReleaseOnDrop(Arc::clone(&provider)); + runtime.block_on(async { + let signers = (0..4) + .map(|index| { + let signer = gated_signer(&provider); + tokio::spawn(async move { signer.sign_blob(vec![index]).await }) + }) + .collect::>(); + provider.wait_for_calls(1).await; + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + + let unrelated = tokio::time::timeout( + std::time::Duration::from_secs(3), + tokio::task::spawn_blocking(|| 42), + ) + .await; + provider.release(); + assert_eq!( + unrelated + .expect("blocking pool exhausted by queued security-key signers") + .unwrap(), + 42 + ); + for signer in signers { + signer.await.unwrap().unwrap(); + } + }); + drop(release); + drop(runtime); + } + #[tokio::test] async fn disabled_and_internal_providers_are_skipped_before_offering_the_key() { let path = Path::new("/home/u/.ssh/id_ed25519_sk"); From 04222bc13d5de97702446a750c9567a318f7911a Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 22 Oct 2026 12:03:20 +0900 Subject: [PATCH 5/7] fix(ssh): refuse host keys that do not match the negotiated type russh 0.64.1 verifies the exchange signature with whatever key the server sends, and check_server_key never compared it with the negotiated host-key algorithm, so a HostKeyAlgorithms restriction (including one that excludes sk-*) did not limit which trusted key type a server could use. OpenSSH's kex_verify_host_key rejects this with SSH_ERR_KEY_TYPE_MISMATCH. kex_done now records Names::key, and check_server_key refuses a key whose type differs before any known_hosts or StrictHostKeyChecking decision, with a new HostKeyTypeMismatch error. Signature names map to their key type (rsa-sha2-256/512 select RSA), ECDSA curves must match, and sk types match only themselves. russh records only the plain algorithm and keeps the certificate flag private, but it parses the blob as a Certificate exactly when a certificate was negotiated, so the certificate half comes from that variant and certificate key data in the plain-key slot is refused. A key received before kex_done fails closed. The existing policy body moved to verify_server_key, which the policy tests now call directly. Tests cover RSA under every rsa-sha2 name, Ed25519 under ECDSA, curve mismatch, certificate versus plain in both directions, sk types, and the handler path; each fails with the check reverted. Refs #328 --- docs/openssh-compatibility-status.md | 2 +- src/ssh/tokio_client/connection.rs | 313 ++++++++++++------ src/ssh/tokio_client/error.rs | 11 + src/ssh/tokio_client/host_key_type.rs | 373 ++++++++++++++++++++++ src/ssh/tokio_client/host_verification.rs | 55 ++-- src/ssh/tokio_client/mod.rs | 1 + 6 files changed, 623 insertions(+), 132 deletions(-) create mode 100644 src/ssh/tokio_client/host_key_type.rs diff --git a/docs/openssh-compatibility-status.md b/docs/openssh-compatibility-status.md index 44c3dd88a..e181cad3d 100644 --- a/docs/openssh-compatibility-status.md +++ b/docs/openssh-compatibility-status.md @@ -35,7 +35,7 @@ The implemented control socket is bssh's private framed-JSON protocol. Its passe Unix-domain forwarding (`-L`/`-R` with a socket path on either side, including `-O forward`/`-O cancel`) never unlinks an existing path before binding and refuses to serve a bound path that is a symlink or not a socket; the listener socket is owner-only (mode 0600) without any mode change through the path name: on Linux the unbound socket is narrowed before `bind(2)` so the node is created 0600; elsewhere it is bound in a private `mkdtemp` directory beside the path, narrowed there with `fchmodat(AT_SYMLINK_NOFOLLOW)`, and moved into place without replacing anything (`renamex_np(RENAME_EXCL)` on macOS, `link(2)` then `unlink(2)` on other BSDs), so the node never appears at the path with a wider mode. Only a socket node is removed on shutdown. `StreamLocalBindUnlink` and `StreamLocalBindMask` are still parsed without a runtime consumer; the OpenSSH defaults (`no`, `0177`) are what the implementation applies. -Host certificates are accepted only after full validation: a `@cert-authority` line (in a known_hosts file or in the `KnownHostsCommand` output, which receives the certificate as `%K`/`%t` and runs once per check) naming the host (or `HostKeyAlias`) with the signing CA key, host certificate type, a principal matching the host name (principals may be case-sensitive wildcard patterns, as in OpenSSH, and are read only after the CA signature verifies), no critical options, a CA signature algorithm permitted by `CASignatureAlgorithms`, the validity window, a verified signature, and no revocation by `@revoked` lines in either source or by any `RevokedHostKeys` file (plain key lists; a KRL or unreadable file anywhere in the list fails closed). On a non-default port a CA line for the bare host name also matches, as OpenSSH's portless retry does. Without a matching CA the certified key is checked as a plain host key, but only if `HostKeyAlgorithms` permits its plain type, so a certificate-only list never falls back to known_hosts or first-use recording; a pinned key (`ServerCheckMethod::PublicKey`/`PublicKeyFile`) never consults CA lines; with a matching CA any other validation failure refuses the connection instead of downgrading, which is stricter than OpenSSH. `HostKeyAlgorithms` accepts certificate names and advertises the certificate form of every default algorithm; russh always sends certificate names ahead of plain names, so a mixed list cannot interleave them, and a certificate-only list yields no plain algorithm at all. `@cert-authority` host fields follow OpenSSH matching, including hashed (`|1|`) entries and `!pattern` negations that exclude a host from the CA's scope. A `RevokedHostKeys` path that cannot be expanded refuses every host key instead of disabling revocation, and `RevokedHostKeys` applies to certificates under `StrictHostKeyChecking=no` as it does to plain keys. +Host certificates are accepted only after full validation: a `@cert-authority` line (in a known_hosts file or in the `KnownHostsCommand` output, which receives the certificate as `%K`/`%t` and runs once per check) naming the host (or `HostKeyAlias`) with the signing CA key, host certificate type, a principal matching the host name (principals may be case-sensitive wildcard patterns, as in OpenSSH, and are read only after the CA signature verifies), no critical options, a CA signature algorithm permitted by `CASignatureAlgorithms`, the validity window, a verified signature, and no revocation by `@revoked` lines in either source or by any `RevokedHostKeys` file (plain key lists; a KRL or unreadable file anywhere in the list fails closed). On a non-default port a CA line for the bare host name also matches, as OpenSSH's portless retry does. Without a matching CA the certified key is checked as a plain host key, but only if `HostKeyAlgorithms` permits its plain type, so a certificate-only list never falls back to known_hosts or first-use recording; a pinned key (`ServerCheckMethod::PublicKey`/`PublicKeyFile`) never consults CA lines; with a matching CA any other validation failure refuses the connection instead of downgrading, which is stricter than OpenSSH. `HostKeyAlgorithms` accepts certificate names and advertises the certificate form of every default algorithm; russh always sends certificate names ahead of plain names, so a mixed list cannot interleave them, and a certificate-only list yields no plain algorithm at all. The host key the server presents must have the type the key exchange negotiated, as `kex_verify_host_key` requires (`SSH_ERR_KEY_TYPE_MISMATCH`): `rsa-sha2-256` and `rsa-sha2-512` select an RSA key, a `*-cert-v01@openssh.com` name selects a certificate for that key type, and ECDSA curves must match, also for certificates. A mismatch fails the connection with `host key type ... does not match the negotiated host key algorithm ...` before any known_hosts or `StrictHostKeyChecking` decision; russh 0.64.1 verifies the exchange signature with whatever key arrives, so without this check a `HostKeyAlgorithms` restriction would not limit which trusted key type a server could use. russh does not hand the host key of a re-key to the client, so this check, like the known_hosts check, covers the initial key exchange only. `@cert-authority` host fields follow OpenSSH matching, including hashed (`|1|`) entries and `!pattern` negations that exclude a host from the CA's scope. A `RevokedHostKeys` path that cannot be expanded refuses every host key instead of disabling revocation, and `RevokedHostKeys` applies to certificates under `StrictHostKeyChecking=no` as it does to plain keys. ## Security keys diff --git a/src/ssh/tokio_client/connection.rs b/src/ssh/tokio_client/connection.rs index 9482d7e0f..76cb9ceda 100644 --- a/src/ssh/tokio_client/connection.rs +++ b/src/ssh/tokio_client/connection.rs @@ -2824,6 +2824,9 @@ pub struct ClientHandler { suppress_auth_banner: bool, emit_kex_diagnostics: bool, connection_info: ConnectionInfoState, + /// Host-key algorithm of the latest key exchange (`Names::key`), which + /// `check_server_key` compares the server's key against. + negotiated_host_key: Option, } impl ClientHandler { @@ -2860,6 +2863,7 @@ impl ClientHandler { suppress_auth_banner: false, emit_kex_diagnostics: false, connection_info: ConnectionInfoState::new(host), + negotiated_host_key: None, } } @@ -3580,6 +3584,7 @@ impl Handler for ClientHandler { _session: &mut russh::client::Session, ) -> impl std::future::Future> + Send { self.connection_info.update_names(names); + self.negotiated_host_key = Some(names.key.clone()); if self.emit_kex_diagnostics || tracing::enabled!(tracing::Level::DEBUG) { for line in kex_diagnostic_lines(names) { crate::diagnosticln!("{line}"); @@ -3740,6 +3745,111 @@ impl Handler for ClientHandler { &mut self, server_key: &russh::keys::PublicKeyOrCertificate, ) -> Result { + super::host_key_type::ensure_recorded_host_key_type( + self.negotiated_host_key.as_ref(), + server_key, + )?; + self.verify_server_key(server_key).await + } + + fn openssh_ext_host_keys_announced( + &mut self, + keys: Vec, + _session: &mut russh::client::Session, + ) -> impl std::future::Future> + Send { + match self.policy.update_host_keys { + UpdateHostKeysPolicy::No => return std::future::ready(Ok(())), + UpdateHostKeysPolicy::Ask => { + tracing::warn!( + "UpdateHostKeys=ask requires a controlling terminal; bssh is non-interactive at the host-key callback and refuses the update" + ); + return std::future::ready(Ok(())); + } + UpdateHostKeysPolicy::Yes => {} + } + let keys = keys + .into_iter() + .filter(|key| { + host_key_algorithm_permitted(&self.policy.host_key_algorithms, &key.algorithm()) + }) + .collect::>(); + if !self.rotation_allowed.load(Ordering::Acquire) || keys.is_empty() { + return std::future::ready(Ok(())); + } + if let Err(error) = super::hostkey_rotation::validate_announced_keys(&keys) { + tracing::warn!("Ignoring host-key announcement: {error}"); + return std::future::ready(Ok(())); + } + if self + .proof_in_flight + .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) + .is_err() + { + tracing::warn!("Ignoring repeated host-key announcement while proof is pending"); + return std::future::ready(Ok(())); + } + + let mut method = &self.server_check; + let mut host_key_alias = None; + while let ServerCheckMethod::HostKeyAlias { + alias, + method: inner, + } = method + { + host_key_alias.get_or_insert(alias.as_str()); + method = inner; + } + let hostname = host_key_alias.unwrap_or(&self.hostname).to_string(); + let port = if host_key_alias.is_some() { + 22 + } else { + self.host.port() + }; + let paths = self.rotation_paths(method); + if paths.is_empty() { + self.proof_in_flight.store(false, Ordering::Release); + tracing::debug!( + "UpdateHostKeys is enabled but no writable user known_hosts file is configured" + ); + return std::future::ready(Ok(())); + } + let mut identities = vec![(hostname.clone(), port)]; + if let Some(address) = check_host_ip_address(&self.policy, self.host, host_key_alias) { + identities.push((address, self.host.port())); + } + let current_key = match self.verified_server_key.lock() { + Ok(guard) => guard.clone(), + Err(_) => { + self.proof_in_flight.store(false, Ordering::Release); + return std::future::ready(Err(super::Error::ServerCheckFailed)); + } + }; + let Some(current_key) = current_key else { + self.proof_in_flight.store(false, Ordering::Release); + return std::future::ready(Ok(())); + }; + + self.hostkey_rotation.submit(PendingHostkeyProof { + hostname, + keys, + known_hosts_paths: paths, + identities, + current_key, + hash_known_hosts: self.policy.hash_known_hosts, + proof_in_flight: Arc::clone(&self.proof_in_flight), + }); + std::future::ready(Ok(())) + } +} + +impl ClientHandler { + /// Host-key policy for a key whose type already matches the negotiated + /// algorithm: CA lines, known_hosts, KnownHostsCommand, SSHFP, revocation, + /// and the StrictHostKeyChecking decision. + pub(super) async fn verify_server_key( + &mut self, + server_key: &russh::keys::PublicKeyOrCertificate, + ) -> Result { let mut method = &self.server_check; let mut host_key_alias = None; while let ServerCheckMethod::HostKeyAlias { @@ -3995,95 +4105,6 @@ impl Handler for ClientHandler { .store(verification.eligible_for_rotation, Ordering::Release); Ok(true) } - - fn openssh_ext_host_keys_announced( - &mut self, - keys: Vec, - _session: &mut russh::client::Session, - ) -> impl std::future::Future> + Send { - match self.policy.update_host_keys { - UpdateHostKeysPolicy::No => return std::future::ready(Ok(())), - UpdateHostKeysPolicy::Ask => { - tracing::warn!( - "UpdateHostKeys=ask requires a controlling terminal; bssh is non-interactive at the host-key callback and refuses the update" - ); - return std::future::ready(Ok(())); - } - UpdateHostKeysPolicy::Yes => {} - } - let keys = keys - .into_iter() - .filter(|key| { - host_key_algorithm_permitted(&self.policy.host_key_algorithms, &key.algorithm()) - }) - .collect::>(); - if !self.rotation_allowed.load(Ordering::Acquire) || keys.is_empty() { - return std::future::ready(Ok(())); - } - if let Err(error) = super::hostkey_rotation::validate_announced_keys(&keys) { - tracing::warn!("Ignoring host-key announcement: {error}"); - return std::future::ready(Ok(())); - } - if self - .proof_in_flight - .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) - .is_err() - { - tracing::warn!("Ignoring repeated host-key announcement while proof is pending"); - return std::future::ready(Ok(())); - } - - let mut method = &self.server_check; - let mut host_key_alias = None; - while let ServerCheckMethod::HostKeyAlias { - alias, - method: inner, - } = method - { - host_key_alias.get_or_insert(alias.as_str()); - method = inner; - } - let hostname = host_key_alias.unwrap_or(&self.hostname).to_string(); - let port = if host_key_alias.is_some() { - 22 - } else { - self.host.port() - }; - let paths = self.rotation_paths(method); - if paths.is_empty() { - self.proof_in_flight.store(false, Ordering::Release); - tracing::debug!( - "UpdateHostKeys is enabled but no writable user known_hosts file is configured" - ); - return std::future::ready(Ok(())); - } - let mut identities = vec![(hostname.clone(), port)]; - if let Some(address) = check_host_ip_address(&self.policy, self.host, host_key_alias) { - identities.push((address, self.host.port())); - } - let current_key = match self.verified_server_key.lock() { - Ok(guard) => guard.clone(), - Err(_) => { - self.proof_in_flight.store(false, Ordering::Release); - return std::future::ready(Err(super::Error::ServerCheckFailed)); - } - }; - let Some(current_key) = current_key else { - self.proof_in_flight.store(false, Ordering::Release); - return std::future::ready(Ok(())); - }; - - self.hostkey_rotation.submit(PendingHostkeyProof { - hostname, - keys, - known_hosts_paths: paths, - identities, - current_key, - hash_known_hosts: self.policy.hash_known_hosts, - proof_in_flight: Arc::clone(&self.proof_in_flight), - }); - std::future::ready(Ok(())) - } } #[cfg(test)] @@ -4303,7 +4324,7 @@ mod known_host_policy_tests { ); let error = handler - .check_server_key(&PublicKeyOrCertificate::from(offered)) + .verify_server_key(&PublicKeyOrCertificate::from(offered)) .await .unwrap_err(); assert!(matches!( @@ -4339,7 +4360,7 @@ mod known_host_policy_tests { assert!( handler - .check_server_key(&PublicKeyOrCertificate::from(offered.clone())) + .verify_server_key(&PublicKeyOrCertificate::from(offered.clone())) .await .unwrap() ); @@ -4703,7 +4724,7 @@ mod revoked_host_keys_policy_tests { ServerCheckMethod::KnownHostsFile(known_hosts.to_str().unwrap().to_string()), policy, ) - .check_server_key(&russh::keys::PublicKeyOrCertificate::PublicKey { + .verify_server_key(&russh::keys::PublicKeyOrCertificate::PublicKey { key: key.public_key().clone(), hash_alg: None, }) @@ -4836,7 +4857,7 @@ mod host_certificate_policy_tests { ServerCheckMethod::AcceptNewInMemory, ] { let result = handler(method.clone(), certificate_only.clone()) - .check_server_key(&cert) + .verify_server_key(&cert) .await; assert!( matches!(result, Err(super::super::Error::ServerCheckFailed)), @@ -4845,7 +4866,7 @@ mod host_certificate_policy_tests { } let result = handler(ServerCheckMethod::KnownHostsFile(known_hosts), policy()) - .check_server_key(&cert) + .verify_server_key(&cert) .await; assert!( matches!(result, Ok(true)), @@ -4884,14 +4905,14 @@ mod host_certificate_policy_tests { ServerCheckMethod::PublicKey(base64(&pinned)), with_ca.clone(), ) - .check_server_key(&cert) + .verify_server_key(&cert) .await; assert!( matches!(result, Ok(false)), "a CA-signed certificate must not satisfy a pin on another key, got {result:?}" ); let result = handler(ServerCheckMethod::PublicKey(base64(&subject)), with_ca) - .check_server_key(&cert) + .verify_server_key(&cert) .await; assert!( matches!(result, Ok(true)), @@ -4914,14 +4935,14 @@ mod host_certificate_policy_tests { let mut revoking = policy(); revoking.revoked_host_keys = RevokedHostKeysPolicy::Files(vec![revoked]); let result = handler(ServerCheckMethod::NoCheck, revoking.clone()) - .check_server_key(&cert) + .verify_server_key(&cert) .await; assert!( matches!(result, Err(super::super::Error::HostKeyRevoked { .. })), "a revoked certified key is refused under NoCheck, got {result:?}" ); let result = handler(ServerCheckMethod::NoCheck, policy()) - .check_server_key(&cert) + .verify_server_key(&cert) .await; assert!(matches!(result, Ok(true)), "{result:?}"); } @@ -5006,7 +5027,7 @@ mod host_certificate_policy_tests { let (policy, method, log) = command_policy(&dir, &[openssh_line("@cert-authority *.example.com", &ca)]); let result = handler(method, policy.clone()) - .check_server_key(&cert) + .verify_server_key(&cert) .await; assert!( matches!(result, Ok(true)), @@ -5031,7 +5052,7 @@ mod host_certificate_policy_tests { .unwrap() .to_string(); let result = handler(ServerCheckMethod::PublicKey(pinned), policy) - .check_server_key(&cert) + .verify_server_key(&cert) .await; assert!(matches!(result, Ok(false)), "{result:?}"); } @@ -5052,7 +5073,7 @@ mod host_certificate_policy_tests { openssh_line("node1.example.com", &subject), ], ); - let result = handler(method, policy).check_server_key(&cert).await; + let result = handler(method, policy).verify_server_key(&cert).await; assert!( matches!(result, Err(super::super::Error::ServerCheckFailed)), "an invalid certificate from a command-trusted CA must be refused, got {result:?}" @@ -5072,7 +5093,7 @@ mod host_certificate_policy_tests { openssh_line("@revoked *", &ca), ], ); - let result = handler(method, policy).check_server_key(&cert).await; + let result = handler(method, policy).verify_server_key(&cert).await; assert!( matches!(result, Err(super::super::Error::HostKeyRevoked { .. })), "a @revoked CA in the command output must refuse the certificate, got {result:?}" @@ -5089,7 +5110,7 @@ mod host_certificate_policy_tests { let dir = tempfile::tempdir().unwrap(); let (policy, method, log) = command_policy(&dir, &[openssh_line("node1.example.com", &subject)]); - let result = handler(method, policy).check_server_key(&cert).await; + let result = handler(method, policy).verify_server_key(&cert).await; assert!( matches!(result, Ok(true)), "the downgraded plain key matches the command output, got {result:?}" @@ -5102,3 +5123,89 @@ mod host_certificate_policy_tests { ); } } + +#[cfg(test)] +mod negotiated_host_key_type_tests { + use super::*; + use russh::keys::ssh_key::EcdsaCurve; + use russh::keys::ssh_key::certificate::{Builder, CertType}; + use russh::keys::{Algorithm, PrivateKey, PublicKeyOrCertificate}; + + fn handler(negotiated: Option) -> ClientHandler { + let mut handler = ClientHandler::new_with_policy( + "node1.example.com".into(), + "127.0.0.1:22".parse().unwrap(), + ServerCheckMethod::NoCheck, + KnownHostRuntimePolicy::unconfigured("user", "node1.example.com", 22), + ); + handler.negotiated_host_key = negotiated; + handler + } + + fn ed25519() -> PrivateKey { + PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap() + } + + /// Even with StrictHostKeyChecking off, a key of a type the exchange did + /// not negotiate is refused, as `kex_verify_host_key` refuses it before + /// any host-key policy runs. + #[tokio::test] + async fn check_server_key_refuses_a_key_of_another_type_than_negotiated() { + let offered = PublicKeyOrCertificate::from(ed25519().public_key().clone()); + let p256 = Algorithm::Ecdsa { + curve: EcdsaCurve::NistP256, + }; + let error = handler(Some(p256)) + .check_server_key(&offered) + .await + .unwrap_err(); + assert!( + matches!( + &error, + super::super::Error::HostKeyTypeMismatch { negotiated, received } + if negotiated == "ecdsa-sha2-nistp256" && received == "ssh-ed25519" + ), + "{error:?}" + ); + + assert!( + handler(Some(Algorithm::Ed25519)) + .check_server_key(&offered) + .await + .unwrap() + ); + assert!(handler(None).check_server_key(&offered).await.is_err()); + } + + #[tokio::test] + async fn check_server_key_refuses_a_certificate_for_another_key_type() { + let mut builder = + Builder::new_with_random_nonce(&mut rand::rng(), ed25519().public_key(), 0, u64::MAX) + .unwrap(); + builder.cert_type(CertType::Host).unwrap(); + builder.valid_principal("node1.example.com").unwrap(); + let certificate = PublicKeyOrCertificate::Certificate(builder.sign(&ed25519()).unwrap()); + let rsa = Algorithm::Rsa { + hash: Some(russh::keys::HashAlg::Sha512), + }; + let error = handler(Some(rsa)) + .check_server_key(&certificate) + .await + .unwrap_err(); + assert!( + matches!( + &error, + super::super::Error::HostKeyTypeMismatch { negotiated, received } + if negotiated == "rsa-sha2-512-cert-v01@openssh.com" + && received == "ssh-ed25519-cert-v01@openssh.com" + ), + "{error:?}" + ); + assert!( + handler(Some(Algorithm::Ed25519)) + .check_server_key(&certificate) + .await + .unwrap() + ); + } +} diff --git a/src/ssh/tokio_client/error.rs b/src/ssh/tokio_client/error.rs index 6ff25f027..2612c554e 100644 --- a/src/ssh/tokio_client/error.rs +++ b/src/ssh/tokio_client/error.rs @@ -190,6 +190,16 @@ pub enum Error { CommandDidntExit, #[error("Host key verification failed")] ServerCheckFailed, + /// The server's host key (or certified key) does not have the type the + /// key exchange negotiated; OpenSSH's `kex_verify_host_key` fails the + /// same case with `SSH_ERR_KEY_TYPE_MISMATCH`. + #[error( + "Host key verification failed: host key type {received} does not match the negotiated host key algorithm {negotiated}" + )] + HostKeyTypeMismatch { + negotiated: String, + received: String, + }, #[error("KnownHostsCommand failed: {reason}")] KnownHostsCommandFailed { reason: String }, /// `port` is not interpolated into the `Display` text, but carrying it is @@ -311,6 +321,7 @@ impl Error { | Self::NoAddressForFamily { .. } | Self::CommandDidntExit | Self::ServerCheckFailed + | Self::HostKeyTypeMismatch { .. } | Self::KnownHostsCommandFailed { .. } | Self::HostKeyChanged { .. } | Self::HostKeyRevoked { .. } diff --git a/src/ssh/tokio_client/host_key_type.rs b/src/ssh/tokio_client/host_key_type.rs new file mode 100644 index 000000000..5edb2252e --- /dev/null +++ b/src/ssh/tokio_client/host_key_type.rs @@ -0,0 +1,373 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! The server's host key must have the type the key exchange negotiated. +//! +//! OpenSSH's `kex_verify_host_key` (`kex.c`) rejects a host key whose type +//! differs from the one `choose_hostkeyalg` derived from the negotiated name +//! (`sshkey_type_from_name`), and, for plain ECDSA, whose curve differs, with +//! `SSH_ERR_KEY_TYPE_MISMATCH`. Signature-algorithm names map to their key +//! type: `rsa-sha2-256` and `rsa-sha2-512` select an RSA key, and +//! `rsa-sha2-512-cert-v01@openssh.com` an RSA certificate. russh 0.64.1 +//! verifies the exchange signature with whatever key the server sends, so +//! without this check a `HostKeyAlgorithms` restriction would not limit which +//! trusted key type the server may use. +//! +//! russh records the negotiated algorithm in [`russh::Names::key`] as the +//! plain key algorithm, also when a certificate was negotiated; whether a +//! certificate was negotiated is private to russh. It does, however, decide +//! from that flag how the server's blob is parsed (`client/kex.rs`): it is +//! handed to `check_server_key` as [`PublicKeyOrCertificate::Certificate`] +//! exactly when a certificate algorithm was negotiated. The certificate half +//! of the negotiated type is therefore taken from that variant, and a plain +//! key slot that carries certificate key data is refused. + +use russh::keys::{Algorithm, PublicKeyOrCertificate}; + +/// A host key type as `sshkey_type_from_name` sees it: the key algorithm +/// and whether it is a certificate. Two types are equal when they name the +/// same kind of key; the RSA signature hash is kept only for messages. +#[derive(Debug, Clone)] +pub(crate) struct HostKeyType { + algorithm: Algorithm, + certificate: bool, +} + +impl PartialEq for HostKeyType { + fn eq(&self, other: &Self) -> bool { + self.certificate == other.certificate + && key_type_of(&self.algorithm) == key_type_of(&other.algorithm) + } +} + +impl Eq for HostKeyType {} + +impl HostKeyType { + /// The type a negotiated host-key algorithm selects. `algorithm` is the + /// plain form russh records (`rsa-sha2-512` for + /// `rsa-sha2-512-cert-v01@openssh.com`). + #[must_use] + pub(crate) fn negotiated(algorithm: &Algorithm, certificate: bool) -> Self { + Self { + algorithm: algorithm.clone(), + certificate, + } + } + + /// The type of the key or certificate the server presented. + #[must_use] + pub(crate) fn presented(server_key: &PublicKeyOrCertificate) -> Self { + match server_key { + PublicKeyOrCertificate::Certificate(certificate) => Self { + algorithm: certificate.public_key().algorithm(), + certificate: true, + }, + PublicKeyOrCertificate::PublicKey { key, .. } => Self { + algorithm: key.algorithm(), + certificate: key.key_data().is_certificate(), + }, + } + } + + /// The SSH name: `rsa-sha2-512-cert-v01@openssh.com` for a negotiated + /// RSA certificate, `ssh-rsa` for a presented RSA key. + fn name(&self) -> String { + if self.certificate { + self.algorithm.to_certificate_type() + } else { + self.algorithm.as_str().to_string() + } + } +} + +/// Collapse an algorithm to the key type it names: the RSA signature hash is +/// a property of the signature, not of the key (`sshkey_type_plain`). +fn key_type_of(algorithm: &Algorithm) -> Algorithm { + match algorithm { + Algorithm::Rsa { .. } => Algorithm::Rsa { hash: None }, + other => other.clone(), + } +} + +/// The negotiated host-key algorithm for `check_server_key`, built from +/// what `kex_done` recorded and the form russh parsed the key in. +#[must_use] +fn negotiated_for(recorded: &Algorithm, server_key: &PublicKeyOrCertificate) -> HostKeyType { + HostKeyType::negotiated( + recorded, + matches!(server_key, PublicKeyOrCertificate::Certificate(_)), + ) +} + +/// [`ensure_negotiated_host_key_type`] against the algorithm `kex_done` +/// recorded. russh calls `kex_done` with the negotiated names before it hands +/// the host key to `check_server_key` (`client/mod.rs`), so a missing record +/// means that order changed, and the key is refused rather than trusted. +pub(crate) fn ensure_recorded_host_key_type( + recorded: Option<&Algorithm>, + server_key: &PublicKeyOrCertificate, +) -> Result<(), super::Error> { + match recorded { + Some(recorded) => { + ensure_negotiated_host_key_type(&negotiated_for(recorded, server_key), server_key) + } + None => Err(super::Error::HostKeyTypeMismatch { + negotiated: "(none negotiated)".to_string(), + received: HostKeyType::presented(server_key).name(), + }), + } +} + +/// Refuse a host key whose type differs from the negotiated one, failing +/// closed. ECDSA curves are compared for certificates as well as plain keys: +/// each curve has its own certificate algorithm name, so a server that +/// answers `ecdsa-sha2-nistp256-cert-v01@openssh.com` with a P-384 +/// certificate is not presenting what was negotiated. +pub(crate) fn ensure_negotiated_host_key_type( + negotiated: &HostKeyType, + server_key: &PublicKeyOrCertificate, +) -> Result<(), super::Error> { + let presented = HostKeyType::presented(server_key); + if presented == *negotiated { + return Ok(()); + } + tracing::debug!( + "host key type {} does not match negotiated host key algorithm {}", + presented.name(), + negotiated.name() + ); + Err(super::Error::HostKeyTypeMismatch { + negotiated: negotiated.name(), + received: presented.name(), + }) +} + +#[cfg(test)] +mod tests { + use super::*; + use russh::keys::ssh_key::certificate::{Builder, CertType}; + use russh::keys::ssh_key::public::{ + EcdsaPublicKey, Ed25519PublicKey, KeyData, SkEcdsaSha2NistP256, SkEd25519, + }; + use russh::keys::ssh_key::{EcdsaCurve, HashAlg, PrivateKey, PublicKey}; + + /// A fixed RSA public key: generating one per test is slow in debug builds. + const RSA_PUBLIC_KEY: &str = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCOpiLYq9KNhwmajZJZuZeAL3OAnbikDpkxJaka6ND9mvsuBMbit7wXsGmrMPd5X0syw6S/gKen1IDJPPZq3o7gx+Uai4z8GmtVccTDrYBD84iSYNfmffsKPKbCXUGrFf1O2X+7niNZO3/g9bKz5vC7dMcsnEZMem83Zo6I5Doj399dagWWvfuFLG/VzL0NDgA5oHD3vsmhp/vnAcOu865kvdagG9stqCNiltjxows0Tt2V8DYB8335QZHl0SAbgnu3VvUDHWYAjHo7604qFe1f3/6vr9bNWqENLtDnau9qSR3cM0GNM4/YC7PLoIf10yLcIYdwJQSN8dXS72Lrm1FZ"; + + fn key_data(algorithm: Algorithm) -> KeyData { + match algorithm { + Algorithm::Rsa { .. } => PublicKey::from_openssh(RSA_PUBLIC_KEY) + .unwrap() + .key_data() + .clone(), + Algorithm::SkEd25519 => { + KeyData::SkEd25519(SkEd25519::new(Ed25519PublicKey([9u8; 32]), "ssh:")) + } + Algorithm::SkEcdsaSha2NistP256 => { + let p256 = Algorithm::Ecdsa { + curve: EcdsaCurve::NistP256, + }; + match key_data(p256) { + KeyData::Ecdsa(EcdsaPublicKey::NistP256(point)) => { + KeyData::SkEcdsaSha2NistP256(SkEcdsaSha2NistP256::new(point, "ssh:")) + } + other => panic!("unexpected P-256 key data: {other:?}"), + } + } + other => PrivateKey::random(&mut rand::rng(), other) + .unwrap() + .public_key() + .key_data() + .clone(), + } + } + + fn plain(algorithm: Algorithm) -> PublicKeyOrCertificate { + PublicKeyOrCertificate::PublicKey { + key: PublicKey::new(key_data(algorithm), ""), + hash_alg: None, + } + } + + fn certificate(algorithm: Algorithm) -> russh::keys::Certificate { + let ca = PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); + let mut builder = + Builder::new_with_random_nonce(&mut rand::rng(), key_data(algorithm), 0, u64::MAX) + .unwrap(); + builder.cert_type(CertType::Host).unwrap(); + builder.valid_principal("host.example").unwrap(); + builder.sign(&ca).unwrap() + } + + /// Mirrors `negotiation.rs`: a plain name is parsed with + /// `Algorithm::new`, a certificate name into the plain algorithm it + /// contains, and the certificate flag is kept separately. + fn negotiated(name: &str) -> HostKeyType { + match name.strip_suffix("-cert-v01@openssh.com") { + Some(_) => { + let algorithm = match name { + "rsa-sha2-256-cert-v01@openssh.com" => Algorithm::Rsa { + hash: Some(HashAlg::Sha256), + }, + "rsa-sha2-512-cert-v01@openssh.com" => Algorithm::Rsa { + hash: Some(HashAlg::Sha512), + }, + other => Algorithm::new_certificate(other).unwrap(), + }; + HostKeyType::negotiated(&algorithm, true) + } + None => HostKeyType::negotiated(&Algorithm::new(name).unwrap(), false), + } + } + + fn check(name: &str, server_key: &PublicKeyOrCertificate) -> Result<(), super::super::Error> { + ensure_negotiated_host_key_type(&negotiated(name), server_key) + } + + #[test] + fn rsa_keys_match_every_rsa_signature_algorithm() { + let rsa = plain(Algorithm::Rsa { hash: None }); + for name in ["ssh-rsa", "rsa-sha2-256", "rsa-sha2-512"] { + check(name, &rsa).unwrap_or_else(|error| panic!("{name}: {error}")); + } + let rsa_certificate = + PublicKeyOrCertificate::Certificate(certificate(Algorithm::Rsa { hash: None })); + for name in [ + "ssh-rsa-cert-v01@openssh.com", + "rsa-sha2-256-cert-v01@openssh.com", + "rsa-sha2-512-cert-v01@openssh.com", + ] { + check(name, &rsa_certificate).unwrap_or_else(|error| panic!("{name}: {error}")); + } + } + + #[test] + fn a_key_of_another_type_is_refused() { + let ed25519 = plain(Algorithm::Ed25519); + let error = check("ecdsa-sha2-nistp256", &ed25519).unwrap_err(); + match &error { + super::super::Error::HostKeyTypeMismatch { + negotiated, + received, + } => { + assert_eq!(negotiated, "ecdsa-sha2-nistp256"); + assert_eq!(received, "ssh-ed25519"); + } + other => panic!("unexpected error: {other:?}"), + } + assert!(error.to_string().contains("does not match"), "{error}"); + assert!(check("rsa-sha2-512", &ed25519).is_err()); + assert!(check("ssh-ed25519", &plain(Algorithm::Rsa { hash: None })).is_err()); + } + + #[test] + fn ecdsa_curves_must_match_for_keys_and_certificates() { + let p384 = Algorithm::Ecdsa { + curve: EcdsaCurve::NistP384, + }; + check("ecdsa-sha2-nistp384", &plain(p384.clone())).unwrap(); + assert!(check("ecdsa-sha2-nistp256", &plain(p384.clone())).is_err()); + let certificate = PublicKeyOrCertificate::Certificate(certificate(p384)); + check("ecdsa-sha2-nistp384-cert-v01@openssh.com", &certificate).unwrap(); + assert!(check("ecdsa-sha2-nistp256-cert-v01@openssh.com", &certificate).is_err()); + } + + #[test] + fn certificates_and_plain_keys_are_not_interchangeable() { + let ed25519_certificate = certificate(Algorithm::Ed25519); + let as_certificate = PublicKeyOrCertificate::Certificate(ed25519_certificate.clone()); + check("ssh-ed25519-cert-v01@openssh.com", &as_certificate).unwrap(); + // A certificate where a plain key was negotiated, in the form russh + // would hand it over: certificate key data in the plain-key slot. + let in_plain_slot = PublicKeyOrCertificate::PublicKey { + key: PublicKey::new(KeyData::from(ed25519_certificate), ""), + hash_alg: None, + }; + let error = check("ssh-ed25519", &in_plain_slot).unwrap_err(); + assert!( + error + .to_string() + .contains("ssh-ed25519-cert-v01@openssh.com"), + "{error}" + ); + // And the reverse: a plain key where a certificate was negotiated. + let error = check( + "ssh-ed25519-cert-v01@openssh.com", + &plain(Algorithm::Ed25519), + ) + .unwrap_err(); + match error { + super::super::Error::HostKeyTypeMismatch { + negotiated, + received, + } => { + assert_eq!(negotiated, "ssh-ed25519-cert-v01@openssh.com"); + assert_eq!(received, "ssh-ed25519"); + } + other => panic!("unexpected error: {other:?}"), + } + } + + #[test] + fn security_key_types_match_only_themselves() { + let sk_ed25519 = plain(Algorithm::SkEd25519); + check("sk-ssh-ed25519@openssh.com", &sk_ed25519).unwrap(); + assert!(check("ssh-ed25519", &sk_ed25519).is_err()); + assert!(check("sk-ssh-ed25519@openssh.com", &plain(Algorithm::Ed25519)).is_err()); + + let sk_ecdsa = plain(Algorithm::SkEcdsaSha2NistP256); + check("sk-ecdsa-sha2-nistp256@openssh.com", &sk_ecdsa).unwrap(); + assert!(check("ecdsa-sha2-nistp256", &sk_ecdsa).is_err()); + assert!(check("sk-ssh-ed25519@openssh.com", &sk_ecdsa).is_err()); + + let sk_certificate = PublicKeyOrCertificate::Certificate(certificate(Algorithm::SkEd25519)); + check("sk-ssh-ed25519-cert-v01@openssh.com", &sk_certificate).unwrap(); + assert!(check("ssh-ed25519-cert-v01@openssh.com", &sk_certificate).is_err()); + assert!(check("sk-ssh-ed25519@openssh.com", &sk_certificate).is_err()); + } + + #[test] + fn a_key_received_before_negotiation_is_refused() { + let key = plain(Algorithm::Ed25519); + assert!(matches!( + ensure_recorded_host_key_type(None, &key), + Err(super::super::Error::HostKeyTypeMismatch { .. }) + )); + ensure_recorded_host_key_type(Some(&Algorithm::Ed25519), &key).unwrap(); + assert!( + ensure_recorded_host_key_type( + Some(&Algorithm::Ecdsa { + curve: EcdsaCurve::NistP256 + }), + &key + ) + .is_err() + ); + } + + #[test] + fn the_certificate_form_follows_the_variant_russh_parsed() { + let recorded = Algorithm::Rsa { + hash: Some(HashAlg::Sha512), + }; + let certificate = + PublicKeyOrCertificate::Certificate(certificate(Algorithm::Rsa { hash: None })); + assert_eq!( + negotiated_for(&recorded, &certificate), + negotiated("rsa-sha2-512-cert-v01@openssh.com") + ); + let key = plain(Algorithm::Rsa { hash: None }); + assert_eq!(negotiated_for(&recorded, &key), negotiated("rsa-sha2-512")); + } +} diff --git a/src/ssh/tokio_client/host_verification.rs b/src/ssh/tokio_client/host_verification.rs index c4ba79a5c..da0402ab5 100644 --- a/src/ssh/tokio_client/host_verification.rs +++ b/src/ssh/tokio_client/host_verification.rs @@ -1462,7 +1462,6 @@ fn print_process_pin_changed_warning(hostname: &str, port: u16, server_public_ke mod tests { use super::*; use crate::ssh::tokio_client::{ClientHandler, Error, ServerCheckMethod}; - use russh::client::Handler; use russh::keys::PrivateKey; use std::path::PathBuf; use tempfile::TempDir; @@ -2001,7 +2000,7 @@ mod tests { let mut handler = handler_for(ServerCheckMethod::KnownHostsFile(path_str)); let result = handler - .check_server_key(&host_key(revoked.public_key())) + .verify_server_key(&host_key(revoked.public_key())) .await; assert!( matches!(result, Err(Error::HostKeyRevoked { .. })), @@ -2066,7 +2065,7 @@ mod tests { "127.0.0.1:22".parse().unwrap(), ServerCheckMethod::KnownHostsFile(path_str), ); - let result = handler.check_server_key(&host_key(key.public_key())).await; + let result = handler.verify_server_key(&host_key(key.public_key())).await; assert!( matches!(result, Ok(true)), "strict mode must match an existing pin regardless of hostname casing, got {result:?}" @@ -2380,7 +2379,7 @@ mod tests { let key = generate_key(); let mut handler = handler_for(ServerCheckMethod::KnownHostsFile(path_str)); - let result = handler.check_server_key(&host_key(key.public_key())).await; + let result = handler.verify_server_key(&host_key(key.public_key())).await; assert!( matches!(result, Ok(false)), "unknown host must be rejected in strict mode, got {result:?}" @@ -2403,7 +2402,7 @@ mod tests { let mut handler = handler_for(ServerCheckMethod::KnownHostsFile(path_str.clone())); let result = handler - .check_server_key(&host_key(imposter.public_key())) + .verify_server_key(&host_key(imposter.public_key())) .await; assert!( matches!(result, Err(Error::HostKeyChanged { .. })), @@ -2413,7 +2412,7 @@ mod tests { // The original key still verifies. let mut handler = handler_for(ServerCheckMethod::KnownHostsFile(path_str)); let result = handler - .check_server_key(&host_key(original.public_key())) + .verify_server_key(&host_key(original.public_key())) .await; assert!(matches!(result, Ok(true))); } @@ -2441,7 +2440,7 @@ mod tests { let mut handler = handler_for(ServerCheckMethod::KnownHostsFile(path_str)); let result = handler - .check_server_key(&host_key(current.public_key())) + .verify_server_key(&host_key(current.public_key())) .await; assert!( matches!(result, Ok(true)), @@ -2455,7 +2454,7 @@ mod tests { let key = generate_key(); let mut handler = handler_for(ServerCheckMethod::NoCheck); - let result = handler.check_server_key(&host_key(key.public_key())).await; + let result = handler.verify_server_key(&host_key(key.public_key())).await; assert!(matches!(result, Ok(true))); assert!(!path.exists(), "NoCheck must not create a known_hosts file"); } @@ -2467,7 +2466,7 @@ mod tests { let key = generate_key(); let mut handler = handler_for(ServerCheckMethod::AcceptNewKnownHostsFile(path_str)); - let result = handler.check_server_key(&host_key(key.public_key())).await; + let result = handler.verify_server_key(&host_key(key.public_key())).await; assert!(matches!(result, Ok(true))); assert_eq!(entry_lines(&path).len(), 1); } @@ -2572,7 +2571,7 @@ mod tests { "127.0.0.1:22".parse().unwrap(), ServerCheckMethod::KnownHostsFile(path_str), ); - let result = handler.check_server_key(&host_key(key.public_key())).await; + let result = handler.verify_server_key(&host_key(key.public_key())).await; assert!( matches!(result, Err(Error::ServerCheckFailed)), "strict mode must refuse an unrecordable hostname, got {result:?}" @@ -2605,7 +2604,7 @@ mod tests { ServerCheckMethod::AcceptNewKnownHostsFile(path_str.clone()), ] { let mut handler = handler_for(check.clone()); - let result = handler.check_server_key(&cert).await; + let result = handler.verify_server_key(&cert).await; assert!( matches!(result, Ok(true)), "{check:?} must accept a certificate from a trusted CA, got {result:?}" @@ -2652,7 +2651,7 @@ mod tests { ) .unwrap(); let mut handler = handler_for(ServerCheckMethod::KnownHostsFile(path_str.clone())); - let result = handler.check_server_key(&cert).await; + let result = handler.verify_server_key(&cert).await; assert!( matches!(result, Ok(true)), "a pinned plain key accepts the downgraded certificate, got {result:?}" @@ -2660,7 +2659,7 @@ mod tests { let (_dir, _unknown_path, unknown_str) = temp_known_hosts(); let mut handler = handler_for(ServerCheckMethod::KnownHostsFile(unknown_str)); - let result = handler.check_server_key(&cert).await; + let result = handler.verify_server_key(&cert).await; assert!( matches!(result, Ok(false)), "strict checking refuses the downgraded key of an unknown host, got {result:?}" @@ -2697,7 +2696,7 @@ mod tests { ) .unwrap(); let mut handler = handler_for(ServerCheckMethod::AcceptNewKnownHostsFile(path_str)); - let result = handler.check_server_key(&cert).await; + let result = handler.verify_server_key(&cert).await; assert!( matches!(result, Err(Error::ServerCheckFailed)), "a trusted CA must not accept a certificate for another principal, got {result:?}" @@ -2713,7 +2712,7 @@ mod tests { let cert = host_certificate(&subject, &ca); let mut handler = handler_for(ServerCheckMethod::NoCheck); - let result = handler.check_server_key(&cert).await; + let result = handler.verify_server_key(&cert).await; assert!( matches!(result, Ok(true)), "NoCheck disables verification for certificates too, got {result:?}" @@ -2762,7 +2761,7 @@ mod tests { ); assert!(matches!( alias_handler - .check_server_key(&host_key(key.public_key())) + .verify_server_key(&host_key(key.public_key())) .await, Ok(true) )); @@ -2774,7 +2773,7 @@ mod tests { ); assert!(matches!( target_against_alias_store - .check_server_key(&host_key(key.public_key())) + .verify_server_key(&host_key(key.public_key())) .await, Ok(false) )); @@ -2786,7 +2785,7 @@ mod tests { ); assert!(matches!( target_handler - .check_server_key(&host_key(key.public_key())) + .verify_server_key(&host_key(key.public_key())) .await, Ok(true) )); @@ -2801,7 +2800,7 @@ mod tests { ); assert!(matches!( alias_against_target_store - .check_server_key(&host_key(key.public_key())) + .verify_server_key(&host_key(key.public_key())) .await, Ok(false) )); @@ -2824,7 +2823,7 @@ mod tests { ); assert!(matches!( - handler.check_server_key(&host_key(key.public_key())).await, + handler.verify_server_key(&host_key(key.public_key())).await, Ok(true) )); let lines = entry_lines(&path); @@ -2852,7 +2851,7 @@ mod tests { ); assert!(matches!( matching_handler - .check_server_key(&host_key(expected.public_key())) + .verify_server_key(&host_key(expected.public_key())) .await, Ok(true) )); @@ -2864,7 +2863,7 @@ mod tests { ); assert!(matches!( empty_handler - .check_server_key(&host_key(expected.public_key())) + .verify_server_key(&host_key(expected.public_key())) .await, Ok(false) )); @@ -2886,7 +2885,7 @@ mod tests { ); assert!(matches!( handler - .check_server_key(&host_key(expected.public_key())) + .verify_server_key(&host_key(expected.public_key())) .await, Ok(true) )); @@ -2897,7 +2896,7 @@ mod tests { let key = generate_key(); let mut handler = handler_for(ServerCheckMethod::KnownHostsFiles(Vec::new())); assert!(matches!( - handler.check_server_key(&host_key(key.public_key())).await, + handler.verify_server_key(&host_key(key.public_key())).await, Ok(false) )); } @@ -2914,7 +2913,7 @@ mod tests { }); assert!(matches!( - handler.check_server_key(&host_key(key.public_key())).await, + handler.verify_server_key(&host_key(key.public_key())).await, Ok(true) )); assert_eq!(entry_lines(&user_path).len(), 1); @@ -2941,7 +2940,7 @@ mod tests { ); assert!(matches!( - handler.check_server_key(&host_key(key.public_key())).await, + handler.verify_server_key(&host_key(key.public_key())).await, Ok(true) )); assert!( @@ -2965,7 +2964,7 @@ mod tests { ); assert!(matches!( first - .check_server_key(&host_key(original.public_key())) + .verify_server_key(&host_key(original.public_key())) .await, Ok(true) )); @@ -2977,7 +2976,7 @@ mod tests { ); assert!(matches!( second - .check_server_key(&host_key(changed.public_key())) + .verify_server_key(&host_key(changed.public_key())) .await, Err(Error::HostKeyChanged { .. }) )); diff --git a/src/ssh/tokio_client/mod.rs b/src/ssh/tokio_client/mod.rs index a7227a4d6..d929ad479 100644 --- a/src/ssh/tokio_client/mod.rs +++ b/src/ssh/tokio_client/mod.rs @@ -25,6 +25,7 @@ mod connection_tests; pub mod error; pub mod file_transfer; mod host_certificate; +mod host_key_type; mod hostkey_rotation; mod known_hosts_command; mod process_stdin; From 20d38c42c2d6deac5d799a94a06ff37022f1f4e1 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 22 Oct 2026 12:03:40 +0900 Subject: [PATCH 6/7] fix(ssh): harden security-key provider loading and signing Library::new opened the provider with RTLD_LAZY, so a library with an unresolved symbol loaded fine and then aborted bssh at the first sk_sign. It is now opened with RTLD_NOW | RTLD_LOCAL, as sshsk_open does with RTLD_NOW (Library::new stays on non-Unix targets). Each validated library is cached per canonical path for the process lifetime instead of being dlopened and dlclosed per identity per connection; failed loads are not cached. Canonicalizing first also means a bare file name refers to a file in the working directory, as OpenSSH's lib_contains_symbol does, rather than one on the loader search path. An empty application is refused before the provider is called, as sshsk_sign refuses it, both in the signer (before the touch notice) and in the external provider. An Ed25519 response must carry exactly 64 bytes in sig_r and nothing in sig_s. The PIN is copied into a wiped buffer only after the NUL check, so a rejected PIN no longer leaves an unwiped copy inside CString's error, SecurityKeyIdentity's Debug prints only the key handle length, and the free_response call has its SAFETY comment. The module doc and docs/openssh-compatibility-status.md record that the provider runs in process: a started sk_sign cannot be cancelled, a hung provider holds the process-wide lock, and the provider can read bssh's memory. New tests compile small C providers at test time on Linux to cover RTLD_NOW, the cache, and the FFI sign path; each new test fails with its fix reverted. Moved external.rs tests to external_tests.rs. Refs #328 --- docs/openssh-compatibility-status.md | 2 +- src/ssh/security_key/external.rs | 260 ++++++++-------- src/ssh/security_key/external_tests.rs | 314 ++++++++++++++++++++ src/ssh/security_key/mod.rs | 39 +++ src/ssh/security_key/signature.rs | 30 ++ src/ssh/tokio_client/security_key_signer.rs | 69 ++++- 6 files changed, 570 insertions(+), 144 deletions(-) create mode 100644 src/ssh/security_key/external_tests.rs diff --git a/docs/openssh-compatibility-status.md b/docs/openssh-compatibility-status.md index e181cad3d..e47a22774 100644 --- a/docs/openssh-compatibility-status.md +++ b/docs/openssh-compatibility-status.md @@ -39,7 +39,7 @@ Host certificates are accepted only after full validation: a `@cert-authority` l ## Security keys -Authenticator-hosted (`sk-ssh-ed25519@openssh.com`, `sk-ecdsa-sha2-nistp256@openssh.com`) keys follow OpenSSH 10.3p1 built without `ENABLE_SK_INTERNAL`. `SecurityKeyProvider` defaults to `$SSH_SK_PROVIDER`; a `$`-prefixed value names an environment variable, an unset variable or `none` disables `sk-*` identities (logged at debug level, as `ssh` does), and `internal` prints a diagnostic and moves on to the next identity until #331 supplies the built-in provider. An external provider is a shared library implementing `sk-api.h` version `0x000a0000` (`sk_api_version`, `sk_enroll`, `sk_sign`, `sk_load_resident_keys`); bssh loads it in process with `dlopen` and calls `sk_sign` on a blocking thread, printing `Confirm user presence for key ...` when the key requires a touch and stderr is a terminal, and prompting once for a PIN when the provider reports `SSH_SK_ERR_PIN_REQUIRED` outside `BatchMode`. OpenSSH runs the same calls in `ssh-sk-helper` to keep the middleware out of its `pledge`d client; bssh has no such sandbox, so a helper would add a private protocol without changing what runs as the user. A provider that cannot be opened (missing file, no `sk_api_version`, wrong major version) is reported before the key is offered and authentication continues; a failure inside `sk_sign` after the server has accepted the key (device absent, PIN required in `BatchMode`, provider error) prints `security key signing failed for : ...` and authentication continues with the next identity on the same connection, as after OpenSSH's `sign_and_send_pubkey` failure: the signer hands russh the request back unsigned, russh sends nothing for it, and bssh stops waiting for the reply. Provider calls are serialized process-wide, because sk-api makes no thread-safety promise and bssh may authenticate to many hosts at once. An `sk-*` identity is checked against the provider from its unencrypted public half before any passphrase prompt, and `~/.ssh/id_ed25519_sk` and `~/.ssh/id_ecdsa_sk` are among the default identities. Static musl release builds cannot `dlopen`; they build, and report that limitation when an external provider is configured. Host keys and host certificates of the `sk-*` types are verified by ssh-key's `sk` verifiers (`SHA256(application) || flags || counter || SHA256(message)`), with no flag policy on the client side, as in OpenSSH. Agent-held `sk-*` keys keep signing through the agent. `-Q key-plain`, `-Q key`, `-Q key-cert`, `-Q key-sig`, `HostKeyAlgorithms` and `PubkeyAcceptedAlgorithms` list the `sk-*` names in the reference order; the only difference from the reference `ssh -Q key-sig` and `ssh -G` is the absent `webauthn-sk-ecdsa-sha2-nistp256@openssh.com` pair. +Authenticator-hosted (`sk-ssh-ed25519@openssh.com`, `sk-ecdsa-sha2-nistp256@openssh.com`) keys follow OpenSSH 10.3p1 built without `ENABLE_SK_INTERNAL`. `SecurityKeyProvider` defaults to `$SSH_SK_PROVIDER`; a `$`-prefixed value names an environment variable, an unset variable or `none` disables `sk-*` identities (logged at debug level, as `ssh` does), and `internal` prints a diagnostic and moves on to the next identity until #331 supplies the built-in provider. An external provider is a shared library implementing `sk-api.h` version `0x000a0000` (`sk_api_version`, `sk_enroll`, `sk_sign`, `sk_load_resident_keys`); bssh loads it in process with `dlopen(RTLD_NOW | RTLD_LOCAL)`, as `sshsk_open` does with `RTLD_NOW`, so a library with an unresolved symbol is refused at load time; each library is opened once per canonical path and kept for the life of the process instead of being reopened and closed for every identity and connection, and a bare file name refers to a file relative to the working directory (OpenSSH's `lib_contains_symbol` opens the path directly) rather than one found through the loader search path. bssh calls `sk_sign` on a blocking thread, printing `Confirm user presence for key ...` when the key requires a touch and stderr is a terminal, and prompting once for a PIN when the provider reports `SSH_SK_ERR_PIN_REQUIRED` outside `BatchMode`. OpenSSH runs the same calls in `ssh-sk-helper` to keep the middleware out of its `pledge`d client; bssh has no such sandbox, so a helper would add a private protocol without changing what runs as the user. Running in process has accepted costs: an `sk_sign` call that has started cannot be cancelled, so a hung provider keeps its blocking thread and the process-wide provider lock until it returns and no other security-key signature in the process can proceed meanwhile (connections queued behind it give up at `ConnectTimeout` without reaching the provider); and the provider can read everything bssh holds in memory, including decrypted keys and passwords for other hosts. A key without an application string is refused before the provider is called, as `sshsk_sign` refuses it, and an Ed25519 response must carry exactly 64 bytes in `sig_r` and nothing in `sig_s`. A provider that cannot be opened (missing file, no `sk_api_version`, wrong major version) is reported before the key is offered and authentication continues; a failure inside `sk_sign` after the server has accepted the key (device absent, PIN required in `BatchMode`, provider error) prints `security key signing failed for : ...` and authentication continues with the next identity on the same connection, as after OpenSSH's `sign_and_send_pubkey` failure: the signer hands russh the request back unsigned, russh sends nothing for it, and bssh stops waiting for the reply. Provider calls are serialized process-wide, because sk-api makes no thread-safety promise and bssh may authenticate to many hosts at once. An `sk-*` identity is checked against the provider from its unencrypted public half before any passphrase prompt, and `~/.ssh/id_ed25519_sk` and `~/.ssh/id_ecdsa_sk` are among the default identities. Static musl release builds cannot `dlopen`; they build, and report that limitation when an external provider is configured. Host keys and host certificates of the `sk-*` types are verified by ssh-key's `sk` verifiers (`SHA256(application) || flags || counter || SHA256(message)`), with no flag policy on the client side, as in OpenSSH. Agent-held `sk-*` keys keep signing through the agent. `-Q key-plain`, `-Q key`, `-Q key-cert`, `-Q key-sig`, `HostKeyAlgorithms` and `PubkeyAcceptedAlgorithms` list the `sk-*` names in the reference order; the only difference from the reference `ssh -Q key-sig` and `ssh -G` is the absent `webauthn-sk-ecdsa-sha2-nistp256@openssh.com` pair. ## ssh_config integration diff --git a/src/ssh/security_key/external.rs b/src/ssh/security_key/external.rs index c2d30204b..8de3b593c 100644 --- a/src/ssh/security_key/external.rs +++ b/src/ssh/security_key/external.rs @@ -17,10 +17,21 @@ //! The ABI is the pinned OpenSSH 10.3 `sk-api.h` (version `0x000a0000`). //! Only `sk_sign` is called; `sk_enroll` and `sk_load_resident_keys` must //! exist (as `sshsk_open` requires) but enrollment stays with `ssh-keygen`. +//! +//! A library is opened with `RTLD_NOW | RTLD_LOCAL`, as `sshsk_open` opens it +//! with `RTLD_NOW`, so an unresolved symbol fails the load instead of +//! aborting bssh at the first `sk_sign`. Each library is opened once per +//! canonical path and kept for the life of the process: OpenSSH's helper +//! process exits after one signature, while bssh would otherwise rerun the +//! library's initializers and `dlclose` it for every identity on every +//! connection, which leaves thread-local destructors pointing into unmapped +//! code. +use std::collections::HashMap; use std::ffi::{CString, c_char, c_int, c_void}; use std::path::{Path, PathBuf}; use std::ptr; +use std::sync::{Arc, LazyLock, Mutex, PoisonError}; use libloading::{Library, Symbol}; use zeroize::Zeroizing; @@ -59,15 +70,24 @@ type SkSignFn = unsafe extern "C" fn( sign_response: *mut *mut RawSignResponse, ) -> c_int; -/// A provider library kept open for the life of the signer. -pub(super) struct ExternalProvider { - path: PathBuf, +/// A validated provider library. Instances live in [`LOADED_LIBRARIES`] and +/// are never dropped, so the library is never unmapped while `sign` exists. +struct LoadedLibrary { sign: SkSignFn, - /// Declared after `sign` so the function pointer is never used once the - /// library is unmapped; both go away together. + /// Held so the code `sign` points into stays mapped. _library: Library, } +/// Validated provider libraries by canonical path, for the process lifetime. +static LOADED_LIBRARIES: LazyLock>>> = + LazyLock::new(|| Mutex::new(HashMap::new())); + +/// An sk-api library as named by one `SecurityKeyProvider` setting. +pub(super) struct ExternalProvider { + path: PathBuf, + loaded: Arc, +} + impl std::fmt::Debug for ExternalProvider { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("ExternalProvider") @@ -77,24 +97,75 @@ impl std::fmt::Debug for ExternalProvider { } impl ExternalProvider { + #[cfg(test)] + fn _loaded_for_tests(&self) -> &Library { + &self.loaded._library + } + pub(super) fn load(path: &Path) -> Result { - // SAFETY: loading a shared object runs its initializers. The path is - // the user's own `SecurityKeyProvider` setting (or `$SSH_SK_PROVIDER`), - // the same trust OpenSSH extends when `ssh-sk-helper` dlopens it. - let library = - unsafe { Library::new(path) }.map_err(|error| SecurityKeyError::LoadFailed { - path: path.to_path_buf(), - reason: describe_load_error(&error), - })?; - let sign = bind_symbols(path, &library)?; + let loaded = load_cached(&LOADED_LIBRARIES, path, |canonical| { + let library = + open_library(canonical).map_err(|error| SecurityKeyError::LoadFailed { + path: path.to_path_buf(), + reason: describe_load_error(&error), + })?; + let sign = bind_symbols(path, &library)?; + Ok(LoadedLibrary { + sign, + _library: library, + }) + })?; Ok(Self { path: path.to_path_buf(), - sign, - _library: library, + loaded, }) } } +/// Return the entry for `path`'s canonical form, running `load` only when +/// none exists yet. A failed load is not remembered, so a provider that is +/// installed or fixed later is picked up by the next connection. +fn load_cached( + cache: &Mutex>>, + path: &Path, + load: F, +) -> Result, SecurityKeyError> +where + F: FnOnce(&Path) -> Result, +{ + // `lib_contains_symbol` opens the configured path itself before + // `sshsk_open` dlopens it, so a bare file name names a file relative to + // the working directory, never one found through the loader search path. + let canonical = std::fs::canonicalize(path).map_err(|error| SecurityKeyError::LoadFailed { + path: path.to_path_buf(), + reason: error.to_string(), + })?; + let mut cache = cache.lock().unwrap_or_else(PoisonError::into_inner); + if let Some(loaded) = cache.get(&canonical) { + return Ok(Arc::clone(loaded)); + } + let loaded = Arc::new(load(&canonical)?); + cache.insert(canonical, Arc::clone(&loaded)); + Ok(loaded) +} + +/// `dlopen(path, RTLD_NOW | RTLD_LOCAL)`. +#[cfg(unix)] +fn open_library(path: &Path) -> Result { + use libloading::os::unix::{Library as UnixLibrary, RTLD_LOCAL, RTLD_NOW}; + // SAFETY: loading a shared object runs its initializers. The path is the + // user's own `SecurityKeyProvider` setting (or `$SSH_SK_PROVIDER`), the + // same trust OpenSSH extends when `ssh-sk-helper` dlopens it. + unsafe { UnixLibrary::open(Some(path), RTLD_NOW | RTLD_LOCAL) }.map(Library::from) +} + +/// `LoadLibraryExW`, which resolves every import at load time. +#[cfg(not(unix))] +fn open_library(path: &Path) -> Result { + // SAFETY: as for the Unix variant; the path is the user's own setting. + unsafe { Library::new(path) } +} + /// Explain a failed `dlopen`, naming the static-linking limitation of musl /// release builds instead of leaving the user with "Dynamic loading not /// supported". @@ -176,14 +247,13 @@ impl SecurityKeyProvider for ExternalProvider { request: &SkSignRequest<'_>, pin: Option<&str>, ) -> Result { + request.validate()?; let application = CString::new(request.application) .map_err(|_| SecurityKeyError::InvalidRequest("application contains NUL"))?; - let pin = pin - .map(|pin| { - CString::new(pin).map_err(|_| SecurityKeyError::InvalidRequest("PIN contains NUL")) - }) - .transpose()?; - let pin_ptr = pin.as_ref().map_or(ptr::null(), |pin| pin.as_ptr()); + let pin = pin.map(pin_buffer).transpose()?; + let pin_ptr = pin + .as_ref() + .map_or(ptr::null(), |pin| pin.as_ptr().cast::()); let mut response: *mut RawSignResponse = ptr::null_mut(); // SAFETY: every pointer is valid for the stated length for the whole @@ -192,7 +262,7 @@ impl SecurityKeyProvider for ExternalProvider { // in `sshsk_sign`, and `response` is a valid out-pointer the provider // fills with memory it allocated with malloc. let code = unsafe { - (self.sign)( + (self.loaded.sign)( request.algorithm.api_code(), request.data.as_ptr(), request.data.len(), @@ -205,9 +275,8 @@ impl SecurityKeyProvider for ExternalProvider { &mut response, ) }; - if let Some(pin) = pin { - drop(Zeroizing::new(pin.into_bytes())); - } + // Scrub the PIN as soon as the provider is done with it. + drop(pin); if code != 0 { if !response.is_null() { // SAFETY: a non-null response was allocated by the provider @@ -226,6 +295,9 @@ impl SecurityKeyProvider for ExternalProvider { // for us; it is read once and then released exactly as // `sshsk_free_sign_response` does. let copied = unsafe { copy_response(response) }; + // SAFETY: `response` is the non-null, provider-allocated struct read + // above; `copy_response` copied out of it without keeping pointers, + // and it is not touched again after this call. unsafe { free_response(response) }; copied } @@ -235,6 +307,22 @@ impl SecurityKeyProvider for ExternalProvider { } } +/// The PIN as a NUL-terminated C string in memory that is wiped on drop. +/// +/// The NUL check runs before anything is copied: `CString::new` would hand a +/// rejected PIN back inside its error, and that copy would be freed without +/// being wiped. The capacity is reserved up front so no reallocation leaves +/// an unwiped copy behind either. +fn pin_buffer(pin: &str) -> Result>, SecurityKeyError> { + if pin.as_bytes().contains(&0) { + return Err(SecurityKeyError::InvalidRequest("PIN contains NUL")); + } + let mut buffer = Zeroizing::new(Vec::with_capacity(pin.len() + 1)); + buffer.extend_from_slice(pin.as_bytes()); + buffer.push(0); + Ok(buffer) +} + /// Copy a component out of provider memory after validating pointer and /// length together: a NULL pointer with a non-zero length, or an absurd /// length, is a corrupt response rather than something to read. @@ -267,6 +355,8 @@ unsafe fn copy_response( ) -> Result { // SAFETY: the caller guarantees `response` is a valid, initialized struct. let raw = unsafe { &*response }; + // SAFETY: the provider owns each component as `len` bytes at its pointer + // (or NULL); `copy_component` validates the pair before reading. let sig_r = unsafe { copy_component(raw.sig_r, raw.sig_r_len, "sig_r invalid") }?; let sig_s = unsafe { copy_component(raw.sig_s, raw.sig_s_len, "sig_s invalid") }?; Ok(SkSignResponse { @@ -306,119 +396,5 @@ unsafe fn free_response(response: *mut RawSignResponse) { } #[cfg(test)] -mod tests { - use super::*; - - #[test] - fn missing_or_unloadable_libraries_report_a_load_failure() { - let directory = tempfile::tempdir().unwrap(); - let missing = directory.path().join("no-such-provider.so"); - match ExternalProvider::load(&missing).unwrap_err() { - SecurityKeyError::LoadFailed { path, reason } => { - assert_eq!(path, missing); - assert!(!reason.is_empty()); - } - other => panic!("unexpected error: {other:?}"), - } - - let garbage = directory.path().join("garbage.so"); - std::fs::write(&garbage, b"this is not an ELF or Mach-O image").unwrap(); - assert!(matches!( - ExternalProvider::load(&garbage).unwrap_err(), - SecurityKeyError::LoadFailed { .. } - )); - } - - #[cfg(unix)] - #[test] - fn a_library_without_sk_api_version_is_not_a_provider() { - let library: Library = libloading::os::unix::Library::this().into(); - let path = Path::new("/self"); - match bind_symbols(path, &library).unwrap_err() { - SecurityKeyError::NotAProvider { - path: reported, - reason, - } => { - assert_eq!(reported, path); - assert!(reason.contains("sk_api_version"), "{reason}"); - } - other => panic!("unexpected error: {other:?}"), - } - // The handle must not be closed: it is the running program. - std::mem::forget(library); - } - - #[test] - fn provider_validation_enforces_version_and_operation_symbols() { - let path = Path::new("/p.so"); - let all = |_: &[u8]| Ok(()); - assert!(validate_provider(path, super::super::SK_API_VERSION_MAJOR, all).is_ok()); - - let error = validate_provider(path, 0x0009_0000, all).unwrap_err(); - assert!(matches!( - error, - SecurityKeyError::UnsupportedVersion { - version: 0x0009_0000, - .. - } - )); - - for missing in ["sk_enroll\0", "sk_load_resident_keys\0", "sk_sign\0"] { - let error = validate_provider(path, super::super::SK_API_VERSION_MAJOR, |name| { - if name == missing.as_bytes() { - Err(format!("undefined symbol {missing}")) - } else { - Ok(()) - } - }) - .unwrap_err(); - match error { - SecurityKeyError::NotAProvider { reason, .. } => { - assert!(reason.contains(missing.trim_end_matches('\0')), "{reason}"); - } - other => panic!("unexpected error: {other:?}"), - } - } - } - - #[test] - fn musl_hint_only_appears_on_static_builds() { - let error = libloading::Error::DlOpenUnknown; - let reason = describe_load_error(&error); - assert_eq!( - reason.contains("statically linked"), - cfg!(target_env = "musl") - ); - } - - #[test] - fn response_components_are_validated_before_being_read() { - let error = unsafe { copy_component(ptr::null(), 32, "sig_r invalid") }.unwrap_err(); - assert_eq!(error, SecurityKeyError::InvalidResponse("sig_r invalid")); - let bytes = [7u8; 4]; - let error = - unsafe { copy_component(bytes.as_ptr(), MAX_COMPONENT_LEN + 1, "sig_s invalid") } - .unwrap_err(); - assert_eq!(error, SecurityKeyError::InvalidResponse("sig_s invalid")); - let copied = - unsafe { copy_component(bytes.as_ptr(), bytes.len(), "sig_r invalid") }.unwrap(); - assert_eq!(&copied[..], &bytes); - let empty = unsafe { copy_component(ptr::null(), 0, "sig_r invalid") }.unwrap(); - assert!(empty.is_empty()); - } - - /// End-to-end sign through the OpenSSH regress `sk-dummy` middleware when - /// its build is present; this is the same library the regress suite uses. - #[test] - fn sk_dummy_provider_loads_and_reports_the_supported_version() { - let path = Path::new(env!("CARGO_MANIFEST_DIR")).join( - "target/openssh-regress/openssh-portable-V_10_3_P1/regress/misc/sk-dummy/sk-dummy.so", - ); - if !path.exists() { - eprintln!("sk-dummy.so not built; skipping"); - return; - } - let provider = ExternalProvider::load(&path).unwrap(); - assert_eq!(provider.describe(), path.display().to_string()); - } -} +#[path = "external_tests.rs"] +mod tests; diff --git a/src/ssh/security_key/external_tests.rs b/src/ssh/security_key/external_tests.rs new file mode 100644 index 000000000..19e08170d --- /dev/null +++ b/src/ssh/security_key/external_tests.rs @@ -0,0 +1,314 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use super::*; + +#[test] +fn missing_or_unloadable_libraries_report_a_load_failure() { + let directory = tempfile::tempdir().unwrap(); + let missing = directory.path().join("no-such-provider.so"); + match ExternalProvider::load(&missing).unwrap_err() { + SecurityKeyError::LoadFailed { path, reason } => { + assert_eq!(path, missing); + assert!(!reason.is_empty()); + } + other => panic!("unexpected error: {other:?}"), + } + + let garbage = directory.path().join("garbage.so"); + std::fs::write(&garbage, b"this is not an ELF or Mach-O image").unwrap(); + assert!(matches!( + ExternalProvider::load(&garbage).unwrap_err(), + SecurityKeyError::LoadFailed { .. } + )); +} + +#[cfg(unix)] +#[test] +fn a_library_without_sk_api_version_is_not_a_provider() { + let library: Library = libloading::os::unix::Library::this().into(); + let path = Path::new("/self"); + match bind_symbols(path, &library).unwrap_err() { + SecurityKeyError::NotAProvider { + path: reported, + reason, + } => { + assert_eq!(reported, path); + assert!(reason.contains("sk_api_version"), "{reason}"); + } + other => panic!("unexpected error: {other:?}"), + } + // The handle must not be closed: it is the running program. + std::mem::forget(library); +} + +#[test] +fn provider_validation_enforces_version_and_operation_symbols() { + let path = Path::new("/p.so"); + let all = |_: &[u8]| Ok(()); + assert!(validate_provider(path, super::super::SK_API_VERSION_MAJOR, all).is_ok()); + + let error = validate_provider(path, 0x0009_0000, all).unwrap_err(); + assert!(matches!( + error, + SecurityKeyError::UnsupportedVersion { + version: 0x0009_0000, + .. + } + )); + + for missing in ["sk_enroll\0", "sk_load_resident_keys\0", "sk_sign\0"] { + let error = validate_provider(path, super::super::SK_API_VERSION_MAJOR, |name| { + if name == missing.as_bytes() { + Err(format!("undefined symbol {missing}")) + } else { + Ok(()) + } + }) + .unwrap_err(); + match error { + SecurityKeyError::NotAProvider { reason, .. } => { + assert!(reason.contains(missing.trim_end_matches('\0')), "{reason}"); + } + other => panic!("unexpected error: {other:?}"), + } + } +} + +#[test] +fn musl_hint_only_appears_on_static_builds() { + let error = libloading::Error::DlOpenUnknown; + let reason = describe_load_error(&error); + assert_eq!( + reason.contains("statically linked"), + cfg!(target_env = "musl") + ); +} + +#[test] +fn response_components_are_validated_before_being_read() { + let error = unsafe { copy_component(ptr::null(), 32, "sig_r invalid") }.unwrap_err(); + assert_eq!(error, SecurityKeyError::InvalidResponse("sig_r invalid")); + let bytes = [7u8; 4]; + let error = unsafe { copy_component(bytes.as_ptr(), MAX_COMPONENT_LEN + 1, "sig_s invalid") } + .unwrap_err(); + assert_eq!(error, SecurityKeyError::InvalidResponse("sig_s invalid")); + let copied = unsafe { copy_component(bytes.as_ptr(), bytes.len(), "sig_r invalid") }.unwrap(); + assert_eq!(&copied[..], &bytes); + let empty = unsafe { copy_component(ptr::null(), 0, "sig_r invalid") }.unwrap(); + assert!(empty.is_empty()); +} + +#[test] +fn the_cache_loads_each_canonical_path_once_and_forgets_failures() { + let directory = tempfile::tempdir().unwrap(); + let library = directory.path().join("provider.so"); + std::fs::write(&library, b"stand-in").unwrap(); + let cache = Mutex::new(HashMap::new()); + let loads = std::cell::Cell::new(0); + let load = |canonical: &Path| { + loads.set(loads.get() + 1); + Ok(canonical.to_path_buf()) + }; + + let first = load_cached(&cache, &library, load).unwrap(); + let again = load_cached(&cache, &library, load).unwrap(); + assert!(Arc::ptr_eq(&first, &again)); + #[cfg(unix)] + { + let link = directory.path().join("link.so"); + std::os::unix::fs::symlink(&library, &link).unwrap(); + let through_link = load_cached(&cache, &link, load).unwrap(); + assert!(Arc::ptr_eq(&first, &through_link)); + } + assert_eq!(loads.get(), 1); + + let other = directory.path().join("other.so"); + std::fs::write(&other, b"stand-in").unwrap(); + let failing = |_: &Path| -> Result { + Err(SecurityKeyError::InvalidResponse("load failed")) + }; + assert!(load_cached(&cache, &other, failing).is_err()); + let recovered = load_cached(&cache, &other, load).unwrap(); + assert_eq!(*recovered, std::fs::canonicalize(&other).unwrap()); + assert_eq!(loads.get(), 2); + + let missing = directory.path().join("missing.so"); + assert!(matches!( + load_cached(&cache, &missing, load).unwrap_err(), + SecurityKeyError::LoadFailed { path, .. } if path == missing + )); +} + +#[test] +fn pins_are_nul_terminated_and_a_nul_inside_is_refused() { + assert_eq!(&pin_buffer("1234").unwrap()[..], b"1234\0"); + assert_eq!(&pin_buffer("").unwrap()[..], b"\0"); + assert_eq!( + pin_buffer("12\0ab").unwrap_err(), + SecurityKeyError::InvalidRequest("PIN contains NUL") + ); +} + +/// Fixture providers compiled from C at test time, so the real `dlopen` +/// path is exercised without the OpenSSH regress tree. Skipped when no C +/// compiler is installed. +#[cfg(target_os = "linux")] +mod compiled { + use super::*; + + const PROVIDER: &str = r#" +#include +#include +#include +struct sk_sign_response { +uint8_t flags; uint32_t counter; +uint8_t *sig_r; size_t sig_r_len; uint8_t *sig_s; size_t sig_s_len; +}; +int bssh_fixture_sign_calls = 0; +uint32_t sk_api_version(void) { return 0x000a0000; } +int sk_enroll(void) { return -2; } +int sk_load_resident_keys(void) { return -2; } +int sk_sign(uint32_t alg, const uint8_t *data, size_t data_len, +const char *application, const uint8_t *key_handle, size_t key_handle_len, +uint8_t flags, const char *pin, void **options, +struct sk_sign_response **out) { +struct sk_sign_response *response; +bssh_fixture_sign_calls++; +if ((response = calloc(1, sizeof(*response))) == NULL || + (response->sig_r = malloc(64)) == NULL) + return -1; +memset(response->sig_r, 0x5a, 64); +response->sig_r_len = 64; +response->flags = flags; +response->counter = 42; +*out = response; +return 0; +} +"#; + + /// Same entry points, but `sk_enroll` calls a function no library + /// defines. Linked with lazy binding, so only `RTLD_NOW` notices. + const UNRESOLVED: &str = r#" +#include +#include +extern int bssh_fixture_symbol_nobody_defines(void); +uint32_t sk_api_version(void) { return 0x000a0000; } +int sk_enroll(void) { return bssh_fixture_symbol_nobody_defines(); } +int sk_load_resident_keys(void) { return -2; } +int sk_sign(void) { return -1; } +"#; + + fn build(directory: &Path, name: &str, source: &str) -> Option { + let source_path = directory.join(format!("{name}.c")); + std::fs::write(&source_path, source).unwrap(); + let library = directory.join(format!("{name}.so")); + let status = std::process::Command::new("cc") + .args(["-shared", "-fPIC", "-Wl,-z,lazy", "-o"]) + .arg(&library) + .arg(&source_path) + .status(); + match status { + Ok(status) if status.success() => Some(library), + _ => { + eprintln!("no working C compiler; skipping"); + None + } + } + } + + fn sign_calls(provider: &ExternalProvider) -> c_int { + // SAFETY: the fixture defines `bssh_fixture_sign_calls` as an + // `int`; it is only read, from the thread running this test. + unsafe { + **provider + ._loaded_for_tests() + .get::<*const c_int>(b"bssh_fixture_sign_calls\0") + .unwrap() + } + } + + fn request<'a>(application: &'a str) -> SkSignRequest<'a> { + SkSignRequest { + algorithm: super::super::super::SkAlgorithm::Ed25519, + data: b"message", + application, + key_handle: &[1, 2, 3], + flags: 0x01, + } + } + + #[test] + fn unresolved_symbols_fail_the_load_like_rtld_now() { + let directory = tempfile::tempdir().unwrap(); + let Some(library) = build(directory.path(), "unresolved", UNRESOLVED) else { + return; + }; + match ExternalProvider::load(&library).unwrap_err() { + SecurityKeyError::LoadFailed { path, reason } => { + assert_eq!(path, library); + assert!( + reason.contains("bssh_fixture_symbol_nobody_defines"), + "{reason}" + ); + } + other => panic!("unexpected error: {other:?}"), + } + } + + #[test] + fn a_provider_is_loaded_once_and_signs_through_the_ffi() { + let directory = tempfile::tempdir().unwrap(); + let Some(library) = build(directory.path(), "provider", PROVIDER) else { + return; + }; + let first = ExternalProvider::load(&library).unwrap(); + let link = directory.path().join("provider-link.so"); + std::os::unix::fs::symlink(&library, &link).unwrap(); + let second = ExternalProvider::load(&link).unwrap(); + assert!(Arc::ptr_eq(&first.loaded, &second.loaded)); + assert_eq!(second.describe(), link.display().to_string()); + + let response = first.sign(&request("ssh:"), Some("1234")).unwrap(); + assert_eq!(&response.sig_r[..], &[0x5a; 64]); + assert!(response.sig_s.is_empty()); + assert_eq!((response.flags, response.counter), (0x01, 42)); + assert_eq!(sign_calls(&first), 1); + + assert_eq!( + second.sign(&request(""), None).unwrap_err(), + SecurityKeyError::InvalidRequest("key has no application") + ); + assert_eq!( + second.sign(&request("ssh:"), Some("12\0")).unwrap_err(), + SecurityKeyError::InvalidRequest("PIN contains NUL") + ); + assert_eq!(sign_calls(&first), 1, "the provider was called anyway"); + } +} + +/// End-to-end sign through the OpenSSH regress `sk-dummy` middleware when +/// its build is present; this is the same library the regress suite uses. +#[test] +fn sk_dummy_provider_loads_and_reports_the_supported_version() { + let path = Path::new(env!("CARGO_MANIFEST_DIR")).join( + "target/openssh-regress/openssh-portable-V_10_3_P1/regress/misc/sk-dummy/sk-dummy.so", + ); + if !path.exists() { + eprintln!("sk-dummy.so not built; skipping"); + return; + } + let provider = ExternalProvider::load(&path).unwrap(); + assert_eq!(provider.describe(), path.display().to_string()); +} diff --git a/src/ssh/security_key/mod.rs b/src/ssh/security_key/mod.rs index 2c40d61ee..c75fd35f4 100644 --- a/src/ssh/security_key/mod.rs +++ b/src/ssh/security_key/mod.rs @@ -29,6 +29,17 @@ //! helper would add a private wire protocol without changing what code runs //! as the user. Provider calls block on touch and PIN entry, so callers run //! them on a blocking thread (see `tokio_client::security_key_signer`). +//! +//! Running in-process has costs the helper avoids, and they are accepted: +//! +//! - An `sk_sign` call that has started cannot be cancelled. A provider that +//! hangs keeps its blocking thread and the process-wide provider lock until +//! it returns, so no other security-key signature in the process can +//! proceed meanwhile. Connections queued behind it give up at their +//! `ConnectTimeout` without ever reaching the provider. +//! - The provider shares bssh's address space, so it can read everything +//! bssh holds in memory, including decrypted keys and passwords for other +//! hosts. Only configure a provider library you would trust with them. mod external; mod signature; @@ -89,6 +100,17 @@ pub struct SkSignRequest<'a> { pub flags: u8, } +impl SkSignRequest<'_> { + /// The argument checks `sshsk_sign` makes before it opens the provider: + /// a key without an application string is refused rather than signed. + pub fn validate(&self) -> Result<(), SecurityKeyError> { + if self.application.is_empty() { + return Err(SecurityKeyError::InvalidRequest("key has no application")); + } + Ok(()) + } +} + /// Where `SecurityKeyProvider` points after `$VAR` expansion. #[derive(Debug, Clone, PartialEq, Eq)] pub enum ProviderSource { @@ -360,6 +382,23 @@ mod tests { ); } + #[test] + fn requests_without_an_application_are_invalid() { + let mut request = SkSignRequest { + algorithm: SkAlgorithm::Ed25519, + data: b"data", + application: "ssh:", + key_handle: &[1, 2], + flags: SK_FLAG_USER_PRESENCE_REQUIRED, + }; + assert_eq!(request.validate(), Ok(())); + request.application = ""; + assert_eq!( + request.validate(), + Err(SecurityKeyError::InvalidRequest("key has no application")) + ); + } + #[test] fn algorithm_codes_and_names_follow_sk_api() { assert_eq!(SkAlgorithm::EcdsaSha2NistP256.api_code(), 0); diff --git a/src/ssh/security_key/signature.rs b/src/ssh/security_key/signature.rs index a96fe8796..6194fdfae 100644 --- a/src/ssh/security_key/signature.rs +++ b/src/ssh/security_key/signature.rs @@ -20,6 +20,9 @@ use zeroize::Zeroizing; use super::{SecurityKeyError, SkAlgorithm}; +/// `crypto_sign_ed25519_BYTES`. +const ED25519_SIGNATURE_LEN: usize = 64; + /// The provider's `struct sk_sign_response`, copied out of C memory. /// /// `sig_s` is empty for Ed25519, whose whole signature travels in `sig_r`. @@ -45,6 +48,22 @@ pub fn encode_signature( if response.sig_r.is_empty() { return Err(SecurityKeyError::InvalidResponse("missing sig_r")); } + // An Ed25519 signature is exactly 64 bytes and travels whole in + // `sig_r`. The pinned `sshsk_ed25519_sig` only checks that + // `sig_r` is present, but `ssh_ed25519_sk_verify` rejects a longer + // one and a shorter one cannot verify, so a response of another + // shape, or one that also fills `sig_s`, is a broken provider and + // is refused before anything is sent to the server. + if response.sig_r.len() != ED25519_SIGNATURE_LEN { + return Err(SecurityKeyError::InvalidResponse( + "Ed25519 sig_r is not 64 bytes", + )); + } + if !response.sig_s.is_empty() { + return Err(SecurityKeyError::InvalidResponse( + "unexpected sig_s for Ed25519", + )); + } response.sig_r.to_vec() } SkAlgorithm::EcdsaSha2NistP256 => { @@ -139,6 +158,17 @@ mod tests { encode_signature(SkAlgorithm::Ed25519, &response(&[], &[])).unwrap_err(), SecurityKeyError::InvalidResponse("missing sig_r") ); + for len in [32, 63, 65] { + assert_eq!( + encode_signature(SkAlgorithm::Ed25519, &response(&vec![1; len], &[])).unwrap_err(), + SecurityKeyError::InvalidResponse("Ed25519 sig_r is not 64 bytes"), + "sig_r of {len} bytes" + ); + } + assert_eq!( + encode_signature(SkAlgorithm::Ed25519, &response(&[1; 64], &[2; 32])).unwrap_err(), + SecurityKeyError::InvalidResponse("unexpected sig_s for Ed25519") + ); assert_eq!( encode_signature(SkAlgorithm::EcdsaSha2NistP256, &response(&[1; 32], &[])).unwrap_err(), SecurityKeyError::InvalidResponse("missing sig_r or sig_s") diff --git a/src/ssh/tokio_client/security_key_signer.rs b/src/ssh/tokio_client/security_key_signer.rs index 188efbd68..c621473db 100644 --- a/src/ssh/tokio_client/security_key_signer.rs +++ b/src/ssh/tokio_client/security_key_signer.rs @@ -43,7 +43,7 @@ use crate::ssh::security_key::{ }; /// Everything the provider needs from an `sk-*` private key file. -#[derive(Debug, Clone)] +#[derive(Clone)] pub(super) struct SecurityKeyIdentity { pub(super) algorithm: SkAlgorithm, application: String, @@ -53,6 +53,24 @@ pub(super) struct SecurityKeyIdentity { pub(super) path: PathBuf, } +/// The key handle is the authenticator's wrapped credential, so `Debug` +/// (which reaches `-vvv` traces through the signer) prints only its length. +impl std::fmt::Debug for SecurityKeyIdentity { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("SecurityKeyIdentity") + .field("algorithm", &self.algorithm) + .field("application", &self.application) + .field( + "key_handle", + &format_args!("", self.key_handle.len()), + ) + .field("flags", &self.flags) + .field("fingerprint", &self.fingerprint) + .field("path", &self.path) + .finish() + } +} + impl SecurityKeyIdentity { /// `Some` for authenticator-hosted keys, `None` for software keys. pub(super) fn from_private_key(key: &russh::keys::PrivateKey, path: &Path) -> Option { @@ -264,6 +282,18 @@ impl SecurityKeySigner { path: self.identity.path.clone(), reason, }; + // `sshsk_sign` refuses a malformed key before opening the provider; + // doing the same here also keeps the touch notice from appearing for + // a signature that will never be attempted. + SkSignRequest { + algorithm: self.identity.algorithm, + data: &data, + application: &self.identity.application, + key_handle: &self.identity.key_handle, + flags: self.identity.flags, + } + .validate() + .map_err(|error| failed(error.to_string()))?; let notify = self.interactive && self.identity.wants_user_presence(); let notify = notify && std::io::stderr().is_terminal(); let data = Arc::new(data); @@ -483,6 +513,43 @@ mod tests { assert!(!had_pin); } + #[test] + fn identity_debug_output_redacts_the_key_handle() { + let public = SkEd25519Public::new(Ed25519PublicKey([9u8; 32]), "ssh:example"); + let handle = vec![0xde, 0xad, 0xbe, 0xef, 0x42]; + let private = SkEd25519Private::new(public, 0x01, handle).unwrap(); + let key = PrivateKey::new(KeypairData::SkEd25519(private), "test").unwrap(); + let identity = SecurityKeyIdentity::from_private_key(&key, Path::new("/k")).unwrap(); + let rendered = format!("{identity:?}"); + assert!(rendered.contains(""), "{rendered}"); + for leaked in ["222, 173, 190, 239", "0xde", "de, ad", "deadbeef"] { + assert!(!rendered.contains(leaked), "{rendered}"); + } + let signer = + SecurityKeySigner::new(Arc::new(RecordingProvider::default()), identity, false); + assert!(!format!("{signer:?}").contains("222, 173, 190, 239")); + } + + #[tokio::test] + async fn a_key_without_an_application_never_reaches_the_provider() { + let public = SkEd25519Public::new(Ed25519PublicKey([9u8; 32]), ""); + let private = SkEd25519Private::new(public, 0x01, vec![1, 2]).unwrap(); + let key = PrivateKey::new(KeypairData::SkEd25519(private), "test").unwrap(); + let identity = SecurityKeyIdentity::from_private_key(&key, Path::new("/k")).unwrap(); + let provider = Arc::new(RecordingProvider::default()); + let signer = SecurityKeySigner::new(provider.clone(), identity, false); + let error = signer.sign_blob(vec![1]).await.unwrap_err(); + assert!( + matches!( + &error, + super::super::Error::SecurityKeySignFailed { reason, .. } + if reason.contains("no application") + ), + "{error:?}" + ); + assert!(provider.calls.lock().unwrap().is_empty()); + } + #[tokio::test] async fn pin_required_in_batch_mode_fails_without_prompting() { let key = sk_ed25519_key(0x05); From 637cbdcb6b84d5f135a3b44a8e087051a01001f5 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 22 Oct 2026 12:13:55 +0900 Subject: [PATCH 7/7] docs: describe security-key auth and host-key type check ARCHITECTURE.md did not mention FIDO sk-* client identities, the SecurityKeyProvider flow, the default id_ed25519_sk and id_ecdsa_sk identities, or the negotiated host-key type check. Add them to the Authentication and host certificate sections, and list host_key_type.rs in the ssh-client module structure. Docs only, no code changed. Other listed docs were already consistent with the code. Refs #328 --- ARCHITECTURE.md | 2 ++ docs/architecture/ssh-client.md | 1 + 2 files changed, 3 insertions(+) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 408b749ba..24172db09 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -863,6 +863,7 @@ but not with themselves; omit the key (plain `#[serial]`) when in doubt. - Private key files with passphrase support - Password authentication (discouraged in production) - Public key authentication preferred +- FIDO security-key identities (`sk-ssh-ed25519@openssh.com`, `sk-ecdsa-sha2-nistp256@openssh.com`): the default identity list includes `id_ed25519_sk` and `id_ecdsa_sk`. Signing goes through a `SecurityKeyProvider` (`src/ssh/security_key/`, `src/ssh/tokio_client/security_key_signer.rs`): the default is `$SSH_SK_PROVIDER`, an external library implementing OpenSSH's `sk-api.h` is loaded with `dlopen` and called on a blocking thread, and `internal` reports a diagnostic because bssh has no built-in provider. A missing or failing provider declines that identity and authentication continues with the next one. Keys held by an agent sign through the agent and need no provider. Static musl builds cannot `dlopen` and report a diagnostic instead. #### macOS `UseKeychain` @@ -904,6 +905,7 @@ validates the certificates it is offered: and runs the `HOSTNAME` invocation before the DNSSEC and known_hosts checks. - Without a matching CA the certified key is checked as a plain host key if `HostKeyAlgorithms` permits its plain type. +- The key or certificate the server sends must have the type the key exchange negotiated (`src/ssh/tokio_client/host_key_type.rs`, mirroring OpenSSH `kex_verify_host_key`), including the `sk-*` types; a mismatch refuses the connection, so a `HostKeyAlgorithms` restriction also limits which trusted key type a server may present. - `ServerCheckMethod::NoCheck` still accepts, because there the operator has turned host verification off outright. diff --git a/docs/architecture/ssh-client.md b/docs/architecture/ssh-client.md index 7e3928bbd..df9189b5d 100644 --- a/docs/architecture/ssh-client.md +++ b/docs/architecture/ssh-client.md @@ -17,6 +17,7 @@ - `tokio_client/authentication.rs` - Authentication methods (378 lines) - `tokio_client/security_key_signer.rs` - russh `Signer` for FIDO `sk-*` identities: resolves `SecurityKeyProvider`, checks the provider before decrypting an `sk-*` key file, calls the provider on a blocking thread (serialized process-wide), prints the touch notice, prompts once for a PIN, and declines a failed signature so authentication continues with the next identity (`authenticate_declinable`) - `ssh/security_key/` - provider interface (`SecurityKeyProvider` trait, `$VAR` resolution, `internal`/`none` handling), the `sk-api.h` `dlopen` binding (`external.rs`), and the `sk-*` signature blob encoder (`signature.rs`) +- `tokio_client/host_key_type.rs` - refuses a server host key or certificate whose type differs from the negotiated host-key algorithm (OpenSSH `kex_verify_host_key`), including `sk-*` types and ECDSA curves - `tokio_client/channel_manager.rs` - Channel operations (230 lines) - `tokio_client/file_transfer.rs` - SFTP file operations (285 lines)