From 0adc3e0dd9c9aefe0c8419ffb366dd21570ba9f1 Mon Sep 17 00:00:00 2001 From: James Hodgkinson Date: Thu, 5 Oct 2023 12:30:46 +1000 Subject: [PATCH] Chasing wooly quadrapeds again (#2163) * I really like well-tended yaks * documenting yaks * spellink * less surprise more good * schema test fix * clippyisms --- Cargo.toml | 1 + libs/client/src/lib.rs | 1 - proto/src/constants.rs | 14 +++--- proto/src/v1.rs | 16 ++++++- server/core/Cargo.toml | 3 +- .../core/src/https/middleware/compression.rs | 44 ++++++++++------- server/lib/src/be/idl_sqlite.rs | 4 +- server/lib/src/constants/entries.rs | 48 +++++++------------ server/lib/src/constants/schema.rs | 2 - server/lib/src/entry.rs | 9 ++-- server/lib/src/filter.rs | 20 ++++---- server/lib/src/lib.rs | 4 +- server/lib/src/value.rs | 1 - server/lib/src/valueset/image/mod.rs | 8 +--- 14 files changed, 85 insertions(+), 90 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index fecbba5b1..693b024ec 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -126,6 +126,7 @@ gix = { version = "0.53.1", default-features = false } gloo = "^0.8.1" hashbrown = { version = "0.14.1", features = ["serde", "inline-more", "ahash"] } hex = "^0.4.3" +http = "0.2.9" hyper = { version = "0.14.27", features = ["full"] } hyper-tls = "0.5.0" idlset = "^0.2.4" diff --git a/libs/client/src/lib.rs b/libs/client/src/lib.rs index d7a52306e..aeb43c4da 100644 --- a/libs/client/src/lib.rs +++ b/libs/client/src/lib.rs @@ -172,7 +172,6 @@ impl KanidmClientBuilder { } } - // TODO #725: Handle these errors better, or at least provide diagnostics - this currently fails silently let mut f = File::open(ca_path).map_err(|e| { error!("{:?}", e); ClientError::ConfigParseIssue(format!("{:?}", e)) diff --git a/proto/src/constants.rs b/proto/src/constants.rs index a8544c8ec..9443ff6f3 100644 --- a/proto/src/constants.rs +++ b/proto/src/constants.rs @@ -161,13 +161,6 @@ pub const ATTR_USERPASSWORD: &str = "userpassword"; pub const ATTR_UUID: &str = "uuid"; pub const ATTR_VERSION: &str = "version"; -// TODO: rust can't deal with this being compiled out, don't try and #[cfg()] them -pub const TEST_ATTR_NON_EXIST: &str = "non-exist"; -pub const TEST_ATTR_TEST_ATTR: &str = "testattr"; -pub const TEST_ATTR_EXTRA: &str = "extra"; -pub const TEST_ATTR_NUMBER: &str = "testattrnumber"; -pub const TEST_ATTR_NOTALLOWED: &str = "notallowed"; - pub const OAUTH2_SCOPE_EMAIL: &str = ATTR_EMAIL; pub const OAUTH2_SCOPE_GROUPS: &str = "groups"; pub const OAUTH2_SCOPE_OPENID: &str = "openid"; @@ -188,3 +181,10 @@ pub const LDAP_ATTR_MEMBER: &str = "member"; pub const LDAP_ATTR_NAME: &str = "name"; pub const LDAP_ATTR_OBJECTCLASS: &str = "objectClass"; pub const LDAP_ATTR_OU: &str = "ou"; + +// rust can't deal with this being compiled out, don't try and #[cfg()] them +pub const TEST_ATTR_NON_EXIST: &str = "non-exist"; +pub const TEST_ATTR_TEST_ATTR: &str = "testattr"; +pub const TEST_ATTR_EXTRA: &str = "extra"; +pub const TEST_ATTR_NUMBER: &str = "testattrnumber"; +pub const TEST_ATTR_NOTALLOWED: &str = "notallowed"; diff --git a/proto/src/v1.rs b/proto/src/v1.rs index c48a464f3..6ee9546a0 100644 --- a/proto/src/v1.rs +++ b/proto/src/v1.rs @@ -19,8 +19,22 @@ use crate::constants::{ATTR_GROUP, ATTR_LDAP_SSHPUBLICKEY}; // These proto implementations are here because they have public definitions -/* ===== errors ===== */ +#[derive(Clone, Copy, Debug)] +pub enum AccountType { + Person, + ServiceAccount, +} +impl ToString for AccountType { + fn to_string(&self) -> String { + match self { + AccountType::Person => "person".to_string(), + AccountType::ServiceAccount => "service_account".to_string(), + } + } +} + +/* ===== errors ===== */ #[derive(Serialize, Deserialize, Debug, PartialEq, Eq)] #[serde(rename_all = "lowercase")] pub enum SchemaError { diff --git a/server/core/Cargo.toml b/server/core/Cargo.toml index 9a19403b8..327cebc11 100644 --- a/server/core/Cargo.toml +++ b/server/core/Cargo.toml @@ -24,7 +24,8 @@ compact_jwt = { workspace = true } cron = { workspace = true } futures = { workspace = true } futures-util = { workspace = true } -http = "0.2.9" +http = { workspace = true } + hyper = { workspace = true } kanidm_proto = { workspace = true } kanidm_utils_users = { workspace = true } diff --git a/server/core/src/https/middleware/compression.rs b/server/core/src/https/middleware/compression.rs index 9528dca6c..60c1a91d2 100644 --- a/server/core/src/https/middleware/compression.rs +++ b/server/core/src/https/middleware/compression.rs @@ -1,23 +1,31 @@ -/* - Let's build a compression middleware! - - The threat of the TLS BREACH attack [1] was considered as part of adding - the CompressMiddleware configuration. - - The attack targets secrets compressed and encrypted in flight with the intent - to infer their content. - - This is not a concern for the paths covered by this configuration - ( /, /ui/, /pkg/ ), - as they're all static content with no secrets in transit - all that data should - come from Kanidm's REST API, which is on a different path and not covered by - the compression middleware. - - - [1] - https://resources.infosecinstitute.com/topic/the-breach-attack/ -*/ +//! Let's build a compression middleware! +//! +//! The threat of the TLS BREACH attack (1) was considered as part of adding +//! the CompressMiddleware configuration. +//! +//! The attack targets secrets which are compressed and encrypted in flight +//! with the intent to infer their content. +//! +//! This is not a concern for the paths covered by this configuration: +//! +//! * `/` +//! * `/ui/` +//! * `/pkg/` +//! +//! as they're all static content with no secrets in transit - all that data should +//! come from Kanidm's REST API, which is on a different path and not covered by +//! the compression middleware. +//! +//! (1) - +//! use tower_http::compression::CompressionLayer; +// TODO: this should skip compression on responses smaller than ~256 bytes because gzip can make them bigger. +/// This builds a compression layer with the following configuration: +/// +/// * No brotli compression - because that's *very* slow to compress dynamically +/// * "Best" quality of compression, usually produces the smallest size. +/// pub fn new() -> CompressionLayer { CompressionLayer::new() .no_br() diff --git a/server/lib/src/be/idl_sqlite.rs b/server/lib/src/be/idl_sqlite.rs index dcbbff208..93291396c 100644 --- a/server/lib/src/be/idl_sqlite.rs +++ b/server/lib/src/be/idl_sqlite.rs @@ -1149,9 +1149,7 @@ impl IdlSqliteWriteTransaction { .prepare(&format!("SELECT cid FROM {}.ruv", self.get_db_name())) .map_err(sqlite_error)?; - let kh_iter = stmt - .query_map([], |row| Ok(row.get(0)?)) - .map_err(sqlite_error)?; + let kh_iter = stmt.query_map([], |row| row.get(0)).map_err(sqlite_error)?; kh_iter .map(|v| { diff --git a/server/lib/src/constants/entries.rs b/server/lib/src/constants/entries.rs index fc6e73c87..abc074d45 100644 --- a/server/lib/src/constants/entries.rs +++ b/server/lib/src/constants/entries.rs @@ -9,7 +9,7 @@ use crate::idm::account::Account; use crate::value::PartialValue; use crate::value::Value; use kanidm_proto::constants::*; -use kanidm_proto::v1::{Filter, OperationError, UiHint}; +use kanidm_proto::v1::{AccountType, Filter, OperationError, UiHint}; #[cfg(test)] use uuid::uuid; @@ -734,13 +734,7 @@ impl TryFrom for EntryInitNew { lazy_static! { /// Builtin System Admin account. pub static ref BUILTIN_ACCOUNT_IDM_ADMIN: BuiltinAccount = BuiltinAccount { - // TODO: this really should be a "are you a service account or a person" enum - classes: vec![ - EntryClass::Account, - EntryClass::ServiceAccount, - EntryClass::MemberOf, - EntryClass::Object, - ], + account_type: AccountType::ServiceAccount, name: "idm_admin", uuid: UUID_IDM_ADMIN, description: "Builtin IDM Admin account.", @@ -1218,8 +1212,7 @@ Attribute::Description, #[derive(Debug, Clone)] /// Built in accounts such as anonymous, idm_admin and admin pub struct BuiltinAccount { - // TODO: this really should be a "are you a service account or a person" enum - pub classes: Vec, + pub account_type: kanidm_proto::v1::AccountType, pub name: &'static str, pub uuid: Uuid, pub description: &'static str, @@ -1229,7 +1222,7 @@ pub struct BuiltinAccount { impl Default for BuiltinAccount { fn default() -> Self { BuiltinAccount { - classes: [EntryClass::Object].to_vec(), + account_type: AccountType::ServiceAccount, name: "", uuid: Uuid::new_v4(), description: "", @@ -1260,17 +1253,20 @@ impl From for EntryInitNew { entry.add_ava(Attribute::Description, Value::new_utf8s(value.description)); entry.add_ava(Attribute::DisplayName, Value::new_utf8s(value.displayname)); - entry.add_ava(Attribute::Class, EntryClass::Object.to_value()); - entry.add_ava(Attribute::Class, EntryClass::Account.to_value()); entry.set_ava( Attribute::Class, - value - .classes - .into_iter() - .map(|c| c.to_value()) - .collect::>(), + vec![ + EntryClass::Account.to_value(), + EntryClass::MemberOf.to_value(), + EntryClass::Object.to_value(), + ], ); - + match value.account_type { + AccountType::Person => entry.add_ava(Attribute::Class, EntryClass::Person.to_value()), + AccountType::ServiceAccount => { + entry.add_ava(Attribute::Class, EntryClass::ServiceAccount.to_value()) + } + } entry } } @@ -1279,24 +1275,14 @@ lazy_static! { /// Builtin System Admin account. pub static ref BUILTIN_ACCOUNT_ADMIN: BuiltinAccount = BuiltinAccount { - classes: vec![ - EntryClass::Account, - EntryClass::ServiceAccount, - EntryClass::MemberOf, - EntryClass::Object, - ], + account_type: AccountType::ServiceAccount, name: "admin", uuid: UUID_ADMIN, description: "Builtin System Admin account.", displayname: "System Administrator", }; pub static ref BUILTIN_ACCOUNT_ANONYMOUS_V1: BuiltinAccount = BuiltinAccount { - classes: [ - EntryClass::Account, - EntryClass::ServiceAccount, - EntryClass::Object, - ] - .to_vec(), + account_type: AccountType::ServiceAccount, name: "anonymous", uuid: UUID_ANONYMOUS, description: "Anonymous access account.", diff --git a/server/lib/src/constants/schema.rs b/server/lib/src/constants/schema.rs index ab48de659..30b18d873 100644 --- a/server/lib/src/constants/schema.rs +++ b/server/lib/src/constants/schema.rs @@ -770,7 +770,6 @@ pub static ref SCHEMA_CLASS_OAUTH2_RS_BASIC: SchemaClass = SchemaClass { systemmay: vec![ Attribute::OAuth2AllowInsecureClientDisablePkce.into()], systemmust: vec![ Attribute::OAuth2RsBasicSecret.into()], - // TODO: is this a class exclude or an attribute exclude? systemexcludes: vec![ EntryClass::OAuth2ResourceServerPublic.into()], ..Default::default() }; @@ -781,7 +780,6 @@ pub static ref SCHEMA_CLASS_OAUTH2_RS_PUBLIC: SchemaClass = SchemaClass { name: EntryClass::OAuth2ResourceServerPublic.into(), description: "The class representing a configured Oauth2 Resource Server with public clients and pkce verification".to_string(), - // TODO: is this a class exclude or an attribute exclude, or both? systemexcludes: vec![EntryClass::OAuth2ResourceServerBasic.into()], ..Default::default() }; diff --git a/server/lib/src/entry.rs b/server/lib/src/entry.rs index 8b07805ee..0cd3f975a 100644 --- a/server/lib/src/entry.rs +++ b/server/lib/src/entry.rs @@ -2971,11 +2971,10 @@ impl Entry { } } - let mut res = Vec::new(); - for (attr, pv) in pairs.into_iter() { - // We use FC directly here instead of f_eq to avoid an excess clone. - res.push(FC::Eq(attr, pv)) // TODO: this is kinda terrible - } + let res: Vec> = pairs + .into_iter() + .map(|(attr, pv)| FC::Eq(attr, pv)) + .collect(); Some(filter_all!(f_and(res))) } diff --git a/server/lib/src/filter.rs b/server/lib/src/filter.rs index 1aa60fdf9..5847e8f83 100644 --- a/server/lib/src/filter.rs +++ b/server/lib/src/filter.rs @@ -658,10 +658,10 @@ impl FilterComp { } } FilterComp::Or(filters) => { - // If all filters are okay, return Ok(Filter::Or()) - // If any is invalid, return the error. - // TODO: ftweedal says an empty or is a valid filter - // in mathematical terms. + // * If all filters are okay, return Ok(Filter::Or()) + // * Any filter is invalid, return the error. + // * An empty "or" is a valid filter in mathematical terms, but we throw an + // error to warn the user because it's super unlikey they want that if filters.is_empty() { return Err(SchemaError::EmptyFilter); }; @@ -673,8 +673,10 @@ impl FilterComp { x.map(FilterComp::Or) } FilterComp::And(filters) => { - // TODO: ftweedal says an empty or is a valid filter - // in mathematical terms. + // * If all filters are okay, return Ok(Filter::Or()) + // * Any filter is invalid, return the error. + // * An empty "and" is a valid filter in mathematical terms, but we throw an + // error to warn the user because it's super unlikey they want that if filters.is_empty() { return Err(SchemaError::EmptyFilter); }; @@ -1013,14 +1015,12 @@ impl FilterResolved { } FilterComp::Pres(a) => { let idx = idxmeta.contains(&(&a, &IndexType::Presence)); - let idx = NonZeroU8::new(idx as u8); - FilterResolved::Pres(a, idx) + FilterResolved::Pres(a, NonZeroU8::new(idx as u8)) } FilterComp::LessThan(a, v) => { // let idx = idxmeta.contains(&(&a, &IndexType::ORDERING)); // TODO: For now, don't emit ordering indexes. - let idx = None; - FilterResolved::LessThan(a, v, idx) + FilterResolved::LessThan(a, v, None) } FilterComp::Or(vs) => FilterResolved::Or( vs.into_iter() diff --git a/server/lib/src/lib.rs b/server/lib/src/lib.rs index 074d7a5f7..59042d049 100644 --- a/server/lib/src/lib.rs +++ b/server/lib/src/lib.rs @@ -8,9 +8,7 @@ #![deny(clippy::suspicious)] #![deny(clippy::perf)] // Specific lints to enforce. -// TODO: can't use this until we have a better way to handle the 'todo' lint? -// #![deny(clippy::todo)] -#![warn(clippy::todo)] +#![deny(clippy::todo)] #![deny(clippy::unimplemented)] #![deny(clippy::unwrap_used)] #![deny(clippy::expect_used)] diff --git a/server/lib/src/value.rs b/server/lib/src/value.rs index 991520d06..33f194dd4 100644 --- a/server/lib/src/value.rs +++ b/server/lib/src/value.rs @@ -1133,7 +1133,6 @@ impl Value { matches!(self, Value::Iutf8(_)) } - // TODO: take this away #[inline(always)] pub fn new_class(s: &str) -> Self { Value::Iutf8(s.to_lowercase()) diff --git a/server/lib/src/valueset/image/mod.rs b/server/lib/src/valueset/image/mod.rs index 1d5e6f6d7..bf9838524 100644 --- a/server/lib/src/valueset/image/mod.rs +++ b/server/lib/src/valueset/image/mod.rs @@ -345,13 +345,7 @@ impl ValueSetT for ValueSetImage { let res: Vec = imgset .iter() - .filter_map(|image| { - if &image.hash_imagevalue() == pv { - Some(image) - } else { - None - } - }) + .filter(|image| &image.hash_imagevalue() == pv) .map(|image| self.set.remove(image)) .collect(); res.into_iter().any(|e| e)