Subject: Re: [bug#71594] [PATCH] file-systems: Allow specifying CIFS credentials in a file.

From: [email-masked]
Date: Wed, 26 Jun 2024 14:32:50 +0200

Hi!

On 2024-06-20T11:22:15-0400, Richard Sent wrote:
>Hi vicvbcun!
>
>vicvbcun <[email-masked]> writes:
>
>> Hi,
>>
>> thanks for the review!
>
>>> 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.
>>
>> 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
>
>Agreed, emulating mount.cifs in totality is too much. My concern with
>divergences in functionality is most users will read mount.cifs
>documentation for CIFS mount-options and whatnot, then potentially get
>bit when Guix does something different.
>In this case the divergence is small and shouldn't cause issues.
As long as what Guix offers is a superset, feature requests won't be 
sent our way :). With the patch, I think there is should be a reasonable 
match between the behaviour as documented in mount.cifs(8) and that of 
Guix.

>I think a XXX: style comment is appropriate.
>
>--8<---------------cut here---------------start------------->8---
>;; Read password, user and domain options from file
>;;
>;; XXX: Unlike mount.cifs this function reads password2 in the
>;; credential file and returns it separately from password.
>--8<---------------cut here---------------end--------------->8---
done, I have elaborated a bit more though

>I wouldn't be surprised if mount.cifs eventually adopts the same
>behavior. I can't think of a reason why putting password2 in the
>credentials file shouldn't be supported.
The `password2' options is seems mainly useful for remounting when a new 
password is available. Creating a temporary credential file might be 
considered overcomplicated. I suspect that when developing the feature 
it just worked when specified on the command line and was forgotten 
about subsequently.

>
>-- 
>Take it easy,
>Richard Sent
>Making my computer weirder one commit at a time.

vicvbcun