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

  • Done
  • quality assurance status badge
Details
3 participants
  • vicvbcun
  • Ludovic Courtès
  • Richard Sent
Owner
unassigned
Submitted by
vicvbcun
Severity
normal
V
V
vicvbcun wrote on 16 Jun 17:59 +0200
(address . guix-patches@gnu.org)
434a45cea2afc5e4de5af5b15bc732b7587a979a.1718550930.git.guix@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
---
`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(+)

Toggle diff (65 lines)
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
R
R
Richard Sent wrote on 18 Jun 15:55 +0200
(name . vicvbcun)(address . guix@ikherbers.com)(address . 71594@debbugs.gnu.org)
877cem1hk1.fsf@freakingpenguin.com
Toggle quote (22 lines)
> + (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.

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

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

Toggle quote (2 lines)
> + (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. ?

slide 25

--
Take it easy,
Richard Sent
Making my computer weirder one commit at a time.
V
V
vicvbcun wrote on 20 Jun 14:58 +0200
[PATCH v2] file-systems: Allow specifying CIFS credentials in a file.
(address . 71594@debbugs.gnu.org)(name . vicvbcun)(address . mail@ikherbers.com)
cef69fb7a7db39e817f2386c63c14fa155233088.1718809540.git.guix@ikherbers.com
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(+)

Toggle diff (70 lines)
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
V
V
vicvbcun wrote on 20 Jun 15:16 +0200
Re: [bug#71594] [PATCH] file-systems: Allow specifying CIFS credentials in a file.
(name . Richard Sent)(address . richard@freakingpenguin.com)(address . 71594@debbugs.gnu.org)
ZnQrsGzmiLNYzT0n@localhost
Hi,

thanks for the review!

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

Toggle quote (2 lines)
> 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.

Toggle quote (6 lines)
> 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 :).


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

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

Toggle quote (11 lines)
> 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
R
R
Richard Sent wrote on 20 Jun 17:22 +0200
(name . vicvbcun)(address . guix@ikherbers.com)(address . 71594@debbugs.gnu.org)
87v823r654.fsf@freakingpenguin.com
Hi vicvbcun!

vicvbcun <guix@ikherbers.com> writes:

Toggle quote (28 lines)
> 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.

Toggle snippet (6 lines)
;; 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.

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.
V
V
vicvbcun wrote on 26 Jun 14:15 +0200
[PATCH v3] file-systems: Allow specifying CIFS credentials in a file.
(address . guix-patches@gnu.org)
77362216cb1e0bdef5917ea6b97284c63288cb4b.1719352537.git.guix@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 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(+)

Toggle diff (78 lines)
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
G
G
guix wrote on 26 Jun 14:32 +0200
Re: [bug#71594] [PATCH] file-systems: Allow specifying CIFS credentials in a file.
(name . Richard Sent)(address . richard@freakingpenguin.com)(address . 71594@debbugs.gnu.org)
ZnwKcgCRf4XDZKRu@localhost
Hi!

On 2024-06-20T11:22:15-0400, Richard Sent wrote:
Toggle quote (37 lines)
>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.

Toggle quote (8 lines)
>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

Toggle quote (3 lines)
>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.

Toggle quote (6 lines)
>
>--
>Take it easy,
>Richard Sent
>Making my computer weirder one commit at a time.

vicvbcun
L
L
Ludovic Courtès wrote on 26 Jul 18:51 +0200
Re: [bug#71594] [PATCH v3] file-systems: Allow specifying CIFS credentials in a file.
(name . vicvbcun)(address . guix@ikherbers.com)
87ttgcaygg.fsf@gnu.org
vicvbcun <guix@ikherbers.com> skribis:

Toggle quote (10 lines)
> 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’.
Closed
?
Your comment

This issue is archived.

To comment on this conversation send an email to 71594@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 71594
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch