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

[pbs-devel] [PATCH proxmox/proxmox-backup/pwt v2 0/15] 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.

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

RFC parts (case-insensitivity/patch #13-#15)
--------------------------------------------
The last three patches implement case-insensitive support for AD
realms, as is also implemented in PVE.

I have separated these out from the rest and marked them as RFC, since
the implementation is unfortunately not all that trivial. It needs some
support in the `proxmox-section-config` crate to support
case-insensitive lookups, and some more support in the `pbs-config`
crate to use the former, depending whether the (AD) realm supports
case-insensitive usernames. I actually forgot the implement/test it
fully in v1, thus lots more code now.

This was implemented in PVE in eb41d20 ("fix #2947 login name for the
LDAP/AD realm can be case-insensitive"), see also the accompanying
ticket [0] and forum report [1].

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. With an case-insensitive AD realm, I also
tried logging in with a non-case-matching variant of the username.

History
-------
v1: https://lists.proxmox.com/pipermail/pbs-devel/2023-August/006410.html

Notable changes v1 -> v2:
  * Applied various review comments pointed out by Lukas & Wolfgang
  * Fully implemented case-insensitive support (as separate patches)

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=2947
[1] https://forum.proxmox.com/threads/ad-sync-authentication.74547/

proxmox:

Christoph Heiss (4):
  ldap: avoid superfluous allocation when calling .search()
  ldap: add method for retrieving root DSE attributes
  auth-api: implement `Display` for `Realm{,Ref}`
  section-config: add method to retrieve case-insensitive entries

 proxmox-auth-api/src/types.rs        |  12 +++
 proxmox-ldap/src/lib.rs              |  31 ++++++--
 proxmox-ldap/tests/assets/glauth.cfg |   1 +
 proxmox-ldap/tests/glauth.rs         |  16 ++++
 proxmox-section-config/Cargo.toml    |   3 +
 proxmox-section-config/src/lib.rs    | 115 ++++++++++++++++++++++++++-
 6 files changed, 170 insertions(+), 8 deletions(-)

proxmox-backup:

Christoph Heiss (9):
  api-types: factor out `LdapMode` -> `ConnectionMode` conversion into
    own fn
  auth: factor out CA store and cert lookup into own fn
  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
  api: add case-insensitive support for Active Directory realms

 docs/user-management.rst               |  41 ++-
 pbs-api-types/src/ad.rs                | 101 +++++++
 pbs-api-types/src/lib.rs               |   8 +
 pbs-config/src/cached_user_info.rs     |  35 ++-
 pbs-config/src/domains.rs              |  80 +++++-
 src/api2/access/domain.rs              |  18 +-
 src/api2/config/access/ad.rs           | 357 +++++++++++++++++++++++++
 src/api2/config/access/mod.rs          |   2 +
 src/api2/config/sync.rs                |  18 +-
 src/auth.rs                            | 120 +++++++--
 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, 942 insertions(+), 59 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 (2):
  window: add Active Directory auth panel
  window: ldap auth edit: add case-sensitive checkbox for AD realms

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

--
2.41.0

[pbs-devel] [PATCH proxmox v2 01/15] ldap: avoid superfluous allocation when calling .search()

[Christoph Heiss]

The `attrs` parameter of `Ldap::search()` is an `impl AsRef<[impl
AsRef<str>]>` anyway, so replace `vec![..]` with `&[..]`.

Suggested-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * New patch

 proxmox-ldap/src/lib.rs | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/proxmox-ldap/src/lib.rs b/proxmox-ldap/src/lib.rs
index b3b5d65..f9862e2 100644
--- a/proxmox-ldap/src/lib.rs
+++ b/proxmox-ldap/src/lib.rs
@@ -181,12 +181,7 @@ impl Connection {

         // only search base to make sure the base_dn exists while avoiding most common size limits
         let (_, _) = ldap
-            .search(
-                &self.config.base_dn,
-                Scope::Base,
-                "(objectClass=*)",
-                vec!["*"],
-            )
+            .search(&self.config.base_dn, Scope::Base, "(objectClass=*)", &["*"])
             .await?
             .success()
             .context("Could not search LDAP realm, base_dn could be incorrect")?;
@@ -319,7 +314,7 @@ impl Connection {
         let query = format!("(&({}={}))", self.config.user_attr, username);

         let (entries, _res) = ldap
-            .search(&self.config.base_dn, Scope::Subtree, &query, vec!["dn"])
+            .search(&self.config.base_dn, Scope::Subtree, &query, &["dn"])
             .await?
             .success()?;

--
2.41.0

[pbs-devel] [PATCH proxmox v2 02/15] 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>
---
Changes v1 -> v2:
  * Avoid superfluous allocation by replacing `vec![..]` with `&[..]`

 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 f9862e2..2df7409 100644
--- a/proxmox-ldap/src/lib.rs
+++ b/proxmox-ldap/src/lib.rs
@@ -193,6 +193,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=*)", &[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 v2 03/15] auth-api: implement `Display` for `Realm{, Ref}`

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * Avoid `write!()` machinery and forward to inner type directly

 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..5b84c4b 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 {
+        fmt::Display::fmt(&self.0, f)
+    }
+}
+
+impl fmt::Display for RealmRef {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        fmt::Display::fmt(&self.0, f)
+    }
+}
+
 #[api(
     type: String,
     format: &PROXMOX_TOKEN_NAME_FORMAT,
--
2.41.0

[pbs-devel] [PATCH proxmox-backup v2 04/15] api-types: factor out `LdapMode` -> `ConnectionMode` conversion into own fn

[Christoph Heiss]

This will be needed by the AD authenticator as well, so avoid duplicate
code.

No functional changes.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * Use module-local function instead of implementing `From<LdapMode>`
    in `pbs-api-types`. See also [0]

[0] https://lists.proxmox.com/pipermail/pbs-devel/2023-August/006444.html

 src/auth.rs | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/auth.rs b/src/auth.rs
index 318d1ff2..e473da08 100644
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -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: ldap_to_conn_mode(config.mode.unwrap_or_default()),
             verify_certificate: config.verify.unwrap_or_default(),
             additional_trusted_certificates: trusted_cert,
             certificate_store_path: ca_store,
@@ -217,6 +211,14 @@ impl LdapAuthenticator {
     }
 }

+fn ldap_to_conn_mode(mode: LdapMode) -> ConnectionMode {
+    match mode {
+        LdapMode::Ldap => ConnectionMode::Ldap,
+        LdapMode::StartTls => ConnectionMode::StartTls,
+        LdapMode::Ldaps => ConnectionMode::Ldaps,
+    }
+}
+
 /// Lookup the autenticator for the specified realm
 pub(crate) fn lookup_authenticator(
     realm: &RealmRef,
--
2.41.0

[pbs-devel] [PATCH proxmox-backup v2 05/15] auth: factor out CA store and cert lookup into own fn

[Christoph Heiss]

This will be needed by the AD authenticator as well, so avoid duplicate
code.

No functional changes.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * No changes

 src/auth.rs | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/src/auth.rs b/src/auth.rs
index e473da08..04eade79 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,
@@ -219,6 +210,19 @@ fn ldap_to_conn_mode(mode: LdapMode) -> ConnectionMode {
     }
 }

+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 v2 06/15] 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>
---
Changes v1 -> v2:
  * No changes

 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 v2 07/15] api: access: add routes for managing AD realms

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * Move `case-sensitive` parameter into separate patch
  * Replace unneeded `block_on()` calls with `.await`

 pbs-api-types/src/ad.rs       |  98 ++++++++++
 pbs-api-types/src/lib.rs      |   3 +
 src/api2/config/access/ad.rs  | 348 ++++++++++++++++++++++++++++++++++
 src/api2/config/access/mod.rs |   2 +
 src/auth.rs                   |  78 +++++++-
 5 files changed, 528 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..910571a0
--- /dev/null
+++ b/pbs-api-types/src/ad.rs
@@ -0,0 +1,98 @@
+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>,
+    /// 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 4764c51a..d2bce842 100644
--- a/pbs-api-types/src/lib.rs
+++ b/pbs-api-types/src/lib.rs
@@ -110,6 +110,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..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;
+
+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::{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);
+    conn.check_connection()
+        .await
+        .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()]
+#[derive(Serialize, Deserialize)]
+#[serde(rename_all = "kebab-case")]
+/// Deletable property name
+pub enum DeletableProperty {
+    /// Fallback AD server address
+    Server2,
+    /// Port
+    Port,
+    /// Comment
+    Comment,
+    /// Verify server certificate
+    Verify,
+    /// Mode (ldap, ldap+starttls or ldaps),
+    Mode,
+    /// Bind Domain
+    BindDn,
+    /// LDAP bind passwort
+    Password,
+    /// User filter
+    Filter,
+    /// Default options for user sync
+    SyncDefaultsOptions,
+    /// user attributes to sync with AD attributes
+    SyncAttributes,
+    /// User classes
+    UserClasses,
+}
+
+#[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);
+    conn.check_connection()
+        .await
+        .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 04eade79..e4362d68 100644
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -19,7 +19,9 @@ use proxmox_auth_api::Keyring;
 use proxmox_ldap::{Config, Connection, ConnectionMode};
 use proxmox_tfa::api::{OpenUserChallengeData, TfaConfig};

-use pbs_api_types::{LdapMode, LdapRealmConfig, OpenIdRealmConfig, RealmRef, Userid, UsernameRef};
+use pbs_api_types::{
+    AdRealmConfig, LdapMode, 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: ldap_to_conn_mode(config.mode.unwrap_or_default()),
+            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 ldap_to_conn_mode(mode: LdapMode) -> ConnectionMode {
     match mode {
         LdapMode::Ldap => ConnectionMode::Ldap,
--
2.41.0

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

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * No changes

 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 e4362d68..31f1a91b 100644
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -310,6 +310,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 v2 09/15] 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>
---
Changes v1 -> v2:
  * Implement `Display` + `FromStr` for `RealmType` using the
    serde_plain crate by deriving them. Previously a separate patch, but
    now folded into this as it was reduced to a two-liner due to this
    change.
  * Fix clippy deref warning

 pbs-api-types/src/lib.rs     |  5 +++
 src/api2/access/domain.rs    | 18 ++++++++--
 src/server/realm_sync_job.rs | 69 ++++++++++++++++++++++++++++++++----
 3 files changed, 84 insertions(+), 8 deletions(-)

diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
index d2bce842..b1ac3c9d 100644
--- a/pbs-api-types/src/lib.rs
+++ b/pbs-api-types/src/lib.rs
@@ -509,8 +509,13 @@ pub enum RealmType {
     OpenId,
     /// An LDAP realm
     Ldap,
+    /// An Active Directory (AD) realm
+    Ad,
 }

+serde_plain::derive_display_from_serialize!(RealmType);
+serde_plain::derive_fromstr_from_deserialize!(RealmType);
+
 #[api(
     properties: {
         realm: {
diff --git a/src/api2/access/domain.rs b/src/api2/access/domain.rs
index 31aa62bc..8f8eebda 100644
--- a/src/api2/access/domain.rs
+++ b/src/api2/access/domain.rs
@@ -1,13 +1,14 @@
 //! List Authentication domains/realms

-use anyhow::{format_err, Error};
+use anyhow::{bail, format_err, Error};
 use serde_json::{json, Value};

 use proxmox_router::{Permission, Router, RpcEnvironment, RpcEnvironmentType, SubdirMap};
 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 Ok(section_type.parse()?);
+        }
+    }
+
+    bail!("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 v2 10/15] manager: add subcommand for managing AD realms

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * No changes

 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 v2 11/15] docs: user-management: add section about AD realm support

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * Add sentence to explain that both LDAP- and AD-syntax can be used
    for `bind-dn`

 docs/user-management.rst | 41 +++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/docs/user-management.rst b/docs/user-management.rst
index 822443f9..40bb5ac7 100644
--- a/docs/user-management.rst
+++ b/docs/user-management.rst
@@ -643,15 +643,42 @@ 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 ``bind-dn`` can be specified either in AD-specific
+``user@company.net`` syntax or the commen LDAP-DN syntax.
+
+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 v2 12/15] 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>
---
Changes v1 -> v2:
  * Change AD bind-dn `emptyText to use AD syntax
  * Add tooltip to bind-dn field to inform user that LDAP syntax can be
    used as well
  * Move case-sensitive checkbox to separate patch

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

diff --git a/src/Makefile b/src/Makefile
index 21fbe76..52094c5 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -95,6 +95,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 a7ffdf8..7f491f2 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..8cb7c80 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,21 @@ 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: 'proxmoxtextfield',
 	    fieldLabel: gettext('User Attribute Name'),
 	    name: 'user-attr',
-	    allowBlank: false,
 	    emptyText: 'uid / sAMAccountName',
+	    cbind: {
+		hidden: '{!isLdap}',
+		allowBlank: '{!isLdap}',
+	    },
 	},
 	{
 	    xtype: 'proxmoxcheckbox',
@@ -103,7 +115,14 @@ Ext.define('Proxmox.panel.LDAPInputPanel', {
 	    fieldLabel: gettext('Bind Domain Name'),
 	    name: 'bind-dn',
 	    allowBlank: false,
-	    emptyText: 'cn=user,dc=company,dc=net',
+	    cbind: {
+		emptyText: get => get('isAd') ? 'user@company.net' : 'cn=user,dc=company,dc=net',
+		autoEl: get => get('isAd') ? {
+		    tag: 'div',
+		    'data-qtip':
+			gettext('LDAP DN syntax can be used as well, e.g. cn=user,dc=company,dc=net'),
+		} : {},
+	    },
 	    bind: {
 		disabled: "{anonymous_search}",
 	    },
--
2.41.0

[pbs-devel] [RFC PATCH proxmox v2 13/15] section-config: add method to retrieve case-insensitive entries

[Christoph Heiss]

Add a new `SectionConfigData::lookup_icase()` method, to lookup	sections
which - after casefolding - might have the same names. Returned as a
list, the caller has to take take responsibility how to handle such
cases.

To have the above a) with no impact on existing code-paths and b) still
have reasonable runtime in the worse case, use a separate hashmap for the
casefolded section ids, which only contains the names of the
non-case-folded.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Biggest concern might be the increased memory usage - approximate
doubles the space needed for section names.

Changes v1 -> v2:
  * New patch

 proxmox-section-config/Cargo.toml |   3 +
 proxmox-section-config/src/lib.rs | 115 +++++++++++++++++++++++++++++-
 2 files changed, 117 insertions(+), 1 deletion(-)

diff --git a/proxmox-section-config/Cargo.toml b/proxmox-section-config/Cargo.toml
index 4da63f3..2c5b2b3 100644
--- a/proxmox-section-config/Cargo.toml
+++ b/proxmox-section-config/Cargo.toml
@@ -18,3 +18,6 @@ serde_json.workspace = true
 proxmox-schema.workspace = true
 # FIXME: remove!
 proxmox-lang.workspace = true
+
+[dev-dependencies]
+serde = { workspace = true, features = [ "derive" ] }
diff --git a/proxmox-section-config/src/lib.rs b/proxmox-section-config/src/lib.rs
index 4441df1..96d824f 100644
--- a/proxmox-section-config/src/lib.rs
+++ b/proxmox-section-config/src/lib.rs
@@ -104,6 +104,7 @@ enum ParseState<'a> {
 #[derive(Debug, Clone)]
 pub struct SectionConfigData {
     pub sections: HashMap<String, (String, Value)>,
+    sections_lowercase: HashMap<String, Vec<String>>,
     pub order: Vec<String>,
 }

@@ -118,6 +119,7 @@ impl SectionConfigData {
     pub fn new() -> Self {
         Self {
             sections: HashMap::new(),
+            sections_lowercase: HashMap::new(),
             order: Vec::new(),
         }
     }
@@ -135,6 +137,15 @@ impl SectionConfigData {
         let json = serde_json::to_value(config)?;
         self.sections
             .insert(section_id.to_string(), (type_name.to_string(), json));
+
+        let section_id_lc = section_id.to_lowercase();
+        if let Some(entry) = self.sections_lowercase.get_mut(&section_id_lc) {
+            entry.push(section_id.to_owned());
+        } else {
+            self.sections_lowercase
+                .insert(section_id_lc, vec![section_id.to_owned()]);
+        }
+
         Ok(())
     }

@@ -158,13 +169,53 @@ impl SectionConfigData {
         }
     }

-    /// Lookup section data as native rust data type.
+    pub fn lookup_json_icase(&self, type_name: &str, id: &str) -> Result<Vec<Value>, Error> {
+        let sections = self
+            .sections_lowercase
+            .get(&id.to_lowercase())
+            .ok_or_else(|| format_err!("no such {} '{}'", type_name, id))?;
+
+        let mut result = Vec::with_capacity(sections.len());
+        for section in sections {
+            let config = self.lookup_json(type_name, section)?;
+            result.push(config.clone());
+        }
+
+        Ok(result)
+    }
+
+    /// Lookup section data as native rust data type, with the section id being compared
+    /// case-sensitive.
     pub fn lookup<T: DeserializeOwned>(&self, type_name: &str, id: &str) -> Result<T, Error> {
         let config = self.lookup_json(type_name, id)?;
         let data = T::deserialize(config)?;
         Ok(data)
     }

+    /// Lookup section data as native rust data type, with the section id being compared
+    /// case-insensitive.
+    pub fn lookup_icase<T: DeserializeOwned>(
+        &self,
+        type_name: &str,
+        id: &str,
+    ) -> Result<Vec<T>, Error> {
+        let config = self.lookup_json_icase(type_name, id)?;
+        let data = config
+            .iter()
+            .fold(Ok(Vec::with_capacity(config.len())), |mut acc, c| {
+                if let Ok(acc) = &mut acc {
+                    match T::deserialize(c) {
+                        Ok(data) => acc.push(data),
+                        Err(err) => return Err(err),
+                    }
+                }
+
+                acc
+            })?;
+
+        Ok(data)
+    }
+
     /// Record section ordering
     ///
     /// Sections are written in the recorder order.
@@ -911,6 +962,68 @@ group: mygroup
     println!("CONFIG:\n{}", raw.unwrap());
 }

+#[test]
+fn test_section_config_id_case_sensitivity() {
+    let filename = "user.cfg";
+
+    const ID_SCHEMA: Schema = StringSchema::new("default id schema.")
+        .min_length(3)
+        .schema();
+    let mut config = SectionConfig::new(&ID_SCHEMA);
+
+    #[derive(Debug, PartialEq, Eq, serde::Deserialize)]
+    struct User {}
+
+    const USER_PROPERTIES: ObjectSchema = ObjectSchema::new(
+        "user properties",
+        &[(
+            "userid",
+            true,
+            &StringSchema::new("The id of the user (name@realm).")
+                .min_length(3)
+                .schema(),
+        )],
+    );
+
+    let plugin = SectionConfigPlugin::new(
+        "user".to_string(),
+        Some("userid".to_string()),
+        &USER_PROPERTIES,
+    );
+    config.register_plugin(plugin);
+
+    let raw = r"
+user: root@pam
+
+user: ambiguous@pam
+
+user: AMBIguous@pam
+";
+
+    let res = config.parse(filename, raw).unwrap();
+
+    assert_eq!(
+        res.lookup::<User>("user", "root@pam")
+            .map_err(|err| format!("{err}")),
+        Ok(User {})
+    );
+    assert_eq!(
+        res.lookup::<User>("user", "ambiguous@pam")
+            .map_err(|err| format!("{err}")),
+        Ok(User {})
+    );
+    assert_eq!(
+        res.lookup::<User>("user", "AMBIguous@pam")
+            .map_err(|err| format!("{err}")),
+        Ok(User {})
+    );
+    assert_eq!(
+        res.lookup_icase::<User>("user", "ambiguous@pam")
+            .map_err(|err| format!("{err}")),
+        Ok(vec![User {}, User {}])
+    );
+}
+
 #[test]
 fn test_section_config_with_all_of_schema() {
     let filename = "storage.cfg";
--
2.41.0

[pbs-devel] [RFC PATCH proxmox-backup v2 14/15] api: add case-insensitive support for Active Directory realms

[Christoph Heiss]

To properly support case-insensitive comparison of user names,
`CachedUserInfo` first needs to gain logic whether to look up the userid
in a case-sensitive or -insensitive manner.

The API part is pretty straight-forward, adding a new `case-sensitive`
parameter to the API (which is on-by-default).

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
It probably would be a good idea to warn the user/administrator in
`CachedUserInfo::is_active_user_id() that an ambiguous userid match
occured, but pulling the `log` crate into `pbs-config` seems
unneccesary. Should the error be bubbled up the call chain?

Changes v1 -> v2:
  * New patch

 pbs-api-types/src/ad.rs            |  3 ++
 pbs-config/src/cached_user_info.rs | 35 ++++++++++++---
 pbs-config/src/domains.rs          | 69 +++++++++++++++++++++++++++++-
 src/api2/config/access/ad.rs       |  9 ++++
 src/api2/config/sync.rs            | 18 +++++++-
 5 files changed, 126 insertions(+), 8 deletions(-)

diff --git a/pbs-api-types/src/ad.rs b/pbs-api-types/src/ad.rs
index 910571a0..446715c7 100644
--- a/pbs-api-types/src/ad.rs
+++ b/pbs-api-types/src/ad.rs
@@ -61,6 +61,9 @@ pub struct AdRealmConfig {
     /// 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>,
diff --git a/pbs-config/src/cached_user_info.rs b/pbs-config/src/cached_user_info.rs
index b9534b80..5731fb0a 100644
--- a/pbs-config/src/cached_user_info.rs
+++ b/pbs-config/src/cached_user_info.rs
@@ -9,7 +9,9 @@ use proxmox_router::UserInformation;
 use proxmox_section_config::SectionConfigData;
 use proxmox_time::epoch_i64;

-use pbs_api_types::{privs_to_priv_names, ApiToken, Authid, User, Userid, ROLE_ADMIN};
+use pbs_api_types::{
+    privs_to_priv_names, AdRealmConfig, ApiToken, Authid, User, Userid, ROLE_ADMIN,
+};

 use crate::acl::{AclTree, ROLE_NAMES};
 use crate::ConfigVersionCache;
@@ -17,6 +19,7 @@ use crate::ConfigVersionCache;
 /// Cache User/Group/Token/Acl configuration data for fast permission tests
 pub struct CachedUserInfo {
     user_cfg: Arc<SectionConfigData>,
+    domains_cfg: Arc<SectionConfigData>,
     acl_tree: Arc<AclTree>,
 }

@@ -56,6 +59,7 @@ impl CachedUserInfo {

         let config = Arc::new(CachedUserInfo {
             user_cfg: crate::user::cached_config()?,
+            domains_cfg: crate::domains::cached_config()?,
             acl_tree: crate::acl::cached_config()?,
         });

@@ -69,19 +73,40 @@ impl CachedUserInfo {

     /// Only exposed for testing
     #[doc(hidden)]
-    pub fn test_new(user_cfg: SectionConfigData, acl_tree: AclTree) -> Self {
+    pub fn test_new(
+        user_cfg: SectionConfigData,
+        domains_cfg: SectionConfigData,
+        acl_tree: AclTree,
+    ) -> Self {
         Self {
             user_cfg: Arc::new(user_cfg),
+            domains_cfg: Arc::new(domains_cfg),
             acl_tree: Arc::new(acl_tree),
         }
     }

     /// Test if a user_id is enabled and not expired
     pub fn is_active_user_id(&self, userid: &Userid) -> bool {
-        if let Ok(info) = self.user_cfg.lookup::<User>("user", userid.as_str()) {
-            info.is_active()
+        // Only Active Directory realms have the possibility to be case-insensitive
+        // Default is case-sensitive
+        let case_sensitive = match self
+            .domains_cfg
+            .lookup::<AdRealmConfig>("ad", userid.realm().as_str())
+        {
+            Ok(ad) => ad.case_sensitive.unwrap_or(true),
+            _ => true,
+        };
+
+        if case_sensitive {
+            self.user_cfg
+                .lookup::<User>("user", userid.as_str())
+                .map(|info| info.is_active())
+                .unwrap_or_default()
         } else {
-            false
+            match self.user_cfg.lookup_icase::<User>("user", userid.as_str()) {
+                Ok(users) if users.len() == 1 => users[0].is_active(),
+                _ => false,
+            }
         }
     }

diff --git a/pbs-config/src/domains.rs b/pbs-config/src/domains.rs
index 4a9beec3..49a7b2d9 100644
--- a/pbs-config/src/domains.rs
+++ b/pbs-config/src/domains.rs
@@ -1,6 +1,9 @@
-use std::collections::HashMap;
+use std::{
+    collections::HashMap,
+    sync::{Arc, RwLock},
+};

-use anyhow::Error;
+use anyhow::{bail, Error};
 use lazy_static::lazy_static;

 use pbs_buildcfg::configdir;
@@ -58,11 +61,73 @@ pub fn config() -> Result<(SectionConfigData, [u8; 32]), Error> {
     Ok((data, digest))
 }

+/// Returns a cached [`SectionConfigData`] or fresh copy read directly from the [default
+/// path](DOMAINS_CFG_FILENAME).
+///
+/// This allows fast access to the domains config without having to read and parse it every time.
+pub fn cached_config() -> Result<Arc<SectionConfigData>, Error> {
+    struct ConfigCache {
+        data: Option<Arc<SectionConfigData>>,
+        last_mtime: i64,
+        last_mtime_nsec: i64,
+    }
+
+    lazy_static! {
+        static ref CACHED_CONFIG: RwLock<ConfigCache> = RwLock::new(ConfigCache {
+            data: None,
+            last_mtime: 0,
+            last_mtime_nsec: 0
+        });
+    }
+
+    let stat = match nix::sys::stat::stat(DOMAINS_CFG_FILENAME) {
+        Ok(stat) => Some(stat),
+        Err(nix::errno::Errno::ENOENT) => None,
+        Err(err) => bail!("unable to stat '{}' - {}", DOMAINS_CFG_FILENAME, err),
+    };
+
+    {
+        // limit scope
+        let cache = CACHED_CONFIG.read().unwrap();
+        if let Some(ref config) = cache.data {
+            if let Some(stat) = stat {
+                if stat.st_mtime == cache.last_mtime && stat.st_mtime_nsec == cache.last_mtime_nsec
+                {
+                    return Ok(config.clone());
+                }
+            } else if cache.last_mtime == 0 && cache.last_mtime_nsec == 0 {
+                return Ok(config.clone());
+            }
+        }
+    }
+
+    let (config, _digest) = config()?;
+    let config = Arc::new(config);
+
+    let mut cache = CACHED_CONFIG.write().unwrap();
+    if let Some(stat) = stat {
+        cache.last_mtime = stat.st_mtime;
+        cache.last_mtime_nsec = stat.st_mtime_nsec;
+    }
+    cache.data = Some(config.clone());
+
+    Ok(config)
+}
+
 pub fn save_config(config: &SectionConfigData) -> Result<(), Error> {
     let raw = CONFIG.write(DOMAINS_CFG_FILENAME, config)?;
     replace_backup_config(DOMAINS_CFG_FILENAME, raw.as_bytes())
 }

+/// Only exposed for testing
+#[doc(hidden)]
+pub fn test_cfg_from_str(raw: &str) -> Result<(SectionConfigData, [u8; 32]), Error> {
+    let cfg = init();
+    let parsed = cfg.parse("test_domains_cfg", raw)?;
+
+    Ok((parsed, [0; 32]))
+}
+
 /// Check if a realm with the given name exists
 pub fn exists(domains: &SectionConfigData, realm: &str) -> bool {
     realm == "pbs" || realm == "pam" || domains.sections.get(realm).is_some()
diff --git a/src/api2/config/access/ad.rs b/src/api2/config/access/ad.rs
index c202291a..f59b80af 100644
--- a/src/api2/config/access/ad.rs
+++ b/src/api2/config/access/ad.rs
@@ -152,6 +152,8 @@ pub enum DeletableProperty {
     SyncAttributes,
     /// User classes
     UserClasses,
+    /// Whether usernames are compared case-sensitive or not
+    CaseSensitive,
 }

 #[api(
@@ -244,6 +246,9 @@ pub async fn update_ad_realm(
                 DeletableProperty::UserClasses => {
                     config.user_classes = None;
                 }
+                DeletableProperty::CaseSensitive => {
+                    config.case_sensitive = None;
+                }
             }
         }
     }
@@ -301,6 +306,10 @@ pub async fn update_ad_realm(
         config.user_classes = Some(user_classes);
     }

+    if let Some(case_sensitive) = update.case_sensitive {
+        config.case_sensitive = Some(case_sensitive);
+    }
+
     let mut ldap_config = if password.is_some() {
         AdAuthenticator::api_type_to_config_with_password(&config, password.clone())?
     } else {
diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
index 01e5f2ce..86b53b72 100644
--- a/src/api2/config/sync.rs
+++ b/src/api2/config/sync.rs
@@ -481,6 +481,22 @@ user: write@pbs
 "###,
     )
     .expect("test user.cfg is not parsable");
+    let (domains_cfg, _) = pbs_config::domains::test_cfg_from_str(
+        r###"
+ldap: ldap_example
+    base-dn dc=example,dc=com
+    mode ldap
+    server1 192.0.2.1
+    user-attr uid
+
+ad: ad_example
+    base-dn dc=example,dc=com
+    mode ldaps
+    server1 192.0.2.1
+
+"###,
+    )
+    .expect("test domains.cfg is not parsable");
     let acl_tree = pbs_config::acl::AclTree::from_raw(
         r###"
 acl:1:/datastore/localstore1:read@pbs,write@pbs:DatastoreAudit
@@ -493,7 +509,7 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator
     )
     .expect("test acl.cfg is not parsable");

-    let user_info = CachedUserInfo::test_new(user_cfg, acl_tree);
+    let user_info = CachedUserInfo::test_new(user_cfg, domains_cfg, acl_tree);

     let root_auth_id = Authid::root_auth_id();

--
2.41.0

[pbs-devel] [RFC PATCH proxmox-widget-toolkit v2 15/15] window: ldap auth edit: add case-sensitive checkbox for AD realms

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * New patch

 src/window/AuthEditLDAP.js | 40 ++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/src/window/AuthEditLDAP.js b/src/window/AuthEditLDAP.js
index 8cb7c80..810fe71 100644
--- a/src/window/AuthEditLDAP.js
+++ b/src/window/AuthEditLDAP.js
@@ -36,21 +36,28 @@ Ext.define('Proxmox.panel.LDAPInputPanel', {
 	    values.type = this.type;
 	}

-	if (values.anonymous_search) {
-	    if (!values.delete) {
-		values.delete = [];
-	    }
-
-	    if (!Array.isArray(values.delete)) {
-		let tmp = values.delete;
-		values.delete = [];
-		values.delete.push(tmp);
-	    }
+	if (!values.delete) {
+	    values.delete = [];
+	} else if (!Array.isArray(values.delete)) {
+	    let tmp = values.delete;
+	    values.delete = [];
+	    values.delete.push(tmp);
+	}

+	if (values.anonymous_search) {
 	    values.delete.push("bind-dn");
 	    values.delete.push("password");
 	}

+	if (values['case-sensitive']) {
+	    // Default is true, so delete if set ..
+	    values.delete.push('case-sensitive');
+	    delete values['case-sensitive'];
+	} else {
+	    // .. but if the checkbox is unticket, explicitly set to 0
+	    values['case-sensitive'] = 0;
+	}
+
 	delete values.anonymous_search;

 	return values;
@@ -92,6 +99,19 @@ Ext.define('Proxmox.panel.LDAPInputPanel', {
 		allowBlank: '{!isLdap}',
 	    },
 	},
+	{
+	    xtype: 'proxmoxcheckbox',
+	    fieldLabel: gettext('Case-sensitive'),
+	    name: 'case-sensitive',
+	    value: 1,
+	    cbind: {
+		hidden: '{!isAd}',
+	    },
+	    autoEl: {
+		tag: 'div',
+		'data-qtip': gettext('Match usernames case-sensitive'),
+	    },
+	},
 	{
 	    xtype: 'proxmoxtextfield',
 	    fieldLabel: gettext('User Attribute Name'),
--
2.41.0

Re: [pbs-devel] [RFC PATCH proxmox-backup v2 14/15] api: add case-insensitive support for Active Directory realms

[Lukas Wagner]

On 8/16/23 16:47, Christoph Heiss wrote:
> To properly support case-insensitive comparison of user names,
> `CachedUserInfo` first needs to gain logic whether to look up the userid
> in a case-sensitive or -insensitive manner.
> 
> The API part is pretty straight-forward, adding a new `case-sensitive`
> parameter to the API (which is on-by-default).
> 

Mhmm, it seems this patch breaks user permissions if logging in as one 
of the case-permutations of the original username.

Assuming you have a user 'test@ad-realm' (mapping to 
'test@ad.example.com' on the AD server) and
the 'case-sensitive = false' in the AD realm settings,
you can login as 'Test@ad-realm' as well as 'test@ad-realm' -
however, if I give the 'test@ad-realm' user permissions for some 
resources, e.g. a data store, the resource will not be accessible if I 
log in as 'Test@ad-realm'.


-- 
- Lukas

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

[Fabian Grünbichler]

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);

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

[Fabian Grünbichler]

nit: the domains.cfg docs currently have:

> You can use the proxmox-backup-manager openid and proxmox-backup-manager ldap commands to manipulate this file.

in them, that might warrant adding the 'ad' command as well.

On August 16, 2023 4:47 pm, Christoph Heiss wrote:
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
> Changes v1 -> v2:
>   * Add sentence to explain that both LDAP- and AD-syntax can be used
>     for `bind-dn`
> 
>  docs/user-management.rst | 41 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/docs/user-management.rst b/docs/user-management.rst
> index 822443f9..40bb5ac7 100644
> --- a/docs/user-management.rst
> +++ b/docs/user-management.rst
> @@ -643,15 +643,42 @@ 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

stray Proxmox VE

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

this ref doesn't work for me because it should be :ref:`user_realms_ldap` (without the leading '_')

`make html` prints

/home/fgruenbichler/Sources/proxmox-backup/docs/user-management.rst:658: WARNING: undefined label: '_user_realms_ldap'

- maybe we could add a check for that to make such things a build error?


> +Active Directory as well, most importantly the bind credentials ``bind-dn``
> +and ``password``. This is typically required by default for Microsoft Active
> +Directory. The ``bind-dn`` can be specified either in AD-specific
> +``user@company.net`` syntax or the commen LDAP-DN syntax.
> +
> +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

that ends up being propagated here ;)

> +set in the LDAP realm configuration dialog window in the GUI and via the
> +``proxmox-backup-manager ldap/ad create/update`` command.

not sure I like that style, IMHO a command should be in a format that
allows copying if possible. in this case, we could just refer to

with the ``proxmox-backup-manager ldap`` and ``proxmox-backup-manager
ad`` commands

if I copy and paste that, I get the usage list with the relevant sub
commands and parameters.

>  User synchronization can started in the GUI at

missing 'be'

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

same here, IMHO splitting the two commands makes it more user friendly.

> --
> 2.41.0
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 

Re: [pbs-devel] [RFC PATCH proxmox-backup v2 14/15] api: add case-insensitive support for Active Directory realms

[Christoph Heiss]

Thanks a lot for testing!

On Mon, Nov 27, 2023 at 10:57:03AM +0100, Lukas Wagner wrote:
>
> On 8/16/23 16:47, Christoph Heiss wrote:
> > To properly support case-insensitive comparison of user names,
> > `CachedUserInfo` first needs to gain logic whether to look up the userid
> > in a case-sensitive or -insensitive manner.
> >
> > The API part is pretty straight-forward, adding a new `case-sensitive`
> > parameter to the API (which is on-by-default).
> >
>
> Mhmm, it seems this patch breaks user permissions if logging in as one of
> the case-permutations of the original username.
>
> Assuming you have a user 'test@ad-realm' (mapping to 'test@ad.example.com'
> on the AD server) and
> the 'case-sensitive = false' in the AD realm settings,
> you can login as 'Test@ad-realm' as well as 'test@ad-realm' -
> however, if I give the 'test@ad-realm' user permissions for some resources,
> e.g. a data store, the resource will not be accessible if I log in as
> 'Test@ad-realm'.

The case-insensitive stuff is really a PITA to retrofit properly, so I
kind of expected for something to turn up ..

Anyway, I'll look into it, thanks again!

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

[Christoph Heiss]

On Tue, Nov 28, 2023 at 09:23:51AM +0100, Fabian Grünbichler wrote:
> [..]
> On August 16, 2023 4:47 pm, Christoph Heiss wrote:
> > +#[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
>
> [..]
> > +
> > +#[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 ;)

Well, the OpenID realm also uses PRIV_SYS_AUDIT for reading. So for the
sake of consistency, AD should use the same.

>
> 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).

Good catch, seems sensible. For the existing LDAP/OpenID endpoints
changing it would probably constitute a breaking change?

Changing both things above for the existing LDAP/OpenID endpoints would
probably constitute a breaking change?


> [..]
> > +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?

The LDAP endpoint for this is actually generic enough that it works for
AD too.
IOW, it just deletes the realm with that particular name from the config
(since names are unique across all realm types anyway) and the LDAP bind
password, if one exists. The infrastructure for the latter is also
reused by the AD realm.

So in favor of not unnecessarily duplicating code, I chose to simply
reuse it as-is.

At the end of the day, AD is just LDAP with some Microsoft
idiosyncrasies, thus I tried to reuse as much code/infrastructure as
possible.

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

[Christoph Heiss]

Thanks for the review!

On Tue, Nov 28, 2023 at 09:33:02AM +0100, Fabian Grünbichler wrote:
>
> nit: the domains.cfg docs currently have:
>
> > You can use the proxmox-backup-manager openid and proxmox-backup-manager ldap commands to manipulate this file.
>
> in them, that might warrant adding the 'ad' command as well.

Ack.

> [..]
> > -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
>
> this ref doesn't work for me because it should be :ref:`user_realms_ldap` (without the leading '_')
>
> `make html` prints
>
> /home/fgruenbichler/Sources/proxmox-backup/docs/user-management.rst:658: WARNING: undefined label: '_user_realms_ldap'

Interesting .. I'll fix it, good catch.

>
> - maybe we could add a check for that to make such things a build error?

Makes sense, TBH. I see what I can do, if I'm already at it.

> [..]
> > +
> > +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
>
> that ends up being propagated here ;)

Ack.

>
> > +set in the LDAP realm configuration dialog window in the GUI and via the
> > +``proxmox-backup-manager ldap/ad create/update`` command.
>
> not sure I like that style, IMHO a command should be in a format that
> allows copying if possible. in this case, we could just refer to
>
> with the ``proxmox-backup-manager ldap`` and ``proxmox-backup-manager
> ad`` commands
>
> if I copy and paste that, I get the usage list with the relevant sub
> commands and parameters.
>
> [..]
> > -be started via the ``proxmox-backup-manager ldap sync`` command.
> > +be started via the ``proxmox-backup-manager ldap/ad sync`` command.
>
> same here, IMHO splitting the two commands makes it more user friendly.

I will "de-duplicate" and reword the above paragraphs as appropriately,
thanks!