[proxy] Prevent unauthorized wake-ups in the "password hack" flow

proxy/src/auth/backend.rs

@@ -11,7 +11,7 @@ use crate::{
         provider::{CachedNodeInfo, ConsoleReqExtra},
         Api,
     },
-    stream, url,
+    scram, stream, url,
 };
 use futures::TryFutureExt;
 use std::borrow::Cow;
@@ -106,6 +106,23 @@ impl<'a, T, E> BackendType<'a, Result<T, E>> {
     }
 }
 
+impl console::AuthInfo {
+    /// Either it's our way ([SCRAM](crate::scram)) or the highway :)
+    /// But seriously, we don't aim to support anything but SCRAM for now.
+    fn scram_or_goodbye(self) -> auth::Result<scram::ServerSecret> {
+        match self {
+            Self::Md5(_) => {
+                info!("auth endpoint chooses MD5");
+                Err(auth::AuthError::bad_auth_method("MD5"))
+            }
+            Self::Scram(secret) => {
+                info!("auth endpoint chooses SCRAM");
+                Ok(secret)
+            }
+        }
+    }
+}
+
 /// True to its name, this function encapsulates our current auth trade-offs.
 /// Here, we choose the appropriate auth flow based on circumstances.
 async fn auth_quirks(

proxy/src/auth/backend/classic.rs

@@ -2,7 +2,7 @@ use super::AuthSuccess;
 use crate::{
     auth::{self, AuthFlow, ClientCredentials},
     compute,
-    console::{self, AuthInfo, CachedNodeInfo, ConsoleReqExtra},
+    console::{self, CachedNodeInfo, ConsoleReqExtra},
     sasl, scram,
     stream::PqStream,
 };
@@ -37,31 +37,16 @@ async fn do_scram(
     Ok(keys)
 }
 
-pub(super) async fn authenticate(
+pub async fn authenticate(
     api: &impl console::Api,
     extra: &ConsoleReqExtra<'_>,
     creds: &ClientCredentials<'_>,
     client: &mut PqStream<impl AsyncRead + AsyncWrite + Unpin>,
 ) -> auth::Result<AuthSuccess<CachedNodeInfo>> {
-    info!("fetching user's authentication info");
-    let info = api.get_auth_info(extra, creds).await?.unwrap_or_else(|| {
-        // If we don't have an authentication secret, we mock one to
-        // prevent malicious probing (possible due to missing protocol steps).
-        // This mocked secret will never lead to successful authentication.
-        info!("authentication info not found, mocking it");
-        AuthInfo::Scram(scram::ServerSecret::mock(creds.user, rand::random()))
-    });
+    let info = console::get_auth_info(api, extra, creds).await?;
 
-    let scram_keys = match info {
-        AuthInfo::Md5(_) => {
-            info!("auth endpoint chooses MD5");
-            return Err(auth::AuthError::bad_auth_method("MD5"));
-        }
-        AuthInfo::Scram(secret) => {
-            info!("auth endpoint chooses SCRAM");
-            do_scram(secret, creds, client).await?
-        }
-    };
+    let secret = info.scram_or_goodbye()?;
+    let scram_keys = do_scram(secret, creds, client).await?;
 
     let mut node = api.wake_compute(extra, creds).await?;
     node.config.auth_keys(AuthKeys::ScramSha256(scram_keys));

proxy/src/auth/backend/hacks.rs

@@ -5,11 +5,33 @@ use crate::{
         self,
         provider::{CachedNodeInfo, ConsoleReqExtra},
     },
-    stream,
+    stream::PqStream,
 };
 use tokio::io::{AsyncRead, AsyncWrite};
 use tracing::{info, warn};
 
+/// Wake the compute node, but only if the password is valid.
+async fn get_compute(
+    api: &impl console::Api,
+    extra: &ConsoleReqExtra<'_>,
+    creds: &mut ClientCredentials<'_>,
+    password: Vec<u8>,
+) -> auth::Result<CachedNodeInfo> {
+    // TODO: this will slow down both "hacks" below; we probably need a cache.
+    let info = console::get_auth_info(api, extra, creds).await?;
+
+    let secret = info.scram_or_goodbye()?;
+    if !secret.matches_password(&password) {
+        info!("our obscure magic indicates that the password doesn't match");
+        return Err(auth::AuthError::auth_failed(creds.user));
+    }
+
+    let mut node = api.wake_compute(extra, creds).await?;
+    node.config.password(password);
+
+    Ok(node)
+}
+
 /// Compared to [SCRAM](crate::scram), cleartext password auth saves
 /// one round trip and *expensive* computations (>= 4096 HMAC iterations).
 /// These properties are benefical for serverless JS workers, so we
@@ -18,7 +40,7 @@ pub async fn cleartext_hack(
     api: &impl console::Api,
     extra: &ConsoleReqExtra<'_>,
     creds: &mut ClientCredentials<'_>,
-    client: &mut stream::PqStream<impl AsyncRead + AsyncWrite + Unpin>,
+    client: &mut PqStream<impl AsyncRead + AsyncWrite + Unpin>,
 ) -> auth::Result<AuthSuccess<CachedNodeInfo>> {
     warn!("cleartext auth flow override is enabled, proceeding");
     let password = AuthFlow::new(client)
@@ -27,8 +49,7 @@ pub async fn cleartext_hack(
         .authenticate()
         .await?;
 
-    let mut node = api.wake_compute(extra, creds).await?;
-    node.config.password(password);
+    let node = get_compute(api, extra, creds, password).await?;
 
     // Report tentative success; compute node will check the password anyway.
     Ok(AuthSuccess {
@@ -43,7 +64,7 @@ pub async fn password_hack(
     api: &impl console::Api,
     extra: &ConsoleReqExtra<'_>,
     creds: &mut ClientCredentials<'_>,
-    client: &mut stream::PqStream<impl AsyncRead + AsyncWrite + Unpin>,
+    client: &mut PqStream<impl AsyncRead + AsyncWrite + Unpin>,
 ) -> auth::Result<AuthSuccess<CachedNodeInfo>> {
     warn!("project not specified, resorting to the password hack auth flow");
     let payload = AuthFlow::new(client)
@@ -55,8 +76,8 @@ pub async fn password_hack(
     info!(project = &payload.project, "received missing parameter");
     creds.project = Some(payload.project.into());
 
-    let mut node = api.wake_compute(extra, creds).await?;
-    node.config.password(payload.password);
+    let password = payload.password.into();
+    let node = get_compute(api, extra, creds, password).await?;
 
     // Report tentative success; compute node will check the password anyway.
     Ok(AuthSuccess {

proxy/src/console.rs

@@ -6,7 +6,9 @@ pub mod messages;
 
 /// Wrappers for console APIs and their mocks.
 pub mod provider;
-pub use provider::{errors, Api, AuthInfo, CachedNodeInfo, ConsoleReqExtra, NodeInfo};
+pub use provider::{
+    errors, get_auth_info, Api, AuthInfo, CachedNodeInfo, ConsoleReqExtra, NodeInfo,
+};
 
 /// Various cache-related types.
 pub mod caches {

proxy/src/console/provider.rs

@@ -9,6 +9,7 @@ use crate::{
 };
 use async_trait::async_trait;
 use std::sync::Arc;
+use tracing::info;
 
 pub mod errors {
     use crate::{
@@ -175,6 +176,12 @@ pub struct NodeInfo {
 pub type NodeInfoCache = TimedLru<Arc<str>, NodeInfo>;
 pub type CachedNodeInfo = timed_lru::Cached<&'static NodeInfoCache>;
 
+/// Various caches for [`console`].
+pub struct ApiCaches {
+    /// Cache for the `wake_compute` API method.
+    pub node_info: NodeInfoCache,
+}
+
 /// This will allocate per each call, but the http requests alone
 /// already require a few allocations, so it should be fine.
 #[async_trait]
@@ -194,8 +201,21 @@ pub trait Api {
     ) -> Result<CachedNodeInfo, errors::WakeComputeError>;
 }
 
-/// Various caches for [`console`].
-pub struct ApiCaches {
-    /// Cache for the `wake_compute` API method.
-    pub node_info: NodeInfoCache,
+/// A more insightful version of [`Api::get_auth_info`] which
+/// knows what to do when we get [`None`] instead of [`AuthInfo`].
+pub async fn get_auth_info(
+    api: &impl Api,
+    extra: &ConsoleReqExtra<'_>,
+    creds: &ClientCredentials<'_>,
+) -> Result<AuthInfo, errors::GetAuthInfoError> {
+    info!("fetching user's authentication info");
+    let info = api.get_auth_info(extra, creds).await?.unwrap_or_else(|| {
+        // If we don't have an authentication secret, we mock one to
+        // prevent malicious probing (possible due to missing protocol steps).
+        // This mocked secret will never lead to successful authentication.
+        info!("authentication info not found, mocking it");
+        AuthInfo::Scram(scram::ServerSecret::mock(creds.user, rand::random()))
+    });
+
+    Ok(info)
 }

proxy/src/proxy/tests.rs

@@ -92,10 +92,10 @@ impl TestAuth for NoAuth {}
 struct Scram(scram::ServerSecret);
 
 impl Scram {
-    fn new(password: &str) -> anyhow::Result<Self> {
+    fn new(password: &[u8]) -> anyhow::Result<Self> {
         let salt = rand::random::<[u8; 16]>();
-        let secret = scram::ServerSecret::build(password, &salt, 256)
-            .context("failed to generate scram secret")?;
+        let secret = scram::ServerSecret::build(password, &salt, 256);
+
         Ok(Scram(secret))
     }
 
@@ -230,11 +230,11 @@ async fn keepalive_is_inherited() -> anyhow::Result<()> {
 }
 
 #[rstest]
-#[case("password_foo")]
-#[case("pwd-bar")]
-#[case("")]
+#[case(b"password_foo")]
+#[case(b"pwd-bar")]
+#[case(b"")]
 #[tokio::test]
-async fn scram_auth_good(#[case] password: &str) -> anyhow::Result<()> {
+async fn scram_auth_good(#[case] password: &[u8]) -> anyhow::Result<()> {
     let (client, server) = tokio::io::duplex(1024);
 
     let (client_config, server_config) =

proxy/src/scram.rs

@@ -12,7 +12,6 @@ mod messages;
 mod secret;
 mod signature;
 
-#[cfg(test)]
 mod password;
 
 pub use exchange::Exchange;

proxy/src/scram/exchange.rs

@@ -73,7 +73,7 @@ impl sasl::Mechanism for Exchange<'_> {
 
                 let server_first_message = client_first_message.build_server_first_message(
                     &(self.nonce)(),
-                    &self.secret.salt_base64,
+                    &self.secret.salt,
                     self.secret.iterations,
                 );
                 let msg = server_first_message.as_str().to_owned();

proxy/src/scram/messages.rs

@@ -75,19 +75,27 @@ impl<'a> ClientFirstMessage<'a> {
     pub fn build_server_first_message(
         &self,
         nonce: &[u8; SCRAM_RAW_NONCE_LEN],
-        salt_base64: &str,
+        salt: &[u8],
         iterations: u32,
     ) -> OwnedServerFirstMessage {
         use std::fmt::Write;
 
         let mut message = String::new();
+
+        // Write base64-encoded combined nonce.
         write!(&mut message, "r={}", self.nonce).unwrap();
         base64::encode_config_buf(nonce, base64::STANDARD, &mut message);
         let combined_nonce = 2..message.len();
-        write!(&mut message, ",s={},i={}", salt_base64, iterations).unwrap();
+
+        // Write base64-encoded salt.
+        write!(&mut message, ",s=").unwrap();
+        base64::encode_config_buf(salt, base64::STANDARD, &mut message);
+
+        // Write number of iterations.
+        write!(&mut message, ",i={iterations}").unwrap();
 
         // This design guarantees that it's impossible to create a
-        // server-first-message without receiving a client-first-message
+        // server-first-message without receiving a client-first-message.
         OwnedServerFirstMessage {
             message,
             nonce: combined_nonce,
@@ -229,4 +237,49 @@ mod tests {
             "SRpfsIVS4Gk11w1LqQ4QvCUBZYQmqXNSDEcHqbQ3CHI="
         );
     }
+
+    #[test]
+    fn build_server_messages() {
+        let input = "n,,n=pepe,r=t8JwklwKecDLwSsA72rHmVju";
+        let client_first_message = ClientFirstMessage::parse(&input).unwrap();
+
+        let nonce = [0; 18];
+        let salt = [1, 2, 3];
+        let iterations = 4096;
+        let server_first_message =
+            client_first_message.build_server_first_message(&nonce, &salt, iterations);
+
+        assert_eq!(
+            server_first_message.message,
+            "r=t8JwklwKecDLwSsA72rHmVjuAAAAAAAAAAAAAAAAAAAAAAAA,s=AQID,i=4096"
+        );
+        assert_eq!(
+            server_first_message.nonce(),
+            "t8JwklwKecDLwSsA72rHmVjuAAAAAAAAAAAAAAAAAAAAAAAA"
+        );
+
+        let input = [
+            "c=eSws",
+            "r=iiYEfS3rOgn8S3rtpSdrOsHtPLWvIkdgmHxA0hf3JNOAG4dU",
+            "p=SRpfsIVS4Gk11w1LqQ4QvCUBZYQmqXNSDEcHqbQ3CHI=",
+        ]
+        .join(",");
+
+        let client_final_message = ClientFinalMessage::parse(&input).unwrap();
+
+        let signature_builder = SignatureBuilder {
+            client_first_message_bare: client_first_message.bare,
+            server_first_message: server_first_message.as_str(),
+            client_final_message_without_proof: &client_final_message.without_proof,
+        };
+
+        let server_key = ScramKey::default();
+        let server_final_message =
+            client_final_message.build_server_final_message(signature_builder, &server_key);
+
+        assert_eq!(
+            server_final_message,
+            "v=XEL4X1vy5LnqIgOo4hOjm7zd1Ceyo9+nBUE+/zVnqLE="
+        );
+    }
 }

proxy/src/scram/password.rs

@@ -1,6 +1,7 @@
 //! Password hashing routines.
 
 use super::key::ScramKey;
+use tracing::warn;
 
 pub const SALTED_PASSWORD_LEN: usize = 32;
 
@@ -13,7 +14,12 @@ pub struct SaltedPassword {
 impl SaltedPassword {
     /// See `scram-common.c : scram_SaltedPassword` for details.
     /// Further reading: <https://datatracker.ietf.org/doc/html/rfc2898> (see `PBKDF2`).
+    /// TODO: implement proper password normalization required by the RFC!
     pub fn new(password: &[u8], salt: &[u8], iterations: u32) -> SaltedPassword {
+        if !password.is_ascii() {
+            warn!("found non-ascii symbols in password! salted password might be broken");
+        }
+
         let one = 1_u32.to_be_bytes(); // magic
 
         let mut current = super::hmac_sha256(password, [salt, &one]);
@@ -30,6 +36,7 @@ impl SaltedPassword {
     }
 
     /// Derive `ClientKey` from a salted hashed password.
+    #[cfg(test)]
     pub fn client_key(&self) -> ScramKey {
         super::hmac_sha256(&self.bytes, [b"Client Key".as_ref()]).into()
     }

proxy/src/scram/secret.rs

@@ -1,15 +1,14 @@
 //! Tools for SCRAM server secret management.
 
-use super::base64_decode_array;
-use super::key::ScramKey;
+use super::{base64_decode_array, key::ScramKey, password::SaltedPassword};
 
-/// Server secret is produced from [password](super::password::SaltedPassword)
+/// Server secret is produced from [password](SaltedPassword)
 /// and is used throughout the authentication process.
 pub struct ServerSecret {
     /// Number of iterations for `PBKDF2` function.
     pub iterations: u32,
     /// Salt used to hash user's password.
-    pub salt_base64: String,
+    pub salt: Vec<u8>,
     /// Hashed `ClientKey`.
     pub stored_key: ScramKey,
     /// Used by client to verify server's signature.
@@ -30,7 +29,7 @@ impl ServerSecret {
 
         let secret = ServerSecret {
             iterations: iterations.parse().ok()?,
-            salt_base64: salt.to_owned(),
+            salt: base64::decode(salt).ok()?,
             stored_key: base64_decode_array(stored_key)?.into(),
             server_key: base64_decode_array(server_key)?.into(),
             doomed: false,
@@ -48,31 +47,31 @@ impl ServerSecret {
 
         Self {
             iterations: 4096,
-            salt_base64: base64::encode(mocked_salt),
+            salt: mocked_salt.into(),
             stored_key: ScramKey::default(),
             server_key: ScramKey::default(),
             doomed: true,
         }
     }
 
+    /// Check if this secret was derived from the given password.
+    pub fn matches_password(&self, password: &[u8]) -> bool {
+        let password = SaltedPassword::new(password, &self.salt, self.iterations);
+        self.server_key == password.server_key()
+    }
+
     /// Build a new server secret from the prerequisites.
-    /// XXX: We only use this function in tests.
     #[cfg(test)]
-    pub fn build(password: &str, salt: &[u8], iterations: u32) -> Option<Self> {
-        // TODO: implement proper password normalization required by the RFC
-        if !password.is_ascii() {
-            return None;
-        }
+    pub fn build(password: &[u8], salt: &[u8], iterations: u32) -> Self {
+        let password = SaltedPassword::new(password, salt, iterations);
 
-        let password = super::password::SaltedPassword::new(password.as_bytes(), salt, iterations);
-
-        Some(Self {
+        Self {
             iterations,
-            salt_base64: base64::encode(salt),
+            salt: salt.into(),
             stored_key: password.client_key().sha256(),
             server_key: password.server_key(),
             doomed: false,
-        })
+        }
     }
 }
 
@@ -87,17 +86,11 @@ mod tests {
         let stored_key = "D5h6KTMBlUvDJk2Y8ELfC1Sjtc6k9YHjRyuRZyBNJns=";
         let server_key = "Pi3QHbcluX//NDfVkKlFl88GGzlJ5LkyPwcdlN/QBvI=";
 
-        let secret = format!(
-            "SCRAM-SHA-256${iterations}:{salt}${stored_key}:{server_key}",
-            iterations = iterations,
-            salt = salt,
-            stored_key = stored_key,
-            server_key = server_key,
-        );
+        let secret = format!("SCRAM-SHA-256${iterations}:{salt}${stored_key}:{server_key}");
 
         let parsed = ServerSecret::parse(&secret).unwrap();
         assert_eq!(parsed.iterations, iterations);
-        assert_eq!(parsed.salt_base64, salt);
+        assert_eq!(base64::encode(parsed.salt), salt);
 
         assert_eq!(base64::encode(parsed.stored_key), stored_key);
         assert_eq!(base64::encode(parsed.server_key), server_key);
@@ -106,9 +99,9 @@ mod tests {
     #[test]
     fn build_scram_secret() {
         let salt = b"salt";
-        let secret = ServerSecret::build("password", salt, 4096).unwrap();
+        let secret = ServerSecret::build(b"password", salt, 4096);
         assert_eq!(secret.iterations, 4096);
-        assert_eq!(secret.salt_base64, base64::encode(salt));
+        assert_eq!(secret.salt, salt);
         assert_eq!(
             base64::encode(secret.stored_key.as_ref()),
             "lF4cRm/Jky763CN4HtxdHnjV4Q8AWTNlKvGmEFFU8IQ="
@@ -118,4 +111,12 @@ mod tests {
             "ub8OgRsftnk2ccDMOt7ffHXNcikRkQkq1lh4xaAqrSw="
         );
     }
+
+    #[test]
+    fn secret_match_password() {
+        let password = b"password";
+        let secret = ServerSecret::build(password, b"salt", 2);
+        assert!(secret.matches_password(password));
+        assert!(!secret.matches_password(b"different"));
+    }
 }