Re: [pbs-devel] [PATCH proxmox-backup v2 07/15] api: access: add routes for managing AD realms

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On August 16, 2023 4:47 pm, Christoph Heiss wrote:
> [..]
> diff --git a/src/api2/config/access/ad.rs b/src/api2/config/access/ad.rs
> new file mode 100644
> index 00000000..c202291a
> --- /dev/null
> +++ b/src/api2/config/access/ad.rs
> @@ -0,0 +1,348 @@
> +use anyhow::{bail, format_err, Error};
> +use hex::FromHex;
> +use serde::{Deserialize, Serialize};
> +use serde_json::Value;
>
> [..]
>
> +#[api(
> +    input: {
> +        properties: {},
> +    },
> +    returns: {
> +        description: "List of configured AD realms.",
> +        type: Array,
> +        items: { type: AdRealmConfig },
> +    },
> +    access: {
> +        permission: &Permission::Privilege(&["access", "domains"], PRIV_REALM_ALLOCATE, false),

this one here

> +    },
> +)]
> +/// List configured AD realms
> +pub fn list_ad_realms(
> +    _param: Value,
> +    rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<Vec<AdRealmConfig>, Error> {
> +    let (config, digest) = domains::config()?;
> +
> +    let list = config.convert_to_typed_array("ad")?;
> +
> +    rpcenv["digest"] = hex::encode(digest).into();
> +
> +    Ok(list)
> +}
> +

> [..]

> +
> +#[api(
> +    input: {
> +        properties: {
> +            realm: {
> +                schema: REALM_ID_SCHEMA,
> +            },
> +        },
> +    },
> +    returns: { type: AdRealmConfig },
> +    access: {
> +        permission: &Permission::Privilege(&["access", "domains"], PRIV_SYS_AUDIT, false),

and this one here don't really agree - copied over from LDAP ;)

also, maybe this one here should check on /access/domains/{realm}
(although that might be postponed to do it in sync with the other
endpoint(s), but it would be more in line with how we handle entity ACLs
in general).

> +    },
> +)]
> +/// Read the AD realm configuration
> +pub fn read_ad_realm(
> +    realm: String,
> +    rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<AdRealmConfig, Error> {
> +    let (domains, digest) = domains::config()?;
> +
> +    let config = domains.lookup("ad", &realm)?;
> +
> +    rpcenv["digest"] = hex::encode(digest).into();
> +
> +    Ok(config)
> +}
> +

> [..]

> +
> +#[api(
> +    protected: true,
> +    input: {
> +        properties: {
> +            realm: {
> +                schema: REALM_ID_SCHEMA,
> +            },
> +            update: {
> +                type: AdRealmConfigUpdater,
> +                flatten: true,
> +            },
> +            password: {
> +                description: "AD bind password",
> +                optional: true,
> +            },
> +            delete: {
> +                description: "List of properties to delete.",
> +                type: Array,
> +                optional: true,
> +                items: {
> +                    type: DeletableProperty,
> +                }
> +            },
> +            digest: {
> +                optional: true,
> +                schema: PROXMOX_CONFIG_DIGEST_SCHEMA,
> +            },
> +        },
> +    },
> +    returns:  { type: AdRealmConfig },
> +    access: {
> +        permission: &Permission::Privilege(&["access", "domains"], PRIV_REALM_ALLOCATE, false),
> +    },
> +)]

this one here might check on /access/domains/{realm} as well - but the
same caveat as above applies, ideally we'd change that together with the
LDAP one at least.

> +/// Update an AD realm configuration
> +pub async fn update_ad_realm(
> +    realm: String,
> +    update: AdRealmConfigUpdater,
> +    password: Option<String>,
> +    delete: Option<Vec<DeletableProperty>>,
> +    digest: Option<String>,
> +    _rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<(), Error> {
> +    let domain_config_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)?;
> +    }
> +
> +    let mut config: AdRealmConfig = domains.lookup("ad", &realm)?;
> +
> 
> [..]
>
> +    domains::save_config(&domains)?;
> +
> +    Ok(())
> +}
> +
> +async fn retrieve_default_naming_context(ldap_config: &LdapConfig) -> Result<String, Error> {
> +    let conn = Connection::new(ldap_config.clone());
> +    match conn.retrieve_root_dse_attr("defaultNamingContext").await {
> +        Ok(base_dn) if !base_dn.is_empty() => Ok(base_dn[0].clone()),
> +        Ok(_) => bail!("server did not provide `defaultNamingContext`"),
> +        Err(err) => bail!("failed to determine base_dn: {err}"),
> +    }
> +}
> +
> +const ITEM_ROUTER: Router = Router::new()
> +    .get(&API_METHOD_READ_AD_REALM)
> +    .put(&API_METHOD_UPDATE_AD_REALM)
> +    .delete(&super::ldap::API_METHOD_DELETE_LDAP_REALM);

this seems a bit weird - as in - why doesn't that endpoint check that
it's actually being passed an LDAP realm?

> +
> +pub const ROUTER: Router = Router::new()
> +    .get(&API_METHOD_LIST_AD_REALMS)
> +    .post(&API_METHOD_CREATE_AD_REALM)
> +    .match_all("realm", &ITEM_ROUTER);