| Message ID | 434a45cea2afc5e4de5af5b15bc732b7587a979a.1718550930.git.guix@ikherbers.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#71594] file-systems: Allow specifying CIFS credentials in a file. | expand |
> + (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
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
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.
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))