[pve-devel] [PATCH pve-storage v2] cifs: use smbclient --use-kerberos for sec=krb5

[pve-devel] [PATCH pve-storage v2] cifs: use smbclient --use-kerberos for sec=krb5

[Hannes Laimer]

With smbclient 4.22 (shipped with Debian trixie) `-U Guest -N` does
not fall back to `no username` anymore, so our connection check can
fail for Kerberos-authenticated shares. smbclient 4.17 (shipped with
Debian bookworm) did fall back to an anonymous session, which then
succeeded when Kerberos was used.

Passing `-U` is never correct for Kerberos. Detect Kerberos via
`sec=krb5...` in the CIFS options and, in that case, avoid adding
guest/username/domain mount options and run:

  smbclient --use-kerberos=required

instead of `-U Guest -N`.

The most recent smbclient changes to the fallback-to-no-user behavior
I could find are from 2016. The handling of `-U` also does not appear
to have changed between these versions, and a default SMB protocol
version change does not seem to be involved either (last one I could
find was from 2019). I did not find a conclusive answer for why this
stopped working, but since we should not use `-U Guest` with Kerberos
at all, this change makes sense regardless.

https://gitlab.com/samba-team/samba/-/commit/35051a860c75bc119e0ac7755bd69a9ea06695a1
https://gitlab.com/samba-team/samba/-/commit/3264b1f317d6c603cc72eb2a150fe244c47aa3ac

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
v2:
 - fix bug in v1, `-o` was added before checking if kbr, and since when
   kbr we didn't add any option this lead to an invalid mount command
 - improve commit message

 src/PVE/Storage/CIFSPlugin.pm | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/src/PVE/Storage/CIFSPlugin.pm b/src/PVE/Storage/CIFSPlugin.pm
index 5b35daf..54f0f4e 100644
--- a/src/PVE/Storage/CIFSPlugin.pm
+++ b/src/PVE/Storage/CIFSPlugin.pm
@@ -66,6 +66,17 @@ sub get_cred_file {
     return undef;
 }
 
+sub cifs_uses_kerberos : prototype($) {
+    my ($scfg) = @_;
+
+    my $options = $scfg->{options};
+    return 0 if !defined($options) || $options eq '';
+
+    $options =~ s/\s+//g;
+
+    return $options =~ m/(?:^|,)sec=krb5(?:i|p)?(?:,|$)/i;
+}
+
 sub cifs_mount : prototype($$$$$) {
     my ($scfg, $storeid, $smbver, $user, $domain) = @_;
 
@@ -75,13 +86,16 @@ sub cifs_mount : prototype($$$$$) {
     $server = "[$server]" if Net::IP::ip_is_ipv6($server);
     my $source = "//${server}/$share$subdir";
 
-    my $cmd = ['/bin/mount', '-t', 'cifs', $source, $mountpoint, '-o', 'soft', '-o'];
+    my $cmd = ['/bin/mount', '-t', 'cifs', $source, $mountpoint, '-o', 'soft'];
 
-    if (my $cred_file = get_cred_file($storeid)) {
-        push @$cmd, "username=$user", '-o', "credentials=$cred_file";
+    if (cifs_uses_kerberos($scfg)) {
+        # no options needed for kerberos, adding username= or domain= would only be informal
+        # adding the if-branch here to have it explicit, and not just by not adding guest
+    } elsif (my $cred_file = get_cred_file($storeid)) {
+        push @$cmd, '-o', "username=$user", '-o', "credentials=$cred_file";
         push @$cmd, '-o', "domain=$domain" if defined($domain);
     } else {
-        push @$cmd, 'guest,username=guest';
+        push @$cmd, '-o', 'guest,username=guest';
     }
 
     push @$cmd, '-o', defined($smbver) ? "vers=$smbver" : "vers=default";
@@ -280,7 +294,9 @@ sub check_connection {
         push @$cmd, '-m', "smb" . int($scfg->{smbversion});
     }
 
-    if (my $cred_file = get_cred_file($storeid)) {
+    if (cifs_uses_kerberos($scfg)) {
+        push @$cmd, '--use-kerberos=required';
+    } elsif (my $cred_file = get_cred_file($storeid)) {
         push @$cmd, '-U', $scfg->{username}, '-A', $cred_file;
         push @$cmd, '-W', $scfg->{domain} if $scfg->{domain};
     } else {
-- 
2.47.3



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