LDAP sync: improve validation of synced attributes

and skip the ones not fitting our schema, while warning the user about
them.

Also warns the user if the specified 'sync_attributes' mapping
contains entries for attributes that don't exist, e.g.
'enabled=active' (since the property on PVE side is called 'enable').

For the 'enable' property, any value coming from the server led to the
user being enabled, even "0", because it is a string. This is not
changed by this patch, by not trying to validate or parse a boolean.

In get_users(), the username is also set in the returned hash, but
without the realm. This doesn't seem to be necessary for syncing,
because the username with the realm is used as a hash key and that's
what's relied upon when updating the config. But the tests require it
to be set, so that is not changed by this patch either.

Relies on the user properties (other than username) to be standard
options called 'user-XYZ'. Could be improved by moving the schema for
user properties from the API module to a module that can be accessed
by both API and plugin here and creating a helper for accessing it.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
(cherry picked from commit cb93636b55c26f641fdbedef10dbb6e80b4502b8)
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>

diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
index fc82a17ad3baa26e1c5cc1a4fd71d3af105f0c15..f31c65564c85c3ebaec859ceef8d9ba55f708a20 100755 (executable)
--- a/src/PVE/Auth/LDAP.pm
+++ b/src/PVE/Auth/LDAP.pm
@@ -172,6 +172,35 @@ sub options {
     };
 }
 
+my $valid_sync_attributes = {
+    username => 1,
+    enable => 1,
+    expire => 1,
+    firstname => 1,
+    lastname => 1,
+    email => 1,
+    comment => 1,
+    keys => 1,
+};
+
+my sub verify_sync_attribute {
+    my ($attr, $value) = @_;
+
+    die "cannot map to invalid user sync attribute '$attr'\n" if !$valid_sync_attributes->{$attr};
+
+    # The attribute does not include the realm, so can't use PVE::Auth::Plugin::verify_username
+    if ($attr eq 'username') {
+       die "value '$value' does not look like a valid user name\n"
+           if $value !~ m/${PVE::Auth::Plugin::user_regex}/;
+       return;
+    }
+
+    return if $attr eq 'enable'; # for backwards compat, don't parse/validate
+
+    my $schema = PVE::JSONSchema::get_standard_option("user-$attr");
+    PVE::JSONSchema::validate($value, $schema, "invalid value '$value'\n");
+}
+
 sub get_scheme_and_port {
     my ($class, $config) = @_;
 
@@ -277,6 +306,10 @@ sub get_users {
 
     foreach my $attr (PVE::Tools::split_list($config->{sync_attributes})) {
        my ($ours, $ldap) = ($attr =~ m/^\s*(\w+)=(.*)\s*$/);
+       if (!$valid_sync_attributes->{$ours}) {
+           warn "bad 'sync_attributes': cannot map to invalid attribute '$ours'\n";
+           next;
+       }
        $ldap_attribute_map->{$ldap} = $ours;
     }
 
@@ -307,7 +340,13 @@ sub get_users {
 
        foreach my $attr (keys %$user_attributes) {
            if (my $ours = $ldap_attribute_map->{$attr}) {
-               $ret->{$username}->{$ours} = $user_attributes->{$attr}->[0];
+               my $value = $user_attributes->{$attr}->[0];
+               eval { verify_sync_attribute($ours, $value) };
+               if (my $err = $@) {
+                   warn "skipping attribute mapping '$attr'->'$ours' for user '$username' - $err";
+                   next;
+               }
+               $ret->{$username}->{$ours} = $value;
            }
        }