GNU bug report logs - #71594
[PATCH] file-systems: Allow specifying CIFS credentials in a file.

Previous Next

Full log

Message #14 received at 71594 <at> debbugs.gnu.org (full text, mbox):

From: vicvbcun <guix <at> ikherbers.com>
To: Richard Sent <richard <at> freakingpenguin.com>
Cc: 71594 <at> debbugs.gnu.org
Subject: Re: [bug#71594] [PATCH] file-systems: Allow specifying CIFS
 credentials in a file.
Date: Thu, 20 Jun 2024 15:16:32 +0200

Hi,

thanks for the review!

On 2024-06-18T09:55:42-0400, Richard Sent wrote:
> [...]
> I'd personally rename this to read-cifs-credential-file or
> cifs-read-credential-file if it's only used with cifs.
done

> You may be able to make this more compact by following a structure
> similar to authorized-shell-directory? in (guix scripts shell).
I rewrote it using `match'; while not more compact, I like it more.

> I believe CIFS will add a password2 mount option in 6.9.4 [1]. We should
> check if mount.cifs supports putting that option in the credentials file
> and match their behavior. If that's too much an ask (Guix's mount.cifs
> may not be new enough), I think a comment or proactive bug report is
> appropriate.

If my understanding is correct, the `password2' option is just a way to 
supply an additional password the kernel may use when rotating 
passwords.

Looking at the latest version of mount.cifs[0], it doesn't seem to 
handle `password2' intentionally: Passing `password2' on the command 
line should work, but only because the return value of `parse_opt_token' 
is not checked for `OPT_ERROR'; in a credentials file it is accepted (as 
`parse_cred_line' only checks for a "pass" prefix) but passed as 
`password' instead.

I think that being able to specify `password2' in a credentials file 
makes sense and my patch doesn't forbid it.

If exposing an interface identical to that of `mount.cifs' and 
preserving the exact semantics (e.g `mount.cifs' complains when multiple 
passwords are specified and takes the first one) is the ultimate goal, 
I'd just shell out to `mount.cifs'. I certainly won't implement all the 
idiosyncrasies :).

0: https://git.samba.org/?p=cifs-utils.git;a=blob;f=mount.cifs.c;h=3b7a6b3c22e8c3b563c7ea92ecb9891fdfac01a6;hb=refs/heads/for-next

> > + (credential-file (and=> (string-match "(^|,)(credentials|cred)=([^,]+)(,|$)" options)
> 
> Line's a bit long, can we add a newline before options?
done

> > + (string-join (read-credential-file credential-file) "," 'prefix)
> 
> Ditto with  ",".
done

> Otherwise looks good to me. Thanks, with this I think we handle every
> mount option the same way as mount.cifs. 😄
> 
> [1]: https://sambaxp.org/fileadmin/user_upload/sambaxp2024-Slides/sxp24-French-accessing_remote.pdf,
> slide 25
> 
> -- 
> Take it easy,
> Richard Sent
> Making my computer weirder one commit at a time.

vicvbcun

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.