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>

diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
index b958f2b96a5a8c206bdaf15099dee5faebd4604a..1b63954cdf91dce24bb27ca384fa02ea65391cfa 100755 (executable)
--- a/src/PVE/Auth/LDAP.pm
+++ b/src/PVE/Auth/LDAP.pm
@@ -166,6 +166,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) = @_;
 
@@ -271,6 +300,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;
     }
 
@@ -301,7 +334,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;
            }
        }