[pbs-devel] [PATCH proxmox/proxmox-backup/pwt 0/12] add Active Directory realm support

[pbs-devel] [PATCH proxmox/proxmox-backup/pwt 0/12] add Active Directory realm support

[Christoph Heiss]

This series adds Active Directory realm support to PBS, much like it
already exists in PVE. The logic matches it as closely as possible.

Patches #1 through #6 are purely preparatory. A lot of functionality
from the existing LDAP realm implementation can be re-used, esp. the
realm sync.

The API, authenticator and realm sync job implementations are partly
simply copied from LDAP, replacing structs and changing some things as
needed. The realm sync job simply reuses the existing LDAP
implementation for the most part, other than setting up some things
differently.

As for the UI, the existing panel for LDAP realms was also generic
enough such that it only needed a few conditionals as what input boxes
to show.

One thing to note is that - unlike PVE - you don't have to specify a
domain name when creating an AD realm. This is due to `proxmox-ldap`
already figuring out the correct, full DN of bind and login users
itself. That is the only use of the domain name in PVE anyway, thus it
is not present here.
The base DN is automatically determined from the `defaultNamingContext`
attribute of the root DSE object. It can be set manually in the config
if the need should arise. So that should be treated more like an
implementation detail.

Testing
-------
I have tested this series using:

 * slapd 2.5.13+dfsg-5 as LDAP server to ensure no regressions
 * Samba 4.18.5 as an Linux-based AD server and
 * AD on Windows Server 2022 to make sure that works as well

For slapd and MS AD, I tested both anonymous binds and authenticated
binds, with Samba only authenticated binds (since there seems to way to
turn on anonymous binds in Samba, at least that I could find ..) as well
as dry-running and actual syncing of users. Further, then also logging
into PBS with a sync'd user.

proxmox:

Christoph Heiss (2):
  ldap: add method for retrieving root DSE attributes
  auth-api: implement `Display` for `Realm{,Ref}`

 proxmox-auth-api/src/types.rs        | 12 ++++++++++++
 proxmox-ldap/src/lib.rs              | 22 ++++++++++++++++++++++
 proxmox-ldap/tests/assets/glauth.cfg |  1 +
 proxmox-ldap/tests/glauth.rs         | 16 ++++++++++++++++
 4 files changed, 51 insertions(+)

proxmox-backup:

Christoph Heiss (9):
  api-types: implement `LdapMode` -> `ConnectionMode` conversion
  auth: factor out CA store and cert lookup into own function
  api-types: implement `Display`, `FromStr` for `RealmType`
  realm sync: generic-ify `LdapSyncSettings` and `GeneralSyncSettings`
  api: access: add routes for managing AD realms
  config: domains: add new "ad" section type for AD realms
  realm sync: add sync job for AD realms
  manager: add subcommand for managing AD realms
  docs: user-management: add section about AD realm support

 docs/user-management.rst               |  40 +++-
 pbs-api-types/Cargo.toml               |   1 +
 pbs-api-types/src/ad.rs                | 101 ++++++++
 pbs-api-types/src/ldap.rs              |  11 +
 pbs-api-types/src/lib.rs               |  38 +++
 pbs-config/src/domains.rs              |  11 +-
 src/api2/access/domain.rs              |  16 +-
 src/api2/config/access/ad.rs           | 314 +++++++++++++++++++++++++
 src/api2/config/access/mod.rs          |   2 +
 src/auth.rs                            | 114 +++++++--
 src/bin/proxmox-backup-manager.rs      |   1 +
 src/bin/proxmox_backup_manager/ad.rs   | 105 +++++++++
 src/bin/proxmox_backup_manager/ldap.rs |   2 +-
 src/bin/proxmox_backup_manager/mod.rs  |   2 +
 src/server/realm_sync_job.rs           | 111 +++++++--
 15 files changed, 818 insertions(+), 51 deletions(-)
 create mode 100644 pbs-api-types/src/ad.rs
 create mode 100644 src/api2/config/access/ad.rs
 create mode 100644 src/bin/proxmox_backup_manager/ad.rs

proxmox-widget-toolkit:

Christoph Heiss (1):
  window: add Active Directory auth panel

 src/Makefile               |  1 +
 src/Schema.js              | 10 ++++++++++
 src/window/AuthEditAD.js   | 14 ++++++++++++++
 src/window/AuthEditLDAP.js | 28 ++++++++++++++++++++++++++--
 4 files changed, 51 insertions(+), 2 deletions(-)
 create mode 100644 src/window/AuthEditAD.js

--
2.41.0

[pbs-devel] [PATCH proxmox 01/12] ldap: add method for retrieving root DSE attributes

[Christoph Heiss]

The root DSE holds common attributes about the LDAP server itself.
Needed to e.g. support Active Directory-based LDAP servers to retrieve
the base DN from the server itself, based on an valid bind.

See also RFC 4512, Section 5.1 [0] for more information about this
special object.

[0] https://www.rfc-editor.org/rfc/rfc4512#section-5.1

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-ldap/src/lib.rs              | 22 ++++++++++++++++++++++
 proxmox-ldap/tests/assets/glauth.cfg |  1 +
 proxmox-ldap/tests/glauth.rs         | 16 ++++++++++++++++
 3 files changed, 39 insertions(+)

diff --git a/proxmox-ldap/src/lib.rs b/proxmox-ldap/src/lib.rs
index b3b5d65..534c0c8 100644
--- a/proxmox-ldap/src/lib.rs
+++ b/proxmox-ldap/src/lib.rs
@@ -198,6 +198,28 @@ impl Connection {
         Ok(())
     }

+    /// Retrieves an attribute from the root DSE according to RFC 4512, Section 5.1
+    /// https://www.rfc-editor.org/rfc/rfc4512#section-5.1
+    pub async fn retrieve_root_dse_attr(&self, attr: &str) -> Result<Vec<String>, Error> {
+        let mut ldap = self.create_connection().await?;
+
+        let (entries, _res) = ldap
+            .search("", Scope::Base, "(objectClass=*)", vec![attr])
+            .await?
+            .success()?;
+
+        if entries.len() > 1 {
+            bail!("found multiple root DSEs with attribute '{attr}'");
+        }
+
+        entries
+            .into_iter()
+            .next()
+            .map(SearchEntry::construct)
+            .and_then(|e| e.attrs.get(attr).cloned())
+            .ok_or_else(|| format_err!("failed to retrieve root DSE attribute '{attr}'"))
+    }
+
     /// Retrive port from LDAP configuration, otherwise use the correct default
     fn port_from_config(&self) -> u16 {
         self.config.port.unwrap_or_else(|| {
diff --git a/proxmox-ldap/tests/assets/glauth.cfg b/proxmox-ldap/tests/assets/glauth.cfg
index 7255169..8abbdc6 100644
--- a/proxmox-ldap/tests/assets/glauth.cfg
+++ b/proxmox-ldap/tests/assets/glauth.cfg
@@ -16,6 +16,7 @@ debug = true
   baseDN = "dc=example,dc=com"
   nameformat = "cn"
   groupformat = "ou"
+  anonymousdse = true

 # to create a passSHA256:   echo -n "mysecret" | openssl dgst -sha256

diff --git a/proxmox-ldap/tests/glauth.rs b/proxmox-ldap/tests/glauth.rs
index 88875d2..74720c1 100644
--- a/proxmox-ldap/tests/glauth.rs
+++ b/proxmox-ldap/tests/glauth.rs
@@ -191,3 +191,19 @@ fn test_check_connection() -> Result<(), Error> {

     Ok(())
 }
+
+#[test]
+#[ignore]
+fn test_retrieve_root_dse_attr() -> Result<(), Error> {
+    let _glauth = GlauthServer::new("tests/assets/glauth.cfg")?;
+
+    let connection = Connection::new(default_config());
+
+    let values = proxmox_async::runtime::block_on(
+        connection.retrieve_root_dse_attr("defaultNamingContext"),
+    )?;
+
+    assert_eq!(values, vec!["dc=example,dc=com"]);
+
+    Ok(())
+}
--
2.41.0

[pbs-devel] [PATCH proxmox 02/12] auth-api: implement `Display` for `Realm{, Ref}`

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-auth-api/src/types.rs | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/proxmox-auth-api/src/types.rs b/proxmox-auth-api/src/types.rs
index 319ac4b..8c6d7eb 100644
--- a/proxmox-auth-api/src/types.rs
+++ b/proxmox-auth-api/src/types.rs
@@ -327,6 +327,18 @@ impl PartialEq<Realm> for &RealmRef {
     }
 }

+impl fmt::Display for Realm {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "{}", self.0)
+    }
+}
+
+impl fmt::Display for RealmRef {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "{}", &self.0)
+    }
+}
+
 #[api(
     type: String,
     format: &PROXMOX_TOKEN_NAME_FORMAT,
--
2.41.0

[pbs-devel] [PATCH proxmox-backup 03/12] api-types: implement `LdapMode` -> `ConnectionMode` conversion

[Christoph Heiss]

No functional changes.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 pbs-api-types/Cargo.toml  |  1 +
 pbs-api-types/src/ldap.rs | 11 +++++++++++
 src/auth.rs               | 12 +++---------
 3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/pbs-api-types/Cargo.toml b/pbs-api-types/Cargo.toml
index 31b69f62..cb584cb5 100644
--- a/pbs-api-types/Cargo.toml
+++ b/pbs-api-types/Cargo.toml
@@ -17,6 +17,7 @@ serde_plain.workspace = true
 proxmox-auth-api = { workspace = true, features = [ "api-types" ] }
 proxmox-human-byte.workspace = true
 proxmox-lang.workspace=true
+proxmox-ldap.workspace = true
 proxmox-schema = { workspace = true, features = [ "api-macro" ] }
 proxmox-serde.workspace = true
 proxmox-time.workspace = true
diff --git a/pbs-api-types/src/ldap.rs b/pbs-api-types/src/ldap.rs
index f3df90a0..e1f7c452 100644
--- a/pbs-api-types/src/ldap.rs
+++ b/pbs-api-types/src/ldap.rs
@@ -1,5 +1,6 @@
 use serde::{Deserialize, Serialize};

+use proxmox_ldap::ConnectionMode;
 use proxmox_schema::{api, ApiStringFormat, ApiType, ArraySchema, Schema, StringSchema, Updater};

 use super::{REALM_ID_SCHEMA, SINGLE_LINE_COMMENT_SCHEMA};
@@ -20,6 +21,16 @@ pub enum LdapMode {
     Ldaps,
 }

+impl From<LdapMode> for ConnectionMode {
+    fn from(value: LdapMode) -> ConnectionMode {
+        match value {
+            LdapMode::Ldap => ConnectionMode::Ldap,
+            LdapMode::StartTls => ConnectionMode::StartTls,
+            LdapMode::Ldaps => ConnectionMode::Ldaps,
+        }
+    }
+}
+
 #[api(
     properties: {
         "realm": {
diff --git a/src/auth.rs b/src/auth.rs
index 318d1ff2..e375ebc4 100644
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -16,10 +16,10 @@ use proxmox_auth_api::api::{Authenticator, LockedTfaConfig};
 use proxmox_auth_api::ticket::{Empty, Ticket};
 use proxmox_auth_api::types::Authid;
 use proxmox_auth_api::Keyring;
-use proxmox_ldap::{Config, Connection, ConnectionMode};
+use proxmox_ldap::{Config, Connection};
 use proxmox_tfa::api::{OpenUserChallengeData, TfaConfig};

-use pbs_api_types::{LdapMode, LdapRealmConfig, OpenIdRealmConfig, RealmRef, Userid, UsernameRef};
+use pbs_api_types::{LdapRealmConfig, OpenIdRealmConfig, RealmRef, Userid, UsernameRef};
 use pbs_buildcfg::configdir;

 use crate::auth_helpers;
@@ -185,12 +185,6 @@ impl LdapAuthenticator {
             servers.push(server.clone());
         }

-        let tls_mode = match config.mode.unwrap_or_default() {
-            LdapMode::Ldap => ConnectionMode::Ldap,
-            LdapMode::StartTls => ConnectionMode::StartTls,
-            LdapMode::Ldaps => ConnectionMode::Ldaps,
-        };
-
         let (ca_store, trusted_cert) = if let Some(capath) = config.capath.as_deref() {
             let path = PathBuf::from(capath);
             if path.is_dir() {
@@ -209,7 +203,7 @@ impl LdapAuthenticator {
             base_dn: config.base_dn.clone(),
             bind_dn: config.bind_dn.clone(),
             bind_password: password,
-            tls_mode,
+            tls_mode: config.mode.unwrap_or_default().into(),
             verify_certificate: config.verify.unwrap_or_default(),
             additional_trusted_certificates: trusted_cert,
             certificate_store_path: ca_store,
--
2.41.0

[pbs-devel] [PATCH proxmox-backup 04/12] auth: factor out CA store and cert lookup into own function

[Christoph Heiss]

No functional changes.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 src/auth.rs | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/src/auth.rs b/src/auth.rs
index e375ebc4..ae6ff729 100644
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -185,16 +185,7 @@ impl LdapAuthenticator {
             servers.push(server.clone());
         }

-        let (ca_store, trusted_cert) = if let Some(capath) = config.capath.as_deref() {
-            let path = PathBuf::from(capath);
-            if path.is_dir() {
-                (Some(path), None)
-            } else {
-                (None, Some(vec![path]))
-            }
-        } else {
-            (None, None)
-        };
+        let (ca_store, trusted_cert) = lookup_ca_store_or_cert_path(config.capath.as_deref());

         Ok(Config {
             servers,
@@ -211,6 +202,19 @@ impl LdapAuthenticator {
     }
 }

+fn lookup_ca_store_or_cert_path(capath: Option<&str>) -> (Option<PathBuf>, Option<Vec<PathBuf>>) {
+    if let Some(capath) = capath {
+        let path = PathBuf::from(capath);
+        if path.is_dir() {
+            (Some(path), None)
+        } else {
+            (None, Some(vec![path]))
+        }
+    } else {
+        (None, None)
+    }
+}
+
 /// Lookup the autenticator for the specified realm
 pub(crate) fn lookup_authenticator(
     realm: &RealmRef,
--
2.41.0

[pbs-devel] [PATCH proxmox-backup 05/12] api-types: implement `Display`, `FromStr` for `RealmType`

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 pbs-api-types/src/lib.rs | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
index 4764c51a..6ebbe514 100644
--- a/pbs-api-types/src/lib.rs
+++ b/pbs-api-types/src/lib.rs
@@ -1,5 +1,9 @@
 //! Basic API types used by most of the PBS code.

+use core::fmt;
+
+use anyhow::{format_err, Error};
+
 use serde::{Deserialize, Serialize};

 use proxmox_auth_api::{APITOKEN_ID_REGEX_STR, USER_ID_REGEX_STR};
@@ -508,6 +512,33 @@ pub enum RealmType {
     Ldap,
 }

+impl fmt::Display for RealmType {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        use RealmType::*;
+        match self {
+            Pam => write!(f, "PAM"),
+            Pbs => write!(f, "PBS"),
+            OpenId => write!(f, "OpenID"),
+            Ldap => write!(f, "LDAP"),
+        }
+    }
+}
+
+impl std::str::FromStr for RealmType {
+    type Err = Error;
+
+    fn from_str(realm_type: &str) -> Result<Self, Error> {
+        use RealmType::*;
+        match realm_type.to_lowercase().as_str() {
+            "pam" => Ok(Pam),
+            "pbs" => Ok(Pbs),
+            "openid" => Ok(OpenId),
+            "ldap" => Ok(Ldap),
+            _ => Err(format_err!("unknown realm type {realm_type}")),
+        }
+    }
+}
+
 #[api(
     properties: {
         realm: {
--
2.41.0

[pbs-devel] [PATCH proxmox-backup 06/12] realm sync: generic-ify `LdapSyncSettings` and `GeneralSyncSettings`

[Christoph Heiss]

Since both only needs a handful of attributes anyway, pass them
explicitly instead of as an LDAP-specific config object, such that these
types can be reused for other realms like the new Active Directory one.

No functional changes.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 src/server/realm_sync_job.rs | 42 ++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/src/server/realm_sync_job.rs b/src/server/realm_sync_job.rs
index 1f92e843..5a7bcf0b 100644
--- a/src/server/realm_sync_job.rs
+++ b/src/server/realm_sync_job.rs
@@ -81,9 +81,14 @@ impl LdapRealmSyncJob {
         };

         let sync_settings = GeneralSyncSettings::default()
-            .apply_config(&config)?
+            .apply_config(config.sync_defaults_options.as_deref())?
             .apply_override(override_settings)?;
-        let sync_attributes = LdapSyncSettings::from_config(&config)?;
+        let sync_attributes = LdapSyncSettings::new(
+            &config.user_attr,
+            config.sync_attributes.as_deref(),
+            config.user_classes.as_deref(),
+            config.filter.as_deref(),
+        )?;

         let ldap_config = auth::LdapAuthenticator::api_type_to_config(&config)?;

@@ -385,14 +390,19 @@ struct LdapSyncSettings {
 }

 impl LdapSyncSettings {
-    fn from_config(config: &LdapRealmConfig) -> Result<Self, Error> {
-        let mut attributes = vec![config.user_attr.clone()];
+    fn new(
+        user_attr: &str,
+        sync_attributes: Option<&str>,
+        user_classes: Option<&str>,
+        user_filter: Option<&str>,
+    ) -> Result<Self, Error> {
+        let mut attributes = vec![user_attr.to_owned()];

         let mut email = None;
         let mut firstname = None;
         let mut lastname = None;

-        if let Some(sync_attributes) = &config.sync_attributes {
+        if let Some(sync_attributes) = &sync_attributes {
             let value = LdapSyncAttributes::API_SCHEMA.parse_property_string(sync_attributes)?;
             let sync_attributes: LdapSyncAttributes = serde_json::from_value(value)?;

@@ -400,20 +410,20 @@ impl LdapSyncSettings {
             firstname = sync_attributes.firstname.clone();
             lastname = sync_attributes.lastname.clone();

-            if let Some(email_attr) = sync_attributes.email {
-                attributes.push(email_attr);
+            if let Some(email_attr) = &sync_attributes.email {
+                attributes.push(email_attr.clone());
             }

-            if let Some(firstname_attr) = sync_attributes.firstname {
-                attributes.push(firstname_attr);
+            if let Some(firstname_attr) = &sync_attributes.firstname {
+                attributes.push(firstname_attr.clone());
             }

-            if let Some(lastname_attr) = sync_attributes.lastname {
-                attributes.push(lastname_attr);
+            if let Some(lastname_attr) = &sync_attributes.lastname {
+                attributes.push(lastname_attr.clone());
             }
         }

-        let user_classes = if let Some(user_classes) = &config.user_classes {
+        let user_classes = if let Some(user_classes) = &user_classes {
             let a = USER_CLASSES_ARRAY.parse_property_string(user_classes)?;
             serde_json::from_value(a)?
         } else {
@@ -426,13 +436,13 @@ impl LdapSyncSettings {
         };

         Ok(Self {
-            user_attr: config.user_attr.clone(),
+            user_attr: user_attr.to_owned(),
             firstname_attr: firstname,
             lastname_attr: lastname,
             email_attr: email,
             attributes,
             user_classes,
-            user_filter: config.filter.clone(),
+            user_filter: user_filter.map(ToOwned::to_owned),
         })
     }
 }
@@ -447,11 +457,11 @@ impl Default for GeneralSyncSettings {
 }

 impl GeneralSyncSettings {
-    fn apply_config(self, config: &LdapRealmConfig) -> Result<Self, Error> {
+    fn apply_config(self, sync_defaults_options: Option<&str>) -> Result<Self, Error> {
         let mut enable_new = None;
         let mut remove_vanished = None;

-        if let Some(sync_defaults_options) = &config.sync_defaults_options {
+        if let Some(sync_defaults_options) = sync_defaults_options {
             let sync_defaults_options = Self::parse_sync_defaults_options(sync_defaults_options)?;

             enable_new = sync_defaults_options.enable_new;
--
2.41.0

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

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 pbs-api-types/src/ad.rs       | 101 +++++++++++
 pbs-api-types/src/lib.rs      |   3 +
 src/api2/config/access/ad.rs  | 314 ++++++++++++++++++++++++++++++++++
 src/api2/config/access/mod.rs |   2 +
 src/auth.rs                   |  78 ++++++++-
 5 files changed, 497 insertions(+), 1 deletion(-)
 create mode 100644 pbs-api-types/src/ad.rs
 create mode 100644 src/api2/config/access/ad.rs

diff --git a/pbs-api-types/src/ad.rs b/pbs-api-types/src/ad.rs
new file mode 100644
index 00000000..446715c7
--- /dev/null
+++ b/pbs-api-types/src/ad.rs
@@ -0,0 +1,101 @@
+use serde::{Deserialize, Serialize};
+
+use proxmox_schema::{api, Updater};
+
+use super::{
+    LdapMode, LDAP_DOMAIN_SCHEMA, REALM_ID_SCHEMA, SINGLE_LINE_COMMENT_SCHEMA,
+    SYNC_ATTRIBUTES_SCHEMA, SYNC_DEFAULTS_STRING_SCHEMA, USER_CLASSES_SCHEMA,
+};
+
+#[api(
+    properties: {
+        "realm": {
+            schema: REALM_ID_SCHEMA,
+        },
+        "comment": {
+            optional: true,
+            schema: SINGLE_LINE_COMMENT_SCHEMA,
+        },
+        "verify": {
+            optional: true,
+            default: false,
+        },
+        "sync-defaults-options": {
+            schema: SYNC_DEFAULTS_STRING_SCHEMA,
+            optional: true,
+        },
+        "sync-attributes": {
+            schema: SYNC_ATTRIBUTES_SCHEMA,
+            optional: true,
+        },
+        "user-classes" : {
+            optional: true,
+            schema: USER_CLASSES_SCHEMA,
+        },
+        "base-dn" : {
+            schema: LDAP_DOMAIN_SCHEMA,
+            optional: true,
+        },
+        "bind-dn" : {
+            schema: LDAP_DOMAIN_SCHEMA,
+            optional: true,
+        }
+    },
+)]
+#[derive(Serialize, Deserialize, Updater, Clone)]
+#[serde(rename_all = "kebab-case")]
+/// AD realm configuration properties.
+pub struct AdRealmConfig {
+    #[updater(skip)]
+    pub realm: String,
+    /// AD server address
+    pub server1: String,
+    /// Fallback AD server address
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub server2: Option<String>,
+    /// AD server Port
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub port: Option<u16>,
+    /// Base domain name. Users are searched under this domain using a `subtree search`.
+    /// Expected to be set only internally to `defaultNamingContext` of the AD server, but can be
+    /// overridden if the need arises.
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub base_dn: Option<String>,
+    /// Whether usernames should be matched case-sensitive
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub case_sensitive: Option<bool>,
+    /// Comment
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub comment: Option<String>,
+    /// Connection security
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub mode: Option<LdapMode>,
+    /// Verify server certificate
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub verify: Option<bool>,
+    /// CA certificate to use for the server. The path can point to
+    /// either a file, or a directory. If it points to a file,
+    /// the PEM-formatted X.509 certificate stored at the path
+    /// will be added as a trusted certificate.
+    /// If the path points to a directory,
+    /// the directory replaces the system's default certificate
+    /// store at `/etc/ssl/certs` - Every file in the directory
+    /// will be loaded as a trusted certificate.
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub capath: Option<String>,
+    /// Bind domain to use for looking up users
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub bind_dn: Option<String>,
+    /// Custom LDAP search filter for user sync
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub filter: Option<String>,
+    /// Default options for AD sync
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub sync_defaults_options: Option<String>,
+    /// List of LDAP attributes to sync from AD to user config
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub sync_attributes: Option<String>,
+    /// User ``objectClass`` classes to sync
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub user_classes: Option<String>,
+}
diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
index 6ebbe514..c622484e 100644
--- a/pbs-api-types/src/lib.rs
+++ b/pbs-api-types/src/lib.rs
@@ -114,6 +114,9 @@ pub use openid::*;
 mod ldap;
 pub use ldap::*;

+mod ad;
+pub use ad::*;
+
 mod remote;
 pub use remote::*;

diff --git a/src/api2/config/access/ad.rs b/src/api2/config/access/ad.rs
new file mode 100644
index 00000000..0803cdfb
--- /dev/null
+++ b/src/api2/config/access/ad.rs
@@ -0,0 +1,314 @@
+use anyhow::{bail, format_err, Error};
+use hex::FromHex;
+use serde_json::Value;
+
+use proxmox_ldap::{Config as LdapConfig, Connection};
+use proxmox_router::{Permission, Router, RpcEnvironment};
+use proxmox_schema::{api, param_bail};
+
+use pbs_api_types::{
+    AdRealmConfig, AdRealmConfigUpdater, PRIV_REALM_ALLOCATE, PRIV_SYS_AUDIT,
+    PROXMOX_CONFIG_DIGEST_SCHEMA, REALM_ID_SCHEMA,
+};
+
+use pbs_config::domains;
+
+use crate::{api2::config::access::ldap::DeletableProperty, auth::AdAuthenticator, auth_helpers};
+
+#[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),
+    },
+)]
+/// 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(
+    protected: true,
+    input: {
+        properties: {
+            config: {
+                type: AdRealmConfig,
+                flatten: true,
+            },
+            password: {
+                description: "AD bind password",
+                optional: true,
+            }
+        },
+    },
+    access: {
+        permission: &Permission::Privilege(&["access", "domains"], PRIV_REALM_ALLOCATE, false),
+    },
+)]
+/// Create a new AD realm
+pub async fn create_ad_realm(
+    mut config: AdRealmConfig,
+    password: Option<String>,
+) -> Result<(), Error> {
+    let domain_config_lock = domains::lock_config()?;
+
+    let (mut domains, _digest) = domains::config()?;
+
+    if domains::exists(&domains, &config.realm) {
+        param_bail!("realm", "realm '{}' already exists.", config.realm);
+    }
+
+    let mut ldap_config =
+        AdAuthenticator::api_type_to_config_with_password(&config, password.clone())?;
+
+    if config.base_dn.is_none() {
+        ldap_config.base_dn = retrieve_default_naming_context(&ldap_config).await?;
+        config.base_dn = Some(ldap_config.base_dn.clone());
+    }
+
+    let conn = Connection::new(ldap_config);
+    proxmox_async::runtime::block_on(conn.check_connection()).map_err(|e| format_err!("{e:#}"))?;
+
+    if let Some(password) = password {
+        auth_helpers::store_ldap_bind_password(&config.realm, &password, &domain_config_lock)?;
+    }
+
+    domains.set_data(&config.realm, "ad", &config)?;
+
+    domains::save_config(&domains)?;
+
+    Ok(())
+}
+
+#[api(
+    input: {
+        properties: {
+            realm: {
+                schema: REALM_ID_SCHEMA,
+            },
+        },
+    },
+    returns: { type: AdRealmConfig },
+    access: {
+        permission: &Permission::Privilege(&["access", "domains"], PRIV_SYS_AUDIT, false),
+    },
+)]
+/// 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),
+    },
+)]
+/// 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)?;
+
+    if let Some(delete) = delete {
+        for delete_prop in delete {
+            match delete_prop {
+                DeletableProperty::Server2 => {
+                    config.server2 = None;
+                }
+                DeletableProperty::Comment => {
+                    config.comment = None;
+                }
+                DeletableProperty::Port => {
+                    config.port = None;
+                }
+                DeletableProperty::Verify => {
+                    config.verify = None;
+                }
+                DeletableProperty::Mode => {
+                    config.mode = None;
+                }
+                DeletableProperty::BindDn => {
+                    config.bind_dn = None;
+                }
+                DeletableProperty::Password => {
+                    auth_helpers::remove_ldap_bind_password(&realm, &domain_config_lock)?;
+                }
+                DeletableProperty::Filter => {
+                    config.filter = None;
+                }
+                DeletableProperty::SyncDefaultsOptions => {
+                    config.sync_defaults_options = None;
+                }
+                DeletableProperty::SyncAttributes => {
+                    config.sync_attributes = None;
+                }
+                DeletableProperty::UserClasses => {
+                    config.user_classes = None;
+                }
+            }
+        }
+    }
+
+    if let Some(server1) = update.server1 {
+        config.server1 = server1;
+    }
+
+    if let Some(server2) = update.server2 {
+        config.server2 = Some(server2);
+    }
+
+    if let Some(port) = update.port {
+        config.port = Some(port);
+    }
+
+    if let Some(base_dn) = update.base_dn {
+        config.base_dn = Some(base_dn);
+    }
+
+    if let Some(comment) = update.comment {
+        let comment = comment.trim().to_string();
+        if comment.is_empty() {
+            config.comment = None;
+        } else {
+            config.comment = Some(comment);
+        }
+    }
+
+    if let Some(mode) = update.mode {
+        config.mode = Some(mode);
+    }
+
+    if let Some(verify) = update.verify {
+        config.verify = Some(verify);
+    }
+
+    if let Some(bind_dn) = update.bind_dn {
+        config.bind_dn = Some(bind_dn);
+    }
+
+    if let Some(filter) = update.filter {
+        config.filter = Some(filter);
+    }
+
+    if let Some(sync_defaults_options) = update.sync_defaults_options {
+        config.sync_defaults_options = Some(sync_defaults_options);
+    }
+
+    if let Some(sync_attributes) = update.sync_attributes {
+        config.sync_attributes = Some(sync_attributes);
+    }
+
+    if let Some(user_classes) = update.user_classes {
+        config.user_classes = Some(user_classes);
+    }
+
+    let mut ldap_config = if password.is_some() {
+        AdAuthenticator::api_type_to_config_with_password(&config, password.clone())?
+    } else {
+        AdAuthenticator::api_type_to_config(&config)?
+    };
+
+    if config.base_dn.is_none() {
+        ldap_config.base_dn = retrieve_default_naming_context(&ldap_config).await?;
+        config.base_dn = Some(ldap_config.base_dn.clone());
+    }
+
+    let conn = Connection::new(ldap_config);
+    proxmox_async::runtime::block_on(conn.check_connection()).map_err(|e| format_err!("{e:#}"))?;
+
+    if let Some(password) = password {
+        auth_helpers::store_ldap_bind_password(&realm, &password, &domain_config_lock)?;
+    }
+
+    domains.set_data(&realm, "ad", &config)?;
+
+    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);
+
+pub const ROUTER: Router = Router::new()
+    .get(&API_METHOD_LIST_AD_REALMS)
+    .post(&API_METHOD_CREATE_AD_REALM)
+    .match_all("realm", &ITEM_ROUTER);
diff --git a/src/api2/config/access/mod.rs b/src/api2/config/access/mod.rs
index 614bd5e6..b551e662 100644
--- a/src/api2/config/access/mod.rs
+++ b/src/api2/config/access/mod.rs
@@ -2,12 +2,14 @@ use proxmox_router::list_subdirs_api_method;
 use proxmox_router::{Router, SubdirMap};
 use proxmox_sortable_macro::sortable;

+pub mod ad;
 pub mod ldap;
 pub mod openid;
 pub mod tfa;

 #[sortable]
 const SUBDIRS: SubdirMap = &sorted!([
+    ("ad", &ad::ROUTER),
     ("ldap", &ldap::ROUTER),
     ("openid", &openid::ROUTER),
     ("tfa", &tfa::ROUTER),
diff --git a/src/auth.rs b/src/auth.rs
index ae6ff729..ce234990 100644
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -19,7 +19,9 @@ use proxmox_auth_api::Keyring;
 use proxmox_ldap::{Config, Connection};
 use proxmox_tfa::api::{OpenUserChallengeData, TfaConfig};

-use pbs_api_types::{LdapRealmConfig, OpenIdRealmConfig, RealmRef, Userid, UsernameRef};
+use pbs_api_types::{
+    AdRealmConfig, LdapRealmConfig, OpenIdRealmConfig, RealmRef, Userid, UsernameRef,
+};
 use pbs_buildcfg::configdir;

 use crate::auth_helpers;
@@ -202,6 +204,80 @@ impl LdapAuthenticator {
     }
 }

+pub struct AdAuthenticator {
+    config: AdRealmConfig,
+}
+
+impl AdAuthenticator {
+    pub fn api_type_to_config(config: &AdRealmConfig) -> Result<Config, Error> {
+        Self::api_type_to_config_with_password(
+            config,
+            auth_helpers::get_ldap_bind_password(&config.realm)?,
+        )
+    }
+
+    pub fn api_type_to_config_with_password(
+        config: &AdRealmConfig,
+        password: Option<String>,
+    ) -> Result<Config, Error> {
+        let mut servers = vec![config.server1.clone()];
+        if let Some(server) = &config.server2 {
+            servers.push(server.clone());
+        }
+
+        let (ca_store, trusted_cert) = lookup_ca_store_or_cert_path(config.capath.as_deref());
+
+        Ok(Config {
+            servers,
+            port: config.port,
+            user_attr: "sAMAccountName".to_owned(),
+            base_dn: config.base_dn.clone().unwrap_or_default(),
+            bind_dn: config.bind_dn.clone(),
+            bind_password: password,
+            tls_mode: config.mode.unwrap_or_default().into(),
+            verify_certificate: config.verify.unwrap_or_default(),
+            additional_trusted_certificates: trusted_cert,
+            certificate_store_path: ca_store,
+        })
+    }
+}
+
+impl Authenticator for AdAuthenticator {
+    /// Authenticate user in AD realm
+    fn authenticate_user<'a>(
+        &'a self,
+        username: &'a UsernameRef,
+        password: &'a str,
+        _client_ip: Option<&'a IpAddr>,
+    ) -> Pin<Box<dyn Future<Output = Result<(), Error>> + Send + 'a>> {
+        Box::pin(async move {
+            let ldap_config = Self::api_type_to_config(&self.config)?;
+            let ldap = Connection::new(ldap_config);
+            ldap.authenticate_user(username.as_str(), password).await?;
+            Ok(())
+        })
+    }
+
+    fn store_password(
+        &self,
+        _username: &UsernameRef,
+        _password: &str,
+        _client_ip: Option<&IpAddr>,
+    ) -> Result<(), Error> {
+        http_bail!(
+            NOT_IMPLEMENTED,
+            "storing passwords is not implemented for Active Directory realms"
+        );
+    }
+
+    fn remove_password(&self, _username: &UsernameRef) -> Result<(), Error> {
+        http_bail!(
+            NOT_IMPLEMENTED,
+            "removing passwords is not implemented for Active Directory realms"
+        );
+    }
+}
+
 fn lookup_ca_store_or_cert_path(capath: Option<&str>) -> (Option<PathBuf>, Option<Vec<PathBuf>>) {
     if let Some(capath) = capath {
         let path = PathBuf::from(capath);
--
2.41.0

[pbs-devel] [PATCH proxmox-backup 08/12] config: domains: add new "ad" section type for AD realms

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 pbs-config/src/domains.rs | 7 ++++++-
 src/auth.rs               | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/pbs-config/src/domains.rs b/pbs-config/src/domains.rs
index 35aa11d5..dcf47f83 100644
--- a/pbs-config/src/domains.rs
+++ b/pbs-config/src/domains.rs
@@ -8,13 +8,14 @@ use proxmox_schema::{ApiType, ObjectSchema};
 use proxmox_section_config::{SectionConfig, SectionConfigData, SectionConfigPlugin};

 use crate::{open_backup_lockfile, replace_backup_config, BackupLockGuard};
-use pbs_api_types::{LdapRealmConfig, OpenIdRealmConfig, REALM_ID_SCHEMA};
+use pbs_api_types::{AdRealmConfig, LdapRealmConfig, OpenIdRealmConfig, REALM_ID_SCHEMA};

 lazy_static! {
     pub static ref CONFIG: SectionConfig = init();
 }

 fn init() -> SectionConfig {
+    const AD_SCHEMA: &ObjectSchema = AdRealmConfig::API_SCHEMA.unwrap_object_schema();
     const LDAP_SCHEMA: &ObjectSchema = LdapRealmConfig::API_SCHEMA.unwrap_object_schema();
     const OPENID_SCHEMA: &ObjectSchema = OpenIdRealmConfig::API_SCHEMA.unwrap_object_schema();

@@ -33,6 +34,10 @@ fn init() -> SectionConfig {

     config.register_plugin(plugin);

+    let plugin = SectionConfigPlugin::new("ad".to_string(), Some(String::from("realm")), AD_SCHEMA);
+
+    config.register_plugin(plugin);
+
     config
 }

diff --git a/src/auth.rs b/src/auth.rs
index ce234990..12b8dc51 100644
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -302,6 +302,8 @@ pub(crate) fn lookup_authenticator(
             let (domains, _digest) = pbs_config::domains::config()?;
             if let Ok(config) = domains.lookup::<LdapRealmConfig>("ldap", realm) {
                 Ok(Box::new(LdapAuthenticator { config }))
+            } else if let Ok(config) = domains.lookup::<AdRealmConfig>("ad", realm) {
+                Ok(Box::new(AdAuthenticator { config }))
             } else if domains.lookup::<OpenIdRealmConfig>("openid", realm).is_ok() {
                 Ok(Box::new(OpenIdAuthenticator()))
             } else {
--
2.41.0

[pbs-devel] [PATCH proxmox-backup 09/12] realm sync: add sync job for AD realms

[Christoph Heiss]

Basically just a thin wrapper over the existing LDAP-based realm sync
job, which retrieves the appropriate config and sets the correct user
attributes.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 pbs-api-types/src/lib.rs     |  4 +++
 src/api2/access/domain.rs    | 16 ++++++++-
 src/server/realm_sync_job.rs | 69 ++++++++++++++++++++++++++++++++----
 3 files changed, 82 insertions(+), 7 deletions(-)

diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
index c622484e..3f86803f 100644
--- a/pbs-api-types/src/lib.rs
+++ b/pbs-api-types/src/lib.rs
@@ -513,6 +513,8 @@ pub enum RealmType {
     OpenId,
     /// An LDAP realm
     Ldap,
+    /// An Active Directory (AD) realm
+    Ad,
 }

 impl fmt::Display for RealmType {
@@ -523,6 +525,7 @@ impl fmt::Display for RealmType {
             Pbs => write!(f, "PBS"),
             OpenId => write!(f, "OpenID"),
             Ldap => write!(f, "LDAP"),
+            Ad => write!(f, "AD"),
         }
     }
 }
@@ -537,6 +540,7 @@ impl std::str::FromStr for RealmType {
             "pbs" => Ok(Pbs),
             "openid" => Ok(OpenId),
             "ldap" => Ok(Ldap),
+            "ad" => Ok(Ad),
             _ => Err(format_err!("unknown realm type {realm_type}")),
         }
     }
diff --git a/src/api2/access/domain.rs b/src/api2/access/domain.rs
index 31aa62bc..027597fa 100644
--- a/src/api2/access/domain.rs
+++ b/src/api2/access/domain.rs
@@ -7,7 +7,8 @@ use proxmox_router::{Permission, Router, RpcEnvironment, RpcEnvironmentType, Sub
 use proxmox_schema::api;

 use pbs_api_types::{
-    Authid, BasicRealmInfo, Realm, PRIV_PERMISSIONS_MODIFY, REMOVE_VANISHED_SCHEMA, UPID_SCHEMA,
+    Authid, BasicRealmInfo, Realm, RealmRef, RealmType, PRIV_PERMISSIONS_MODIFY,
+    REMOVE_VANISHED_SCHEMA, UPID_SCHEMA,
 };

 use crate::server::jobstate::Job;
@@ -102,6 +103,7 @@ pub fn sync_realm(
     let upid_str = crate::server::do_realm_sync_job(
         job,
         realm.clone(),
+        realm_type_from_name(&*realm)?,
         &auth_id,
         None,
         to_stdout,
@@ -120,6 +122,18 @@ pub fn sync_realm(
     Ok(json!(upid_str))
 }

+fn realm_type_from_name(realm: &RealmRef) -> Result<RealmType, Error> {
+    let config = pbs_config::domains::config()?.0;
+
+    for (name, (section_type, _)) in config.sections.iter() {
+        if name == realm.as_str() {
+            return section_type.parse();
+        }
+    }
+
+    Err(format_err!("unable to find realm {realm}"))
+}
+
 const SYNC_ROUTER: Router = Router::new().post(&API_METHOD_SYNC_REALM);
 const SYNC_SUBDIRS: SubdirMap = &[("sync", &SYNC_ROUTER)];

diff --git a/src/server/realm_sync_job.rs b/src/server/realm_sync_job.rs
index 5a7bcf0b..de8124d7 100644
--- a/src/server/realm_sync_job.rs
+++ b/src/server/realm_sync_job.rs
@@ -10,9 +10,9 @@ use proxmox_sys::{task_log, task_warn};
 use std::{collections::HashSet, sync::Arc};

 use pbs_api_types::{
-    ApiToken, Authid, LdapRealmConfig, Realm, RemoveVanished, SyncAttributes as LdapSyncAttributes,
-    SyncDefaultsOptions, User, Userid, EMAIL_SCHEMA, FIRST_NAME_SCHEMA, LAST_NAME_SCHEMA,
-    REMOVE_VANISHED_ARRAY, USER_CLASSES_ARRAY,
+    AdRealmConfig, ApiToken, Authid, LdapRealmConfig, Realm, RealmType, RemoveVanished,
+    SyncAttributes as LdapSyncAttributes, SyncDefaultsOptions, User, Userid, EMAIL_SCHEMA,
+    FIRST_NAME_SCHEMA, LAST_NAME_SCHEMA, REMOVE_VANISHED_ARRAY, USER_CLASSES_ARRAY,
 };

 use crate::{auth, server::jobstate::Job};
@@ -22,6 +22,7 @@ use crate::{auth, server::jobstate::Job};
 pub fn do_realm_sync_job(
     mut job: Job,
     realm: Realm,
+    realm_type: RealmType,
     auth_id: &Authid,
     _schedule: Option<String>,
     to_stdout: bool,
@@ -46,8 +47,19 @@ pub fn do_realm_sync_job(
             };

             async move {
-                let sync_job = LdapRealmSyncJob::new(worker, realm, &override_settings, dry_run)?;
-                sync_job.sync().await
+                match realm_type {
+                    RealmType::Ldap => {
+                        LdapRealmSyncJob::new(worker, realm, &override_settings, dry_run)?
+                            .sync()
+                            .await
+                    }
+                    RealmType::Ad => {
+                        AdRealmSyncJob::new(worker, realm, &override_settings, dry_run)?
+                            .sync()
+                            .await
+                    }
+                    _ => bail!("cannot sync realm {realm} of type {realm_type}"),
+                }
             }
         },
     )?;
@@ -55,6 +67,51 @@ pub fn do_realm_sync_job(
     Ok(upid_str)
 }

+/// Implementation for syncing Active Directory realms. Merely a thin wrapper over
+/// `LdapRealmSyncJob`, as AD is just LDAP with some special requirements.
+struct AdRealmSyncJob(LdapRealmSyncJob);
+
+impl AdRealmSyncJob {
+    fn new(
+        worker: Arc<WorkerTask>,
+        realm: Realm,
+        override_settings: &GeneralSyncSettingsOverride,
+        dry_run: bool,
+    ) -> Result<Self, Error> {
+        let (domains, _digest) = pbs_config::domains::config()?;
+        let config = if let Ok(config) = domains.lookup::<AdRealmConfig>("ad", realm.as_str()) {
+            config
+        } else {
+            bail!("unknown Active Directory realm '{}'", realm.as_str());
+        };
+
+        let sync_settings = GeneralSyncSettings::default()
+            .apply_config(config.sync_defaults_options.as_deref())?
+            .apply_override(override_settings)?;
+        let sync_attributes = LdapSyncSettings::new(
+            "sAMAccountName",
+            config.sync_attributes.as_deref(),
+            config.user_classes.as_deref(),
+            config.filter.as_deref(),
+        )?;
+
+        let ldap_config = auth::AdAuthenticator::api_type_to_config(&config)?;
+
+        Ok(Self(LdapRealmSyncJob {
+            worker,
+            realm,
+            general_sync_settings: sync_settings,
+            ldap_sync_settings: sync_attributes,
+            ldap_config,
+            dry_run,
+        }))
+    }
+
+    async fn sync(&self) -> Result<(), Error> {
+        self.0.sync().await
+    }
+}
+
 /// Implemenation for syncing LDAP realms
 struct LdapRealmSyncJob {
     worker: Arc<WorkerTask>,
@@ -77,7 +134,7 @@ impl LdapRealmSyncJob {
         let config = if let Ok(config) = domains.lookup::<LdapRealmConfig>("ldap", realm.as_str()) {
             config
         } else {
-            bail!("unknown realm '{}'", realm.as_str());
+            bail!("unknown LDAP realm '{}'", realm.as_str());
         };

         let sync_settings = GeneralSyncSettings::default()
--
2.41.0

[pbs-devel] [PATCH proxmox-backup 10/12] manager: add subcommand for managing AD realms

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 pbs-config/src/domains.rs              |   4 +
 src/bin/proxmox-backup-manager.rs      |   1 +
 src/bin/proxmox_backup_manager/ad.rs   | 105 +++++++++++++++++++++++++
 src/bin/proxmox_backup_manager/ldap.rs |   2 +-
 src/bin/proxmox_backup_manager/mod.rs  |   2 +
 5 files changed, 113 insertions(+), 1 deletion(-)
 create mode 100644 src/bin/proxmox_backup_manager/ad.rs

diff --git a/pbs-config/src/domains.rs b/pbs-config/src/domains.rs
index dcf47f83..4a9beec3 100644
--- a/pbs-config/src/domains.rs
+++ b/pbs-config/src/domains.rs
@@ -100,3 +100,7 @@ pub fn complete_openid_realm_name(_arg: &str, _param: &HashMap<String, String>)
 pub fn complete_ldap_realm_name(_arg: &str, _param: &HashMap<String, String>) -> Vec<String> {
     complete_realm_of_type("ldap")
 }
+
+pub fn complete_ad_realm_name(_arg: &str, _param: &HashMap<String, String>) -> Vec<String> {
+    complete_realm_of_type("ad")
+}
diff --git a/src/bin/proxmox-backup-manager.rs b/src/bin/proxmox-backup-manager.rs
index b4cb6cb3..577db9fa 100644
--- a/src/bin/proxmox-backup-manager.rs
+++ b/src/bin/proxmox-backup-manager.rs
@@ -437,6 +437,7 @@ async fn run() -> Result<(), Error> {
         .insert("disk", disk_commands())
         .insert("dns", dns_commands())
         .insert("ldap", ldap_commands())
+        .insert("ad", ad_commands())
         .insert("network", network_commands())
         .insert("node", node_commands())
         .insert("user", user_commands())
diff --git a/src/bin/proxmox_backup_manager/ad.rs b/src/bin/proxmox_backup_manager/ad.rs
new file mode 100644
index 00000000..90b34143
--- /dev/null
+++ b/src/bin/proxmox_backup_manager/ad.rs
@@ -0,0 +1,105 @@
+use anyhow::Error;
+use serde_json::Value;
+
+use proxmox_router::{cli::*, ApiHandler, RpcEnvironment};
+use proxmox_schema::api;
+
+use pbs_api_types::REALM_ID_SCHEMA;
+
+use crate::api2;
+
+#[api(
+    input: {
+        properties: {
+            "output-format": {
+                schema: OUTPUT_FORMAT,
+                optional: true,
+            },
+        }
+    }
+)]
+/// List configured AD realms
+fn list_ad_realms(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+    let output_format = get_output_format(&param);
+
+    let info = &api2::config::access::ad::API_METHOD_LIST_AD_REALMS;
+    let mut data = match info.handler {
+        ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
+        _ => unreachable!(),
+    };
+
+    let options = default_table_format_options()
+        .column(ColumnConfig::new("realm"))
+        .column(ColumnConfig::new("server1"))
+        .column(ColumnConfig::new("comment"));
+
+    format_and_print_result_full(&mut data, &info.returns, &output_format, &options);
+
+    Ok(Value::Null)
+}
+
+#[api(
+    input: {
+        properties: {
+            realm: {
+                schema: REALM_ID_SCHEMA,
+            },
+            "output-format": {
+                schema: OUTPUT_FORMAT,
+                optional: true,
+            },
+        }
+    }
+)]
+/// Show AD realm configuration
+pub fn show_ad_realm(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+    let output_format = get_output_format(&param);
+
+    let info = &api2::config::access::ad::API_METHOD_READ_AD_REALM;
+    let mut data = match info.handler {
+        ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
+        _ => unreachable!(),
+    };
+
+    let options = default_table_format_options();
+    format_and_print_result_full(&mut data, &info.returns, &output_format, &options);
+
+    Ok(Value::Null)
+}
+
+pub fn ad_commands() -> CommandLineInterface {
+    let cmd_def = CliCommandMap::new()
+        .insert("list", CliCommand::new(&API_METHOD_LIST_AD_REALMS))
+        .insert(
+            "show",
+            CliCommand::new(&crate::API_METHOD_SHOW_AD_REALM)
+                .arg_param(&["realm"])
+                .completion_cb("realm", pbs_config::domains::complete_ad_realm_name),
+        )
+        .insert(
+            "create",
+            CliCommand::new(&api2::config::access::ad::API_METHOD_CREATE_AD_REALM)
+                .arg_param(&["realm"])
+                .completion_cb("realm", pbs_config::domains::complete_ad_realm_name),
+        )
+        .insert(
+            "update",
+            CliCommand::new(&api2::config::access::ad::API_METHOD_UPDATE_AD_REALM)
+                .arg_param(&["realm"])
+                .completion_cb("realm", pbs_config::domains::complete_ad_realm_name),
+        )
+        .insert(
+            "delete",
+            CliCommand::new(&api2::config::access::ldap::API_METHOD_DELETE_LDAP_REALM)
+                .arg_param(&["realm"])
+                .completion_cb("realm", pbs_config::domains::complete_ad_realm_name),
+        )
+        .insert(
+            "sync",
+            CliCommand::new(&crate::API_METHOD_SYNC_LDAP_REALM)
+                .arg_param(&["realm"])
+                .completion_cb("realm", pbs_config::domains::complete_ad_realm_name),
+        );
+
+    cmd_def.into()
+}
diff --git a/src/bin/proxmox_backup_manager/ldap.rs b/src/bin/proxmox_backup_manager/ldap.rs
index 7ff4ad1d..196825a6 100644
--- a/src/bin/proxmox_backup_manager/ldap.rs
+++ b/src/bin/proxmox_backup_manager/ldap.rs
@@ -98,7 +98,7 @@ fn show_ldap_realm(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Valu
     },
 )]
 /// Sync a given LDAP realm
-async fn sync_ldap_realm(param: Value) -> Result<Value, Error> {
+pub async fn sync_ldap_realm(param: Value) -> Result<Value, Error> {
     let realm = required_string_param(&param, "realm")?;
     let client = connect_to_localhost()?;

diff --git a/src/bin/proxmox_backup_manager/mod.rs b/src/bin/proxmox_backup_manager/mod.rs
index 8a1c140c..b60dd684 100644
--- a/src/bin/proxmox_backup_manager/mod.rs
+++ b/src/bin/proxmox_backup_manager/mod.rs
@@ -2,6 +2,8 @@ mod acl;
 pub use acl::*;
 mod acme;
 pub use acme::*;
+mod ad;
+pub use ad::*;
 mod cert;
 pub use cert::*;
 mod datastore;
--
2.41.0

[pbs-devel] [PATCH proxmox-backup 11/12] docs: user-management: add section about AD realm support

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 docs/user-management.rst | 40 +++++++++++++++++++++++++++++++++-------
 1 file changed, 33 insertions(+), 7 deletions(-)

diff --git a/docs/user-management.rst b/docs/user-management.rst
index 822443f9..3634c027 100644
--- a/docs/user-management.rst
+++ b/docs/user-management.rst
@@ -643,15 +643,41 @@ A full list of all configuration parameters can be found at :ref:`domains.cfg`.
   server, you must also add them as a user of that realm in Proxmox Backup
   Server. This can be carried out automatically with syncing.

-User Synchronization in LDAP realms
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+.. _user_realms_ad:

-It is possible to automatically sync users for LDAP-based realms, rather than
-having to add them to Proxmox VE manually. Synchronization options can be set
-in the LDAP realm configuration dialog window in the GUI and via the
-``proxmox-backup-manager ldap create/update`` command.
+Active Directory
+~~~~~~~~~~~~~~~~
+
+Proxmox Backup Server can also utilize external Microsoft Active Directory
+servers for user authentication.
+To achieve this, a realm of the type ``ad`` has to be configured.
+
+For an Active Directory realm, the authentication domain name and the server
+address must be specified. Most options from :ref:`_user_realms_ldap` apply to
+Active Directory as well, most importantly the bind credentials ``bind-dn``
+and ``password``. This is typically required by default for Microsoft Active
+Directory.
+
+The authentication domain name must only be specified if anonymous bind is
+requested. If bind credentials are given, the domain name is automatically
+inferred from the bind users' base domain, as reported by the Active Directory
+server.
+
+A full list of all configuration parameters can be found at :ref:`domains.cfg`.
+
+.. note:: In order to allow a particular user to authenticate using the Active
+  Directory server, you must also add them as a user of that realm in Proxmox
+  Backup Server. This can be carried out automatically with syncing.
+
+User Synchronization in LDAP/AD realms
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+It is possible to automatically sync users for LDAP and AD-based realms, rather
+than having to add them to Proxmox VE manually. Synchronization options can be
+set in the LDAP realm configuration dialog window in the GUI and via the
+``proxmox-backup-manager ldap/ad create/update`` command.
 User synchronization can started in the GUI at
 Configuration > Access Control > Realms by selecting a realm and pressing the
 `Sync` button. In the sync dialog, some of the default options set in the realm
 configuration can be overridden. Alternatively, user synchronization can also
-be started via the ``proxmox-backup-manager ldap sync`` command.
+be started via the ``proxmox-backup-manager ldap/ad sync`` command.
--
2.41.0

[pbs-devel] [PATCH proxmox-widget-toolkit 12/12] window: add Active Directory auth panel

[Christoph Heiss]

As AD realms are mostly just LDAP, reuse the LDAP panel and just
show/hide some elements based on the type.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 src/Makefile               |  1 +
 src/Schema.js              | 10 ++++++++++
 src/window/AuthEditAD.js   | 14 ++++++++++++++
 src/window/AuthEditLDAP.js | 28 ++++++++++++++++++++++++++--
 4 files changed, 51 insertions(+), 2 deletions(-)
 create mode 100644 src/window/AuthEditAD.js

diff --git a/src/Makefile b/src/Makefile
index baa90ec..32225af 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -87,6 +87,7 @@ JSSRC=					\
 	window/AuthEditBase.js		\
 	window/AuthEditOpenId.js	\
 	window/AuthEditLDAP.js		\
+	window/AuthEditAD.js		\
 	window/TfaWindow.js		\
 	window/AddTfaRecovery.js	\
 	window/AddTotp.js		\
diff --git a/src/Schema.js b/src/Schema.js
index b247b1e..2fbcceb 100644
--- a/src/Schema.js
+++ b/src/Schema.js
@@ -29,6 +29,16 @@ Ext.define('Proxmox.Schema', { // a singleton
 	    pwchange: false,
 	    sync: true,
 	},
+	ad: {
+	    name: gettext('Active Directory Server'),
+	    ipanel: 'pmxAuthADPanel',
+	    syncipanel: 'pmxAuthADSyncPanel',
+	    add: true,
+	    edit: true,
+	    tfa: true,
+	    pwchange: false,
+	    sync: true,
+	},
     },
     // to add or change existing for product specific ones
     overrideAuthDomains: function(extra) {
diff --git a/src/window/AuthEditAD.js b/src/window/AuthEditAD.js
new file mode 100644
index 0000000..0de7494
--- /dev/null
+++ b/src/window/AuthEditAD.js
@@ -0,0 +1,14 @@
+Ext.define('Proxmox.panel.ADInputPanel', {
+    extend: 'Proxmox.panel.LDAPInputPanel',
+    xtype: 'pmxAuthADPanel',
+
+    type: 'ad',
+    onlineHelp: 'user-realms-ad',
+});
+
+Ext.define('Proxmox.panel.ADSyncInputPanel', {
+    extend: 'Proxmox.panel.LDAPSyncInputPanel',
+    xtype: 'pmxAuthADSyncPanel',
+
+    type: 'ad',
+});
diff --git a/src/window/AuthEditLDAP.js b/src/window/AuthEditLDAP.js
index 6aafb98..b336e5a 100644
--- a/src/window/AuthEditLDAP.js
+++ b/src/window/AuthEditLDAP.js
@@ -64,6 +64,12 @@ Ext.define('Proxmox.panel.LDAPInputPanel', {
 	return values;
     },

+    cbindData: function(config) {
+	return {
+	    isLdap: this.type === 'ldap',
+	    isAd: this.type === 'ad',
+	};
+    },

     column1: [
 	{
@@ -80,15 +86,33 @@ Ext.define('Proxmox.panel.LDAPInputPanel', {
 	    xtype: 'proxmoxtextfield',
 	    fieldLabel: gettext('Base Domain Name'),
 	    name: 'base-dn',
-	    allowBlank: false,
 	    emptyText: 'cn=Users,dc=company,dc=net',
+	    cbind: {
+		hidden: '{!isLdap}',
+		allowBlank: '{!isLdap}',
+	    },
+	},
+	{
+	    xtype: 'proxmoxcheckbox',
+	    fieldLabel: gettext('Case-sensitive'),
+	    name: 'case-sensitive',
+	    cbind: {
+		hidden: '{!isAd}',
+	    },
+	    autoEl: {
+		tag: 'div',
+		'data-qtip': gettext('Match usernames case-sensitive'),
+	    },
 	},
 	{
 	    xtype: 'proxmoxtextfield',
 	    fieldLabel: gettext('User Attribute Name'),
 	    name: 'user-attr',
-	    allowBlank: false,
 	    emptyText: 'uid / sAMAccountName',
+	    cbind: {
+		hidden: '{!isLdap}',
+		allowBlank: '{!isLdap}',
+	    },
 	},
 	{
 	    xtype: 'proxmoxcheckbox',
--
2.41.0

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

[Lukas Wagner]

On Tue Aug 8, 2023 at 2:22 PM CEST, Christoph Heiss wrote:
> +/// 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> {
(...)
> +
> +    if config.base_dn.is_none() {
> +        ldap_config.base_dn = retrieve_default_naming_context(&ldap_config).await?;
> +        config.base_dn = Some(ldap_config.base_dn.clone());
> +    }
> +
> +    let conn = Connection::new(ldap_config);
> +    proxmox_async::runtime::block_on(conn.check_connection()).map_err(|e| format_err!("{e:#}"))?;
We are already in an async function, so we should be able to use .await the
future? Unless I'm missing something.

> +
> +    if let Some(password) = password {
> +        auth_helpers::store_ldap_bind_password(&realm, &password, &domain_config_lock)?;
> +    }
> +
> +    domains.set_data(&realm, "ad", &config)?;
> +
> +    domains::save_config(&domains)?;
> +
> +    Ok(())
> +}

General remark regarding the update-handler: You are missing the
'case-sensitive' parameter, so updating that parameter does not work.

Re: [pbs-devel] [PATCH proxmox-backup 09/12] realm sync: add sync job for AD realms

[Lukas Wagner]

On Tue Aug 8, 2023 at 2:22 PM CEST, Christoph Heiss wrote:
>  use crate::server::jobstate::Job;
> @@ -102,6 +103,7 @@ pub fn sync_realm(
>      let upid_str = crate::server::do_realm_sync_job(
>          job,
>          realm.clone(),
> +        realm_type_from_name(&*realm)?,
^ Clippy: warning: deref which would be done by auto-deref, change to
`&realm`

Re: [pbs-devel] [PATCH proxmox-backup 11/12] docs: user-management: add section about AD realm support

[Lukas Wagner]

On Tue Aug 8, 2023 at 2:22 PM CEST, Christoph Heiss wrote:
> +For an Active Directory realm, the authentication domain name and the server
> +address must be specified. Most options from :ref:`_user_realms_ldap` apply to
> +Active Directory as well, most importantly the bind credentials ``bind-dn``
> +and ``password``. This is typically required by default for Microsoft Active
> +Directory.
^ Maybe mention here that one can use the LDAP DN form *and* the AD syntax.

Re: [pbs-devel] [PATCH proxmox-widget-toolkit 12/12] window: add Active Directory auth panel

[Lukas Wagner]

On Tue Aug 8, 2023 at 2:22 PM CEST, Christoph Heiss wrote:
> As AD realms are mostly just LDAP, reuse the LDAP panel and just
> show/hide some elements based on the type.

Maybe it would make sense to also change the the bind-dn field, so that it
shows the (I think) more common AD-syntax in its empty text (user@<domain>
instead of the LDAP DN syntax). A tooltip indicating that you can use both
variants could be helpful as well.

Also, I noticed that when updating the realm, the value of the 'case-sensitive'
checkbox is not persisted when changed (seems like the reason is that the
update handler in proxmox-backup's API does not consider the parameter at all)

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

[Christoph Heiss]

Thanks for the review!

On Wed, Aug 09, 2023 at 12:12:25PM +0200, Lukas Wagner wrote:
>
> On Tue Aug 8, 2023 at 2:22 PM CEST, Christoph Heiss wrote:
> > +/// 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 conn = Connection::new(ldap_config);
> > +    proxmox_async::runtime::block_on(conn.check_connection()).map_err(|e| format_err!("{e:#}"))?;
> We are already in an async function, so we should be able to use .await the
> future? Unless I'm missing something.
Yeah, you're right. Seems I forgot about that when I made that function
async while adding the retrieve_default_naming_context() call. I'll fix
that up for v2.

>
> > +
> > +    if let Some(password) = password {
> > +        auth_helpers::store_ldap_bind_password(&realm, &password, &domain_config_lock)?;
> > +    }
> > +
> > +    domains.set_data(&realm, "ad", &config)?;
> > +
> > +    domains::save_config(&domains)?;
> > +
> > +    Ok(())
> > +}
>
> General remark regarding the update-handler: You are missing the
> 'case-sensitive' parameter, so updating that parameter does not work.
Good catch, thanks! I'll fix that up too.

Re: [pbs-devel] [PATCH proxmox-widget-toolkit 12/12] window: add Active Directory auth panel

[Christoph Heiss]

On Wed, Aug 09, 2023 at 12:13:24PM +0200, Lukas Wagner wrote:
>
> On Tue Aug 8, 2023 at 2:22 PM CEST, Christoph Heiss wrote:
> > As AD realms are mostly just LDAP, reuse the LDAP panel and just
> > show/hide some elements based on the type.
>
> Maybe it would make sense to also change the the bind-dn field, so that it
> shows the (I think) more common AD-syntax in its empty text (user@<domain>
> instead of the LDAP DN syntax). A tooltip indicating that you can use both
> variants could be helpful as well.
Seems very sensible, I will add/change that - same for the documentation
regarding the same thing, as you mentioned in your previously email.
Thanks for the suggestion!

>
> Also, I noticed that when updating the realm, the value of the 'case-sensitive'
> checkbox is not persisted when changed (seems like the reason is that the
> update handler in proxmox-backup's API does not consider the parameter at all)
Yeah, I forgot to add that bit to the update handler, will fix that.

Re: [pbs-devel] [PATCH proxmox 01/12] ldap: add method for retrieving root DSE attributes

[Wolfgang Bumiller]

On Tue, Aug 08, 2023 at 02:22:03PM +0200, Christoph Heiss wrote:
> The root DSE holds common attributes about the LDAP server itself.
> Needed to e.g. support Active Directory-based LDAP servers to retrieve
> the base DN from the server itself, based on an valid bind.
> 
> See also RFC 4512, Section 5.1 [0] for more information about this
> special object.
> 
> [0] https://www.rfc-editor.org/rfc/rfc4512#section-5.1
> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>  proxmox-ldap/src/lib.rs              | 22 ++++++++++++++++++++++
>  proxmox-ldap/tests/assets/glauth.cfg |  1 +
>  proxmox-ldap/tests/glauth.rs         | 16 ++++++++++++++++
>  3 files changed, 39 insertions(+)
> 
> diff --git a/proxmox-ldap/src/lib.rs b/proxmox-ldap/src/lib.rs
> index b3b5d65..534c0c8 100644
> --- a/proxmox-ldap/src/lib.rs
> +++ b/proxmox-ldap/src/lib.rs
> @@ -198,6 +198,28 @@ impl Connection {
>          Ok(())
>      }
> 
> +    /// Retrieves an attribute from the root DSE according to RFC 4512, Section 5.1
> +    /// https://www.rfc-editor.org/rfc/rfc4512#section-5.1
> +    pub async fn retrieve_root_dse_attr(&self, attr: &str) -> Result<Vec<String>, Error> {
> +        let mut ldap = self.create_connection().await?;
> +
> +        let (entries, _res) = ldap
> +            .search("", Scope::Base, "(objectClass=*)", vec![attr])

The last parameter of `search` is an `impl AsRef<[impl AsRef<str>]>`, so
you can just pass `&[attr]` here.
The 2 other existing search calls in the crate should also be adapted,
there's no point in the extra allocation.

> +            .await?
> +            .success()?;
> +
> +        if entries.len() > 1 {
> +            bail!("found multiple root DSEs with attribute '{attr}'");
> +        }
> +
> +        entries
> +            .into_iter()
> +            .next()
> +            .map(SearchEntry::construct)
> +            .and_then(|e| e.attrs.get(attr).cloned())
> +            .ok_or_else(|| format_err!("failed to retrieve root DSE attribute '{attr}'"))
> +    }
> +
>      /// Retrive port from LDAP configuration, otherwise use the correct default
>      fn port_from_config(&self) -> u16 {
>          self.config.port.unwrap_or_else(|| {

Re: [pbs-devel] [PATCH proxmox 02/12] auth-api: implement `Display` for `Realm{, Ref}`

[Wolfgang Bumiller]

On Tue, Aug 08, 2023 at 02:22:04PM +0200, Christoph Heiss wrote:
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>  proxmox-auth-api/src/types.rs | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/proxmox-auth-api/src/types.rs b/proxmox-auth-api/src/types.rs
> index 319ac4b..8c6d7eb 100644
> --- a/proxmox-auth-api/src/types.rs
> +++ b/proxmox-auth-api/src/types.rs
> @@ -327,6 +327,18 @@ impl PartialEq<Realm> for &RealmRef {
>      }
>  }
> 
> +impl fmt::Display for Realm {
> +    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
> +        write!(f, "{}", self.0)

`self.0` is a String, we don't need to involve the whole formatting
machinery, just use `f.write_str(&self.0)`.
Btw. for a mere "forwarding" an inner value to its `Display` trait one
can also skip the formatting machinery and use
`fmt::Display::fmt(&self.0, f)` directly.

> +    }
> +}
> +
> +impl fmt::Display for RealmRef {
> +    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
> +        write!(f, "{}", &self.0)

same

> +    }
> +}
> +
>  #[api(
>      type: String,
>      format: &PROXMOX_TOKEN_NAME_FORMAT,
> --
> 2.41.0

Re: [pbs-devel] [PATCH proxmox-backup 03/12] api-types: implement `LdapMode` -> `ConnectionMode` conversion

[Wolfgang Bumiller]

On Tue, Aug 08, 2023 at 02:22:05PM +0200, Christoph Heiss wrote:
> No functional changes.
> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>  pbs-api-types/Cargo.toml  |  1 +
>  pbs-api-types/src/ldap.rs | 11 +++++++++++
>  src/auth.rs               | 12 +++---------
>  3 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/pbs-api-types/Cargo.toml b/pbs-api-types/Cargo.toml
> index 31b69f62..cb584cb5 100644
> --- a/pbs-api-types/Cargo.toml
> +++ b/pbs-api-types/Cargo.toml
> @@ -17,6 +17,7 @@ serde_plain.workspace = true
>  proxmox-auth-api = { workspace = true, features = [ "api-types" ] }
>  proxmox-human-byte.workspace = true
>  proxmox-lang.workspace=true
> +proxmox-ldap.workspace = true

The api type crate should strive to be somewhat lightweight, as it will
also end up being used in with wasm at some point where we definitely
can't pull this in.

If it really makes sense to have this locally, it should be
feature-guarded.

>  proxmox-schema = { workspace = true, features = [ "api-macro" ] }
>  proxmox-serde.workspace = true
>  proxmox-time.workspace = true
> diff --git a/pbs-api-types/src/ldap.rs b/pbs-api-types/src/ldap.rs
> index f3df90a0..e1f7c452 100644
> --- a/pbs-api-types/src/ldap.rs
> +++ b/pbs-api-types/src/ldap.rs
> @@ -1,5 +1,6 @@
>  use serde::{Deserialize, Serialize};
> 
> +use proxmox_ldap::ConnectionMode;
>  use proxmox_schema::{api, ApiStringFormat, ApiType, ArraySchema, Schema, StringSchema, Updater};
> 
>  use super::{REALM_ID_SCHEMA, SINGLE_LINE_COMMENT_SCHEMA};
> @@ -20,6 +21,16 @@ pub enum LdapMode {
>      Ldaps,
>  }
> 
> +impl From<LdapMode> for ConnectionMode {
> +    fn from(value: LdapMode) -> ConnectionMode {
> +        match value {
> +            LdapMode::Ldap => ConnectionMode::Ldap,
> +            LdapMode::StartTls => ConnectionMode::StartTls,
> +            LdapMode::Ldaps => ConnectionMode::Ldaps,
> +        }
> +    }
> +}
> +
>  #[api(
>      properties: {
>          "realm": {
> diff --git a/src/auth.rs b/src/auth.rs
> index 318d1ff2..e375ebc4 100644
> --- a/src/auth.rs
> +++ b/src/auth.rs
> @@ -16,10 +16,10 @@ use proxmox_auth_api::api::{Authenticator, LockedTfaConfig};
>  use proxmox_auth_api::ticket::{Empty, Ticket};
>  use proxmox_auth_api::types::Authid;
>  use proxmox_auth_api::Keyring;
> -use proxmox_ldap::{Config, Connection, ConnectionMode};
> +use proxmox_ldap::{Config, Connection};
>  use proxmox_tfa::api::{OpenUserChallengeData, TfaConfig};
> 
> -use pbs_api_types::{LdapMode, LdapRealmConfig, OpenIdRealmConfig, RealmRef, Userid, UsernameRef};
> +use pbs_api_types::{LdapRealmConfig, OpenIdRealmConfig, RealmRef, Userid, UsernameRef};
>  use pbs_buildcfg::configdir;
> 
>  use crate::auth_helpers;
> @@ -185,12 +185,6 @@ impl LdapAuthenticator {
>              servers.push(server.clone());
>          }
> 
> -        let tls_mode = match config.mode.unwrap_or_default() {
> -            LdapMode::Ldap => ConnectionMode::Ldap,
> -            LdapMode::StartTls => ConnectionMode::StartTls,
> -            LdapMode::Ldaps => ConnectionMode::Ldaps,
> -        };
> -
>          let (ca_store, trusted_cert) = if let Some(capath) = config.capath.as_deref() {
>              let path = PathBuf::from(capath);
>              if path.is_dir() {
> @@ -209,7 +203,7 @@ impl LdapAuthenticator {
>              base_dn: config.base_dn.clone(),
>              bind_dn: config.bind_dn.clone(),
>              bind_password: password,
> -            tls_mode,
> +            tls_mode: config.mode.unwrap_or_default().into(),
>              verify_certificate: config.verify.unwrap_or_default(),
>              additional_trusted_certificates: trusted_cert,
>              certificate_store_path: ca_store,
> --
> 2.41.0

Re: [pbs-devel] [PATCH proxmox-backup 05/12] api-types: implement `Display`, `FromStr` for `RealmType`

[Wolfgang Bumiller]

On Tue, Aug 08, 2023 at 02:22:07PM +0200, Christoph Heiss wrote:
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>  pbs-api-types/src/lib.rs | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
> index 4764c51a..6ebbe514 100644
> --- a/pbs-api-types/src/lib.rs
> +++ b/pbs-api-types/src/lib.rs
> @@ -1,5 +1,9 @@
>  //! Basic API types used by most of the PBS code.
> 
> +use core::fmt;
> +
> +use anyhow::{format_err, Error};
> +
>  use serde::{Deserialize, Serialize};
> 
>  use proxmox_auth_api::{APITOKEN_ID_REGEX_STR, USER_ID_REGEX_STR};
> @@ -508,6 +512,33 @@ pub enum RealmType {
>      Ldap,
>  }


RealmType implements Serialize and Deserialize, but uses lowercase for
its serialized values.

To my knowledge we don't yet have any types where Serialize+Deserialize
vs Display+FromStr differ like this.

If we really want this it should be commented + described in the commit
message. I do wonder though, if there's any harm in just also changing
the deserializer to use the case-insensitive `FromStr` by replacing the
derive with `serde_plain::derive_deserialize_from_fromstr!()`.

> 
> +impl fmt::Display for RealmType {
> +    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
> +        use RealmType::*;
> +        match self {
> +            Pam => write!(f, "PAM"),
> +            Pbs => write!(f, "PBS"),
> +            OpenId => write!(f, "OpenID"),
> +            Ldap => write!(f, "LDAP"),
> +        }
> +    }
> +}
> +
> +impl std::str::FromStr for RealmType {
> +    type Err = Error;
> +
> +    fn from_str(realm_type: &str) -> Result<Self, Error> {
> +        use RealmType::*;
> +        match realm_type.to_lowercase().as_str() {
> +            "pam" => Ok(Pam),
> +            "pbs" => Ok(Pbs),
> +            "openid" => Ok(OpenId),
> +            "ldap" => Ok(Ldap),
> +            _ => Err(format_err!("unknown realm type {realm_type}")),
> +        }
> +    }
> +}
> +
>  #[api(
>      properties: {
>          realm: {
> --
> 2.41.0

Re: [pbs-devel] [PATCH proxmox-backup 03/12] api-types: implement `LdapMode` -> `ConnectionMode` conversion

[Christoph Heiss]

Thanks for the review!

On Fri, Aug 11, 2023 at 12:36:41PM +0200, Wolfgang Bumiller wrote:
>
> On Tue, Aug 08, 2023 at 02:22:05PM +0200, Christoph Heiss wrote:
> > No functional changes.
> >
> > Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> > ---
> >  pbs-api-types/Cargo.toml  |  1 +
> >  pbs-api-types/src/ldap.rs | 11 +++++++++++
> >  src/auth.rs               | 12 +++---------
> >  3 files changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/pbs-api-types/Cargo.toml b/pbs-api-types/Cargo.toml
> > index 31b69f62..cb584cb5 100644
> > --- a/pbs-api-types/Cargo.toml
> > +++ b/pbs-api-types/Cargo.toml
> > @@ -17,6 +17,7 @@ serde_plain.workspace = true
> >  proxmox-auth-api = { workspace = true, features = [ "api-types" ] }
> >  proxmox-human-byte.workspace = true
> >  proxmox-lang.workspace=true
> > +proxmox-ldap.workspace = true
>
> The api type crate should strive to be somewhat lightweight, as it will
> also end up being used in with wasm at some point where we definitely
> can't pull this in.
>
> If it really makes sense to have this locally, it should be
> feature-guarded.
Ack, I'll drop this then and go with a simple, local function in
src/auth.rs instead. It is only needed in two places there anyway, and
the feature-gating isn't worth it just to be able to use `.into()` in
two places IMO ..

>
> >  proxmox-schema = { workspace = true, features = [ "api-macro" ] }
> >  proxmox-serde.workspace = true
> >  proxmox-time.workspace = true
> > diff --git a/pbs-api-types/src/ldap.rs b/pbs-api-types/src/ldap.rs
> > index f3df90a0..e1f7c452 100644
> > --- a/pbs-api-types/src/ldap.rs
> > +++ b/pbs-api-types/src/ldap.rs
> > @@ -1,5 +1,6 @@
> >  use serde::{Deserialize, Serialize};
> >
> > +use proxmox_ldap::ConnectionMode;
> >  use proxmox_schema::{api, ApiStringFormat, ApiType, ArraySchema, Schema, StringSchema, Updater};
> >
> >  use super::{REALM_ID_SCHEMA, SINGLE_LINE_COMMENT_SCHEMA};
> > @@ -20,6 +21,16 @@ pub enum LdapMode {
> >      Ldaps,
> >  }
> >
> > +impl From<LdapMode> for ConnectionMode {
> > +    fn from(value: LdapMode) -> ConnectionMode {
> > +        match value {
> > +            LdapMode::Ldap => ConnectionMode::Ldap,
> > +            LdapMode::StartTls => ConnectionMode::StartTls,
> > +            LdapMode::Ldaps => ConnectionMode::Ldaps,
> > +        }
> > +    }
> > +}
> > +
> >  #[api(
> >      properties: {
> >          "realm": {
> > diff --git a/src/auth.rs b/src/auth.rs
> > index 318d1ff2..e375ebc4 100644
> > --- a/src/auth.rs
> > +++ b/src/auth.rs
> > @@ -16,10 +16,10 @@ use proxmox_auth_api::api::{Authenticator, LockedTfaConfig};
> >  use proxmox_auth_api::ticket::{Empty, Ticket};
> >  use proxmox_auth_api::types::Authid;
> >  use proxmox_auth_api::Keyring;
> > -use proxmox_ldap::{Config, Connection, ConnectionMode};
> > +use proxmox_ldap::{Config, Connection};
> >  use proxmox_tfa::api::{OpenUserChallengeData, TfaConfig};
> >
> > -use pbs_api_types::{LdapMode, LdapRealmConfig, OpenIdRealmConfig, RealmRef, Userid, UsernameRef};
> > +use pbs_api_types::{LdapRealmConfig, OpenIdRealmConfig, RealmRef, Userid, UsernameRef};
> >  use pbs_buildcfg::configdir;
> >
> >  use crate::auth_helpers;
> > @@ -185,12 +185,6 @@ impl LdapAuthenticator {
> >              servers.push(server.clone());
> >          }
> >
> > -        let tls_mode = match config.mode.unwrap_or_default() {
> > -            LdapMode::Ldap => ConnectionMode::Ldap,
> > -            LdapMode::StartTls => ConnectionMode::StartTls,
> > -            LdapMode::Ldaps => ConnectionMode::Ldaps,
> > -        };
> > -
> >          let (ca_store, trusted_cert) = if let Some(capath) = config.capath.as_deref() {
> >              let path = PathBuf::from(capath);
> >              if path.is_dir() {
> > @@ -209,7 +203,7 @@ impl LdapAuthenticator {
> >              base_dn: config.base_dn.clone(),
> >              bind_dn: config.bind_dn.clone(),
> >              bind_password: password,
> > -            tls_mode,
> > +            tls_mode: config.mode.unwrap_or_default().into(),
> >              verify_certificate: config.verify.unwrap_or_default(),
> >              additional_trusted_certificates: trusted_cert,
> >              certificate_store_path: ca_store,
> > --
> > 2.41.0

Re: [pbs-devel] [PATCH proxmox-backup 05/12] api-types: implement `Display`, `FromStr` for `RealmType`

[Christoph Heiss]

Thanks for the review & insights (on this and the other patches too)!

On Fri, Aug 11, 2023 at 12:58:21PM +0200, Wolfgang Bumiller wrote:
>
> On Tue, Aug 08, 2023 at 02:22:07PM +0200, Christoph Heiss wrote:
> > Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> > ---
> >  pbs-api-types/src/lib.rs | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
> > index 4764c51a..6ebbe514 100644
> > --- a/pbs-api-types/src/lib.rs
> > +++ b/pbs-api-types/src/lib.rs
> > @@ -1,5 +1,9 @@
> >  //! Basic API types used by most of the PBS code.
> >
> > +use core::fmt;
> > +
> > +use anyhow::{format_err, Error};
> > +
> >  use serde::{Deserialize, Serialize};
> >
> >  use proxmox_auth_api::{APITOKEN_ID_REGEX_STR, USER_ID_REGEX_STR};
> > @@ -508,6 +512,33 @@ pub enum RealmType {
> >      Ldap,
> >  }
>
>
> RealmType implements Serialize and Deserialize, but uses lowercase for
> its serialized values.
>
> To my knowledge we don't yet have any types where Serialize+Deserialize
> vs Display+FromStr differ like this.
I'll change it so that these two implementations match.

>
> If we really want this it should be commented + described in the commit
> message. I do wonder though, if there's any harm in just also changing
> the deserializer to use the case-insensitive `FromStr` by replacing the
> derive with `serde_plain::derive_deserialize_from_fromstr!()`.
`serde_plain` does look interesting, didn't know about this crate!

Although I guess it would make more sense to instead just use
`serde_plain::derive_display_from_serialize()` and its `FromStr`
counterpart for the below, seeing as `Serialize` + `Deserialize` are
already implemented anyway (instead of switching things around).

>
> >
> > +impl fmt::Display for RealmType {
> > +    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
> > +        use RealmType::*;
> > +        match self {
> > +            Pam => write!(f, "PAM"),
> > +            Pbs => write!(f, "PBS"),
> > +            OpenId => write!(f, "OpenID"),
> > +            Ldap => write!(f, "LDAP"),
> > +        }
> > +    }
> > +}
> > +
> > +impl std::str::FromStr for RealmType {
> > +    type Err = Error;
> > +
> > +    fn from_str(realm_type: &str) -> Result<Self, Error> {
> > +        use RealmType::*;
> > +        match realm_type.to_lowercase().as_str() {
> > +            "pam" => Ok(Pam),
> > +            "pbs" => Ok(Pbs),
> > +            "openid" => Ok(OpenId),
> > +            "ldap" => Ok(Ldap),
> > +            _ => Err(format_err!("unknown realm type {realm_type}")),
> > +        }
> > +    }
> > +}
> > +
> >  #[api(
> >      properties: {
> >          realm: {
> > --
> > 2.41.0