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

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

[vicvbcun]

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

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

[Richard Sent]

> +  (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.

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

[vicvbcun]

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

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

[Richard Sent]

Hi vicvbcun!

vicvbcun <guix@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.

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

[guix]

Hi!

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

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

[vicvbcun]

From: vicvbcun <mail@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

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

[vicvbcun]

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

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

[Ludovic Courtès]

vicvbcun <guix@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’.