From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 7380893415 for ; Wed, 4 Jan 2023 12:17:02 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 47EE51E027 for ; Wed, 4 Jan 2023 12:17:02 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 4 Jan 2023 12:17:00 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 3021344EC7 for ; Wed, 4 Jan 2023 12:17:00 +0100 (CET) Date: Wed, 4 Jan 2023 12:16:55 +0100 From: Wolfgang Bumiller To: Lukas Wagner Cc: pbs-devel@lists.proxmox.com Message-ID: <20230104111655.nzousywozzknspeh@casey.proxmox.com> References: <20230103142308.656240-1-l.wagner@proxmox.com> <20230103142308.656240-5-l.wagner@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230103142308.656240-5-l.wagner@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.168 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_SBL_A 0.1 Contains URL's A record listed in the Spamhaus SBL blocklist [metrics.rs] Subject: Re: [pbs-devel] [PATCH proxmox-backup 04/17] api: add routes for managing LDAP realms X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 04 Jan 2023 11:17:02 -0000 On Tue, Jan 03, 2023 at 03:22:55PM +0100, Lukas Wagner wrote: > Note: bind-passwords set via the API are not stored in `domains.cfg`, > but in a separate `ldap_passwords.json` file located in > `/etc/proxmox-backup/`. > Similar to the already existing `shadow.json`, the file is > stored with 0600 permissions and is owned by root. > > Signed-off-by: Lukas Wagner > --- > pbs-config/src/domains.rs | 16 +- > src/api2/config/access/ldap.rs | 316 +++++++++++++++++++++++++++++++++ > src/api2/config/access/mod.rs | 7 +- > src/auth_helpers.rs | 51 ++++++ > 4 files changed, 387 insertions(+), 3 deletions(-) > create mode 100644 src/api2/config/access/ldap.rs > > diff --git a/pbs-config/src/domains.rs b/pbs-config/src/domains.rs > index 12d4543d..6cef3ec5 100644 > --- a/pbs-config/src/domains.rs > +++ b/pbs-config/src/domains.rs > @@ -7,13 +7,15 @@ use proxmox_schema::{ApiType, Schema}; > use proxmox_section_config::{SectionConfig, SectionConfigData, SectionConfigPlugin}; > > use crate::{open_backup_lockfile, replace_backup_config, BackupLockGuard}; > -use pbs_api_types::{OpenIdRealmConfig, REALM_ID_SCHEMA}; > +use pbs_api_types::{LdapRealmConfig, OpenIdRealmConfig, REALM_ID_SCHEMA}; > > lazy_static! { > pub static ref CONFIG: SectionConfig = init(); > } > > fn init() -> SectionConfig { > + let mut config = SectionConfig::new(&REALM_ID_SCHEMA); > + > let obj_schema = match OpenIdRealmConfig::API_SCHEMA { > Schema::Object(ref obj_schema) => obj_schema, > _ => unreachable!(), > @@ -24,7 +26,17 @@ fn init() -> SectionConfig { > Some(String::from("realm")), > obj_schema, > ); > - let mut config = SectionConfig::new(&REALM_ID_SCHEMA); > + > + config.register_plugin(plugin); > + > + let obj_schema = match LdapRealmConfig::API_SCHEMA { > + Schema::Object(ref obj_schema) => obj_schema, > + _ => unreachable!(), ^ if you already touch this code, please update it to use compile time checks here we got with improved const fn support a few rust versions ago, see `metrics.rs`: const LDAP_SCHEMA: &ObjectSchema = LdapRealmConfig::API_SCHEMA.unwrap_object_schema() (must be a `const` rather than a `let` binding to be checked at compile time) > + }; > + > + let plugin = > + SectionConfigPlugin::new("ldap".to_string(), Some(String::from("realm")), obj_schema); > + > config.register_plugin(plugin); > > config > diff --git a/src/api2/config/access/ldap.rs b/src/api2/config/access/ldap.rs > new file mode 100644 > index 00000000..14bbf9ea > --- /dev/null > +++ b/src/api2/config/access/ldap.rs > @@ -0,0 +1,316 @@ > +/// Create a new LDAP realm > +pub fn create_ldap_realm(mut config: LdapRealmConfig) -> Result<(), Error> { > + let _lock = domains::lock_config()?; > + > + let (mut domains, _digest) = domains::config()?; > + > + if config.realm == "pbs" > + || config.realm == "pam" > + || domains.sections.get(&config.realm).is_some() ^ perhaps replace this & its openid version with an `exists` helper in `pbs_config::domains` You can just have that take the `&SectionConfigData` but bonus points if you also add a `Domains` type and don't actually expose the `SectionConfigData` anywhere (but that's probably better off in a separate patch series...) > + { > + param_bail!("realm", "realm '{}' already exists.", config.realm); > + } > + > + // If a bind password is set, take it out of the config struct and > + // save it separately with proper protection > + if let Some(password) = config.password.take() { > + auth_helpers::store_ldap_bind_password(&config.realm, &password)?; > + } ^ doesn't this sort of make `password` a dead field in `LdapRealmconfig`? It seems to me it would be better to have an explicit password parameter on this API call with the rest of the `LdapRealmConfig` `flatten`ed into the parameter list? That way we can never accidentally use the password from that field and don't need debug assertions. > + > + debug_assert!(config.password.is_none()); > + > + domains.set_data(&config.realm, "ldap", &config)?; > + > + domains::save_config(&domains)?; > + > + Ok(()) > +} > + > +#[api( > + protected: true, > + input: { > + properties: { > + realm: { > + schema: REALM_ID_SCHEMA, > + }, > + digest: { > + optional: true, > + schema: PROXMOX_CONFIG_DIGEST_SCHEMA, > + }, > + }, > + }, > + access: { > + permission: &Permission::Privilege(&["access", "domains"], PRIV_REALM_ALLOCATE, false), > + }, > +)] > +/// Remove an LDAP realm configuration > +pub fn delete_ldap_realm( > + realm: String, > + digest: Option, > + _rpcenv: &mut dyn RpcEnvironment, > +) -> Result<(), Error> { > + let _lock = domains::lock_config()?; > + > + let (mut domains, expected_digest) = domains::config()?; > + > + if let Some(ref digest) = digest { > + let digest = <[u8; 32]>::from_hex(digest)?; > + crate::tools::detect_modified_configuration_file(&digest, &expected_digest)?; > + } > + > + if domains.sections.remove(&realm).is_none() { > + http_bail!(NOT_FOUND, "realm '{}' does not exist.", realm); > + } > + > + domains::save_config(&domains)?; > + > + auth_helpers::remove_ldap_bind_password(&realm)?; ^ The password removal should probably only log errors, but not actually fail the API call? Since the config is already stored and you wouldn't be able to re-run the same call. > + > + Ok(()) > +} > + > +#[api( > + input: { > + properties: { > + realm: { > + schema: REALM_ID_SCHEMA, > + }, > + }, > + }, > + returns: { type: LdapRealmConfig }, > + access: { > + permission: &Permission::Privilege(&["access", "domains"], PRIV_SYS_AUDIT, false), > + }, > +)] > +/// Read the LDAP realm configuration > +pub fn read_ldap_realm( > + realm: String, > + rpcenv: &mut dyn RpcEnvironment, > +) -> Result { > + let (domains, digest) = domains::config()?; > + > + let config = domains.lookup("ldap", &realm)?; > + > + rpcenv["digest"] = hex::encode(digest).into(); > + > + Ok(config) > +} > + > +#[api()] > +#[derive(Serialize, Deserialize)] > +#[serde(rename_all = "kebab-case")] > +#[allow(non_camel_case_types)] ^ Please don't continue this trend, just name the variants correctly. > +/// Deletable property name > +pub enum DeletableProperty { > + /// Fallback LDAP server address > + server2, > + /// Port > + port, > + /// Comment > + comment, > + /// Verify server certificate > + verify, > + /// Mode (ldap, ldap+starttls or ldaps), > + mode, > + /// Bind Domain > + bind_dn, > + /// Bind password > + password, > +} > diff --git a/src/auth_helpers.rs b/src/auth_helpers.rs > index 57e02900..79128811 100644 > --- a/src/auth_helpers.rs > +++ b/src/auth_helpers.rs > @@ -11,6 +11,7 @@ use proxmox_sys::fs::{file_get_contents, replace_file, CreateOptions}; > > use pbs_api_types::Userid; > use pbs_buildcfg::configdir; > +use serde_json::json; > > fn compute_csrf_secret_digest(timestamp: i64, secret: &[u8], userid: &Userid) -> String { > let mut hasher = sha::Sha256::new(); > @@ -180,3 +181,53 @@ pub fn private_auth_key() -> &'static PKey { > > &KEY > } > + > +const LDAP_PASSWORDS_FILENAME: &str = configdir!("/ldap_passwords.json"); > + Please include in the comments of all store & remove that they need the domain config lock to be held. Or actually, maybe move the store & remove ones into the api module where they're actually used and drop the `pub`. > +/// Store LDAP bind passwords in protected file > +pub fn store_ldap_bind_password(realm: &str, password: &str) -> Result<(), Error> { > + let mut data = proxmox_sys::fs::file_get_json(LDAP_PASSWORDS_FILENAME, Some(json!({})))?; > + data[realm] = password.into(); > + > + let mode = nix::sys::stat::Mode::from_bits_truncate(0o0600); > + let options = proxmox_sys::fs::CreateOptions::new() > + .perm(mode) > + .owner(nix::unistd::ROOT) > + .group(nix::unistd::Gid::from_raw(0)); > + > + let data = serde_json::to_vec_pretty(&data)?; > + proxmox_sys::fs::replace_file(LDAP_PASSWORDS_FILENAME, &data, options, true)?; > + > + Ok(()) > +} > + > +/// Remove stored LDAP bind password > +pub fn remove_ldap_bind_password(realm: &str) -> Result<(), Error> { > + let mut data = proxmox_sys::fs::file_get_json(LDAP_PASSWORDS_FILENAME, Some(json!({})))?; > + if let Some(map) = data.as_object_mut() { > + map.remove(realm); > + } > + > + let mode = nix::sys::stat::Mode::from_bits_truncate(0o0600); > + let options = proxmox_sys::fs::CreateOptions::new() > + .perm(mode) > + .owner(nix::unistd::ROOT) > + .group(nix::unistd::Gid::from_raw(0)); > + > + let data = serde_json::to_vec_pretty(&data)?; > + proxmox_sys::fs::replace_file(LDAP_PASSWORDS_FILENAME, &data, options, true)?; > + > + Ok(()) > +} > + > +/// Retrieve stored LDAP bind password > +pub fn get_ldap_bind_password(realm: &str) -> Result, Error> { > + let data = proxmox_sys::fs::file_get_json(LDAP_PASSWORDS_FILENAME, Some(json!({})))?; > + > + let password = data > + .get(realm) > + .and_then(|s| s.as_str()) > + .map(|s| s.to_owned()); > + > + Ok(password) > +} > -- > 2.30.2