From b7852d1d7112c8b3068f4761ea62225a045e3393 Mon Sep 17 00:00:00 2001 From: Firstyear Date: Sun, 5 Nov 2023 10:33:25 +1000 Subject: [PATCH] pw min length in account policy (#2289) --- book/src/account_policy.md | 20 +- libs/client/src/group.rs | 12 + proto/src/constants.rs | 1 + proto/src/v1.rs | 11 +- server/lib/src/constants/acp.rs | 3 + server/lib/src/constants/entries.rs | 3 + server/lib/src/constants/mod.rs | 2 +- server/lib/src/constants/schema.rs | 12 +- server/lib/src/constants/uuids.rs | 2 + server/lib/src/idm/accountpolicy.rs | 21 +- server/lib/src/idm/credupdatesession.rs | 295 ++++++++++++++++++++-- server/lib/src/idm/server.rs | 83 +----- server/lib/src/server/migrations.rs | 1 + tools/cli/src/cli/group/account_policy.rs | 12 + tools/cli/src/opt/kanidm.rs | 8 + 15 files changed, 370 insertions(+), 116 deletions(-) diff --git a/book/src/account_policy.md b/book/src/account_policy.md index 27f8929d3..7082b5418 100644 --- a/book/src/account_policy.md +++ b/book/src/account_policy.md @@ -21,6 +21,7 @@ parts. | value | ordering | | ---------------- | -------------- | | auth-session | smallest value | +| password-minimum-length | largest value | | privilege-expiry | smallest value | ### Example Resolution @@ -29,6 +30,7 @@ If we had two policies where the first defined: ``` auth-session: 86400 +password-minimum-length: 10 privilege-expiry: 600 ``` @@ -36,14 +38,17 @@ And the second ``` auth-session: 3600 +password-minimum-length: 15 privilege-expiry: 3600 ``` As the value of auth-session from the second is smaller we would take that. We would take the -smallest value of privilege-expiry from the first. This leaves: +smallest value of privilege-expiry from the first. We would take the largest value of +password-minimum-length. This leaves: ``` auth-session: 3600 +password-minimum-length: 15 privilege-expiry: 600 ``` @@ -73,6 +78,19 @@ kanidm group account-policy auth-expiry kanidm group account-policy auth-expiry my_admin_group 86400 ``` +## Setting Minimum Password Length + +The password-minimum-length value defines the character length of passwords that are acceptable. There +are no-other tunables for passwords in account policy. Other settings such as complexity, symbols, +numbers and so on, have been proven to not matter in any real world attacks. + +To set this value: + +``` +kanidm group account-policy password-minimum-length +kanidm group account-policy password-minimum-length my_admin_group 12 +``` + ## Setting Maximum Privilege Time The privilege-expiry time defines how long a session retains its write privileges after a diff --git a/libs/client/src/group.rs b/libs/client/src/group.rs index f1b949611..db295fbbf 100644 --- a/libs/client/src/group.rs +++ b/libs/client/src/group.rs @@ -21,6 +21,18 @@ impl KanidmClient { .await } + pub async fn group_account_policy_password_minimum_length_set( + &self, + id: &str, + length: u32, + ) -> Result<(), ClientError> { + self.perform_put_request( + &format!("/v1/group/{}/_attr/auth_password_minimum_length", id), + vec![length.to_string()], + ) + .await + } + pub async fn group_account_policy_privilege_expiry_set( &self, id: &str, diff --git a/proto/src/constants.rs b/proto/src/constants.rs index 15151f64c..d449c1fb0 100644 --- a/proto/src/constants.rs +++ b/proto/src/constants.rs @@ -53,6 +53,7 @@ pub const ATTR_ATTR: &str = "attr"; pub const ATTR_ATTRIBUTENAME: &str = "attributename"; pub const ATTR_ATTRIBUTETYPE: &str = "attributetype"; pub const ATTR_AUTH_SESSION_EXPIRY: &str = "authsession_expiry"; +pub const ATTR_AUTH_PASSWORD_MINIMUM_LENGTH: &str = "auth_password_minimum_length"; pub const ATTR_BADLIST_PASSWORD: &str = "badlist_password"; pub const ATTR_CLAIM: &str = "claim"; pub const ATTR_CLASS: &str = "class"; diff --git a/proto/src/v1.rs b/proto/src/v1.rs index d56337856..7b03233ab 100644 --- a/proto/src/v1.rs +++ b/proto/src/v1.rs @@ -89,7 +89,7 @@ pub enum ConsistencyError { DeniedName(Uuid), } -#[derive(Serialize, Deserialize, Debug, ToSchema)] +#[derive(Serialize, Deserialize, Debug, ToSchema, PartialEq, Eq, PartialOrd, Ord)] #[serde(rename_all = "lowercase")] pub enum PasswordFeedback { // https://docs.rs/zxcvbn/latest/zxcvbn/feedback/enum.Suggestion.html @@ -122,8 +122,9 @@ pub enum PasswordFeedback { NamesAndSurnamesByThemselvesAreEasyToGuess, CommonNamesAndSurnamesAreEasyToGuess, // Custom - TooShort(usize), + TooShort(u32), BadListed, + DontReusePasswords, } /// Human-readable PasswordFeedback result. @@ -163,6 +164,12 @@ impl fmt::Display for PasswordFeedback { PasswordFeedback::DatesAreOftenEasyToGuess => { write!(f, "Dates are often easy to guess.") } + PasswordFeedback::DontReusePasswords => { + write!( + f, + "Don't reuse passwords that already exist on your account" + ) + } PasswordFeedback::NamesAndSurnamesByThemselvesAreEasyToGuess => { write!(f, "Names and surnames by themselves are easy to guess.") } diff --git a/server/lib/src/constants/acp.rs b/server/lib/src/constants/acp.rs index 68d2b3d82..c4e324d83 100644 --- a/server/lib/src/constants/acp.rs +++ b/server/lib/src/constants/acp.rs @@ -1272,16 +1272,19 @@ lazy_static! { Attribute::Name, Attribute::Uuid, Attribute::AuthSessionExpiry, + Attribute::AuthPasswordMinimumLength, Attribute::PrivilegeExpiry, ], modify_removed_attrs: vec![ Attribute::Class, Attribute::AuthSessionExpiry, + Attribute::AuthPasswordMinimumLength, Attribute::PrivilegeExpiry, ], modify_present_attrs: vec![ Attribute::Class, Attribute::AuthSessionExpiry, + Attribute::AuthPasswordMinimumLength, Attribute::PrivilegeExpiry, ], modify_classes: vec![ diff --git a/server/lib/src/constants/entries.rs b/server/lib/src/constants/entries.rs index ad9fdcabf..80a05070d 100644 --- a/server/lib/src/constants/entries.rs +++ b/server/lib/src/constants/entries.rs @@ -54,6 +54,7 @@ pub enum Attribute { AttributeName, AttributeType, AuthSessionExpiry, + AuthPasswordMinimumLength, BadlistPassword, Claim, Class, @@ -231,6 +232,7 @@ impl TryFrom for Attribute { ATTR_ATTRIBUTENAME => Attribute::AttributeName, ATTR_ATTRIBUTETYPE => Attribute::AttributeType, ATTR_AUTH_SESSION_EXPIRY => Attribute::AuthSessionExpiry, + ATTR_AUTH_PASSWORD_MINIMUM_LENGTH => Attribute::AuthPasswordMinimumLength, ATTR_BADLIST_PASSWORD => Attribute::BadlistPassword, ATTR_CLAIM => Attribute::Claim, ATTR_CLASS => Attribute::Class, @@ -384,6 +386,7 @@ impl From for &'static str { Attribute::AttributeName => ATTR_ATTRIBUTENAME, Attribute::AttributeType => ATTR_ATTRIBUTETYPE, Attribute::AuthSessionExpiry => ATTR_AUTH_SESSION_EXPIRY, + Attribute::AuthPasswordMinimumLength => ATTR_AUTH_PASSWORD_MINIMUM_LENGTH, Attribute::BadlistPassword => ATTR_BADLIST_PASSWORD, Attribute::Claim => ATTR_CLAIM, Attribute::Class => ATTR_CLASS, diff --git a/server/lib/src/constants/mod.rs b/server/lib/src/constants/mod.rs index 9a5ca9fc4..8c5f3880f 100644 --- a/server/lib/src/constants/mod.rs +++ b/server/lib/src/constants/mod.rs @@ -76,7 +76,7 @@ pub const RECYCLEBIN_MAX_AGE: u64 = 604_800; pub const AUTH_SESSION_TIMEOUT: u64 = 300; // 5 minute mfa reg window pub const MFAREG_SESSION_TIMEOUT: u64 = 300; -pub const PW_MIN_LENGTH: usize = 10; +pub const PW_MIN_LENGTH: u32 = 10; // Maximum - Sessions have no upper bound. pub const MAXIMUM_AUTH_SESSION_EXPIRY: u32 = u32::MAX; diff --git a/server/lib/src/constants/schema.rs b/server/lib/src/constants/schema.rs index f019645c6..d510e0f12 100644 --- a/server/lib/src/constants/schema.rs +++ b/server/lib/src/constants/schema.rs @@ -229,6 +229,15 @@ pub static ref SCHEMA_ATTR_AUTH_PRIVILEGE_EXPIRY: SchemaAttribute = SchemaAttrib ..Default::default() }; +pub static ref SCHEMA_ATTR_AUTH_PASSWORD_MINIMUM_LENGTH: SchemaAttribute = SchemaAttribute { + uuid: UUID_SCHEMA_ATTR_AUTH_PASSWORD_MINIMUM_LENGTH, + name: Attribute::AuthPasswordMinimumLength.into(), + + description: "Minimum length of passwords.".to_string(), + syntax: SyntaxType::Uint32, + ..Default::default() +}; + pub static ref SCHEMA_ATTR_LOGINSHELL: SchemaAttribute = SchemaAttribute { uuid: UUID_SCHEMA_ATTR_LOGINSHELL, name: Attribute::LoginShell.into(), @@ -636,7 +645,8 @@ pub static ref SCHEMA_CLASS_ACCOUNT_POLICY: SchemaClass = SchemaClass { description: "Policies applied to accounts that are members of a group".to_string(), systemmay: vec![ Attribute::AuthSessionExpiry.into(), - Attribute::PrivilegeExpiry.into() + Attribute::PrivilegeExpiry.into(), + Attribute::AuthPasswordMinimumLength.into(), ], systemsupplements: vec![Attribute::Group.into()], ..Default::default() diff --git a/server/lib/src/constants/uuids.rs b/server/lib/src/constants/uuids.rs index cb53d3faa..6e6587dfc 100644 --- a/server/lib/src/constants/uuids.rs +++ b/server/lib/src/constants/uuids.rs @@ -245,6 +245,8 @@ pub const UUID_SCHEMA_ATTR_LDAP_ALLOW_UNIX_PW_BIND: Uuid = uuid!("00000000-0000-0000-0000-ffff00000145"); pub const UUID_SCHEMA_CLASS_ACCOUNT_POLICY: Uuid = uuid!("00000000-0000-0000-0000-ffff00000146"); +pub const UUID_SCHEMA_ATTR_AUTH_PASSWORD_MINIMUM_LENGTH: Uuid = + uuid!("00000000-0000-0000-0000-ffff00000147"); // System and domain infos // I'd like to strongly criticise william of the past for making poor choices about these allocations. diff --git a/server/lib/src/idm/accountpolicy.rs b/server/lib/src/idm/accountpolicy.rs index c81a2a17b..7dbd82579 100644 --- a/server/lib/src/idm/accountpolicy.rs +++ b/server/lib/src/idm/accountpolicy.rs @@ -32,6 +32,7 @@ impl From for CredentialPolicy { pub(crate) struct AccountPolicy { privilege_expiry: u32, authsession_expiry: u32, + pw_min_length: u32, credential_policy: CredentialPolicy, } @@ -50,21 +51,26 @@ impl From<&EntrySealedCommitted> for Option { let privilege_expiry = val .get_ava_single_uint32(Attribute::PrivilegeExpiry) .unwrap_or(MAXIMUM_AUTH_PRIVILEGE_EXPIRY); + let pw_min_length = val + .get_ava_single_uint32(Attribute::AuthPasswordMinimumLength) + .unwrap_or(PW_MIN_LENGTH); let credential_policy = CredentialPolicy::default(); Some(AccountPolicy { privilege_expiry, authsession_expiry, + pw_min_length, credential_policy, }) } } -#[derive(Clone)] +#[derive(Clone, Debug)] #[cfg_attr(test, derive(Default))] pub(crate) struct ResolvedAccountPolicy { privilege_expiry: u32, authsession_expiry: u32, + pw_min_length: u32, credential_policy: CredentialPolicy, } @@ -77,6 +83,7 @@ impl ResolvedAccountPolicy { let mut accumulate = ResolvedAccountPolicy { privilege_expiry: MAXIMUM_AUTH_PRIVILEGE_EXPIRY, authsession_expiry: MAXIMUM_AUTH_SESSION_EXPIRY, + pw_min_length: PW_MIN_LENGTH, credential_policy: CredentialPolicy::default(), }; @@ -91,6 +98,11 @@ impl ResolvedAccountPolicy { accumulate.authsession_expiry = acc_pol.authsession_expiry } + // Take larger pw min len + if acc_pol.pw_min_length > accumulate.pw_min_length { + accumulate.pw_min_length = acc_pol.pw_min_length + } + // Take the greater credential type policy if acc_pol.credential_policy > accumulate.credential_policy { accumulate.credential_policy = acc_pol.credential_policy @@ -108,6 +120,10 @@ impl ResolvedAccountPolicy { self.authsession_expiry } + pub(crate) fn pw_min_length(&self) -> u32 { + self.pw_min_length + } + /* pub(crate) fn credential_policy(&self) -> CredentialPolicy { self.credential_policy @@ -125,12 +141,14 @@ mod tests { let policy_a = AccountPolicy { privilege_expiry: 100, authsession_expiry: 100, + pw_min_length: 11, credential_policy: CredentialPolicy::MfaRequired, }; let policy_b = AccountPolicy { privilege_expiry: 150, authsession_expiry: 50, + pw_min_length: 15, credential_policy: CredentialPolicy::PasskeyRequired, }; @@ -138,6 +156,7 @@ mod tests { assert_eq!(rap.privilege_expiry(), 100); assert_eq!(rap.authsession_expiry(), 50); + assert_eq!(rap.pw_min_length(), 15); assert_eq!(rap.credential_policy, CredentialPolicy::PasskeyRequired); } diff --git a/server/lib/src/idm/credupdatesession.rs b/server/lib/src/idm/credupdatesession.rs index 7f0444808..422e0a903 100644 --- a/server/lib/src/idm/credupdatesession.rs +++ b/server/lib/src/idm/credupdatesession.rs @@ -27,6 +27,8 @@ use crate::server::access::Access; use crate::utils::{backup_code_from_random, readable_password_from_random, uuid_from_duration}; use crate::value::{CredUpdateSessionPerms, IntentTokenState}; +use super::accountpolicy::ResolvedAccountPolicy; + const MAXIMUM_CRED_UPDATE_TTL: Duration = Duration::from_secs(900); // Default 1 hour. const DEFAULT_INTENT_TTL: Duration = Duration::from_secs(3600); @@ -37,8 +39,9 @@ const MINIMUM_INTENT_TTL: Duration = Duration::from_secs(300); #[derive(Debug)] pub enum PasswordQuality { - TooShort(usize), + TooShort(u32), BadListed, + DontReusePasswords, Feedback(Vec), } @@ -87,6 +90,8 @@ pub(crate) struct CredentialUpdateSession { issuer: String, // Current credentials - these are on the Account! account: Account, + // The account policy applied to this account + resolved_account_policy: ResolvedAccountPolicy, // What intent was used to initiate this session. intent_token_id: Option, @@ -131,6 +136,7 @@ impl fmt::Debug for CredentialUpdateSession { f.debug_struct("CredentialUpdateSession") .field("account.spn", &self.account.spn) .field("account.unix", &self.account.unix_extn().is_some()) + .field("resolved_account_policy", &self.resolved_account_policy) .field("intent_token_id", &self.intent_token_id) .field("primary.detail()", &primary) .field("passkeys.list()", &passkeys) @@ -339,7 +345,7 @@ impl<'a> IdmServerProxyWriteTransaction<'a> { &mut self, target: Uuid, ident: &Identity, - ) -> Result<(Account, CredUpdateSessionPerms), OperationError> { + ) -> Result<(Account, ResolvedAccountPolicy, CredUpdateSessionPerms), OperationError> { let entry = self.qs_write.internal_search_uuid(target)?; security_info!( @@ -356,7 +362,8 @@ impl<'a> IdmServerProxyWriteTransaction<'a> { } // Is target an account? This checks for us. - let account = Account::try_from_entry_rw(entry.as_ref(), &mut self.qs_write)?; + let (account, resolved_account_policy) = + Account::try_from_entry_with_policy(entry.as_ref(), &mut self.qs_write)?; let effective_perms = self .qs_write @@ -512,6 +519,7 @@ impl<'a> IdmServerProxyWriteTransaction<'a> { security_info!(%primary_can_edit, %passkeys_can_edit, %unixcred_can_edit, %sshpubkey_can_edit, %ext_cred_portal_can_view, "Proceeding"); Ok(( account, + resolved_account_policy, CredUpdateSessionPerms { ext_cred_portal_can_view, passkeys_can_edit, @@ -528,6 +536,7 @@ impl<'a> IdmServerProxyWriteTransaction<'a> { sessionid: Uuid, intent_token_id: Option, account: Account, + resolved_account_policy: ResolvedAccountPolicy, perms: CredUpdateSessionPerms, ct: Duration, ) -> Result<(CredentialUpdateSessionToken, CredentialUpdateSessionStatus), OperationError> { @@ -588,6 +597,7 @@ impl<'a> IdmServerProxyWriteTransaction<'a> { // - store account policy (if present) let session = CredentialUpdateSession { account, + resolved_account_policy, issuer, intent_token_id, ext_cred_portal, @@ -638,7 +648,11 @@ impl<'a> IdmServerProxyWriteTransaction<'a> { event: &InitCredentialUpdateIntentEvent, ct: Duration, ) -> Result { - let (account, perms) = self.validate_init_credential_update(event.target, &event.ident)?; + let (account, _resolved_account_policy, perms) = + self.validate_init_credential_update(event.target, &event.ident)?; + + // We should check in the acc-pol if we can proceed? + // Is there a reason account policy might deny us from proceeding? // ==== AUTHORISATION CHECKED === @@ -775,7 +789,8 @@ impl<'a> IdmServerProxyWriteTransaction<'a> { }; // Is target an account? This checks for us. - let account = Account::try_from_entry_rw(entry.as_ref(), &mut self.qs_write)?; + let (account, resolved_account_policy) = + Account::try_from_entry_with_policy(entry.as_ref(), &mut self.qs_write)?; // Check there is not already a user session in progress with this intent token. // Is there a need to revoke intent tokens? @@ -885,24 +900,39 @@ impl<'a> IdmServerProxyWriteTransaction<'a> { // ========== // Okay, good to exchange. - self.create_credupdate_session(session_id, Some(intent_id), account, perms, current_time) + self.create_credupdate_session( + session_id, + Some(intent_id), + account, + resolved_account_policy, + perms, + current_time, + ) } #[instrument(level = "debug", skip_all)] pub fn init_credential_update( &mut self, event: &InitCredentialUpdateEvent, - ct: Duration, + current_time: Duration, ) -> Result<(CredentialUpdateSessionToken, CredentialUpdateSessionStatus), OperationError> { - let (account, perms) = self.validate_init_credential_update(event.target, &event.ident)?; + let (account, resolved_account_policy, perms) = + self.validate_init_credential_update(event.target, &event.ident)?; // ==== AUTHORISATION CHECKED === // This is the expiry time, so that our cleanup task can "purge up to now" rather // than needing to do calculations. - let sessionid = uuid_from_duration(ct + MAXIMUM_CRED_UPDATE_TTL, self.sid); + let sessionid = uuid_from_duration(current_time + MAXIMUM_CRED_UPDATE_TTL, self.sid); // Build the cred update session. - self.create_credupdate_session(sessionid, None, account, perms, ct) + self.create_credupdate_session( + sessionid, + None, + account, + resolved_account_policy, + perms, + current_time, + ) } #[instrument(level = "trace", skip(self))] @@ -1231,25 +1261,48 @@ impl<'a> IdmServerCredUpdateTransaction<'a> { Ok(status) } - #[instrument(level = "debug", skip(self))] + #[instrument(level = "trace", skip(self))] fn check_password_quality( &self, cleartext: &str, + resolved_account_policy: &ResolvedAccountPolicy, related_inputs: &[&str], + radius_secret: Option<&str>, ) -> Result<(), PasswordQuality> { // password strength and badlisting is always global, rather than per-pw-policy. // pw-policy as check on the account is about requirements for mfa for example. // is the password at least 10 char? - if cleartext.len() < PW_MIN_LENGTH { - return Err(PasswordQuality::TooShort(PW_MIN_LENGTH)); + let pw_min_length = resolved_account_policy.pw_min_length(); + if cleartext.len() < pw_min_length as usize { + return Err(PasswordQuality::TooShort(pw_min_length)); + } + + if let Some(some_radius_secret) = radius_secret { + if cleartext.contains(some_radius_secret) { + return Err(PasswordQuality::DontReusePasswords); + } + } + + // zxcvbn doesn't appear to be picking these up? + for related in related_inputs { + if cleartext.contains(related) { + return Err(PasswordQuality::Feedback(vec![ + PasswordFeedback::NamesAndSurnamesByThemselvesAreEasyToGuess, + PasswordFeedback::AvoidDatesAndYearsThatAreAssociatedWithYou, + ])); + } } // does the password pass zxcvbn? - let entropy = zxcvbn::zxcvbn(cleartext, related_inputs).map_err(|e| { admin_error!("zxcvbn check failure (password empty?) {:?}", e); - PasswordQuality::TooShort(PW_MIN_LENGTH) + // Return some generic feedback when the password is this bad. + PasswordQuality::Feedback(vec![ + PasswordFeedback::UseAFewWordsAvoidCommonPhrases, + PasswordFeedback::AddAnotherWordOrTwo, + PasswordFeedback::NoNeedForSymbolsDigitsOrUppercaseLetters, + ]) })?; // PW's should always be enforced as strong as possible. @@ -1263,7 +1316,12 @@ impl<'a> IdmServerCredUpdateTransaction<'a> { .map(|v| v.clone()) .map_err(|e| { security_info!("zxcvbn returned no feedback when score < 3 -> {:?}", e); - PasswordQuality::TooShort(PW_MIN_LENGTH) + // Return some generic feedback when the password is this bad. + PasswordQuality::Feedback(vec![ + PasswordFeedback::UseAFewWordsAvoidCommonPhrases, + PasswordFeedback::AddAnotherWordOrTwo, + PasswordFeedback::NoNeedForSymbolsDigitsOrUppercaseLetters, + ]) })?; security_info!(?feedback, "pw quality feedback"); @@ -1397,17 +1455,24 @@ impl<'a> IdmServerCredUpdateTransaction<'a> { return Err(OperationError::AccessDenied); }; - // Check pw quality (future - acc policy applies). - self.check_password_quality(pw, session.account.related_inputs().as_slice()) - .map_err(|e| match e { - PasswordQuality::TooShort(sz) => { - OperationError::PasswordQuality(vec![PasswordFeedback::TooShort(sz)]) - } - PasswordQuality::BadListed => { - OperationError::PasswordQuality(vec![PasswordFeedback::BadListed]) - } - PasswordQuality::Feedback(feedback) => OperationError::PasswordQuality(feedback), - })?; + self.check_password_quality( + pw, + &session.resolved_account_policy, + session.account.related_inputs().as_slice(), + session.account.radius_secret.as_deref(), + ) + .map_err(|e| match e { + PasswordQuality::TooShort(sz) => { + OperationError::PasswordQuality(vec![PasswordFeedback::TooShort(sz)]) + } + PasswordQuality::BadListed => { + OperationError::PasswordQuality(vec![PasswordFeedback::BadListed]) + } + PasswordQuality::DontReusePasswords => { + OperationError::PasswordQuality(vec![PasswordFeedback::DontReusePasswords]) + } + PasswordQuality::Feedback(feedback) => OperationError::PasswordQuality(feedback), + })?; let ncred = match &session.primary { Some(primary) => { @@ -1832,6 +1897,7 @@ mod tests { use kanidm_proto::v1::{ AuthAllowed, AuthIssueSession, AuthMech, CUExtPortal, CredentialDetailType, + PasswordFeedback, }; use uuid::uuid; use webauthn_authenticator_rs::softpasskey::SoftPasskey; @@ -1845,7 +1911,7 @@ mod tests { use crate::credential::totp::Totp; use crate::event::CreateEvent; use crate::idm::delayed::DelayedAction; - use crate::idm::event::{AuthEvent, AuthResult}; + use crate::idm::event::{AuthEvent, AuthResult, RegenerateRadiusSecretEvent}; use crate::idm::server::{IdmServer, IdmServerDelayed}; use crate::idm::AuthState; use crate::prelude::*; @@ -1997,6 +2063,13 @@ mod tests { .internal_search_uuid(TESTPERSON_UUID) .expect("failed"); + // Setup the radius creds to ensure we don't use them anywhere else. + let rrse = RegenerateRadiusSecretEvent::new_internal(TESTPERSON_UUID); + + let _ = idms_prox_write + .regenerate_radius_secret(&rrse) + .expect("Failed to reset radius credential 1"); + let cur = idms_prox_write.init_credential_update( &InitCredentialUpdateEvent::new_impersonate_entry(testperson), ct, @@ -2370,6 +2443,172 @@ mod tests { .is_none()); } + #[idm_test] + async fn test_idm_credential_update_password_quality_checks( + idms: &IdmServer, + _idms_delayed: &mut IdmServerDelayed, + ) { + let ct = Duration::from_secs(TEST_CURRENT_TIME); + let (cust, _) = setup_test_session(idms, ct).await; + + // Get the radius pw + + let mut r_txn = idms.proxy_read().await; + + let radius_secret = r_txn + .qs_read + .internal_search_uuid(TESTPERSON_UUID) + .expect("No such entry") + .get_ava_single_secret(Attribute::RadiusSecret) + .expect("No radius secret found") + .to_string(); + + drop(r_txn); + + let cutxn = idms.cred_update_transaction().await; + + // Get the credential status - this should tell + // us the details of the credentials, as well as + // if they are ready and valid to commit? + let c_status = cutxn + .credential_update_status(&cust, ct) + .expect("Failed to get the current session status."); + + trace!(?c_status); + + assert!(c_status.primary.is_none()); + + // Test initially creating a credential. + // - pw first + + let err = cutxn + .credential_primary_set_password(&cust, ct, "password") + .unwrap_err(); + trace!(?err); + assert!(match err { + OperationError::PasswordQuality(details) + if details == vec!(PasswordFeedback::TooShort(PW_MIN_LENGTH),) => + true, + _ => false, + }); + + let err = cutxn + .credential_primary_set_password(&cust, ct, "password1234") + .unwrap_err(); + trace!(?err); + assert!(match err { + OperationError::PasswordQuality(details) + if details + == vec!( + PasswordFeedback::AddAnotherWordOrTwo, + PasswordFeedback::ThisIsACommonPassword, + ) => + true, + _ => false, + }); + + let err = cutxn + .credential_primary_set_password(&cust, ct, &radius_secret) + .unwrap_err(); + trace!(?err); + assert!(match err { + OperationError::PasswordQuality(details) + if details == vec!(PasswordFeedback::DontReusePasswords,) => + true, + _ => false, + }); + + let err = cutxn + .credential_primary_set_password(&cust, ct, "testperson2023") + .unwrap_err(); + trace!(?err); + assert!(match err { + OperationError::PasswordQuality(details) + if details + == vec!( + PasswordFeedback::NamesAndSurnamesByThemselvesAreEasyToGuess, + PasswordFeedback::AvoidDatesAndYearsThatAreAssociatedWithYou, + ) => + true, + _ => false, + }); + + let err = cutxn + .credential_primary_set_password( + &cust, + ct, + "demo_badlist_shohfie3aeci2oobur0aru9uushah6EiPi2woh4hohngoighaiRuepieN3ongoo1", + ) + .unwrap_err(); + trace!(?err); + assert!(match err { + OperationError::PasswordQuality(details) + if details == vec!(PasswordFeedback::BadListed) => + true, + _ => false, + }); + + assert!(c_status.can_commit); + + drop(cutxn); + } + + #[idm_test] + async fn test_idm_credential_update_password_min_length_account_policy( + idms: &IdmServer, + _idms_delayed: &mut IdmServerDelayed, + ) { + let ct = Duration::from_secs(TEST_CURRENT_TIME); + + // Set the account policy min pw length + let test_pw_min_length = PW_MIN_LENGTH * 2; + + let mut idms_prox_write = idms.proxy_write(ct).await; + + let modlist = ModifyList::new_purge_and_set( + Attribute::AuthPasswordMinimumLength, + Value::Uint32(test_pw_min_length), + ); + idms_prox_write + .qs_write + .internal_modify_uuid(UUID_IDM_ALL_ACCOUNTS, &modlist) + .expect("Unable to change default session exp"); + + assert!(idms_prox_write.commit().is_ok()); + // This now will affect all accounts for the next cred update. + + let (cust, _) = setup_test_session(idms, ct).await; + + let cutxn = idms.cred_update_transaction().await; + + // Get the credential status - this should tell + // us the details of the credentials, as well as + // if they are ready and valid to commit? + let c_status = cutxn + .credential_update_status(&cust, ct) + .expect("Failed to get the current session status."); + + trace!(?c_status); + + assert!(c_status.primary.is_none()); + + // Test initially creating a credential. + // - pw first + + let err = cutxn + .credential_primary_set_password(&cust, ct, "password") + .unwrap_err(); + trace!(?err); + assert!(match err { + OperationError::PasswordQuality(details) + if details == vec!(PasswordFeedback::TooShort(test_pw_min_length),) => + true, + _ => false, + }); + + drop(cutxn); + } + // Test set of primary account password // - fail pw quality checks etc // - set correctly. diff --git a/server/lib/src/idm/server.rs b/server/lib/src/idm/server.rs index 766214cc4..28d37bb20 100644 --- a/server/lib/src/idm/server.rs +++ b/server/lib/src/idm/server.rs @@ -1557,7 +1557,7 @@ impl<'a> IdmServerProxyWriteTransaction<'a> { // // is the password at least 10 char? - if cleartext.len() < PW_MIN_LENGTH { + if cleartext.len() < PW_MIN_LENGTH as usize { return Err(OperationError::PasswordQuality(vec![ PasswordFeedback::TooShort(PW_MIN_LENGTH), ])); @@ -1678,15 +1678,6 @@ impl<'a> IdmServerProxyWriteTransaction<'a> { // If we got here, then pre-apply succeeded, and that means access control // passed. Now we can do the extra checks. - // Check the password quality. - // Ask if tis all good - this step checks pwpolicy and such - - self.check_password_quality(pce.cleartext.as_str(), account.related_inputs().as_slice()) - .map_err(|e| { - request_error!(err = ?e, "check_password_quality"); - e - })?; - // And actually really apply it now. self.qs_write.modify_apply(mp).map_err(|e| { request_error!(error = ?e); @@ -2541,30 +2532,6 @@ mod tests { assert!(r1 != r2); } - #[idm_test] - async fn test_idm_radius_secret_rejected_from_account_credential( - idms: &IdmServer, - _idms_delayed: &IdmServerDelayed, - ) { - let mut idms_prox_write = idms.proxy_write(duration_from_epoch_now()).await; - let rrse = RegenerateRadiusSecretEvent::new_internal(UUID_ADMIN); - - let r1 = idms_prox_write - .regenerate_radius_secret(&rrse) - .expect("Failed to reset radius credential 1"); - - // Try and set that as the main account password, should fail. - let pce = PasswordChangeEvent::new_internal(UUID_ADMIN, r1.as_str()); - let e = idms_prox_write.set_account_password(&pce); - assert!(e.is_err()); - - let pce = UnixPasswordChangeEvent::new_internal(UUID_ADMIN, r1.as_str()); - let e = idms_prox_write.set_unix_account_password(&pce); - assert!(e.is_err()); - - assert!(idms_prox_write.commit().is_ok()); - } - #[idm_test] async fn test_idm_radiusauthtoken(idms: &IdmServer, _idms_delayed: &IdmServerDelayed) { let mut idms_prox_write = idms.proxy_write(duration_from_epoch_now()).await; @@ -2589,54 +2556,6 @@ mod tests { assert!(r1 == tok_r.secret); } - #[idm_test] - async fn test_idm_simple_password_reject_weak( - idms: &IdmServer, - _idms_delayed: &IdmServerDelayed, - ) { - // len check - let mut idms_prox_write = idms.proxy_write(duration_from_epoch_now()).await; - - let pce = PasswordChangeEvent::new_internal(UUID_ADMIN, "password"); - let e = idms_prox_write.set_account_password(&pce); - assert!(e.is_err()); - - // zxcvbn check - let pce = PasswordChangeEvent::new_internal(UUID_ADMIN, "password1234"); - let e = idms_prox_write.set_account_password(&pce); - assert!(e.is_err()); - - // Check the "name" checking works too (I think admin may hit a common pw rule first) - let pce = PasswordChangeEvent::new_internal(UUID_ADMIN, "admin_nta"); - let e = idms_prox_write.set_account_password(&pce); - assert!(e.is_err()); - - // Check that the demo badlist password is rejected. - let pce = PasswordChangeEvent::new_internal( - UUID_ADMIN, - "demo_badlist_shohfie3aeci2oobur0aru9uushah6EiPi2woh4hohngoighaiRuepieN3ongoo1", - ); - let e = idms_prox_write.set_account_password(&pce); - assert!(e.is_err()); - - assert!(idms_prox_write.commit().is_ok()); - } - - #[idm_test] - async fn test_idm_simple_password_reject_badlist( - idms: &IdmServer, - _idms_delayed: &IdmServerDelayed, - ) { - let mut idms_prox_write = idms.proxy_write(duration_from_epoch_now()).await; - - // Check that the badlist password inserted is rejected. - let pce = PasswordChangeEvent::new_internal(UUID_ADMIN, "bad@no3IBTyqHu$list"); - let e = idms_prox_write.set_account_password(&pce); - assert!(e.is_err()); - - assert!(idms_prox_write.commit().is_ok()); - } - #[idm_test] async fn test_idm_unixusertoken(idms: &IdmServer, _idms_delayed: &IdmServerDelayed) { let mut idms_prox_write = idms.proxy_write(duration_from_epoch_now()).await; diff --git a/server/lib/src/server/migrations.rs b/server/lib/src/server/migrations.rs index fa2d30abe..885bb8848 100644 --- a/server/lib/src/server/migrations.rs +++ b/server/lib/src/server/migrations.rs @@ -558,6 +558,7 @@ impl<'a> QueryServerWriteTransaction<'a> { SCHEMA_ATTR_API_TOKEN_SESSION.clone().into(), SCHEMA_ATTR_AUTH_SESSION_EXPIRY.clone().into(), SCHEMA_ATTR_AUTH_PRIVILEGE_EXPIRY.clone().into(), + SCHEMA_ATTR_AUTH_PASSWORD_MINIMUM_LENGTH.clone().into(), SCHEMA_ATTR_BADLIST_PASSWORD.clone().into(), SCHEMA_ATTR_CREDENTIAL_UPDATE_INTENT_TOKEN.clone().into(), SCHEMA_ATTR_DEVICEKEYS.clone().into(), diff --git a/tools/cli/src/cli/group/account_policy.rs b/tools/cli/src/cli/group/account_policy.rs index 67b895cd9..f85d18d99 100644 --- a/tools/cli/src/cli/group/account_policy.rs +++ b/tools/cli/src/cli/group/account_policy.rs @@ -6,6 +6,7 @@ impl GroupAccountPolicyOpt { match self { GroupAccountPolicyOpt::Enable { copt, .. } | GroupAccountPolicyOpt::AuthSessionExpiry { copt, .. } + | GroupAccountPolicyOpt::PasswordMinimumLength { copt, .. } | GroupAccountPolicyOpt::PrivilegedSessionExpiry { copt, .. } => copt.debug, } } @@ -31,6 +32,17 @@ impl GroupAccountPolicyOpt { println!("Updated authsession expiry."); } } + GroupAccountPolicyOpt::PasswordMinimumLength { name, length, copt } => { + let client = copt.to_client(OpType::Write).await; + if let Err(e) = client + .group_account_policy_password_minimum_length_set(name, *length) + .await + { + handle_client_error(e, copt.output_mode); + } else { + println!("Updated password minimum length."); + } + } GroupAccountPolicyOpt::PrivilegedSessionExpiry { name, expiry, copt } => { let client = copt.to_client(OpType::Write).await; if let Err(e) = client diff --git a/tools/cli/src/opt/kanidm.rs b/tools/cli/src/opt/kanidm.rs index 82b7861b6..654a7e968 100644 --- a/tools/cli/src/opt/kanidm.rs +++ b/tools/cli/src/opt/kanidm.rs @@ -125,6 +125,14 @@ pub enum GroupAccountPolicyOpt { #[clap(flatten)] copt: CommonOpt, }, + /// Set the minimum length of passwords for accounts + #[clap(name = "password-minimum-length")] + PasswordMinimumLength { + name: String, + length: u32, + #[clap(flatten)] + copt: CommonOpt, + }, /// Configure and display the privilege session expiry /// Set the maximum time for privilege session expiry #[clap(name = "privilege-expiry")]