[pbs-devel] [PATCH proxmox-backup 04/17] api: add routes for managing LDAP realms
Wolfgang Bumiller
w.bumiller at proxmox.com
Wed Jan 4 12:16:55 CET 2023
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 <l.wagner at proxmox.com>
> ---
> 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<String>,
> + _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<LdapRealmConfig, Error> {
> + 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<Private> {
>
> &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<Option<String>, 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
More information about the pbs-devel
mailing list