* [pve-devel] [PATCH access-control/manager 0/2] ldap: check bind connection on realm add/update @ 2023-07-27 13:33 Christoph Heiss 2023-07-27 13:33 ` [pve-devel] [PATCH access-control 1/2] api: domains: add off-by-default `check-connection` parameter Christoph Heiss 2023-07-27 13:33 ` [pve-devel] [PATCH manager 2/2] ui: ldap: add 'Check connection' checkbox as advanced option Christoph Heiss 0 siblings, 2 replies; 7+ messages in thread From: Christoph Heiss @ 2023-07-27 13:33 UTC (permalink / raw) To: pve-devel First of, this removes the dreaded LDAP DN regex. Further, upon saving a LDAP realm in the UI, it tries to connect & bind using the provided credentials, providing the user with immediate feedback whether they are valid or not. The same approach is already implemented in PBS [0], and I'll plan to implement the same for PMG too, if & when the PVE side is done. Testing ------- Changes were tested against slapd 2.5.13+dfsg-5, using both the web UI and `pveum` to create and update realms with different combinations of valid and invalid parameters, mixed with using new `check-connection` parameter. Prior art --------- This completely supersedes the previous series [1]. This series is a complete new approach to it (also why this also isn't marked as v3), which previously tried to solve this using a new schema format by validated DNs using Net::LDAP::Util::canonical_dn(). But this has the problem that it does not support AD-specific DN syntax. After a off-list discussion with Lukas (summary [2]), it was decided to rather implement it much like PBS does it - simply drop the explicit validation of DN parameters, instead just trying to connect & bind to the target server - although I'm always open for other/better suggestions to tackle this. [0] https://git.proxmox.com/?p=proxmox-backup.git;a=commitdiff;h=5210f3b5 [1] https://lists.proxmox.com/pipermail/pve-devel/2023-July/058392.html [2] https://lists.proxmox.com/pipermail/pve-devel/2023-July/058540.html -- 2.41.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [pve-devel] [PATCH access-control 1/2] api: domains: add off-by-default `check-connection` parameter 2023-07-27 13:33 [pve-devel] [PATCH access-control/manager 0/2] ldap: check bind connection on realm add/update Christoph Heiss @ 2023-07-27 13:33 ` Christoph Heiss 2023-07-28 8:29 ` Lukas Wagner 2023-07-27 13:33 ` [pve-devel] [PATCH manager 2/2] ui: ldap: add 'Check connection' checkbox as advanced option Christoph Heiss 1 sibling, 1 reply; 7+ messages in thread From: Christoph Heiss @ 2023-07-27 13:33 UTC (permalink / raw) To: pve-devel Removes the dreaded DN regex, instead introducing a connect/bind check on creation/update, aligning it with the way PBS does it. This is enabled by default for new realms, but opt-in for existing, to not break backwards-compatibility. Additionally, it has the benefit that instead of letting a sync fail on the first try due to e.g. bad bind credentials, it gives the user some direct feedback when trying to add/update a LDAP realm, if enabled. Should be rather a corner case, but it's the easiest way for us to accomodate and the most versatile for users needing this. This is part of the result of a previous discussion [0], and the same approach is already implemented for PBS [1]. [0] https://lists.proxmox.com/pipermail/pve-devel/2023-May/056839.html [1] https://git.proxmox.com/?p=proxmox-backup.git;a=commitdiff;h=5210f3b5 Signed-off-by: Christoph Heiss <c.heiss@proxmox.com> --- The decision to put this behind an optional (off-by-default) parameter was made during an off-list discussion with Lukas. The summary of that can be found as a follow-up on the previous series [2]. Note that the `base_dn`/`group_dn` now goes completely unvalided - but again, that is how it works in PBS. We could use canonical_dn() for this (as implemented in the previous series [3]), depending on whether we want this kind of "divergence" between PVE & PBS. I'm happy to re-add that should be the way forward. [2] https://lists.proxmox.com/pipermail/pve-devel/2023-July/058540.html [3] https://lists.proxmox.com/pipermail/pve-devel/2023-July/058392.html src/PVE/API2/Domains.pm | 8 ++++++++ src/PVE/Auth/LDAP.pm | 26 +++++++++++++++++--------- src/PVE/Auth/Plugin.pm | 8 ++++++++ 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/src/PVE/API2/Domains.pm b/src/PVE/API2/Domains.pm index 9332aa7..e921208 100644 --- a/src/PVE/API2/Domains.pm +++ b/src/PVE/API2/Domains.pm @@ -165,6 +165,10 @@ __PACKAGE__->register_method ({ } $plugin->on_add_hook($realm, $config, password => $password); + # Only for LDAP/AD, implied through the existence of the 'check-connection' param + $plugin->check_connection($realm, $config, password => $password) + if defined($param->{'check-connection'}) && $param->{'check-connection'}; + cfs_write_file($domainconfigfile, $cfg); }, "add auth server failed"); @@ -237,6 +241,10 @@ __PACKAGE__->register_method ({ $plugin->on_update_hook($realm, $config); } + # Only for LDAP/AD, implied through the existence of the 'check-connection' param + $plugin->check_connection($realm, $config, password => $password) + if defined($param->{'check-connection'}) && $param->{'check-connection'}; + cfs_write_file($domainconfigfile, $cfg); }, "update auth server failed"); diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm index fc82a17..c4c4cda 100755 --- a/src/PVE/Auth/LDAP.pm +++ b/src/PVE/Auth/LDAP.pm @@ -10,9 +10,6 @@ use PVE::Tools; use base qw(PVE::Auth::Plugin); -my $dn_part_regex = qr!("[^"]+"|[^ ,+"/<>;=#][^,+"/<>;=]*[^ ,+"/<>;=]|[^ ,+"/<>;=#])!; -our $dn_regex = qr!\w+=${dn_part_regex}(,\s*\w+=${dn_part_regex})*!; - sub type { return 'ldap'; } @@ -22,7 +19,6 @@ sub properties { base_dn => { description => "LDAP base domain name", type => 'string', - pattern => $dn_regex, optional => 1, maxLength => 256, }, @@ -36,7 +32,6 @@ sub properties { bind_dn => { description => "LDAP bind domain name", type => 'string', - pattern => $dn_regex, optional => 1, maxLength => 256, }, @@ -94,7 +89,6 @@ sub properties { description => "LDAP base domain name for group sync. If not set, the" ." base_dn will be used.", type => 'string', - pattern => $dn_regex, optional => 1, maxLength => 256, }, @@ -137,7 +131,14 @@ sub properties { type => 'boolean', optional => 1, default => 1, - } + }, + 'check-connection' => { + description => 'Check bind connection to LDAP server.', + type => 'boolean', + optional => 1, + # TODO: Make it enabled-by-default with PVE 9.0? + default => 0, + }, }; } @@ -169,6 +170,7 @@ sub options { 'sync-defaults-options' => { optional => 1 }, mode => { optional => 1 }, 'case-sensitive' => { optional => 1 }, + 'check-connection' => { optional => 1 }, }; } @@ -184,7 +186,7 @@ sub get_scheme_and_port { } sub connect_and_bind { - my ($class, $config, $realm) = @_; + my ($class, $config, $realm, $param) = @_; my $servers = [$config->{server1}]; push @$servers, $config->{server2} if $config->{server2}; @@ -215,7 +217,7 @@ sub connect_and_bind { if ($config->{bind_dn}) { my $bind_dn = $config->{bind_dn}; - my $bind_pass = ldap_get_credentials($realm); + my $bind_pass = $param->{password} || ldap_get_credentials($realm); die "missing password for realm $realm\n" if !defined($bind_pass); PVE::LDAP::ldap_bind($ldap, $bind_dn, $bind_pass); } elsif ($config->{cert} && $config->{certkey}) { @@ -454,4 +456,10 @@ sub on_delete_hook { ldap_delete_credentials($realm); } +sub check_connection { + my ($class, $realm, $config, %param) = @_; + + $class->connect_and_bind($config, $realm, \%param); +} + 1; diff --git a/src/PVE/Auth/Plugin.pm b/src/PVE/Auth/Plugin.pm index 2f64a13..429bc88 100755 --- a/src/PVE/Auth/Plugin.pm +++ b/src/PVE/Auth/Plugin.pm @@ -317,4 +317,12 @@ sub on_delete_hook { # do nothing by default } +# called during addition and updates of realms (before the new domain config gets written) +# die to abort addition/update in case the connection/bind fails +# NOTE: runs in a storage config *locked* context +sub check_connection { + my ($class, $realm, $config, %param) = @_; + # do nothing by default +} + 1; -- 2.41.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH access-control 1/2] api: domains: add off-by-default `check-connection` parameter 2023-07-27 13:33 ` [pve-devel] [PATCH access-control 1/2] api: domains: add off-by-default `check-connection` parameter Christoph Heiss @ 2023-07-28 8:29 ` Lukas Wagner 2023-08-01 9:17 ` Christoph Heiss 0 siblings, 1 reply; 7+ messages in thread From: Lukas Wagner @ 2023-07-28 8:29 UTC (permalink / raw) To: Proxmox VE development discussion, c.heiss On Thu Jul 27, 2023 at 3:33 PM CEST, Christoph Heiss wrote: > Removes the dreaded DN regex, instead introducing a connect/bind check > on creation/update, aligning it with the way PBS does it. This is > enabled by default for new realms, but opt-in for existing, to not break > backwards-compatibility. > > Additionally, it has the benefit that instead of letting a sync fail on > the first try due to e.g. bad bind credentials, it gives the user some > direct feedback when trying to add/update a LDAP realm, if enabled. > > Should be rather a corner case, but it's the easiest way for us to > accomodate and the most versatile for users needing this. > I think it would be enough to have the 'check-connection' parameter only for the API call itself, I wouldn't store it in the domains.cfg configuration file. That would imply that the 'Check configuration' checkbox that you introduce in the next patch could *always* be ticked, even for old realms. So whenever you create/update an LDAP/AD realm configuration you have to explicitly tell it "hey, I do not want the check right now, my LDAP server is down currently". My main point in making the check behavior opt-in rather was so that scripts/API consumers continue to work as before. For the GUI however, it should be fine to just always check by default, unless the behavior is explicitly turned off. > + }, > + 'check-connection' => { > + description => 'Check bind connection to LDAP server.', > + type => 'boolean', > + optional => 1, > + # TODO: Make it enabled-by-default with PVE 9.0? ^ This wouldn't be necessary any more. > + default => 0, > + }, > }; > } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH access-control 1/2] api: domains: add off-by-default `check-connection` parameter 2023-07-28 8:29 ` Lukas Wagner @ 2023-08-01 9:17 ` Christoph Heiss 0 siblings, 0 replies; 7+ messages in thread From: Christoph Heiss @ 2023-08-01 9:17 UTC (permalink / raw) To: Lukas Wagner; +Cc: Proxmox VE development discussion Thanks for the review! On Fri, Jul 28, 2023 at 10:29:26AM +0200, Lukas Wagner wrote: > > On Thu Jul 27, 2023 at 3:33 PM CEST, Christoph Heiss wrote: > > [..] > > I think it would be enough to have the 'check-connection' parameter only for > the API call itself, I wouldn't store it in the domains.cfg configuration > file. > > That would imply that the 'Check configuration' checkbox that you introduce in > the next patch could *always* be ticked, even for old realms. So whenever you > create/update an LDAP/AD realm configuration you have to explicitly tell it > "hey, I do not want the check right now, my LDAP server is down currently". > > My main point in making the check behavior opt-in rather was so that > scripts/API consumers continue to work as before. For the GUI however, it > should be fine to just always check by default, unless the behavior is > explicitly turned off. My thought here was that if you once explicitly turn it off, it should stay off (if that's needed for /some/ reason). But OTOH, making it always opt-out for the GUI definitely makes sense too. I'll give the series a new spin with your suggestions implemented, seems sensible enough to me. Most importantly, we can get rid of the regex in the end :^) > > > + }, > > + 'check-connection' => { > > + description => 'Check bind connection to LDAP server.', > > + type => 'boolean', > > + optional => 1, > > + # TODO: Make it enabled-by-default with PVE 9.0? > ^ This wouldn't be necessary any more. > > + default => 0, > > + }, > > }; > > } > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [pve-devel] [PATCH manager 2/2] ui: ldap: add 'Check connection' checkbox as advanced option 2023-07-27 13:33 [pve-devel] [PATCH access-control/manager 0/2] ldap: check bind connection on realm add/update Christoph Heiss 2023-07-27 13:33 ` [pve-devel] [PATCH access-control 1/2] api: domains: add off-by-default `check-connection` parameter Christoph Heiss @ 2023-07-27 13:33 ` Christoph Heiss 2023-07-28 8:37 ` Lukas Wagner 1 sibling, 1 reply; 7+ messages in thread From: Christoph Heiss @ 2023-07-27 13:33 UTC (permalink / raw) To: pve-devel The checkbox is enabled by default for new realms, setting the new `check-connection` parameter. Won't effect existing configurations, i.e. being opt-in for them, to not break existing setups. Signed-off-by: Christoph Heiss <c.heiss@proxmox.com> --- As this uses the newly introduced `check-connection` API parameter, a bump of libpve-access-control will be needed. www/manager6/dc/AuthEditLDAP.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/www/manager6/dc/AuthEditLDAP.js b/www/manager6/dc/AuthEditLDAP.js index 2ce16e58c..6f0472ae3 100644 --- a/www/manager6/dc/AuthEditLDAP.js +++ b/www/manager6/dc/AuthEditLDAP.js @@ -79,6 +79,21 @@ Ext.define('PVE.panel.LDAPInputPanel', { }, ]; + me.advancedItems = [ + { + xtype: 'proxmoxcheckbox', + fieldLabel: gettext('Check connection'), + name: 'check-connection', + uncheckedValue: 0, + checked: me.isCreate, + autoEl: { + tag: 'div', + 'data-qtip': + gettext('Verify connection parameters and bind credentials on save'), + }, + }, + ]; + me.callParent(); }, onGetValues: function(values) { -- 2.41.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH manager 2/2] ui: ldap: add 'Check connection' checkbox as advanced option 2023-07-27 13:33 ` [pve-devel] [PATCH manager 2/2] ui: ldap: add 'Check connection' checkbox as advanced option Christoph Heiss @ 2023-07-28 8:37 ` Lukas Wagner 2023-08-01 9:18 ` Christoph Heiss 0 siblings, 1 reply; 7+ messages in thread From: Lukas Wagner @ 2023-07-28 8:37 UTC (permalink / raw) To: Proxmox VE development discussion, c.heiss On Thu Jul 27, 2023 at 3:33 PM CEST, Christoph Heiss wrote: > The checkbox is enabled by default for new realms, setting the new > `check-connection` parameter. > > Won't effect existing configurations, i.e. being opt-in for them, to not > break existing setups. As mentioned in my other reply, I think setting the new parameter by default should not be a problem, as long as it is only in the GUI. > > --- a/www/manager6/dc/AuthEditLDAP.js > +++ b/www/manager6/dc/AuthEditLDAP.js > @@ -79,6 +79,21 @@ Ext.define('PVE.panel.LDAPInputPanel', { > }, > ]; > > + me.advancedItems = [ > + { > + xtype: 'proxmoxcheckbox', > + fieldLabel: gettext('Check connection'), > + name: 'check-connection', > + uncheckedValue: 0, > + checked: me.isCreate, > + autoEl: { > + tag: 'div', > + 'data-qtip': > + gettext('Verify connection parameters and bind credentials on save'), > + }, > + }, > + ]; > + > me.callParent(); > }, > onGetValues: function(values) { AD realms have their own GUI component, so I guess it would also make sense to add the new parameter there. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH manager 2/2] ui: ldap: add 'Check connection' checkbox as advanced option 2023-07-28 8:37 ` Lukas Wagner @ 2023-08-01 9:18 ` Christoph Heiss 0 siblings, 0 replies; 7+ messages in thread From: Christoph Heiss @ 2023-08-01 9:18 UTC (permalink / raw) To: Lukas Wagner; +Cc: Proxmox VE development discussion On Fri, Jul 28, 2023 at 10:37:12AM +0200, Lukas Wagner wrote: > > On Thu Jul 27, 2023 at 3:33 PM CEST, Christoph Heiss wrote: > > The checkbox is enabled by default for new realms, setting the new > > `check-connection` parameter. > > > > Won't effect existing configurations, i.e. being opt-in for them, to not > > break existing setups. > As mentioned in my other reply, I think setting the new parameter by default > should not be a problem, as long as it is only in the GUI. > > > > --- a/www/manager6/dc/AuthEditLDAP.js > > +++ b/www/manager6/dc/AuthEditLDAP.js > > @@ -79,6 +79,21 @@ Ext.define('PVE.panel.LDAPInputPanel', { > > }, > > ]; > > > > + me.advancedItems = [ > > + { > > + xtype: 'proxmoxcheckbox', > > + fieldLabel: gettext('Check connection'), > > + name: 'check-connection', > > + uncheckedValue: 0, > > + checked: me.isCreate, > > + autoEl: { > > + tag: 'div', > > + 'data-qtip': > > + gettext('Verify connection parameters and bind credentials on save'), > > + }, > > + }, > > + ]; > > + > > me.callParent(); > > }, > > onGetValues: function(values) { > > AD realms have their own GUI component, so I guess it would also make sense to > add the new parameter there. Right, forgot that. I'll add it for v2, thanks! ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-08-01 9:19 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-07-27 13:33 [pve-devel] [PATCH access-control/manager 0/2] ldap: check bind connection on realm add/update Christoph Heiss 2023-07-27 13:33 ` [pve-devel] [PATCH access-control 1/2] api: domains: add off-by-default `check-connection` parameter Christoph Heiss 2023-07-28 8:29 ` Lukas Wagner 2023-08-01 9:17 ` Christoph Heiss 2023-07-27 13:33 ` [pve-devel] [PATCH manager 2/2] ui: ldap: add 'Check connection' checkbox as advanced option Christoph Heiss 2023-07-28 8:37 ` Lukas Wagner 2023-08-01 9:18 ` Christoph Heiss
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox