Hi Maxim,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

>> +(define %guix-channel-introduction
>> +  ;; Introduction of the official 'guix channel.  The chosen commit is the
>> +  ;; first one that introduces '.guix-authorizations' on the 'core-updates'
>> +  ;; branch that was eventually merged in 'master'.  Any branch starting
>> +  ;; before that commit cannot be merged or it will be rejected by 'guix pull'
>> +  ;; & co.
>> +  (make-channel-introduction
>> +   "87a40d7203a813921b3ef0805c2b46c0026d6c31"
>> +   (base16-string->bytevector
>> +    (string-downcase
>> +     (string-filter char-set:hex-digit            ;mbakke
>> +                    "BBB0 2DDF 2CEA F6A8 0D1D  E643 A2A0 6DF2 A33A 54FA")))
>> +   #f))                   ;TODO: Add an intro signature so it can be exported.
>
> The GnuPG key fingerprint is SHA1 derived, which isn't cryptographically
> secure.  This doesn't mean fingerprints are unsafe *now* (given that
> forging a key to match it isn't currently practical),

Fingerprints are used as an index in the keyring here.  If somebody
introduced a second OpenPGP key with the same fingerprint in the keyring
and we picked the wrong one when verifying a signature, signature
verification would just fail.  So I think it’s perfectly OK here.

> but I don't think we should create something *today* that relies on
> SHA1 for trust.  My point is made moot by the fact that Git uses SHA1
> too... but that's another issue.  Just saying, but not blocking or
> requesting change, as I don't have a good solution for that, short of
> patching GnuPG and Git.

Right, we’re following the OpenPGP standard (which I think is fine in
this respect, at least for this use case) and Git (which is less fine).

Git commit signatures are over computed over the raw commit, something
like:

--8<---------------cut here---------------start------------->8---
tree 6e3ed6ad90013f8cb685556b0669c8a12f24bd09
parent 394566f7f4a99748f59b86da5bc551903ece5052
author Ludovic Courtès <ludo@gnu.org> 1591626893 +0200
committer Ludovic Courtès <ludo@gnu.org> 1591648112 +0200

tests: Move OpenPGP helpers to (guix tests gnupg).

* tests/git-authenticate.scm (key-id): Remove.
(%ed25519-public-key-file, %ed25519-secret-key-file)
(%ed25519bis-public-key-file, %ed25519bis-secret-key-file)
(read-openpgp-packet, key-fingerprint): Move to...
* guix/tests/gnupg.scm: ... here.
--8<---------------cut here---------------end--------------->8---

One could forge a tree object with the same SHA1: signature verification
would still pass, but the checkout could be very different.

The “SHA-1 is a Shambles” paper reads:

  The GIT developers have been working on replacing SHA-1 for a
  while[16], and they use a collision detection library [SS17] to
  mitigate the risks of collision attacks.

  [16] https://git-scm.com/docs/hash-function-transition/

On the Fediverse, someone pointed out that Bitcoin Core developers
compute a SHA512 hash of Git trees to mitigate it:

  https://github.com/bitcoin/bitcoin/tree/master/contrib/verify-commits

What they do is add a “Tree-SHA512:” line to commit logs and check
those in ‘verify-commits.py’:

--8<---------------cut here---------------start------------->8---
# Check the Tree-SHA512
if (verify_tree or prev_commit == "") and current_commit not in incorrect_sha512_allowed:
    tree_hash = tree_sha512sum(current_commit)
    if ("Tree-SHA512: {}".format(tree_hash)) not in subprocess.check_output([GIT, 'show', '-s', '--format=format:%B', current_commit]).decode('utf8').splitlines():
        print("Tree-SHA512 did not match for commit " + current_commit, file=sys.stderr)
        sys.exit(1)
--8<---------------cut here---------------end--------------->8---

We could do something similar, maybe optionally, but verification would
be expensive (you need to traverse all the blobs of the whole tree for
each commit being authenticated).

At this point, I’d wait for Git’s SHA256 migration to happen, though
<https://git-scm.com/docs/hash-function-transition/> doesn’t specify an
ETA.

>> +          ;; If it's our first time, verify CHANNEL's introductory commit.
>> +          (when (null? authenticated-commits)
>> +            (verify-introductory-commit repository
>> +                                        (channel-introduction channel)
>> +                                        keyring))
>> +
>> +          (call-with-progress-reporter reporter
>> +            (lambda (report)
>> +              (authenticate-commits repository commits
>> +                                    #:keyring keyring
>> +                                    #:report-progress report)))
>> +
>> +          (unless (null? commits)
>
> That condition is already checked above, but OK to be defensive.

Oh that’s a leftover, I’ve removed it for clarity.

>> +        (when (guix-channel? channel)
>> +          (warning (G_ "the code of channel '~a' cannot be authenticated~%")
>> +                   (channel-name channel))))
>> +
>
> Perhaps the warning message could say why.

Good point.

Below are changes I made locally to account for your feedback.

Thank you!

Ludo’.