[bug#69780,4/4] DRAFT news: Add entry for ‘guix git authenticate’ changes.

* etc/news.scm: Add entry.

Change-Id: I661a0c0bfc373b87a70508ad9a735315c96ba4a5
---
 etc/news.scm | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Hi there.

Haven’t tested, but looks like now I won’t get confused which guix git
authenticate command from my bash history is the right one for which
repository. :)

Could you add this German translation:

Ludovic Courtès <ludo@gnu.org> writes:
> +        (title
> +         (en "@command{guix git authenticate} usage simplified")

          (de "@command{guix git authenticate} ist leichter nutzbar"))


> )
> +        (body
> +         (en "Usage of the @command{guix git authenticate} command

          (de "Der Befehl @command{guix git authenticate} kann jetzt einfacher
benutzt werden.  Mit dem Befehl können Kanalautoren und Entwickler die
Provenienz ihres Codes überprüfen.

Beim ersten Gebrauch speichert @command{guix git authenticate} Commit und
Unterzeichner (wie in der @dfn{Kanaleinführung}) in der Datei
@file{.git/config} Ihres Repositorys, so dass Sie sie bei späteren
Ausführungen nicht mehr auf der Befehlszeile angeben müssen.  Auch werden
Git-Hooks für pre-push und post-checkout installiert, wenn es bisher keine
Hooks dieser Art gibt.

Führen Sie @command{info \"(guix.de) Aufruf von guix git authenticate\"}
aus, wenn Sie mehr wissen wollen.")))




Regards,
Florian

Hello Ludo’,
This looks like a great way to improve protection for guix developers. Saving the input, which still initially comes from the developer, and using it automatically in the future means less overhead for developers and less chances for something to go wrong in the process. I'm trying it locally on my machine and there are a few things I noticed.

> +  (if (or (file-exists? pre-push-hook)
> +          (file-exists? fpost-checkout-hook))
> +      (begin
> +        (warning (G_ "not overriding pre-existing hooks '~a' and '~a'~%")
> +                 pre-push-hook post-checkout-hook)
> +        (display-hint (G_ "Consider running @command{guix git authenticate}
> +from your pre-push and update hooks so your repository is automatically
> +authenticated before you push or receive updates.")))

When the developer builds guix a pre-push hook is already installed, from etc/git/pre-push. This runs `make authenticate` itself but also runs `make check-channel-news`. I don't think we can just get rid of that file because then people would lose check-channel-news, but it seems useful to have this functionality built into the authenticate script so that other projects can use it. Unfortunately, unconditionally appending to the script could cause problems because the developer could have added their own hook which could have been written in any language. Perhaps we could update etc/git/pre-push to call the authenticate script in the new way and install any hooks that do not clobber (eg, if pre-push exists then we skip that hook but install the rest)?

> +  (define post-checkout-hook
> +    (in-vicinity directory "hooks/post-checkout"))

The post-checkout hook does not run when `git pull` is called. Instead, the post-merge hook is called. Note that post-merge does not receive the same set of arguments as post-checkout. I had success replacing "$newrev" with "$(git rev-parse HEAD)". We could leave out the value completely for post-merge because the script will use HEAD by default if no end is given. But maybe we don't want to rely on default behavior for a script that will not be automatically updated with the tool.

I can think of more ways that a developer could end up on a new commit. For example, running `git fetch` followed by `git reset --hard`. I'm not sure if it makes to support every possible case because that could get complicated very quickly. But git pull is the most common way to get updates (at least in my experience, which could have a sample bias) so I think it makes sense to at least support that.

> +while read local_ref local_oid remote_ref remote_oid
> +do
> +  guix git authenticate --end=\"$local_ref\"
> +done\n")

I believe this should be "$local_oid", not "$local_ref".

> +(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)))

I am wondering how this would work if somebody is working with multiple branches, in particular if they do not all use the same introduction. Maybe that doesn't need to be addressed in this patch series but it might be easier to address it in the future if the branch name was included in the config file instead assuming that a specific introduction applies to every branch in a given checkout (for example, by using "guix.authentication.introduction-commit.branch-name"). This is probably more relevant to external users of the tool than to the guix repository itself. The logistics of using unrelated branches simultaneously seems complicated and not very helpful, especially when channels are such an appealing alternative. But it could be useful for other projects.

> +(define (configured? repository)
> +  "Return true if REPOSITORY already container introduction info in its
> +'config' file."

Typo: this should be "contains", not "container"

Regards,
Skyler

Hi!

"pelzflorian (Florian Pelz)" <pelzflorian@pelzflorian.de> skribis:

> Haven’t tested, but looks like now I won’t get confused which guix git
> authenticate command from my bash history is the right one for which
> repository. :)

Heheh.  :-)

> Could you add this German translation:

Will do, thanks!

Ludo’.

Hi Skyler,

Skyler Ferris <skyvine@protonmail.com> skribis:

>> +  (if (or (file-exists? pre-push-hook)
>> +          (file-exists? fpost-checkout-hook))
>> +      (begin
>> +        (warning (G_ "not overriding pre-existing hooks '~a' and '~a'~%")
>> +                 pre-push-hook post-checkout-hook)
>> +        (display-hint (G_ "Consider running @command{guix git authenticate}
>> +from your pre-push and update hooks so your repository is automatically
>> +authenticated before you push or receive updates.")))
>
> When the developer builds guix a pre-push hook is already installed,
> from etc/git/pre-push.

Right.  Like I wrote when replying to Tomas, I view this as a helper
primarily for people outside Guix itself, because Guix already has its
own hooks installed as you write.

We could discuss what to do with Guix’s own hooks, but to me that’s a
separate issue.

>> +  (define post-checkout-hook
>> +    (in-vicinity directory "hooks/post-checkout"))
>
> The post-checkout hook does not run when `git pull` is called. Instead, the post-merge hook is called. Note that post-merge does not receive the same set of arguments as post-checkout. I had success replacing "$newrev" with "$(git rev-parse HEAD)". We could leave out the value completely for post-merge because the script will use HEAD by default if no end is given. But maybe we don't want to rely on default behavior for a script that will not be automatically updated with the tool.
>
> I can think of more ways that a developer could end up on a new commit. For example, running `git fetch` followed by `git reset --hard`. I'm not sure if it makes to support every possible case because that could get complicated very quickly. But git pull is the most common way to get updates (at least in my experience, which could have a sample bias) so I think it makes sense to at least support that.

I spent time looking for the “right” hook and couldn’t find anything
really satisfying.  Ideally, I’d want a hook that runs on ‘fetch’, for
each new reference.

Is post-merge better than post-checkout?  githooks(5) says ‘post-merge’
“is invoked by git-merge(1), which happens when a git pull is done on a
local repository.”  Is it actually invoked when ‘git pull’ does *not*
trigger a merge?

>> +while read local_ref local_oid remote_ref remote_oid
>> +do
>> +  guix git authenticate --end=\"$local_ref\"
>> +done\n")
>
> I believe this should be "$local_oid", not "$local_ref".

Oops, noted.

>> +(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)))
>
> I am wondering how this would work if somebody is working with multiple branches, in particular if they do not all use the same introduction. Maybe that doesn't need to be addressed in this patch series but it might be easier to address it in the future if the branch name was included in the config file instead assuming that a specific introduction applies to every branch in a given checkout (for example, by using "guix.authentication.introduction-commit.branch-name"). This is probably more relevant to external users of the tool than to the guix repository itself. The logistics of using unrelated branches simultaneously seems complicated and not very helpful, especially when channels are such an appealing alternative. But it could be useful for other projects.

Very good point.  Now, what would it look like?

Currently we have:

--8<---------------cut here---------------start------------->8---
[guix "authentication"]
        introduction-commit = 9edb3f66fd807b096b48283debdcddccfea34bad
        introduction-signer = BBB0 2DDF 2CEA F6A8 0D1D  E643 A2A0 6DF2 A33A 54FA
        keyring = keyring
--8<---------------cut here---------------end--------------->8---

Using this configuration format, it seems there’s no room left for a
branch name, or am I overlooking something?

Or we could take the risk of removing ‘guix’ and make it:

--8<---------------cut here---------------start------------->8---
[authentication "master"]
        introduction-commit = 9edb3f66fd807b096b48283debdcddccfea34bad
        introduction-signer = BBB0 2DDF 2CEA F6A8 0D1D  E643 A2A0 6DF2 A33A 54FA
        keyring = keyring
--8<---------------cut here---------------end--------------->8---

WDYT?

Thanks for your feedback!

Ludo’.

On 3/19/24 07:12, Ludovic Courtès wrote:

> Right.  Like I wrote when replying to Tomas, I view this as a helper
> primarily for people outside Guix itself, because Guix already has its
> own hooks installed as you write.
>
> We could discuss what to do with Guix’s own hooks, but to me that’s a
> separate issue.

Fair enough, that can be addressed in a separate patch. On a related note, this patch reminded me of an idea I mentioned in a private thread with the security team about writing a package that provides `guix git authenticate` as a standalone script, which can be done by extracting the dependent modules and building just them (I have successfully done this for the "(guix build utils)" library, for shell-like guile scripts that have a lighter footprint than all of guix). This will help people who are installing guix from source for the first time. They will not have guix itself available, but it would be easier for them to use the standalone script than to install all of guix (which they will presumably be replacing anyway). Is there anything else around `guix git authenticate` you are working on that might conflict with this? I am starting to look at it now that I have been reminded.

> I spent time looking for the “right” hook and couldn’t find anything
> really satisfying.  Ideally, I’d want a hook that runs on ‘fetch’, for
> each new reference.
>
> Is post-merge better than post-checkout?  githooks(5) says ‘post-merge’
> “is invoked by git-merge(1), which happens when a git pull is done on a
> local repository.”  Is it actually invoked when ‘git pull’ does *not*
> trigger a merge?

That gave me pause too, but I tested it with a pull that caused a fast-forward (no separate merge commit) and it still ran the hook. I looked for a `post-fetch` hook but couldn't find one, I agree that would be ideal.

> Very good point.  Now, what would it look like?
>
> Currently we have:
>
> --8<---------------cut here---------------start------------->8---
> [guix "authentication"]
>         introduction-commit = 9edb3f66fd807b096b48283debdcddccfea34bad
>         introduction-signer = BBB0 2DDF 2CEA F6A8 0D1D  E643 A2A0 6DF2 A33A 54FA
>         keyring = keyring
> --8<---------------cut here---------------end--------------->8---
>
> Using this configuration format, it seems there’s no room left for a
> branch name, or am I overlooking something?
>
> Or we could take the risk of removing ‘guix’ and make it:
>
> --8<---------------cut here---------------start------------->8---
> [authentication "master"]
>         introduction-commit = 9edb3f66fd807b096b48283debdcddccfea34bad
>         introduction-signer = BBB0 2DDF 2CEA F6A8 0D1D  E643 A2A0 6DF2 A33A 54FA
>         keyring = keyring
> --8<---------------cut here---------------end--------------->8---
>
> WDYT?

Hmm, I didn't realize that git limited the number of components available in config names, but it looks like you're correct - my suggestion of appending `.branch-name` caused git to produce an error that the config was invalid.

But we don't necessarily need git to be aware of the semantic meaning of the branch name since the value is calculated by the guile script. So we could just use a hyphen as a separator and have "introduction-commit-master" and "introduction-commit-feature" as separate values unrelated to each other (aside from being in the "guix authentication" namespace).

Regards,
Skyler

On 3/20/24 18:43, Skyler Ferris wrote:

> That gave me pause too, but I tested it with a pull that caused a fast-forward (no separate merge commit) and it still ran the hook. I looked for a `post-fetch` hook but couldn't find one, I agree that would be ideal.

It turns out that a post-fetch hook has been discussed on the git mailing list. Notably, someone asked about it for a very similar use-case starting at (1). Much of the discussion is about the way this person implemented the hook, because they wanted to prevent refs from being updated instead of simply alerting the user of the potentially bad commits. There is also a categorical objection to the idea of implementing a post-fetch hook (2), in part because modifying refs could lead to problems for the environment (such as clients re-fetching, then rejecting, the same commits repeatedly leading to excess bandwidth use) (3). In spite of this, there are WIP patches at the end of the thread implementing a tweak-fetch hook at the end of the thread (4), but I don't think they were ever completed... haven't looked at those too closely yet but maybe there's something somewhere else about them.

(1) https://lore.kernel.org/git/5898be69-4211-d441-494d-93477179cf0e@gaspard.io/
(2) https://lore.kernel.org/git/25bd770c-6a48-5b5d-04cc-6d02784ea3e7@gaspard.io/
(3) https://lore.kernel.org/git/87lgg1bwmi.fsf@evledraar.gmail.com/
(4) https://lore.kernel.org/git/30753d19-d77d-1a1a-ba42-afcd6fbb4223@gaspard.io/

Hi!

Skyler Ferris <skyvine@protonmail.com> skribis:

> Fair enough, that can be addressed in a separate patch. On a related note, this patch reminded me of an idea I mentioned in a private thread with the security team about writing a package that provides `guix git authenticate` as a standalone script, which can be done by extracting the dependent modules and building just them (I have successfully done this for the "(guix build utils)" library, for shell-like guile scripts that have a lighter footprint than all of guix). This will help people who are installing guix from source for the first time. They will not have guix itself available, but it would be easier for them to use the standalone script than to install all of guix (which they will presumably be replacing anyway). Is there anything else around `guix git authenticate` you are working on that might conflict with this? I am starting to look at it now that I have been reminded.

I must say I’m usually focusing on the use case where people building
Guix from source already have a working Guix installation.  In that
case, it’s that installation that provides a source of trust.

For someone building entirely from source, I don’t know what the right
solution is.  It could be extracting ‘guix git authenticate’ as you say,
but then we’re just offsetting the problem.  We could just as well
recommend building from a signed tag (or signed source tarball) after
verifying it.  Dunno what a good bootstrapping story might be.

>> I spent time looking for the “right” hook and couldn’t find anything
>> really satisfying.  Ideally, I’d want a hook that runs on ‘fetch’, for
>> each new reference.
>>
>> Is post-merge better than post-checkout?  githooks(5) says ‘post-merge’
>> “is invoked by git-merge(1), which happens when a git pull is done on a
>> local repository.”  Is it actually invoked when ‘git pull’ does *not*
>> trigger a merge?
>
> That gave me pause too, but I tested it with a pull that caused a fast-forward (no separate merge commit) and it still ran the hook. I looked for a `post-fetch` hook but couldn't find one, I agree that would be ideal.

OK, thanks for checking.

>> Using this configuration format, it seems there’s no room left for a
>> branch name, or am I overlooking something?

[...]

> Hmm, I didn't realize that git limited the number of components available in config names, but it looks like you're correct - my suggestion of appending `.branch-name` caused git to produce an error that the config was invalid.
>
> But we don't necessarily need git to be aware of the semantic meaning of the branch name since the value is calculated by the guile script. So we could just use a hyphen as a separator and have "introduction-commit-master" and "introduction-commit-feature" as separate values unrelated to each other (aside from being in the "guix authentication" namespace).

Hmm right.  Not pretty, but why not.

We could still have ‘introduction-commit’ as a default, and
‘introduction-commit-BRANCH’ would take precedence over it when present.

Thanks for your feedback!

Ludo’.