Re: [pbs-devel] [PATCH proxmox-backup 04/17] api: add routes for managing LDAP realms

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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@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