diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 408b749b..24172db0 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/Cargo.lock b/Cargo.lock index 8796a899..8800da23 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 462588a4..54c2d118 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/README.md b/README.md index ba43407d..6dbb1bf1 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 329b54ee..df9189b5 100644 --- a/docs/architecture/ssh-client.md +++ b/docs/architecture/ssh-client.md @@ -15,6 +15,9 @@ **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`, 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) diff --git a/docs/architecture/ssh-config-parser.md b/docs/architecture/ssh-config-parser.md index e9a24cdb..e17beb02 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 0f71682f..76033ae2 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. 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 + .TP .B PubkeyAcceptedAlgorithms Specifies the signature algorithms that will be used for public key authentication. @@ -2081,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 db9241e0..e47a2277 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. | @@ -35,7 +35,11 @@ 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 + +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 @@ -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 8a068d94..4291d372 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 2127b294..c5f485ea 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(), )); } } @@ -688,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() } } @@ -724,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/mod.rs b/src/ssh/mod.rs index 61c7a2bd..534190fa 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 00000000..8de3b593 --- /dev/null +++ b/src/ssh/security_key/external.rs @@ -0,0 +1,400 @@ +// 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`. +//! +//! 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; + +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 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, + /// 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") + .field("path", &self.path) + .finish_non_exhaustive() + } +} + +impl ExternalProvider { + #[cfg(test)] + fn _loaded_for_tests(&self) -> &Library { + &self.loaded._library + } + + pub(super) fn load(path: &Path) -> Result { + 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(), + 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". +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 { + request.validate()?; + let application = CString::new(request.application) + .map_err(|_| SecurityKeyError::InvalidRequest("application contains NUL"))?; + 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 + // 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.loaded.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, + ) + }; + // 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 + // 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) }; + // 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 + } + + fn describe(&self) -> String { + self.path.display().to_string() + } +} + +/// 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. +/// +/// # 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 }; + // 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 { + 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)] +#[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 00000000..19e08170 --- /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 new file mode 100644 index 00000000..c75fd35f --- /dev/null +++ b/src/ssh/security_key/mod.rs @@ -0,0 +1,415 @@ +// 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`). +//! +//! 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; + +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, +} + +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 { + /// 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 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); + 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 00000000..6194fdfa --- /dev/null +++ b/src/ssh/security_key/signature.rs @@ -0,0 +1,177 @@ +// 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}; + +/// `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`. +#[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")); + } + // 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 => { + 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") + ); + 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/ssh_config/dump.rs b/src/ssh/ssh_config/dump.rs index 74eb6e94..19c55061 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 3e2e6105..29ff3754 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 956be6ef..a4d20b91 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 1f0ed68c..d5e38a75 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 8e98bac5..db8b25d0 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 70bb2de8..dc0ef1bb 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 7317979d..c6b9bd15 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 83617c00..c7158667 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 500c5ea1..e89d502c 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 f80a41b1..1379bcad 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: SecurityKeySource<'_>, + interactive: bool, ) -> Result<(), super::Error> { let hashes = if accepted.is_empty() { default_hashes(handle, &key.algorithm()).await? @@ -802,38 +820,105 @@ 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, interactive).await?; + 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 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(()); + if result.success() { + return Ok(()); + } } } 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)] -struct LocalPrivateKeySigner { - key: Arc, +enum LocalSigner { + Software(Arc), + SecurityKey(super::security_key_signer::SecurityKeySigner), +} + +impl LocalSigner { + /// Pick the signer for `key`; `sk-*` keys need a usable provider before + /// 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: SecurityKeySource<'_>, + interactive: bool, + ) -> Result { + match super::security_key_signer::SecurityKeyIdentity::from_private_key(&key, path) { + Some(identity) => { + 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, + identity, + interactive, + ), + )) + } + None => Ok(Self::Software(Arc::new(key))), + } + } } -impl russh::Signer for LocalPrivateKeySigner { +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,7 +971,13 @@ async fn authenticate_certificate_file( accepted: Vec, allow_passphrase_prompt: bool, 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, @@ -926,11 +1017,29 @@ async fn authenticate_certificate_file( ))); } - let mut signer = LocalPrivateKeySigner { key: Arc::new(key) }; + let mut signer = LocalSigner::for_private_key( + key, + &key_file_path, + 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(()); } @@ -1251,10 +1360,14 @@ 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` 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 @@ -1263,6 +1376,8 @@ fn is_retryable_rejection(error: &super::Error) -> bool { | super::Error::KeyInvalid(_) | super::Error::KeyPassphrasePromptDisabled { .. } | super::Error::IdentityAlgorithmExcluded(_) + | super::Error::SecurityKeyUnavailable { .. } + | super::Error::SecurityKeySignFailed { .. } | super::Error::PasswordWrong | super::Error::AgentConnectionFailed | super::Error::AgentRequestIdentitiesFailed @@ -1330,7 +1445,13 @@ async fn authenticate_single( accepted_algorithms, allow_passphrase_prompt, 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, @@ -1341,8 +1462,14 @@ async fn authenticate_single( authenticate_private_key_with_policy( handle, username, - Arc::new(key), + key, + &key_file_path, &accepted_algorithms, + SecurityKeySource { + setting: security_key_provider.as_deref(), + preopened: preopened_provider, + }, + allow_passphrase_prompt, ) .await?; } @@ -1353,6 +1480,7 @@ async fn authenticate_single( accepted_algorithms, allow_passphrase_prompt, use_keychain, + security_key_provider, } => { authenticate_certificate_file( handle, @@ -1363,6 +1491,7 @@ async fn authenticate_single( accepted_algorithms, allow_passphrase_prompt, use_keychain, + security_key_provider, ) .await?; } @@ -1727,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(); @@ -1753,18 +2022,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 +2086,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 +2482,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 c6f1c68a..76cb9ced 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(); @@ -2823,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 { @@ -2859,6 +2863,7 @@ impl ClientHandler { suppress_auth_banner: false, emit_kex_diagnostics: false, connection_info: ConnectionInfoState::new(host), + negotiated_host_key: None, } } @@ -3579,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}"); @@ -3739,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 { @@ -3994,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)] @@ -4302,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!( @@ -4338,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() ); @@ -4702,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, }) @@ -4835,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)), @@ -4844,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)), @@ -4883,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)), @@ -4913,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:?}"); } @@ -5005,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)), @@ -5030,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:?}"); } @@ -5051,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:?}" @@ -5071,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:?}" @@ -5088,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:?}" @@ -5101,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/connection_tests.rs b/src/ssh/tokio_client/connection_tests.rs index e6bb1a7d..a293a65e 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 b4b0c192..2612c554 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 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, + }, #[error("Permission denied (password).")] PasswordWrong, #[error( @@ -173,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 @@ -294,6 +321,7 @@ impl Error { | Self::NoAddressForFamily { .. } | Self::CommandDidntExit | Self::ServerCheckFailed + | Self::HostKeyTypeMismatch { .. } | Self::KnownHostsCommandFailed { .. } | Self::HostKeyChanged { .. } | Self::HostKeyRevoked { .. } @@ -302,6 +330,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/host_key_type.rs b/src/ssh/tokio_client/host_key_type.rs new file mode 100644 index 00000000..5edb2252 --- /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 c4ba79a5..da0402ab 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 68949eb4..d929ad47 100644 --- a/src/ssh/tokio_client/mod.rs +++ b/src/ssh/tokio_client/mod.rs @@ -25,10 +25,12 @@ 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; 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 00000000..c621473d --- /dev/null +++ b/src/ssh/tokio_client/security_key_signer.rs @@ -0,0 +1,818 @@ +// 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. +//! +//! 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, LazyLock, Mutex}; + +use tokio::sync::Notify; + +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(Clone)] +pub(super) struct SecurityKeyIdentity { + pub(super) algorithm: SkAlgorithm, + application: String, + key_handle: Zeroizing>, + flags: u8, + fingerprint: String, + 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 { + 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())) + } + } +} + +/// 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 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)] +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)] +pub(super) struct SecurityKeySigner { + provider: Arc, + identity: Arc, + /// `BatchMode no`: a touch notice may be printed and a PIN prompted. + interactive: bool, + decline: Arc, +} + +impl SecurityKeySigner { + pub(super) fn new( + provider: Arc, + identity: SecurityKeyIdentity, + interactive: bool, + ) -> Self { + Self { + 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(), + 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); + 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 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, + 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 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(), + }) + } + 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) + } + } + } +} + +#[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); + } + + #[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); + 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(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); + } + + /// 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"); + 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 f1227c84..0ac6dced 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 diff --git a/tests/ssh_compat_output_test.rs b/tests/ssh_compat_output_test.rs index 5937c9e4..c6059dce 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")); }