Auth cleanup

lib/cluelessh-keys/src/public.rs

@@ -2,14 +2,15 @@
 
 // <https://datatracker.ietf.org/doc/html/rfc4716> exists but is kinda weird
 
-use std::fmt::Display;
+use std::fmt::{Debug, Display};
 
 use base64::Engine;
-use tracing::debug;
 
 use cluelessh_format::{ParseError, Reader, Writer};
 
-#[derive(Debug, Clone, PartialEq, Eq)]
+use crate::signature::Signature;
+
+#[derive(Clone, PartialEq, Eq)]
 pub enum PublicKey {
     Ed25519 {
         public_key: ed25519_dalek::VerifyingKey,
@@ -87,27 +88,14 @@ impl PublicKey {
         }
     }
 
-    pub fn verify_signature(&self, data: &[u8], signature: &[u8]) -> bool {
+    pub fn verify_signature(&self, data: &[u8], signature: &Signature) -> bool {
         match self {
-            PublicKey::Ed25519 { public_key } => {
-                let mut s = Reader::new(signature);
-                let Ok(alg) = s.utf8_string() else {
-                    return false;
-                };
-                if alg != "ssh-ed25519" {
-                    return false;
+            PublicKey::Ed25519 { public_key } => match signature {
+                Signature::Ed25519 { signature } => {
+                    public_key.verify_strict(data, &signature).is_ok()
                 }
-                let Ok(signature) = s.string() else {
-                    return false;
-                };
-
-                let Ok(signature) = ed25519_dalek::Signature::from_slice(signature) else {
-                    debug!("Invalid signature length");
-                    return false;
-                };
-
-                public_key.verify_strict(data, &signature).is_ok()
-            }
+                _ => false,
+            },
             PublicKey::EcdsaSha2NistP256 { .. } => {
                 todo!("ecdsa-sha2-nistp256 signature verification")
             }
@@ -115,6 +103,12 @@ impl PublicKey {
     }
 }
 
+impl Debug for PublicKey {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        Display::fmt(&self, f)
+    }
+}
+
 impl Display for PublicKey {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {

lib/cluelessh-keys/src/signature.rs

@@ -18,7 +18,7 @@ pub fn signature_data(session_id: [u8; 32], username: &str, pubkey: &PublicKey)
     s.finish()
 }
 
-#[derive(Debug)]
+#[derive(Debug, Clone, Copy)]
 pub enum Signature {
     Ed25519 { signature: ed25519_dalek::Signature },
     EcdsaSha2NistP256 { signature: p256::ecdsa::Signature },

lib/cluelessh-protocol/src/lib.rs

@@ -277,6 +277,7 @@ pub mod auth {
     use std::collections::{HashSet, VecDeque};
 
     use cluelessh_format::{numbers, NameList};
+    use cluelessh_keys::{public::PublicKey, signature::Signature};
     use cluelessh_transport::{packet::Packet, peer_error, Result};
     use tracing::debug;
 
@@ -293,7 +294,7 @@ pub mod auth {
     pub enum ServerRequest {
         VerifyPassword(VerifyPassword),
         /// Check whether a pubkey is usable.
-        CheckPubkey(CheckPubkey),
+        CheckPubkey(CheckPublicKey),
         /// Verify the signature from a pubkey.
         VerifySignature(VerifySignature),
     }
@@ -305,20 +306,18 @@ pub mod auth {
     }
 
     #[derive(Debug, Clone)]
-    pub struct CheckPubkey {
+    pub struct CheckPublicKey {
         pub user: String,
-        pub session_identifier: [u8; 32],
-        pub pubkey_alg_name: String,
-        pub pubkey: Vec<u8>,
+        pub public_key: PublicKey,
     }
 
     #[derive(Debug, Clone)]
     pub struct VerifySignature {
         pub user: String,
         pub session_identifier: [u8; 32],
-        pub pubkey_alg_name: String,
-        pub pubkey: Vec<u8>,
-        pub signature: Vec<u8>,
+        pub public_key: PublicKey,
+        /// The signature. Guaranteed to match the algorithm of `public_key`.
+        pub signature: Signature,
     }
 
     #[derive(Debug, PartialEq, Eq, Hash)]
@@ -405,24 +404,31 @@ pub mod auth {
                     let pubkey_alg_name = p.utf8_string()?;
                     let public_key_blob = p.string()?;
 
+                    let public_key = PublicKey::from_wire_encoding(public_key_blob)?;
+                    if pubkey_alg_name != public_key.algorithm_name() {
+                        return Err(peer_error!("algorithm name mismatch"));
+                    }
+
                     // Whether the client is just checking whether the public key is allowed.
                     if !has_signature {
-                        self.server_requests
-                            .push_back(ServerRequest::CheckPubkey(CheckPubkey {
+                        self.server_requests.push_back(ServerRequest::CheckPubkey(
+                            CheckPublicKey {
                                 user: username.to_owned(),
-                                session_identifier: self.session_ident,
-                                pubkey_alg_name: pubkey_alg_name.to_owned(),
-                                pubkey: public_key_blob.to_vec(),
-                            }));
+                                public_key,
+                            },
+                        ));
                     } else {
                         let signature = p.string()?;
+                        let signature = Signature::from_wire_encoding(signature)?;
+                        if signature.algorithm_name() != public_key.algorithm_name() {
+                            return Err(peer_error!("signature algorithm name mismatch"));
+                        }
                         self.server_requests
                             .push_back(ServerRequest::VerifySignature(VerifySignature {
                                 user: username.to_owned(),
                                 session_identifier: self.session_ident,
-                                pubkey_alg_name: pubkey_alg_name.to_owned(),
-                                pubkey: public_key_blob.to_vec(),
-                                signature: signature.to_vec(),
+                                public_key,
+                                signature,
                             }));
                     }
                 }
@@ -443,9 +449,12 @@ pub mod auth {
             Ok(())
         }
 
-        pub fn pubkey_check_result(&mut self, is_ok: bool, alg: &str, key_blob: &[u8]) {
+        pub fn pubkey_check_result(&mut self, is_ok: bool, key: PublicKey) {
             if is_ok {
-                self.queue_packet(Packet::new_msg_userauth_pk_ok(alg.as_bytes(), key_blob));
+                self.queue_packet(Packet::new_msg_userauth_pk_ok(
+                    key.algorithm_name().as_bytes(),
+                    &key.to_wire_encoding(),
+                ));
             } else {
                 self.send_failure();
                 // It's ok, don't treat this as a fatal failure.

lib/cluelessh-tokio/src/server.rs

@@ -14,7 +14,7 @@ use tokio::{
 };
 
 use cluelessh_protocol::{
-    auth::{AuthOption, CheckPubkey, VerifyPassword, VerifySignature},
+    auth::{AuthOption, CheckPublicKey, VerifyPassword, VerifySignature},
     ChannelUpdateKind, SshStatus,
 };
 use eyre::{eyre, ContextCompat, OptionExt, Result, WrapErr};
@@ -53,7 +53,7 @@ pub struct ServerConnection<S> {
 
 enum Operation {
     VerifyPassword(String, Result<bool>),
-    CheckPubkey(Result<bool>, String, Vec<u8>),
+    CheckPubkey(Result<bool>, PublicKey),
     VerifySignature(String, Result<bool>),
     KeyExchangeResponseReceived(Result<KeyExchangeResponse>),
 }
@@ -64,7 +64,7 @@ pub type AuthFn<A, R> = Arc<dyn Fn(A) -> BoxFuture<'static, R> + Send + Sync>;
 pub struct ServerAuth {
     pub verify_password: Option<AuthFn<VerifyPassword, Result<bool>>>,
     pub verify_signature: Option<AuthFn<VerifySignature, Result<bool>>>,
-    pub check_pubkey: Option<AuthFn<CheckPubkey, Result<bool>>>,
+    pub check_pubkey: Option<AuthFn<CheckPublicKey, Result<bool>>>,
     pub do_key_exchange: AuthFn<KeyExchangeParameters, Result<KeyExchangeResponse>>,
     pub auth_banner: Option<String>,
 }
@@ -215,8 +215,7 @@ impl<S: AsyncRead + AsyncWrite> ServerConnection<S> {
                             let _ = send
                                 .send(Operation::CheckPubkey(
                                     result,
-                                    check_pubkey.pubkey_alg_name,
-                                    check_pubkey.pubkey,
+                                    check_pubkey.public_key,
                                 ))
                                 .await;
                         });
@@ -350,8 +349,8 @@ impl<S: AsyncRead + AsyncWrite> ServerConnection<S> {
                     Some(Operation::VerifySignature(user, result)) => if let Some(auth) = self.proto.auth() {
                         auth.verification_result(result?, user);
                     },
-                    Some(Operation::CheckPubkey(result, alg, key_blob)) => if let Some(auth) = self.proto.auth() {
-                        auth.pubkey_check_result(result?, &alg, &key_blob);
+                    Some(Operation::CheckPubkey(result, public_key)) => if let Some(auth) = self.proto.auth() {
+                        auth.pubkey_check_result(result?, public_key);
                     },
                     Some(Operation::VerifyPassword(user, result)) => if let Some(auth) = self.proto.auth() {
                         auth.verification_result(result?, user);