Skip to content

Commit 31cf233

Browse files
committed
Make password hash iterations configurable
Move password hash iterations to a system var. Defaults to 600_000 and uses 600_000 for all system users.
1 parent a0ee2a0 commit 31cf233

File tree

12 files changed

+122
-38
lines changed

12 files changed

+122
-38
lines changed

src/adapter/src/catalog/open.rs

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -324,7 +324,10 @@ impl Catalog {
324324
if let Some(password) = config.external_login_password_mz_system {
325325
let role_auth = RoleAuth {
326326
role_id: MZ_SYSTEM_ROLE_ID,
327-
password_hash: Some(scram256_hash(&password).map_err(|_| {
327+
// The external login password for mz_system should use
328+
// the default hash iterations as that is a known secure
329+
// setting.
330+
password_hash: Some(scram256_hash(&password, &None).map_err(|_| {
328331
AdapterError::Internal("Failed to hash mz_system password.".to_owned())
329332
})?),
330333
updated_at: SYSTEM_TIME(),
@@ -1097,17 +1100,17 @@ fn add_new_remove_old_builtin_cluster_replicas_migration(
10971100
.get_mut(&cluster.id)
10981101
.unwrap_or(&mut empty_map);
10991102

1100-
let builtin_cluster_boostrap_config =
1103+
let builtin_cluster_bootstrap_config =
11011104
builtin_cluster_config_map.get_config(builtin_replica.cluster_name)?;
11021105
if replica_names.remove(builtin_replica.name).is_none()
11031106
// NOTE(SangJunBak): We need to explicitly check the replication factor because
11041107
// BUILT_IN_CLUSTER_REPLICAS is constant throughout all deployments but the replication
11051108
// factor is configurable on bootstrap.
1106-
&& builtin_cluster_boostrap_config.replication_factor > 0
1109+
&& builtin_cluster_bootstrap_config.replication_factor > 0
11071110
{
11081111
let replica_size = match cluster.config.variant {
11091112
ClusterVariant::Managed(ClusterVariantManaged { ref size, .. }) => size.clone(),
1110-
ClusterVariant::Unmanaged => builtin_cluster_boostrap_config.size,
1113+
ClusterVariant::Unmanaged => builtin_cluster_bootstrap_config.size,
11111114
};
11121115

11131116
let config = builtin_cluster_replica_config(replica_size);

src/adapter/src/catalog/transact.rs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ use mz_repr::{CatalogItemId, ColumnName, Diff, GlobalId, SqlColumnType, strconv}
4646
use mz_sql::ast::RawDataType;
4747
use mz_sql::catalog::{
4848
CatalogDatabase, CatalogError as SqlCatalogError, CatalogItem as SqlCatalogItem, CatalogRole,
49-
CatalogSchema, DefaultPrivilegeAclItem, DefaultPrivilegeObject, PasswordAction,
49+
CatalogSchema, DefaultPrivilegeAclItem, DefaultPrivilegeObject, PasswordAction, PasswordConfig,
5050
RoleAttributesRaw, RoleMembership, RoleVars,
5151
};
5252
use mz_sql::names::{
@@ -680,12 +680,17 @@ impl Catalog {
680680

681681
let mut existing_role = state.get_role(&id).clone();
682682
let password = attributes.password.clone();
683+
let non_default_password_hash_iterations =
684+
attributes.non_default_password_hash_iterations.clone();
683685
existing_role.attributes = attributes.into();
684686
existing_role.vars = vars;
685687
let password_action = if nopassword {
686688
PasswordAction::Clear
687689
} else if let Some(password) = password {
688-
PasswordAction::Set(password)
690+
PasswordAction::Set(PasswordConfig {
691+
password,
692+
non_default_password_hash_iterations,
693+
})
689694
} else {
690695
PasswordAction::NoChange
691696
};

src/adapter/src/coord/command_handler.rs

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -363,7 +363,15 @@ impl Coordinator {
363363
mock_nonce: String,
364364
nonce: String,
365365
tx: oneshot::Sender<Result<SASLChallengeResponse, AdapterError>>| {
366-
let opts = mz_auth::hash::mock_sasl_challenge(&role_name, &mock_nonce);
366+
let opts = mz_auth::hash::mock_sasl_challenge(
367+
&role_name,
368+
&mock_nonce,
369+
&Some(
370+
self.catalog()
371+
.system_config()
372+
.default_password_hash_iterations(),
373+
),
374+
);
367375
let _ = tx.send(Ok(SASLChallengeResponse {
368376
iteration_count: mz_ore::cast::u32_to_usize(opts.iterations.get()),
369377
salt: BASE64_STANDARD.encode(opts.salt),

src/adapter/src/coord/sequencer/inner.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3530,6 +3530,11 @@ impl Coordinator {
35303530

35313531
if let Some(password) = attrs.password {
35323532
attributes.password = Some(password);
3533+
attributes.non_default_password_hash_iterations = Some(
3534+
self.catalog()
3535+
.system_config()
3536+
.default_password_hash_iterations(),
3537+
)
35333538
}
35343539

35353540
if let Some(superuser) = attrs.superuser {

src/auth/src/hash.rs

Lines changed: 32 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -18,15 +18,15 @@ use itertools::Itertools;
1818

1919
use crate::password::Password;
2020

21-
/// The default iteration count as suggested by
22-
/// <https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html>
23-
const DEFAULT_ITERATIONS: NonZeroU32 = NonZeroU32::new(600_000).unwrap();
24-
2521
/// The default salt size, which isn't currently configurable.
2622
const DEFAULT_SALT_SIZE: usize = 32;
2723

2824
const SHA256_OUTPUT_LEN: usize = 32;
2925

26+
/// The default iteration count as suggested by
27+
/// <https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html>
28+
const DEFAULT_ITERATIONS: NonZeroU32 = NonZeroU32::new(600_000).unwrap();
29+
3030
/// The options for hashing a password
3131
#[derive(Debug, PartialEq)]
3232
pub struct HashOpts {
@@ -70,21 +70,22 @@ impl Display for HashError {
7070

7171
/// Hashes a password using PBKDF2 with SHA256
7272
/// and a random salt.
73-
pub fn hash_password(password: &Password) -> Result<PasswordHash, HashError> {
73+
pub fn hash_password(
74+
password: &Password,
75+
iterations: &Option<NonZeroU32>,
76+
) -> Result<PasswordHash, HashError> {
7477
let mut salt = [0u8; DEFAULT_SALT_SIZE];
7578
openssl::rand::rand_bytes(&mut salt).map_err(HashError::Openssl)?;
79+
let iterations = iterations.unwrap_or(DEFAULT_ITERATIONS);
7680

7781
let hash = hash_password_inner(
78-
&HashOpts {
79-
iterations: DEFAULT_ITERATIONS,
80-
salt,
81-
},
82+
&HashOpts { iterations, salt },
8283
password.to_string().as_bytes(),
8384
)?;
8485

8586
Ok(PasswordHash {
8687
salt,
87-
iterations: DEFAULT_ITERATIONS,
88+
iterations,
8889
hash,
8990
})
9091
}
@@ -115,8 +116,11 @@ pub fn hash_password_with_opts(
115116
/// Hashes a password using PBKDF2 with SHA256,
116117
/// and returns it in the SCRAM-SHA-256 format.
117118
/// The format is SCRAM-SHA-256$<iterations>:<salt>$<stored_key>:<server_key>
118-
pub fn scram256_hash(password: &Password) -> Result<String, HashError> {
119-
let hashed_password = hash_password(password)?;
119+
pub fn scram256_hash(
120+
password: &Password,
121+
iterations: &Option<NonZeroU32>,
122+
) -> Result<String, HashError> {
123+
let hashed_password = hash_password(password, iterations)?;
120124
Ok(scram256_hash_inner(hashed_password).to_string())
121125
}
122126

@@ -207,14 +211,18 @@ fn generate_signature(key: &[u8], message: &str) -> Result<Vec<u8>, VerifyError>
207211
// Generate a mock challenge based on the username and client nonce
208212
// We do this so that we can present a deterministic challenge even for
209213
// nonexistent users, to avoid user enumeration attacks.
210-
pub fn mock_sasl_challenge(username: &str, mock_nonce: &str) -> HashOpts {
214+
pub fn mock_sasl_challenge(
215+
username: &str,
216+
mock_nonce: &str,
217+
iterations: &Option<NonZeroU32>,
218+
) -> HashOpts {
211219
let mut buf = Vec::with_capacity(username.len() + mock_nonce.len());
212220
buf.extend_from_slice(username.as_bytes());
213221
buf.extend_from_slice(mock_nonce.as_bytes());
214222
let digest = openssl::sha::sha256(&buf);
215223

216224
HashOpts {
217-
iterations: DEFAULT_ITERATIONS,
225+
iterations: iterations.unwrap_or(DEFAULT_ITERATIONS),
218226
salt: digest,
219227
}
220228
}
@@ -326,7 +334,11 @@ mod tests {
326334
#[cfg_attr(miri, ignore)] // unsupported operation: can't call foreign function `OPENSSL_init_ssl` on OS `linux`
327335
fn test_hash_password() {
328336
let password = "password".to_string();
329-
let hashed_password = hash_password(&password.into()).expect("Failed to hash password");
337+
let hashed_password = hash_password(
338+
&password.into(),
339+
&Some(NonZeroU32::new(100).expect("Trust me on this")),
340+
)
341+
.expect("Failed to hash password");
330342
assert_eq!(hashed_password.iterations, DEFAULT_ITERATIONS);
331343
assert_eq!(hashed_password.salt.len(), DEFAULT_SALT_SIZE);
332344
assert_eq!(hashed_password.hash.len(), SHA256_OUTPUT_LEN);
@@ -336,7 +348,7 @@ mod tests {
336348
#[cfg_attr(miri, ignore)] // unsupported operation: can't call foreign function `OPENSSL_init_ssl` on OS `linux`
337349
fn test_scram256_hash() {
338350
let password = "password".into();
339-
let scram_hash = scram256_hash(&password).expect("Failed to hash password");
351+
let scram_hash = scram256_hash(&password, &None).expect("Failed to hash password");
340352

341353
let res = scram256_verify(&password, &scram_hash);
342354
assert!(res.is_ok());
@@ -363,16 +375,16 @@ mod tests {
363375
fn test_mock_sasl_challenge() {
364376
let username = "alice";
365377
let mock = "cnonce";
366-
let opts1 = mock_sasl_challenge(username, mock);
367-
let opts2 = mock_sasl_challenge(username, mock);
378+
let opts1 = mock_sasl_challenge(username, mock, &None);
379+
let opts2 = mock_sasl_challenge(username, mock, &None);
368380
assert_eq!(opts1, opts2);
369381
}
370382

371383
#[mz_ore::test]
372384
#[cfg_attr(miri, ignore)]
373385
fn test_sasl_verify_success() {
374386
let password: Password = "password".into();
375-
let hashed_password = scram256_hash(&password).expect("hash password");
387+
let hashed_password = scram256_hash(&password, &None).expect("hash password");
376388
let auth_message = "n=user,r=clientnonce,s=somesalt"; // arbitrary auth message
377389

378390
// Parse client_key and server_key from the SCRAM hash
@@ -426,7 +438,7 @@ mod tests {
426438
#[cfg_attr(miri, ignore)]
427439
fn test_sasl_verify_invalid_proof() {
428440
let password: Password = "password".into();
429-
let hashed_password = scram256_hash(&password).expect("hash password");
441+
let hashed_password = scram256_hash(&password, &None).expect("hash password");
430442
let auth_message = "n=user,r=clientnonce,s=somesalt";
431443
// Provide an obviously invalid base64 proof (different size / random)
432444
let bad_proof = BASE64_STANDARD.encode([0u8; 32]);

src/catalog/src/durable/transaction.rs

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -357,8 +357,11 @@ impl<'a> Transaction<'a> {
357357
oid: u32,
358358
) -> Result<(), CatalogError> {
359359
if let Some(ref password) = attributes.password {
360-
let hash =
361-
mz_auth::hash::scram256_hash(password).expect("password hash should be valid");
360+
let hash = mz_auth::hash::scram256_hash(
361+
password,
362+
&attributes.non_default_password_hash_iterations,
363+
)
364+
.expect("password hash should be valid");
362365
match self.role_auth.insert(
363366
RoleAuthKey { role_id: id },
364367
RoleAuthValue {
@@ -1489,8 +1492,11 @@ impl<'a> Transaction<'a> {
14891492

14901493
match password {
14911494
PasswordAction::Set(new_password) => {
1492-
let hash = mz_auth::hash::scram256_hash(&new_password)
1493-
.expect("password hash should be valid");
1495+
let hash = mz_auth::hash::scram256_hash(
1496+
&new_password.password,
1497+
&new_password.non_default_password_hash_iterations,
1498+
)
1499+
.expect("password hash should be valid");
14941500
let value = RoleAuthValue {
14951501
password_hash: Some(hash),
14961502
updated_at: SYSTEM_TIME(),

src/sql/src/catalog.rs

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ use std::collections::{BTreeMap, BTreeSet};
1616
use std::error::Error;
1717
use std::fmt;
1818
use std::fmt::{Debug, Display, Formatter};
19+
use std::num::NonZeroU32;
1920
use std::str::FromStr;
2021
use std::sync::LazyLock;
2122
use std::time::{Duration, Instant};
@@ -486,11 +487,20 @@ pub trait CatalogSchema {
486487
fn privileges(&self) -> &PrivilegeMap;
487488
}
488489

490+
/// Parameters used to modify password
491+
#[derive(Debug, Clone, Eq, PartialEq, Arbitrary)]
492+
pub struct PasswordConfig {
493+
/// The Password.
494+
pub password: Password,
495+
/// a non default iteration count for hashing the password.
496+
pub non_default_password_hash_iterations: Option<NonZeroU32>,
497+
}
498+
489499
/// A modification of a role password in the catalog
490500
#[derive(Debug, Clone, Eq, PartialEq, Arbitrary)]
491501
pub enum PasswordAction {
492502
/// Set a new password.
493-
Set(Password),
503+
Set(PasswordConfig),
494504
/// Remove the existing password.
495505
Clear,
496506
/// Leave the existing password unchanged.
@@ -499,14 +509,16 @@ pub enum PasswordAction {
499509

500510
/// A raw representation of attributes belonging to a [`CatalogRole`] that we might
501511
/// get as input from the user. This includes the password.
502-
/// This struct explicity does not implement `Serialize` or `Deserialize` to avoid
512+
/// This struct explicitly does not implement `Serialize` or `Deserialize` to avoid
503513
/// accidentally serializing passwords.
504514
#[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Arbitrary)]
505515
pub struct RoleAttributesRaw {
506516
/// Indicates whether the role has inheritance of privileges.
507517
pub inherit: bool,
508518
/// The raw password of the role. This is for self managed auth, not cloud.
509519
pub password: Option<Password>,
520+
/// An optoinal override of the hash iterations used to securely store passwords. This is for self-managed auth
521+
pub non_default_password_hash_iterations: Option<NonZeroU32>,
510522
/// Whether or not this user is a superuser.
511523
pub superuser: Option<bool>,
512524
/// Whether this role is login
@@ -534,6 +546,7 @@ impl RoleAttributesRaw {
534546
RoleAttributesRaw {
535547
inherit: true,
536548
password: None,
549+
non_default_password_hash_iterations: None,
537550
superuser: None,
538551
login: None,
539552
_private: (),
@@ -604,6 +617,7 @@ impl From<RoleAttributes> for RoleAttributesRaw {
604617
RoleAttributesRaw {
605618
inherit,
606619
password: None,
620+
non_default_password_hash_iterations: None,
607621
superuser,
608622
login,
609623
_private: (),
@@ -616,6 +630,7 @@ impl From<PlannedRoleAttributes> for RoleAttributesRaw {
616630
PlannedRoleAttributes {
617631
inherit,
618632
password,
633+
non_default_password_hash_iterations,
619634
superuser,
620635
login,
621636
..
@@ -625,6 +640,7 @@ impl From<PlannedRoleAttributes> for RoleAttributesRaw {
625640
RoleAttributesRaw {
626641
inherit: inherit.unwrap_or(default_attributes.inherit),
627642
password,
643+
non_default_password_hash_iterations,
628644
superuser,
629645
login,
630646
_private: (),

0 commit comments

Comments
 (0)