From ab2ea8cfa53cd32b9075f7c1af32325137cddccf Mon Sep 17 00:00:00 2001 From: Conrad Ludgate Date: Tue, 4 Jul 2023 14:54:59 +0100 Subject: [PATCH] use pbkdf2 crate (#4626) ## Problem While pbkdf2 is a simple algorithm, we should probably use a well tested implementation ## Summary of changes * Use pbkdf2 crate * Use arrays like the hmac comment says ## Checklist before requesting a review - [X] I have performed a self-review of my code. - [X] If it is a core feature, I have added thorough tests. - [ ] Do we need to implement analytics? if so did you add the relevant metrics to the dashboard? - [ ] If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section. --- Cargo.lock | 11 ++++++ Cargo.toml | 1 + proxy/Cargo.toml | 1 + proxy/src/scram.rs | 71 +++++++++++++++++++++++++++++++++---- proxy/src/scram/password.rs | 52 ++++++++++++++++++++------- 5 files changed, 116 insertions(+), 20 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0c0ed530fb..b163d4fe46 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2617,6 +2617,16 @@ dependencies = [ "windows-sys 0.45.0", ] +[[package]] +name = "pbkdf2" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f0ca0b5a68607598bf3bad68f32227a8164f6254833f84eafaac409cd6746c31" +dependencies = [ + "digest", + "hmac", +] + [[package]] name = "peeking_take_while" version = "0.1.2" @@ -3010,6 +3020,7 @@ dependencies = [ "once_cell", "opentelemetry", "parking_lot 0.12.1", + "pbkdf2", "pin-project-lite", "postgres-native-tls", "postgres_backend", diff --git a/Cargo.toml b/Cargo.toml index 4b9bd8b961..f36e8f6569 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -86,6 +86,7 @@ opentelemetry = "0.18.0" opentelemetry-otlp = { version = "0.11.0", default_features=false, features = ["http-proto", "trace", "http", "reqwest-client"] } opentelemetry-semantic-conventions = "0.10.0" parking_lot = "0.12" +pbkdf2 = "0.12.1" pin-project-lite = "0.2" prometheus = {version = "0.13", default_features=false, features = ["process"]} # removes protobuf dependency prost = "0.11" diff --git a/proxy/Cargo.toml b/proxy/Cargo.toml index c8b0cc7cf0..438dd62315 100644 --- a/proxy/Cargo.toml +++ b/proxy/Cargo.toml @@ -29,6 +29,7 @@ metrics.workspace = true once_cell.workspace = true opentelemetry.workspace = true parking_lot.workspace = true +pbkdf2.workspace = true pin-project-lite.workspace = true postgres_backend.workspace = true pq_proto.workspace = true diff --git a/proxy/src/scram.rs b/proxy/src/scram.rs index 7cc4191435..85854427ed 100644 --- a/proxy/src/scram.rs +++ b/proxy/src/scram.rs @@ -45,17 +45,74 @@ fn hmac_sha256<'a>(key: &[u8], parts: impl IntoIterator) -> [u8 let mut mac = Hmac::::new_from_slice(key).expect("bad key size"); parts.into_iter().for_each(|s| mac.update(s)); - // TODO: maybe newer `hmac` et al already migrated to regular arrays? - let mut result = [0u8; 32]; - result.copy_from_slice(mac.finalize().into_bytes().as_slice()); - result + mac.finalize().into_bytes().into() } fn sha256<'a>(parts: impl IntoIterator) -> [u8; 32] { let mut hasher = Sha256::new(); parts.into_iter().for_each(|s| hasher.update(s)); - let mut result = [0u8; 32]; - result.copy_from_slice(hasher.finalize().as_slice()); - result + hasher.finalize().into() +} + +#[cfg(test)] +mod tests { + use crate::sasl::{Mechanism, Step}; + + use super::{password::SaltedPassword, Exchange, ServerSecret}; + + #[test] + fn happy_path() { + let iterations = 4096; + let salt_base64 = "QSXCR+Q6sek8bf92"; + let pw = SaltedPassword::new( + b"pencil", + base64::decode(salt_base64).unwrap().as_slice(), + iterations, + ); + + let secret = ServerSecret { + iterations, + salt_base64: salt_base64.to_owned(), + stored_key: pw.client_key().sha256(), + server_key: pw.server_key(), + doomed: false, + }; + const NONCE: [u8; 18] = [ + 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, + ]; + let mut exchange = Exchange::new(&secret, || NONCE, None); + + let client_first = "n,,n=user,r=rOprNGfwEbeRWgbNEkqO"; + let client_final = "c=biws,r=rOprNGfwEbeRWgbNEkqOAQIDBAUGBwgJCgsMDQ4PEBES,p=rw1r5Kph5ThxmaUBC2GAQ6MfXbPnNkFiTIvdb/Rear0="; + let server_first = + "r=rOprNGfwEbeRWgbNEkqOAQIDBAUGBwgJCgsMDQ4PEBES,s=QSXCR+Q6sek8bf92,i=4096"; + let server_final = "v=qtUDIofVnIhM7tKn93EQUUt5vgMOldcDVu1HC+OH0o0="; + + exchange = match exchange.exchange(client_first).unwrap() { + Step::Continue(exchange, message) => { + assert_eq!(message, server_first); + exchange + } + Step::Success(_, _) => panic!("expected continue, got success"), + Step::Failure(f) => panic!("{f}"), + }; + + let key = match exchange.exchange(client_final).unwrap() { + Step::Success(key, message) => { + assert_eq!(message, server_final); + key + } + Step::Continue(_, _) => panic!("expected success, got continue"), + Step::Failure(f) => panic!("{f}"), + }; + + assert_eq!( + key.as_bytes(), + [ + 74, 103, 1, 132, 12, 31, 200, 48, 28, 54, 82, 232, 207, 12, 138, 189, 40, 32, 134, + 27, 125, 170, 232, 35, 171, 167, 166, 41, 70, 228, 182, 112, + ] + ); + } } diff --git a/proxy/src/scram/password.rs b/proxy/src/scram/password.rs index 656780d853..022f2842dd 100644 --- a/proxy/src/scram/password.rs +++ b/proxy/src/scram/password.rs @@ -14,19 +14,7 @@ impl SaltedPassword { /// See `scram-common.c : scram_SaltedPassword` for details. /// Further reading: (see `PBKDF2`). pub fn new(password: &[u8], salt: &[u8], iterations: u32) -> SaltedPassword { - let one = 1_u32.to_be_bytes(); // magic - - let mut current = super::hmac_sha256(password, [salt, &one]); - let mut result = current; - for _ in 1..iterations { - current = super::hmac_sha256(password, [current.as_ref()]); - // TODO: result = current.zip(result).map(|(x, y)| x ^ y), issue #80094 - for (i, x) in current.iter().enumerate() { - result[i] ^= x; - } - } - - result.into() + pbkdf2::pbkdf2_hmac_array::(password, salt, iterations).into() } /// Derive `ClientKey` from a salted hashed password. @@ -46,3 +34,41 @@ impl From<[u8; SALTED_PASSWORD_LEN]> for SaltedPassword { Self { bytes } } } + +#[cfg(test)] +mod tests { + use super::SaltedPassword; + + fn legacy_pbkdf2_impl(password: &[u8], salt: &[u8], iterations: u32) -> SaltedPassword { + let one = 1_u32.to_be_bytes(); // magic + + let mut current = super::super::hmac_sha256(password, [salt, &one]); + let mut result = current; + for _ in 1..iterations { + current = super::super::hmac_sha256(password, [current.as_ref()]); + // TODO: result = current.zip(result).map(|(x, y)| x ^ y), issue #80094 + for (i, x) in current.iter().enumerate() { + result[i] ^= x; + } + } + + result.into() + } + + #[test] + fn pbkdf2() { + let password = "a-very-secure-password"; + let salt = "such-a-random-salt"; + let iterations = 4096; + let output = [ + 203, 18, 206, 81, 4, 154, 193, 100, 147, 41, 211, 217, 177, 203, 69, 210, 194, 211, + 101, 1, 248, 156, 96, 0, 8, 223, 30, 87, 158, 41, 20, 42, + ]; + + let actual = SaltedPassword::new(password.as_bytes(), salt.as_bytes(), iterations); + let expected = legacy_pbkdf2_impl(password.as_bytes(), salt.as_bytes(), iterations); + + assert_eq!(actual.bytes, output); + assert_eq!(actual.bytes, expected.bytes); + } +}