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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 71594 in the body.
You can then email your comments to 71594 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

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

From: vicvbcun <guix <at> ikherbers.com>
To: guix-patches <at> gnu.org
Subject: [PATCH] file-systems: Allow specifying CIFS credentials in a file.
Date: Sun, 16 Jun 2024 17:59:38 +0200

As files in the store and /etc/fstab are world readable, specifying the
password in the file-system record is suboptimal.  To mitigate this,
`mount.cifs' supports reading `username', `password' and `domain' options from
a file named by the `credentials' or `cred' option.

* gnu/build/file-systems.scm (mount-file-system): Read mount options from the
file specified via the `credentials' or `cred' option if specified.

Change-Id: I786c5da373fc26d45fe7a876c56a8c4854d18532
---
`read-credential-file' is certainly not very elegant, but it matches what
`mount.cifs' does.

 gnu/build/file-systems.scm | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/gnu/build/file-systems.scm b/gnu/build/file-systems.scm
index ae29b36c4e..f0c16453e8 100644
--- a/gnu/build/file-systems.scm
+++ b/gnu/build/file-systems.scm
@@ -39,6 +39,7 @@ (define-module (gnu build file-systems)
   #:use-module (ice-9 match)
   #:use-module (ice-9 rdelim)
   #:use-module (ice-9 regex)
+  #:use-module (ice-9 string-fun)
   #:use-module (system foreign)
   #:autoload   (system repl repl) (start-repl)
   #:use-module (srfi srfi-1)
@@ -1186,6 +1187,28 @@ (define* (mount-file-system fs #:key (root "/root")
                                 (string-append "," options)
                                 "")))))
 
+  (define (read-credential-file file)
+    ;; Read password, user and domain options from file
+    (with-input-from-file file
+      (lambda ()
+        (let loop
+            ((next-line (read-line))
+             (lines '()))
+          (if (not (eof-object? next-line))
+              (loop (read-line)
+                    (cond
+                     ((string-match "^[[:space:]]*pass" next-line)
+                      ;; mount.cifs escapes commas in the password by doubling
+                      ;; them
+                      (cons (string-replace-substring (string-trim next-line) "," ",,")
+                            lines))
+                     ((string-match "^[[:space:]]*(user|dom)" next-line)
+                      (cons (string-trim next-line) lines))
+                     ;; Ignore all other lines.
+                     (else
+                      lines)))
+              lines)))))
+
   (define (mount-cifs source mount-point type flags options)
     ;; Source is of form "//<server-ip-or-host>/<service>"
     (let* ((regex-match (string-match "//([^/]+)/(.+)" source))
@@ -1194,6 +1217,8 @@ (define* (mount-file-system fs #:key (root "/root")
            ;; Match ",guest,", ",guest$", "^guest,", or "^guest$," not
            ;; e.g. user=foo,pass=notaguest
            (guest? (string-match "(^|,)(guest)($|,)" options))
+           (credential-file (and=> (string-match "(^|,)(credentials|cred)=([^,]+)(,|$)" options)
+                                   (cut match:substring <> 3)))
            ;; Perform DNS resolution now instead of attempting kernel dns
            ;; resolver upcalling. /sbin/request-key does not exist and the
            ;; kernel hardcodes the path.
@@ -1218,6 +1243,10 @@ (define* (mount-file-system fs #:key (root "/root")
                                 ;; ignores it. Also, avoiding excess commas
                                 ;; when deleting is a pain.
                                 (string-append "," options)
+                                "")
+                            (if credential-file
+                                ;; The "credentials" option is ignored too.
+                                (string-join (read-credential-file credential-file) "," 'prefix)
                                 "")))))
 
   (let* ((type    (file-system-type fs))

base-commit: 2195f70936b7aeec123d4e95345f1007d3a7bb06
-- 
2.45.1

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

From: Richard Sent <richard <at> freakingpenguin.com>
To: vicvbcun <guix <at> ikherbers.com>
Cc: 71594 <at> debbugs.gnu.org
Subject: Re: [bug#71594] [PATCH] file-systems: Allow specifying CIFS
 credentials in a file.
Date: Tue, 18 Jun 2024 09:55:42 -0400

> +  (define (read-credential-file file)
> +    ;; Read password, user and domain options from file
> +    (with-input-from-file file
> +      (lambda ()
> +        (let loop
> +            ((next-line (read-line))
> +             (lines '()))
> +          (if (not (eof-object? next-line))
> +              (loop (read-line)
> +                    (cond
> +                     ((string-match "^[[:space:]]*pass" next-line)
> +                      ;; mount.cifs escapes commas in the password by doubling
> +                      ;; them
> +                      (cons (string-replace-substring (string-trim next-line) "," ",,")
> +                            lines))
> +                     ((string-match "^[[:space:]]*(user|dom)" next-line)
> +                      (cons (string-trim next-line) lines))
> +                     ;; Ignore all other lines.
> +                     (else
> +                      lines)))
> +              lines)))))

I'd personally rename this to read-cifs-credential-file or
cifs-read-credential-file if it's only used with cifs.

You may be able to make this more compact by following a structure
similar to authorized-shell-directory? in (guix scripts shell).

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.

> + (credential-file (and=> (string-match "(^|,)(credentials|cred)=([^,]+)(,|$)" options)

Line's a bit long, can we add a newline before options?

> + (string-join (read-credential-file credential-file) "," 'prefix)

Ditto with  ",".

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.

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

From: vicvbcun <guix <at> ikherbers.com>
To: 71594 <at> debbugs.gnu.org
Cc: vicvbcun <mail <at> ikherbers.com>
Subject: [PATCH v2] file-systems: Allow specifying CIFS credentials in a file.
Date: Thu, 20 Jun 2024 14:58:23 +0200

From: vicvbcun <mail <at> ikherbers.com>

As files in the store and /etc/fstab are world readable, specifying the
password in the file-system record is suboptimal.  To mitigate this,
`mount.cifs' supports reading `username', `password' and `domain' options from
a file named by the `credentials' or `cred' option.

* gnu/build/file-systems.scm (mount-file-system): Read mount options from the
file specified via the `credentials' or `cred' option if specified.

Change-Id: I786c5da373fc26d45fe7a876c56a8c4854d18532
---
Changes since v1:

  - rename `read-credential-file' to `read-cifs-credential-file' and rewrite
    using `match'

  - break lines earlier

 gnu/build/file-systems.scm | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/gnu/build/file-systems.scm b/gnu/build/file-systems.scm
index ae29b36c4e..387b4c1af4 100644
--- a/gnu/build/file-systems.scm
+++ b/gnu/build/file-systems.scm
@@ -39,6 +39,7 @@ (define-module (gnu build file-systems)
   #:use-module (ice-9 match)
   #:use-module (ice-9 rdelim)
   #:use-module (ice-9 regex)
+  #:use-module (ice-9 string-fun)
   #:use-module (system foreign)
   #:autoload   (system repl repl) (start-repl)
   #:use-module (srfi srfi-1)
@@ -1186,6 +1187,31 @@ (define* (mount-file-system fs #:key (root "/root")
                                 (string-append "," options)
                                 "")))))
 
+  (define (read-cifs-credential-file file)
+    ;; Read password, user and domain options from file
+    (with-input-from-file file
+      (lambda ()
+        (let loop
+            ((next-line (read-line))
+             (lines '()))
+          (match next-line
+            ((? eof-object?)
+             lines)
+            ((= string-trim line)
+             (loop (read-line)
+                   (cond
+                    ((string-prefix? "pass" line)
+                     ;; mount.cifs escapes commas in the password by doubling
+                     ;; them
+                     (cons (string-replace-substring line "," ",,")
+                           lines))
+                    ((or (string-prefix? "user" line)
+                         (string-prefix? "dom" line))
+                     (cons line lines))
+                    ;; Ignore all other lines.
+                    (else
+                     lines)))))))))
+
   (define (mount-cifs source mount-point type flags options)
     ;; Source is of form "//<server-ip-or-host>/<service>"
     (let* ((regex-match (string-match "//([^/]+)/(.+)" source))
@@ -1194,6 +1220,9 @@ (define* (mount-file-system fs #:key (root "/root")
            ;; Match ",guest,", ",guest$", "^guest,", or "^guest$," not
            ;; e.g. user=foo,pass=notaguest
            (guest? (string-match "(^|,)(guest)($|,)" options))
+           (credential-file (and=> (string-match "(^|,)(credentials|cred)=([^,]+)(,|$)"
+                                                 options)
+                                   (cut match:substring <> 3)))
            ;; Perform DNS resolution now instead of attempting kernel dns
            ;; resolver upcalling. /sbin/request-key does not exist and the
            ;; kernel hardcodes the path.
@@ -1218,6 +1247,11 @@ (define* (mount-file-system fs #:key (root "/root")
                                 ;; ignores it. Also, avoiding excess commas
                                 ;; when deleting is a pain.
                                 (string-append "," options)
+                                "")
+                            (if credential-file
+                                ;; The "credentials" option is ignored too.
+                                (string-join (read-cifs-credential-file credential-file)
+                                             "," 'prefix)
                                 "")))))
 
   (let* ((type    (file-system-type fs))

base-commit: 2195f70936b7aeec123d4e95345f1007d3a7bb06
-- 
2.45.1

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

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

From: Richard Sent <richard <at> freakingpenguin.com>
To: vicvbcun <guix <at> ikherbers.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 11:22:15 -0400

Hi vicvbcun!

vicvbcun <guix <at> ikherbers.com> 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. 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---

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.

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

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

From: vicvbcun <guix <at> ikherbers.com>
To: guix-patches <at> gnu.org
Subject: [PATCH v3] file-systems: Allow specifying CIFS credentials in a file.
Date: Wed, 26 Jun 2024 14:15:28 +0200

As files in the store and /etc/fstab are world readable, specifying the
password in the file-system record is suboptimal.  To mitigate this,
`mount.cifs' supports reading `username', `password' and `domain' options from
a file named by the `credentials' or `cred' option.

* gnu/build/file-systems.scm (mount-file-system): Read mount options from the
file specified via the `credentials' or `cred' option if specified.

Change-Id: I786c5da373fc26d45fe7a876c56a8c4854d18532
---
Changes since v2:
  - Add an implementation note to `read-cifs-credential-file'.

Changes since v1:

  - rename `read-credential-file' to `read-cifs-credential-file' and rewrite
    using `match'

  - break lines earlier

 gnu/build/file-systems.scm | 42 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/gnu/build/file-systems.scm b/gnu/build/file-systems.scm
index ae29b36c4e..58e8170c0d 100644
--- a/gnu/build/file-systems.scm
+++ b/gnu/build/file-systems.scm
@@ -39,6 +39,7 @@ (define-module (gnu build file-systems)
   #:use-module (ice-9 match)
   #:use-module (ice-9 rdelim)
   #:use-module (ice-9 regex)
+  #:use-module (ice-9 string-fun)
   #:use-module (system foreign)
   #:autoload   (system repl repl) (start-repl)
   #:use-module (srfi srfi-1)
@@ -1186,6 +1187,39 @@ (define* (mount-file-system fs #:key (root "/root")
                                 (string-append "," options)
                                 "")))))
 
+  (define (read-cifs-credential-file file)
+    ;; Read password, user and domain options from file
+    ;;
+    ;; XXX: As of version 7.0, mount.cifs strips all lines of leading
+    ;; whitespace, parses those starting with "pass", "user" and "dom" into
+    ;; "pass=", "user=" and "domain=" options respectively and ignores
+    ;; everything else.  To simplify the implementation, we pass those lines
+    ;; as is.  As a consequence, the "password2" option can be specified in a
+    ;; credential file with the expected semantics (see:
+    ;; https://issues.guix.gnu.org/71594#3).
+    (with-input-from-file file
+      (lambda ()
+        (let loop
+            ((next-line (read-line))
+             (lines '()))
+          (match next-line
+            ((? eof-object?)
+             lines)
+            ((= string-trim line)
+             (loop (read-line)
+                   (cond
+                    ((string-prefix? "pass" line)
+                     ;; mount.cifs escapes commas in the password by doubling
+                     ;; them
+                     (cons (string-replace-substring line "," ",,")
+                           lines))
+                    ((or (string-prefix? "user" line)
+                         (string-prefix? "dom" line))
+                     (cons line lines))
+                    ;; Ignore all other lines.
+                    (else
+                     lines)))))))))
+
   (define (mount-cifs source mount-point type flags options)
     ;; Source is of form "//<server-ip-or-host>/<service>"
     (let* ((regex-match (string-match "//([^/]+)/(.+)" source))
@@ -1194,6 +1228,9 @@ (define* (mount-file-system fs #:key (root "/root")
            ;; Match ",guest,", ",guest$", "^guest,", or "^guest$," not
            ;; e.g. user=foo,pass=notaguest
            (guest? (string-match "(^|,)(guest)($|,)" options))
+           (credential-file (and=> (string-match "(^|,)(credentials|cred)=([^,]+)(,|$)"
+                                                 options)
+                                   (cut match:substring <> 3)))
            ;; Perform DNS resolution now instead of attempting kernel dns
            ;; resolver upcalling. /sbin/request-key does not exist and the
            ;; kernel hardcodes the path.
@@ -1218,6 +1255,11 @@ (define* (mount-file-system fs #:key (root "/root")
                                 ;; ignores it. Also, avoiding excess commas
                                 ;; when deleting is a pain.
                                 (string-append "," options)
+                                "")
+                            (if credential-file
+                                ;; The "credentials" option is ignored too.
+                                (string-join (read-cifs-credential-file credential-file)
+                                             "," 'prefix)
                                 "")))))
 
   (let* ((type    (file-system-type fs))

base-commit: 2195f70936b7aeec123d4e95345f1007d3a7bb06
-- 
2.45.1

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

From: 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: Wed, 26 Jun 2024 14:32:50 +0200

Hi!

On 2024-06-20T11:22:15-0400, Richard Sent wrote:
>Hi vicvbcun!
>
>vicvbcun <guix <at> ikherbers.com> 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

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: vicvbcun <guix <at> ikherbers.com>
Cc: Richard Sent <richard <at> freakingpenguin.com>, 71594-done <at> debbugs.gnu.org
Subject: Re: [bug#71594] [PATCH v3] file-systems: Allow specifying CIFS
 credentials in a file.
Date: Fri, 26 Jul 2024 18:51:27 +0200

vicvbcun <guix <at> ikherbers.com> skribis:

> As files in the store and /etc/fstab are world readable, specifying the
> password in the file-system record is suboptimal.  To mitigate this,
> `mount.cifs' supports reading `username', `password' and `domain' options from
> a file named by the `credentials' or `cred' option.
>
> * gnu/build/file-systems.scm (mount-file-system): Read mount options from the
> file specified via the `credentials' or `cred' option if specified.
>
> Change-Id: I786c5da373fc26d45fe7a876c56a8c4854d18532

Applied! Thank you and thanks Richard for reviewing it.

Ludo’.

Previous Next

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