Re: [pbs-devel] [PATCH proxmox-backup 06/17] auth: add LDAP realm authenticator

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

On Tue, Jan 03, 2023 at 03:22:57PM +0100, Lukas Wagner wrote:
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  src/auth.rs | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/src/auth.rs b/src/auth.rs
> index f1d5c0a1..101bec0e 100644
> --- a/src/auth.rs
> +++ b/src/auth.rs
> @@ -8,9 +8,12 @@ use std::process::{Command, Stdio};
>  use anyhow::{bail, format_err, Error};
>  use serde_json::json;
>  
> -use pbs_api_types::{RealmRef, Userid, UsernameRef};
> +use pbs_api_types::{LdapMode, LdapRealmConfig, RealmRef, Userid, UsernameRef};
>  use pbs_buildcfg::configdir;
>  
> +use crate::auth_helpers;
> +use crate::server::ldap::{LdapConfig, LdapConnection, LdapConnectionMode};
> +
>  pub trait ProxmoxAuthenticator {
>      fn authenticate_user(&self, username: &UsernameRef, password: &str) -> Result<(), Error>;
>      fn store_password(&self, username: &UsernameRef, password: &str) -> Result<(), Error>;
> @@ -122,12 +125,45 @@ impl ProxmoxAuthenticator for PBS {
>      }
>  }
>  
> +#[allow(clippy::upper_case_acronyms)]
> +pub struct LDAP {
> +    config: LdapRealmConfig,
> +}
> +
> +impl ProxmoxAuthenticator for LDAP {
> +    /// Authenticate user in LDAP realm
> +    fn authenticate_user(&self, username: &UsernameRef, password: &str) -> Result<(), Error> {
> +        let ldap_config = ldap_api_type_to_ldap_config(&self.config)?;
> +
> +        let ldap = LdapConnection::new(ldap_config);
> +
> +        proxmox_async::runtime::block_on(ldap.authenticate_user(username.as_str(), password))
> +    }
> +
> +    fn store_password(&self, _username: &UsernameRef, _password: &str) -> Result<(), Error> {
> +        // do not store password for LDAP users
> +        Ok(())

Actually this should fail.
Otherwise this will make change-password API calls "succeed" without
actually doing anything, but IMO it makes more sense to return a
meaningful error there.

(Perhaps even a http_bail!(NOT_IMPLEMENTED) though I'm not really sure
how the GUI would deal with that ;-) )

> +    }
> +
> +    fn remove_password(&self, _username: &UsernameRef) -> Result<(), Error> {
> +        // do not remove password for LDAP users

^ same here

> +        Ok(())
> +    }
> +}
> +
>  /// Lookup the autenticator for the specified realm
>  pub fn lookup_authenticator(realm: &RealmRef) -> Result<Box<dyn ProxmoxAuthenticator>, Error> {
>      match realm.as_str() {
>          "pam" => Ok(Box::new(PAM())),
>          "pbs" => Ok(Box::new(PBS())),
> -        _ => bail!("unknown realm '{}'", realm.as_str()),
> +        realm => {
> +            let (domains, _digest) = pbs_config::domains::config()?;
> +            if let Ok(config) = domains.lookup::<LdapRealmConfig>("ldap", realm) {
> +                Ok(Box::new(LDAP { config }))
> +            } else {
> +                bail!("unknown realm '{}'", realm);
> +            }
> +        }
>      }
>  }
>  
> @@ -135,3 +171,28 @@ pub fn lookup_authenticator(realm: &RealmRef) -> Result<Box<dyn ProxmoxAuthentic
>  pub fn authenticate_user(userid: &Userid, password: &str) -> Result<(), Error> {
>      lookup_authenticator(userid.realm())?.authenticate_user(userid.name(), password)
>  }
> +
> +// TODO: Is there a better place for this?

IMO the best way is to just not :-) (as I mentioned in a reply to the
earlier patches)

Although that would make it more difficult to make this reusable and...
well...
we may still need it in the end, we'll see...

> +pub fn ldap_api_type_to_ldap_config(config: &LdapRealmConfig) -> Result<LdapConfig, Error> {
> +    let mut servers = vec![config.server1.clone()];
> +    if let Some(server) = &config.server2 {
> +        servers.push(server.clone());
> +    }
> +
> +    let tls_mode = match config.mode.unwrap_or_default() {
> +        LdapMode::Ldap => LdapConnectionMode::Ldap,
> +        LdapMode::StartTls => LdapConnectionMode::StartTls,
> +        LdapMode::Ldaps => LdapConnectionMode::Ldaps,
> +    };
> +
> +    Ok(LdapConfig {
> +        servers,
> +        port: config.port,
> +        user_attr: config.user_attr.clone(),
> +        base_dn: config.base_dn.clone(),
> +        bind_dn: config.bind_dn.clone(),
> +        bind_password: auth_helpers::get_ldap_bind_password(&config.realm)?,
> +        tls_mode,
> +        verify_certificate: config.verify.unwrap_or_default(),
> +    })
> +}
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
>