[bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit.

["Ludovic Courtès" <ludo@gnu.org>]

Hi Attila,

Sorry for dropping the ball.  On the private guix-security mailing list,
I just sent an analysis showing why the bug reported there was, AFAICS,
not a bug; however, the test here is different and seems to be poised to
expose a different problem.

Attila Lendvai <attila@lendvai.name> skribis:

> This test used to fail before a recent fix to authenticate-repository.
>
> * tests/git-authenticate.scm: New test "signed commits, .guix-authorizations,
> channel-introduction".

I won’t comment on the whole test for now, because it’s too complex, but
here’s what I noticed:


[...]

> +            (checkout "main" orphan)
> +            (add "noise0")
> +            (add ".guix-authorizations"
> +                 ,(object->string
> +                   `(authorizations
> +                     (version 0)
> +                     ((,(key-fingerprint key1) (name "Alice"))
> +                      ;; Notice that key2 is not authorized at this point.
> +                      (,(key-fingerprint key3) (name "Charlie"))))))
> +            (commit "commit 0" (signer ,(key-fingerprint key3)))
> +            (add "noise1")
> +            (commit "commit 1" (signer ,(key-fingerprint key1)))
> +            (add "noise2")
> +            (commit "commit 2" (signer ,(key-fingerprint key1))))
> +        (with-repository dir repo
> +          (let* ((commit-0 (find-commit repo "commit 0"))
> +                 (check-from
> +                  (lambda* (commit #:key (should-fail? #false) (key key1)
> +                                   (historical-authorizations
> +                                    ;; Let's mark key3 to be trusted
> +                                    ;; unconditionally, so that it authorizes
> +                                    ;; commit 0.
> +                                    (list (key-fingerprint-vector key3))))

This is exercising support for “historical authorizations”, which works
slightly differently than the regular ‘.guix-authorizations’-process
used by ‘guix pull’ & co.

> +                      (set! commit (find-commit repo commit))

Mutation is making it harder for me to understand what the test does.

> +            (with-git-repository dir
> +                ;; Drop the faulty commit 3
> +                `((reset ,(oid->string (commit-id (find-commit repo "commit 2"))))
> +                  (add "noise 4")
> +                  (add ".guix-authorizations"
> +                       ,(object->string
> +                         ;; Remove key3, add key2.
> +                         `(authorizations
> +                           (version 0)
> +                           ((,(key-fingerprint key1) (name "Alice"))
> +                            (,(key-fingerprint key2) (name "Bob"))))))
> +                  (commit "commit 4" (signer ,(key-fingerprint key2))))

Due to ‘reset’ here, the intro commit that ‘check-from’ passes to
‘authenticate-repository’ is no longer the right one (IIUC).

> +              ;; This should fail because even though commit 4 adds key2 to
> +              ;; .guix-authorizations, but commit 1 was created prior to that,
> +              ;; therefore it is not authorized.
> +              (check-from "commit 1" #:should-fail? #true)
> +              ;; This should pass, because it's a valid channel intro at commit 4
> +              (check-from "commit 4" #:key key2))
> +            (with-git-repository dir
> +                `((add "noise 5")
> +                  (commit "commit 5" (signer ,(key-fingerprint key2))))
> +              ;; It is not very intuitive why commit 1 and 2 should be trusted
> +              ;; at this point: commit 4 has previously been used as a channel
> +              ;; intro, thus it got marked as trusted in the ~/.cache/.
> +              ;; Because commit 1 and 2 are among its parents, it should also
> +              ;; be trusted at this point because of the cache.  Note that
> +              ;; it's debatable whether this semantics is a good idea, but
> +              ;; this is how git-authenticate is and has been implemented for
> +              ;; a while (modulo failing to update the cache in the past when
> +              ;; taking certain code paths).

Any commit in the closure of one of those listed in
~/.cache/guix/authentication is considered authentic.

So, if you arrange to populate that file with IDs of commits that were
not authenticated, then yes, you’ll find that ‘authenticate-repository’
reports surprising things.  But that’s because fundamentally, we cannot
protect the user from self-inflicted attacks.

To summarize, I do not see a bug here, but I’m also not sure what the
test was trying to demonstrate.

Could you boil it down?  (Keep in mind that it takes a lot of time to
merely review and under the test and claim.)

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

On guix-security (in your Dec. 22, 2021, message), you provided a
different test case, showing that the introductory commit’s signature is
not verified when ‘authenticate-repository’ is asked to authenticate an
empty commit range (introductory commit = tip of the branch).  This is
due to the (null? commit) condition in ‘authenticate-repository’:

  ;; When COMMITS is empty, it's because END-COMMIT is in the closure of
  ;; START-COMMIT and/or AUTHENTICATED-COMMITS, in which case it's known to
  ;; be authentic already.
  (if (null? commits)
      '()
      (let ((reporter (make-reporter start-commit end-commit commits)))
        ;; If it's our first time, verify START-COMMIT's signature.
        (when (null? authenticated-commits)
          (verify-introductory-commit repository keyring
                                      start-commit signer))

        …))

When START = END, the (null? commits) condition is true, and thus
‘verify-introductory-commit’ is not called.  This is the “bug”, although
START = END is a situation where there’s not much to do anyway.

As I wrote, we could move the ‘verify-introductory-commit’ call before
the ‘if’, specifically for this test case, but that doesn’t strike me as
useful; it may also have a visible performance impact on real cases.

Thanks,
Ludo’.