From 9ad7caad657b4fac2e6f85516b7e060d19e09072 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 22 Oct 2026 23:08:38 +0900 Subject: [PATCH 1/2] fix: Reject security-key policies the server cannot enforce Parse no-touch-required and verify-required in authorized_keys and reject matching SK user keys unless neither presence nor verification is required. russh does not expose signature flags to the authentication callback, so accepting other policies would silently bypass their requirements. Cover both SK algorithms, ordinary keys, and per-entry policy in actual handler callback tests. Removing the guard makes both regression tests fail; restoring it passes. Document the restriction and the signature-flag enforcement follow-up. Refs #338. --- docs/openssh-compatibility-status.md | 4 ++ docs/security.md | 6 ++ src/server/auth/publickey.rs | 24 +++++++ tests/server_sk_policy_test.rs | 100 +++++++++++++++++++++++++++ 4 files changed, 134 insertions(+) create mode 100644 tests/server_sk_policy_test.rs diff --git a/docs/openssh-compatibility-status.md b/docs/openssh-compatibility-status.md index 5922eb713..011b7983e 100644 --- a/docs/openssh-compatibility-status.md +++ b/docs/openssh-compatibility-status.md @@ -85,3 +85,7 @@ Security regression coverage preserves trusted-file-only shell Match execution, The binding API proposal is [russh PR #794](https://github.com/Eugeny/russh/pull/794). Dependency adoption is conditional on upstream merge: only an immutable merged revision from the official upstream may be used temporarily, followed by a crates.io release. An unmerged contributor branch is not an approved dependency. Current code must continue to fail closed on unsupported agent constraints. Remaining control details include complete forwarding-channel enumeration, allocated remote-port replies, multiplexed `-W`, TTY-failure notifications and `~user` expansion. Unix-domain forwarding is implemented; raw mux-v4 interoperability is not. Eager `zlib` is validated; delayed `zlib@openssh.com` is not advertised. Non-pipe/socket character-device stdin still uses the existing blocking adapter; the SCP fix does not claim every stdin source is cancellation-safe. + +### Server security-key authorization + +bssh-server fails closed for `sk-ssh-ed25519@openssh.com` and `sk-ecdsa-sha2-nistp256@openssh.com` user keys: russh does not expose signature UP/UV flags to the authentication callback. An `authorized_keys` line must have `no-touch-required` and must not have `verify-required` to authorize either type. The default touch policy and every verification-required policy are rejected and logged at info level. Ordinary keys are unaffected. This server restriction is distinct from client-side security-key signing and host-key verification described above; [#340](https://github.com/lablup/bssh/issues/340) tracks actual signature-flag enforcement. Security-key user certificates remain out of scope. diff --git a/docs/security.md b/docs/security.md index 26d6f563e..edac75929 100644 --- a/docs/security.md +++ b/docs/security.md @@ -55,6 +55,12 @@ auth: - Restrict authorized_keys file permissions to 0600 - Regularly rotate keys (annually or when personnel changes) +### Security-key user authentication + +bssh-server currently cannot inspect the user-presence (UP/touch) or user-verification (UV/PIN or biometric) flags of security-key signatures through russh. It therefore rejects `sk-ssh-ed25519@openssh.com` and `sk-ecdsa-sha2-nistp256@openssh.com` user keys by default. A matching `authorized_keys` entry is accepted only if it explicitly contains `no-touch-required` and does not contain `verify-required`. Adding both options still rejects the key. Rejections are logged at info level. + +Use an ordinary key or an SSH server that enforces UP/UV when touch or verification is required. Only use `no-touch-required` when deliberately authorizing authentication without either requirement; it is a relaxation of policy, not a way to enforce touch. Ordinary public keys are unaffected. Signature-flag enforcement is tracked in [#340](https://github.com/lablup/bssh/issues/340); security-key user certificates are outside this support. + ### Password Authentication If password authentication is required, use strong passwords with Argon2id hashing: diff --git a/src/server/auth/publickey.rs b/src/server/auth/publickey.rs index ea3d37244..4fac29c74 100644 --- a/src/server/auth/publickey.rs +++ b/src/server/auth/publickey.rs @@ -213,6 +213,12 @@ pub struct AuthKeyOptions { /// Disable agent forwarding pub no_agent_forwarding: bool, + /// Permit security-key signatures without user presence (touch). + pub no_touch_required: bool, + + /// Require security-key user verification (PIN or biometric). + pub verify_required: bool, + /// Disable X11 forwarding pub no_x11_forwarding: bool, } @@ -286,6 +292,22 @@ impl PublicKeyVerifier { // Check if key matches any authorized key for authorized_key in &authorized_keys { if self.keys_match(key, authorized_key) { + // russh verifies the signature but does not expose its UP/UV flags + // to auth_publickey. Never authorize a policy we cannot enforce. + if matches!( + key.algorithm(), + russh::keys::ssh_key::Algorithm::SkEd25519 + | russh::keys::ssh_key::Algorithm::SkEcdsaSha2NistP256 + ) && (!authorized_key.options.no_touch_required + || authorized_key.options.verify_required) + { + tracing::info!( + user = %username, + key_type = %key.algorithm(), + "Security key rejected: user presence or verification policy cannot be enforced; only no-touch-required without verify-required is supported" + ); + continue; + } tracing::info!( user = %username, key_type = %key.algorithm(), @@ -635,6 +657,8 @@ fn parse_key_options(options_str: &str) -> Result { } else { // Boolean option match option { + "no-touch-required" => options.no_touch_required = true, + "verify-required" => options.verify_required = true, "no-pty" => options.no_pty = true, "no-port-forwarding" => options.no_port_forwarding = true, "no-agent-forwarding" => options.no_agent_forwarding = true, diff --git a/tests/server_sk_policy_test.rs b/tests/server_sk_policy_test.rs new file mode 100644 index 000000000..315d6da2a --- /dev/null +++ b/tests/server_sk_policy_test.rs @@ -0,0 +1,100 @@ +// 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 std::sync::Arc; + +use bssh::server::{config::ServerConfig, handler::SshHandler, session::SessionManager}; +use russh::keys::ssh_key::{ + PublicKey, + public::{Ed25519PublicKey, KeyData, SkEcdsaSha2NistP256, SkEd25519}, +}; +use russh::server::{Auth, Handler}; +use tokio::sync::RwLock; + +fn keys() -> [PublicKey; 3] { + let ed = Ed25519PublicKey([7; 32]); + let signing = p256::ecdsa::SigningKey::from_slice(&[1; 32]).unwrap(); + let point = signing.verifying_key().to_sec1_point(false); + [ + PublicKey::new(KeyData::SkEd25519(SkEd25519::new(ed, "ssh:")), ""), + PublicKey::new( + KeyData::SkEcdsaSha2NistP256(SkEcdsaSha2NistP256::new(point, "ssh:")), + "", + ), + PublicKey::new(KeyData::Ed25519(ed), ""), + ] +} + +async fn authenticate(key: &PublicKey, lines: &[String]) -> bool { + let dir = tempfile::tempdir().unwrap(); + let user_dir = dir.path().join("alice"); + std::fs::create_dir(&user_dir).unwrap(); + let path = user_dir.join("authorized_keys"); + std::fs::write(&path, lines.join("\n")).unwrap(); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&user_dir, std::fs::Permissions::from_mode(0o700)).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); + } + let config = ServerConfig::builder() + .allow_publickey_auth(true) + .allow_password_auth(false) + .authorized_keys_dir(dir.path()) + .build(); + let mut handler = SshHandler::new( + None, + Arc::new(config), + Arc::new(RwLock::new(SessionManager::new())), + ); + matches!( + handler.auth_publickey("alice", key).await.unwrap(), + Auth::Accept + ) +} + +#[tokio::test] +async fn server_sk_policy_enforces_each_authorized_key_option_combination() { + for (index, key) in keys().iter().enumerate() { + for options in [ + "", + "no-touch-required", + "verify-required", + "no-touch-required,verify-required", + "verify-required,no-touch-required", + "no-pty,no-touch-required", + ] { + let line = format!("{options} {}", key.to_openssh().unwrap()); + let accepted = authenticate(key, &[line]).await; + let expected = index == 2 + || (options.contains("no-touch-required") && !options.contains("verify-required")); + assert_eq!(accepted, expected, "{}: {options}", key.algorithm()); + } + } +} + +#[tokio::test] +async fn server_sk_policy_evaluates_matching_entries_independently() { + let keys = keys(); + for key in &keys[..2] { + let public = key.to_openssh().unwrap(); + let restricted = format!("verify-required {public}"); + let allowed = format!("no-touch-required {public}"); + assert!(authenticate(key, &[restricted.clone(), allowed.clone()]).await); + assert!(authenticate(key, &[allowed, restricted]).await); + // An exception for a different key must never relax this key's policy. + let unrelated = format!("no-touch-required {}", keys[2].to_openssh().unwrap()); + assert!(!authenticate(key, &[public, unrelated]).await); + } +} From 0d70b64d998392c43cd4f3c35e087770bcb14cc9 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 22 Oct 2026 23:17:02 +0900 Subject: [PATCH 2/2] fix: Preserve first matching security-key authorization policy Reject immediately when the first matching authorized_keys entry requires an unsupported touch or verification policy. Continuing to a later duplicate allowed a no-touch-required entry to weaken the first entry's policy. Update handler regressions and documentation for duplicate ordering and unrelated keys. Focused tests pass, and restoring the previous continue behavior fails the duplicate-entry regression. Refs #338. --- docs/openssh-compatibility-status.md | 2 ++ docs/security.md | 2 ++ src/server/auth/publickey.rs | 4 +++- tests/server_sk_policy_test.rs | 9 +++++---- 4 files changed, 12 insertions(+), 5 deletions(-) diff --git a/docs/openssh-compatibility-status.md b/docs/openssh-compatibility-status.md index 011b7983e..a16e1911b 100644 --- a/docs/openssh-compatibility-status.md +++ b/docs/openssh-compatibility-status.md @@ -89,3 +89,5 @@ Remaining control details include complete forwarding-channel enumeration, alloc ### Server security-key authorization bssh-server fails closed for `sk-ssh-ed25519@openssh.com` and `sk-ecdsa-sha2-nistp256@openssh.com` user keys: russh does not expose signature UP/UV flags to the authentication callback. An `authorized_keys` line must have `no-touch-required` and must not have `verify-required` to authorize either type. The default touch policy and every verification-required policy are rejected and logged at info level. Ordinary keys are unaffected. This server restriction is distinct from client-side security-key signing and host-key verification described above; [#340](https://github.com/lablup/bssh/issues/340) tracks actual signature-flag enforcement. Security-key user certificates remain out of scope. + +For duplicate user-key entries, the first matching `authorized_keys` line determines the security-key policy. A later `no-touch-required` entry cannot override an earlier matching line that requires touch or verification; unrelated keys are skipped. diff --git a/docs/security.md b/docs/security.md index edac75929..8babfca45 100644 --- a/docs/security.md +++ b/docs/security.md @@ -61,6 +61,8 @@ bssh-server currently cannot inspect the user-presence (UP/touch) or user-verifi Use an ordinary key or an SSH server that enforces UP/UV when touch or verification is required. Only use `no-touch-required` when deliberately authorizing authentication without either requirement; it is a relaxation of policy, not a way to enforce touch. Ordinary public keys are unaffected. Signature-flag enforcement is tracked in [#340](https://github.com/lablup/bssh/issues/340); security-key user certificates are outside this support. +For duplicate user-key entries, the first matching `authorized_keys` line determines the security-key policy. A later `no-touch-required` entry cannot override an earlier matching line that requires touch or verification; unrelated keys are skipped. + ### Password Authentication If password authentication is required, use strong passwords with Argon2id hashing: diff --git a/src/server/auth/publickey.rs b/src/server/auth/publickey.rs index 4fac29c74..5d080706e 100644 --- a/src/server/auth/publickey.rs +++ b/src/server/auth/publickey.rs @@ -306,7 +306,9 @@ impl PublicKeyVerifier { key_type = %key.algorithm(), "Security key rejected: user presence or verification policy cannot be enforced; only no-touch-required without verify-required is supported" ); - continue; + // The first matching entry determines the policy; a later + // duplicate must not weaken its presence/verification requirement. + return Ok(false); } tracing::info!( user = %username, diff --git a/tests/server_sk_policy_test.rs b/tests/server_sk_policy_test.rs index 315d6da2a..02d12cc75 100644 --- a/tests/server_sk_policy_test.rs +++ b/tests/server_sk_policy_test.rs @@ -85,16 +85,17 @@ async fn server_sk_policy_enforces_each_authorized_key_option_combination() { } #[tokio::test] -async fn server_sk_policy_evaluates_matching_entries_independently() { +async fn server_sk_policy_uses_first_matching_entry() { let keys = keys(); for key in &keys[..2] { let public = key.to_openssh().unwrap(); let restricted = format!("verify-required {public}"); let allowed = format!("no-touch-required {public}"); - assert!(authenticate(key, &[restricted.clone(), allowed.clone()]).await); - assert!(authenticate(key, &[allowed, restricted]).await); + assert!(!authenticate(key, &[restricted.clone(), allowed.clone()]).await); + assert!(authenticate(key, &[allowed.clone(), restricted]).await); // An exception for a different key must never relax this key's policy. let unrelated = format!("no-touch-required {}", keys[2].to_openssh().unwrap()); - assert!(!authenticate(key, &[public, unrelated]).await); + assert!(!authenticate(key, &[public, unrelated.clone()]).await); + assert!(authenticate(key, &[unrelated, allowed]).await); } }