[bug#69780] [PATCH 1/4] git authenticate: Record introduction and keyring in ‘.git/config’.

[Tomas Volf]

On 2024-03-13 18:42:19 +0100, Ludovic Courtès wrote:
> * guix/scripts/git/authenticate.scm (%default-options): Remove
> ‘keyring-reference’.
> (config-value, configured-introduction, configured-keyring-reference)
> (configured?, record-configuration): New procedures.
> (guix-git-authenticate)[missing-arguments]: New procedure.
> Use ‘configured-introduction’ when zero arguments are given.
> Use ‘configured-keyring-reference’ when ‘-k’ is not passed.  Add call to
> ‘record-configuration’.
> * doc/guix.texi (Invoking guix git authenticate): Document it.
>
> Change-Id: I66e111a83f50407b52da71662629947f83a78bbc
> ---
>  doc/guix.texi                     |  12 ++-
>  guix/scripts/git/authenticate.scm | 128 ++++++++++++++++++++++--------
>  tests/guix-git-authenticate.sh    |   9 ++-
>  3 files changed, 112 insertions(+), 37 deletions(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 858d5751bf..ac0766b98c 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -7615,8 +7615,16 @@ Invoking guix git authenticate
>  and non-zero on failure.  @var{commit} above denotes the first commit
>  where authentication takes place, and @var{signer} is the OpenPGP
>  fingerprint of public key used to sign @var{commit}.  Together, they
> -form a ``channel introduction'' (@pxref{channel-authentication, channel
> -introduction}).  The options below allow you to fine-tune the process.
> +form a @dfn{channel introduction} (@pxref{channel-authentication, channel
> +introduction}).  On your first successful run, the introduction is
> +recorded in the @file{.git/config} file of your checkout, allowing you
> +to omit them from subsequent invocations:
> +
> +@example
> +guix git authenticate [@var{options}@dots{}]
> +@end example
> +
> +The options below allow you to fine-tune the process.
>
>  @table @code
>  @item --repository=@var{directory}
> diff --git a/guix/scripts/git/authenticate.scm 
> b/guix/scripts/git/authenticate.scm
> index 6ff5cee682..d3cc4065df 100644
> --- a/guix/scripts/git/authenticate.scm
> +++ b/guix/scripts/git/authenticate.scm
> @@ -31,6 +31,7 @@ (define-module (guix scripts git authenticate)
>    #:use-module (srfi srfi-1)
>    #:use-module (srfi srfi-26)
>    #:use-module (srfi srfi-37)
> +  #:use-module (srfi srfi-71)
>    #:use-module (ice-9 format)
>    #:use-module (ice-9 match)
>    #:export (guix-git-authenticate))
> @@ -73,8 +74,60 @@ (define %options
>                    (alist-cons 'show-stats? #t result)))))
>
>  (define %default-options
> -  '((directory . ".")
> -    (keyring-reference . "keyring")))
> +  '((directory . ".")))
> +
> +(define (config-value config key)
> +  "Return the config value associated with KEY, or #f if no such config was
> +found."
> +  (catch 'git-error
> +    (lambda ()
> +      (config-entry-value (config-get-entry config key)))
> +    (const #f)))
> +
> +(define (configured-introduction repository)
> +  "Return two values: the commit and signer fingerprint (strings) as
> +configured in REPOSITORY.  Error out if one or both were missing."
> +  (let* ((config (repository-config repository))
> +         (commit (config-value config 
> "guix.authentication.introduction-commit"))
> +         (signer (config-value config 
> "guix.authentication.introduction-signer")))
> +    (unless (and commit signer)
> +      (leave (G_ "unknown introductory commit and signer~%")))
> +    (values commit signer)))
> +
> +(define (configured-keyring-reference repository)
> +  "Return the keyring reference configured in REPOSITORY or #f if missing."
> +  (let ((config (repository-config repository)))
> +    (config-value config "guix.authentication.keyring")))
> +
> +(define (configured? repository)
> +  "Return true if REPOSITORY already container introduction info in its
> +'config' file."
> +  (let ((config (repository-config repository)))
> +    (and (config-value config "guix.authentication.introduction-commit")
> +         (config-value config "guix.authentication.introduction-signer"))))
> +
> +(define* (record-configuration repository
> +                               #:key commit signer keyring-reference)
> +  "Record COMMIT, SIGNER, and KEYRING-REFERENCE in the 'config' file of
> +REPOSITORY."
> +  (define directory
> +    (repository-directory repository))
> +
> +  (define config-file
> +    (in-vicinity directory "config"))

I do not think this will work with worktrees.  It will create the config file in
the worktree's git directory, but that file will be ignored by git.

    scheme@(guile-user)> (repository-discover "/home/xx/src/guix-wt/patch-1")
    $7 = "/home/xx/src/guix/.git/worktrees/orig/"
    scheme@(guile-user)> (repository-open $7)
    $8 = #<git-repository 128cbe0>
    scheme@(guile-user)> (repository-directory $8)
    $9 = "/home/xx/src/guix/.git/worktrees/orig/"
    scheme@(guile-user)> (in-vicinity $9 "config")
    $10 = "/home/xx/src/guix/.git/worktrees/orig/config"

The $10 should be "/home/xx/src/guix/.git/config" instead.

> +
> +  (call-with-port (open-file config-file "a")
> +    (lambda (port)
> +      (format port "
> +# Added by 'guix git authenticate'.
> +[guix \"authentication\"]
> +        introduction-commit = ~a
> +        introduction-signer = ~a
> +        keyring = ~a~%"
> +              commit signer keyring-reference)))

I guess these specific values might not need any escaping?  But the escaping
(and the previous problem) would be solved by just shelling out to the `git
config --local ...' to set the value.  Something to consider.

> +
> +  (info (G_ "introduction and keyring configuration recorded in '~a'~%")
> +        config-file))
>
>  (define (show-stats stats)
>    "Display STATS, an alist containing commit signing stats as returned by
> @@ -156,35 +209,48 @@ (define (guix-git-authenticate . args)
>          (progress-reporter/bar (length commits))
>          progress-reporter/silent))
>
> +  (define (missing-arguments)
> +    (leave (G_ "wrong number of arguments; \
> +expected COMMIT and SIGNER~%")))
> +
>    (with-error-handling
>      (with-git-error-handling
> -     (match (command-line-arguments options)
> -       ((commit signer)
> -        (let* ((directory   (assoc-ref options 'directory))
> -               (show-stats? (assoc-ref options 'show-stats?))
> -               (keyring     (assoc-ref options 'keyring-reference))
> -               (repository  (repository-open directory))
> -               (end         (match (assoc-ref options 'end-commit)
> -                              (#f  (reference-target
> -                                    (repository-head repository)))
> -                              (oid oid)))
> -               (history     (match (assoc-ref options 
> 'historical-authorizations)
> -                              (#f '())
> -                              (file (call-with-input-file file
> -                                      read-authorizations))))
> -               (cache-key   (or (assoc-ref options 'cache-key)
> -                                (repository-cache-key repository))))
> -          (define stats
> -            (authenticate-repository repository (string->oid commit)
> -                                     (openpgp-fingerprint* signer)
> -                                     #:end end
> -                                     #:keyring-reference keyring
> -                                     #:historical-authorizations history
> -                                     #:cache-key cache-key
> -                                     #:make-reporter make-reporter))
> +     (let* ((directory   (assoc-ref options 'directory))
> +            (show-stats? (assoc-ref options 'show-stats?))
> +            (repository  (repository-open directory))
> +            (commit signer (match (command-line-arguments options)
> +                             ((commit signer)
> +                              (values commit signer))
> +                             (()
> +                              (configured-introduction repository))
> +                             (_
> +                              (missing-arguments))))
> +            (keyring     (or (assoc-ref options 'keyring-reference)
> +                             (configured-keyring-reference repository)
> +                             "keyring"))
> +            (end         (match (assoc-ref options 'end-commit)
> +                           (#f  (reference-target
> +                                 (repository-head repository)))
> +                           (oid oid)))
> +            (history     (match (assoc-ref options 
> 'historical-authorizations)
> +                           (#f '())
> +                           (file (call-with-input-file file
> +                                   read-authorizations))))
> +            (cache-key   (or (assoc-ref options 'cache-key)
> +                             (repository-cache-key repository))))
> +       (define stats
> +         (authenticate-repository repository (string->oid commit)
> +                                  (openpgp-fingerprint* signer)
> +                                  #:end end
> +                                  #:keyring-reference keyring
> +                                  #:historical-authorizations history
> +                                  #:cache-key cache-key
> +                                  #:make-reporter make-reporter))
>
> -          (when (and show-stats? (not (null? stats)))
> -            (show-stats stats))))
> -       (_
> -        (leave (G_ "wrong number of arguments; \
> -expected COMMIT and SIGNER~%")))))))
> +       (unless (configured? repository)
> +         (record-configuration repository
> +                               #:commit commit #:signer signer
> +                               #:keyring-reference keyring))

Hm, so this records the information only on the very first successful
authentication?  So if I re-run with different values (e.g. I am creating a new
channel and `git commit --amend'-ed after checking the authorization), I am
stuck with the (now) invalid values forever until I edit the .git/config by
hand?

Also (as Skyler Ferris already mentioned) this ignores the case of multiple
branches with different authentication origins (I actually do have such use
case), so the suggested option of storing it per-branch might be nice.  Not sure
how to deal with pruning of old values though (if at all?).

> +
> +       (when (and show-stats? (not (null? stats)))
> +         (show-stats stats))))))
> diff --git a/tests/guix-git-authenticate.sh b/tests/guix-git-authenticate.sh
> index ec89f941e6..db60816d45 100644
> --- a/tests/guix-git-authenticate.sh
> +++ b/tests/guix-git-authenticate.sh
> @@ -1,5 +1,5 @@
>  # GNU Guix --- Functional package management for GNU
> -# Copyright © 2020, 2022 Ludovic Courtès <ludo@gnu.org>
> +# Copyright © 2020, 2022, 2024 Ludovic Courtès <ludo@gnu.org>
>  #
>  # This file is part of GNU Guix.
>  #
> @@ -40,10 +40,11 @@ guix git authenticate "$intro_commit" "$intro_signer"     
> \
>       --end=9549f0283a78fe36f2d4ff2a04ef8ad6b0c02604 && false
>
>  # The v1.2.0 commit is a descendant of $intro_commit and it satisfies the
> -# authorization invariant.
> +# authorization invariant.  No need to repeat $intro_commit and $intro_signer
> +# because it should have been recorded in '.git/config'.
>  v1_2_0_commit="a099685659b4bfa6b3218f84953cbb7ff9e88063"
> -guix git authenticate "$intro_commit" "$intro_signer"        \
> -     --cache-key="$cache_key" --stats                        \
> +guix git authenticate                                \
> +     --cache-key="$cache_key" --stats                \
>       --end="$v1_2_0_commit"
>
>  rm "$XDG_CACHE_HOME/guix/authentication/$cache_key"
> --
> 2.41.0

Have a nice day,
Tomas Volf

--
There are only two hard things in Computer Science:
cache invalidation, naming things and off-by-one errors.

signature.asc
Description: PGP signature