public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Friedrich Weber <f.weber@proxmox.com>
To: Lukas Wagner <l.wagner@proxmox.com>
Cc: Proxmox Backup Server development discussion
	<pbs-devel@lists.proxmox.com>
Subject: Re: [pbs-devel] [PATCH v3 proxmox{, -backup, -widget-toolkit} 00/18] add LDAP realm support
Date: Fri, 10 Feb 2023 15:01:21 +0100	[thread overview]
Message-ID: <496ccddd-344e-1c29-83ea-82630847c738@proxmox.com> (raw)
In-Reply-To: <20230209133128.695211-1-l.wagner@proxmox.com>

Tested this with slapd and it worked nicely! I found two small issues, 
see below.

Tested-by: Friedrich Weber <f.weber@proxmox.com>

## Sync fails if some LDAP user attribute violates API schema

1) Create LDAP users:
  * "a" with givenName "a" (one character)
  * "foo" with givenName "foo"
2) Add an LDAP realm, set "First Name attribute" to "givenName"
3) Sync->Preview succeeds
4) Sync->Sync fails with:

2023-02-10T14:44:14+01:00: starting realm sync for ldap
2023-02-10T14:44:14+01:00: creating user a@ldap
2023-02-10T14:44:14+01:00: creating user foo@ldap
2023-02-10T14:44:14+01:00: TASK ERROR: could not store user config

No new users were created. Not creating "a" is okay, as the API schema 
requires firstnames to have length >=2, but the sync should probably 
just skip "a" with a warning and create "foo" anyway.

## Web UI shows an error after editing an LDAP realm

1) Add an LDAP realm with default settings
2) Edit the realm and set "user classes" to "inetorgperson", click OK
3) Error message dialog "parameter verification errors" / "delete: value 
'filter,sync-defaults-options,sync-attributes' is not defined in the 
enumeration"

The decoded form data of the corresponding PUT request sets:

	"delete": [
		"server2",
		"port",
		"bind-dn",
		"password",
		"filter,sync-defaults-options,sync-attributes"
	]

... which looks like some parsing logic got confused somewhere and fused 
the last three items together.

On 09/02/2023 14:31, Lukas Wagner wrote:
> This patch series adds support for adding LDAP realms, including user sync.
> 
> The configuration scheme in `pbs-api-types` is based on the one from PVE,
> with some slight differences:
>    * consistent use of kebab-case for properties
>    * only support `mode` instead of the deprecated `secure` property
> 
> The GUI is mostly based on the implementation from PVE, with some slight
> adaptations - for details, please refer to the commit messages.
> The GUI components were added to the widget-toolkit repo, at some point PVE
> could be adapted to use the same implemention as PBS.
> 
> This patch series adds a new dependency to the `proxmox-ldap` crate,
> introduced in [1]. This also brings in `ldap3` and `lber` as new transitive
> dependencies. Both crates were already packaged and are available on the
> repository, thanks to Fabian.
> 
> The implementation was tested against the following LDAP servers:
>    * slapd 2.5.13 on Ubuntu Server 22.04 (LDAP, LDAPS, STARTTLS)
>    * Windows Server 2022 Active Directory (LDAP)
>    * glauth 2.1.0 (LDAP, LDAPS)
> 
> Some notes for testers:
>    * I can recommend `glauth` for testing: It is an LDAP server implementation
>      in a statically-compiled Go binary that can be configured using a single,
>      simple to understand configuration file. I can share my config if needed.
> 
> 
> Note: This patch series includes a cherry-picked commit from Hannes' series from
> [2]. The functionality was needed for user sync.
> 
> Changes v2 --> v3:
>    * Dropped the `Ldap` prefix for structs from the `proxmox-ldap` crate
>    * minor clippy fixes
>    * added a `OpenIdAuthenticator` that implements dummy-implements
>      `ProxmoxAuthenticator` - otherwise, manually adding users to
>      OpenId realms does not work
>    * Changed the naming of the different authenticators in `auth.rs`
>      e.g PAM --> PamAuthenticator, LDAP --> LdapAuthenticator
>      This allows us to drop some clippy-allow directives
> 
> Changes v1 --> v2:
>    * add pbs_config::exists helper function
>    * Remove now unused `password` field from `LdapRealmConfig`, add
>      additional password paramter to routes which need it
>    * Only log a warning instead of failing completely when removing a
>      stored password does not work
>    * Proper naming for `DeleteableProperty` struct
>    * Document that the domain config lock must be held when
>      the LDAP password helper functions are called.
>      Also added a &BackupLockGuard as a parameter, to make sure that
>      at least *something* is locked.
>    * moved `handle_worker` function to the `proxmox_rest_server` crate,
>      so that it is usable for both, the LDAP management CLI and the debug
>      CLI.
>    * Made user authentication async,
>     `ProxmoxAuthenticator::authenticate_user` now returns a boxed future
>    * Promoted `src/server/ldap.rs` to be its own crate - this will be
>      useful when PVE uses the same LDAP implemenation via perlmod one
>      day.
> 
> 
> [1] https://lists.proxmox.com/pipermail/pbs-devel/2023-January/005833.html
> [2] https://lists.proxmox.com/pipermail/pbs-devel/2022-December/005774.html
> 
> proxmox:
> Lukas Wagner (1):
>    rest-server: add handle_worker from backup debug cli
> 
>   proxmox-rest-server/src/worker_task.rs | 23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
> 
> proxmox-backup:
> Hannes Laimer (1):
>    pbs-config: add delete_authid to ACL-tree
> 
> Lukas Wagner (12):
>    debug cli: use handle_worker in proxmox-rest-server
>    ui: add 'realm' field in user edit
>    api-types: add LDAP configuration type
>    api: add routes for managing LDAP realms
>    auth: add LDAP realm authenticator
>    api-types: add config options for LDAP user sync
>    server: add LDAP realm sync job
>    manager: add commands for managing LDAP realms
>    docs: add configuration file reference for domains.cfg
>    docs: add documentation for LDAP realms
>    auth: add dummy OpenIdAuthenticator struct
>    auth: unify naming for all authenticator implementations
> 
>   Cargo.toml                             |   2 +
>   docs/Makefile                          |   6 +-
>   docs/conf.py                           |   1 +
>   docs/config/domains/format.rst         |  27 ++
>   docs/config/domains/man5.rst           |  21 ++
>   docs/configuration-files.rst           |  16 +
>   docs/user-management.rst               |  58 ++++
>   pbs-api-types/src/ldap.rs              | 199 +++++++++++
>   pbs-api-types/src/lib.rs               |   5 +
>   pbs-api-types/src/user.rs              |   2 +-
>   pbs-config/src/acl.rs                  |  71 ++++
>   pbs-config/src/domains.rs              |  43 ++-
>   src/api2/access/domain.rs              |  85 ++++-
>   src/api2/access/mod.rs                 |   8 +-
>   src/api2/access/tfa.rs                 |  15 +-
>   src/api2/config/access/ldap.rs         | 352 +++++++++++++++++++
>   src/api2/config/access/mod.rs          |   7 +-
>   src/api2/config/access/openid.rs       |   5 +-
>   src/auth.rs                            | 208 +++++++++--
>   src/auth_helpers.rs                    |  58 ++++
>   src/bin/docgen.rs                      |   1 +
>   src/bin/proxmox-backup-manager.rs      |   1 +
>   src/bin/proxmox_backup_debug/api.rs    |  27 +-
>   src/bin/proxmox_backup_manager/ldap.rs | 152 ++++++++
>   src/bin/proxmox_backup_manager/mod.rs  |   2 +
>   src/server/mod.rs                      |   3 +
>   src/server/realm_sync_job.rs           | 463 +++++++++++++++++++++++++
>   www/OnlineHelpInfo.js                  |   8 +
>   www/Utils.js                           |   4 +-
>   www/window/UserEdit.js                 |  95 ++++-
>   30 files changed, 1840 insertions(+), 105 deletions(-)
>   create mode 100644 docs/config/domains/format.rst
>   create mode 100644 docs/config/domains/man5.rst
>   create mode 100644 pbs-api-types/src/ldap.rs
>   create mode 100644 src/api2/config/access/ldap.rs
>   create mode 100644 src/bin/proxmox_backup_manager/ldap.rs
>   create mode 100644 src/server/realm_sync_job.rs
> 
> promxox-widget-toolkit:
> Lukas Wagner (4):
>    auth ui: add LDAP realm edit panel
>    auth ui: add LDAP sync UI
>    auth ui: add `onlineHelp` for AuthEditLDAP
>    auth ui: add `firstname` and `lastname` sync-attribute fields
> 
>   src/Makefile               |   2 +
>   src/Schema.js              |  12 ++
>   src/panel/AuthView.js      |  24 +++
>   src/window/AuthEditLDAP.js | 367 +++++++++++++++++++++++++++++++++++++
>   src/window/SyncWindow.js   | 192 +++++++++++++++++++
>   5 files changed, 597 insertions(+)
>   create mode 100644 src/window/AuthEditLDAP.js
>   create mode 100644 src/window/SyncWindow.js
> 
> 




      parent reply	other threads:[~2023-02-10 14:01 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-09 13:31 Lukas Wagner
2023-02-09 13:31 ` [pbs-devel] [PATCH v3 proxmox 01/18] rest-server: add handle_worker from backup debug cli Lukas Wagner
2023-02-10  9:44   ` [pbs-devel] applied: " Wolfgang Bumiller
2023-02-09 13:31 ` [pbs-devel] [PATCH v3 proxmox-backup 02/18] debug cli: use handle_worker in proxmox-rest-server Lukas Wagner
2023-02-09 13:31 ` [pbs-devel] [PATCH v3 proxmox-backup 03/18] pbs-config: add delete_authid to ACL-tree Lukas Wagner
2023-02-09 13:31 ` [pbs-devel] [PATCH v3 proxmox-backup 04/18] ui: add 'realm' field in user edit Lukas Wagner
2023-02-09 13:31 ` [pbs-devel] [PATCH v3 proxmox-backup 05/18] api-types: add LDAP configuration type Lukas Wagner
2023-02-09 13:31 ` [pbs-devel] [PATCH v3 proxmox-backup 06/18] api: add routes for managing LDAP realms Lukas Wagner
2023-02-09 13:31 ` [pbs-devel] [PATCH v3 proxmox-backup 07/18] auth: add LDAP realm authenticator Lukas Wagner
2023-02-09 13:31 ` [pbs-devel] [PATCH v3 proxmox-backup 08/18] api-types: add config options for LDAP user sync Lukas Wagner
2023-02-09 13:31 ` [pbs-devel] [PATCH v3 proxmox-backup 09/18] server: add LDAP realm sync job Lukas Wagner
2023-02-09 13:31 ` [pbs-devel] [PATCH v3 proxmox-backup 10/18] manager: add commands for managing LDAP realms Lukas Wagner
2023-02-10 10:16   ` Fabian Grünbichler
2023-02-10 10:30     ` Lukas Wagner
2023-02-10 12:42       ` Wolfgang Bumiller
2023-02-09 13:31 ` [pbs-devel] [PATCH v3 proxmox-backup 11/18] docs: add configuration file reference for domains.cfg Lukas Wagner
2023-02-09 13:31 ` [pbs-devel] [PATCH v3 proxmox-backup 12/18] docs: add documentation for LDAP realms Lukas Wagner
2023-02-09 13:31 ` [pbs-devel] [PATCH v3 proxmox-backup 13/18] auth: add dummy OpenIdAuthenticator struct Lukas Wagner
2023-02-09 13:31 ` [pbs-devel] [PATCH v3 proxmox-backup 14/18] auth: unify naming for all authenticator implementations Lukas Wagner
2023-02-09 13:31 ` [pbs-devel] [PATCH v3 proxmox-widget-toolkit 15/18] auth ui: add LDAP realm edit panel Lukas Wagner
2023-02-09 13:31 ` [pbs-devel] [PATCH v3 proxmox-widget-toolkit 16/18] auth ui: add LDAP sync UI Lukas Wagner
2023-02-09 13:31 ` [pbs-devel] [PATCH v3 proxmox-widget-toolkit 17/18] auth ui: add `onlineHelp` for AuthEditLDAP Lukas Wagner
2023-02-09 13:31 ` [pbs-devel] [PATCH v3 proxmox-widget-toolkit 18/18] auth ui: add `firstname` and `lastname` sync-attribute fields Lukas Wagner
2023-02-10 12:39 ` [pbs-devel] partially-applied: [PATCH v3 proxmox{, -backup, -widget-toolkit} 00/18] add LDAP realm support Wolfgang Bumiller
2023-02-10 14:01 ` Friedrich Weber [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=496ccddd-344e-1c29-83ea-82630847c738@proxmox.com \
    --to=f.weber@proxmox.com \
    --cc=l.wagner@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal