Skip to content

Commit

Permalink
proxy: use postgres_protocol scram/sasl code (#4748)
Browse files Browse the repository at this point in the history
1) `scram::password` was used in tests only. can be replaced with
`postgres_protocol::password`.
2) `postgres_protocol::authentication::sasl` provides a client impl of
SASL which improves our ability to test
  • Loading branch information
conradludgate authored Feb 19, 2024
1 parent 587cb70 commit d0d4871
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 128 deletions.
5 changes: 2 additions & 3 deletions proxy/src/proxy/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,8 @@ struct Scram(scram::ServerSecret);

impl Scram {
fn new(password: &str) -> 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).context("failed to generate scram secret")?;
Ok(Scram(secret))
}

Expand Down
56 changes: 38 additions & 18 deletions proxy/src/scram.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@ mod messages;
mod secret;
mod signature;

#[cfg(any(test, doc))]
mod password;

pub use exchange::{exchange, Exchange};
pub use key::ScramKey;
pub use secret::ServerSecret;
Expand Down Expand Up @@ -59,27 +56,21 @@ fn sha256<'a>(parts: impl IntoIterator<Item = &'a [u8]>) -> [u8; 32] {

#[cfg(test)]
mod tests {
use postgres_protocol::authentication::sasl::{ChannelBinding, ScramSha256};

use crate::sasl::{Mechanism, Step};

use super::{password::SaltedPassword, Exchange, ServerSecret};
use super::{Exchange, ServerSecret};

#[test]
fn happy_path() {
fn snapshot() {
let iterations = 4096;
let salt_base64 = "QSXCR+Q6sek8bf92";
let pw = SaltedPassword::new(
b"pencil",
base64::decode(salt_base64).unwrap().as_slice(),
iterations,
);
let salt = "QSXCR+Q6sek8bf92";
let stored_key = "FO+9jBb3MUukt6jJnzjPZOWc5ow/Pu6JtPyju0aqaE8=";
let server_key = "qxJ1SbmSAi5EcS0J5Ck/cKAm/+Ixa+Kwp63f4OHDgzo=";
let secret = format!("SCRAM-SHA-256${iterations}:{salt}${stored_key}:{server_key}",);
let secret = ServerSecret::parse(&secret).unwrap();

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,
];
Expand Down Expand Up @@ -121,4 +112,33 @@ mod tests {
]
);
}

fn run_round_trip_test(server_password: &str, client_password: &str) {
let scram_secret = ServerSecret::build(server_password).unwrap();
let sasl_client =
ScramSha256::new(client_password.as_bytes(), ChannelBinding::unsupported());

let outcome = super::exchange(
&scram_secret,
sasl_client,
crate::config::TlsServerEndPoint::Undefined,
)
.unwrap();

match outcome {
crate::sasl::Outcome::Success(_) => {}
crate::sasl::Outcome::Failure(r) => panic!("{r}"),
}
}

#[test]
fn round_trip() {
run_round_trip_test("pencil", "pencil")
}

#[test]
#[should_panic(expected = "password doesn't match")]
fn failure() {
run_round_trip_test("pencil", "eraser")
}
}
2 changes: 1 addition & 1 deletion proxy/src/scram/key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
/// Faithfully taken from PostgreSQL.
pub const SCRAM_KEY_LEN: usize = 32;

/// One of the keys derived from the [password](super::password::SaltedPassword).
/// One of the keys derived from the user's password.
/// We use the same structure for all keys, i.e.
/// `ClientKey`, `StoredKey`, and `ServerKey`.
#[derive(Clone, Default, PartialEq, Eq, Debug)]
Expand Down
74 changes: 0 additions & 74 deletions proxy/src/scram/password.rs

This file was deleted.

37 changes: 5 additions & 32 deletions proxy/src/scram/secret.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
use super::base64_decode_array;
use super::key::ScramKey;

/// Server secret is produced from [password](super::password::SaltedPassword)
/// Server secret is produced from user's password,
/// and is used throughout the authentication process.
#[derive(Clone, Eq, PartialEq, Debug)]
pub struct ServerSecret {
Expand Down Expand Up @@ -59,21 +59,10 @@ impl ServerSecret {
/// 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;
}

let password = super::password::SaltedPassword::new(password.as_bytes(), salt, iterations);

Some(Self {
iterations,
salt_base64: base64::encode(salt),
stored_key: password.client_key().sha256(),
server_key: password.server_key(),
doomed: false,
})
pub fn build(password: &str) -> Option<Self> {
Self::parse(&postgres_protocol::password::scram_sha_256(
password.as_bytes(),
))
}
}

Expand Down Expand Up @@ -103,20 +92,4 @@ mod tests {
assert_eq!(base64::encode(parsed.stored_key), stored_key);
assert_eq!(base64::encode(parsed.server_key), server_key);
}

#[test]
fn build_scram_secret() {
let salt = b"salt";
let secret = ServerSecret::build("password", salt, 4096).unwrap();
assert_eq!(secret.iterations, 4096);
assert_eq!(secret.salt_base64, base64::encode(salt));
assert_eq!(
base64::encode(secret.stored_key.as_ref()),
"lF4cRm/Jky763CN4HtxdHnjV4Q8AWTNlKvGmEFFU8IQ="
);
assert_eq!(
base64::encode(secret.server_key.as_ref()),
"ub8OgRsftnk2ccDMOt7ffHXNcikRkQkq1lh4xaAqrSw="
);
}
}

1 comment on commit d0d4871

@github-actions
Copy link

@github-actions github-actions bot commented on d0d4871 Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2517 tests run: 2389 passed, 0 failed, 128 skipped (full report)


Flaky tests (10)

Postgres 16

  • test_statvfs_pressure_usage: debug
  • test_empty_branch_remote_storage_upload: debug
  • test_remote_timeline_client_calls_started_metric: debug
  • test_sharding_split_smoke: release
  • test_tenant_delete_scrubber: debug
  • test_empty_tenant_size: debug
  • test_vm_bit_clear_on_heap_lock: debug

Postgres 15

  • test_pageserver_init_node_id: debug

Postgres 14

  • test_compute_pageserver_connection_stress: release
  • test_sharding_split_smoke: release

Code coverage* (full report)

  • functions: 28.8% (6673 of 23157 functions)
  • lines: 47.7% (40409 of 84718 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
d0d4871 at 2024-02-19T15:35:33.688Z :recycle:

Please sign in to comment.