diff mbox series

[bug#42048,6/6] services: provenance: Save channel introductions.

Message ID 20200625211605.29316-6-ludo@gnu.org
State Accepted
Headers show
Series [bug#42048,1/6] channels: Add 'openpgp-fingerprint->bytevector'. | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job

Commit Message

Ludovic Courtès June 25, 2020, 9:16 p.m. UTC
* gnu/services.scm (channel->code): Include CHANNEL's introduction, if
any, unless CHANNEL is the singleton %DEFAULT-CHANNELS.
(channel->sexp): Add comment.
* guix/scripts/system.scm (sexp->channel): Change pattern to allow for
extensibility.
---
 gnu/services.scm        | 26 ++++++++++++++++++++++----
 guix/scripts/system.scm |  4 +++-
 2 files changed, 25 insertions(+), 5 deletions(-)

Comments

Ricardo Wurmus June 30, 2020, 3:53 p.m. UTC | #1
I looked through the changes and while I don’t fully understand the need
for adding the introduction to the provenance data, it looks good to
me.  Thank you!

One thing that I worry about is authentication of channels that are
added as dependencies of user-selected channels.  Let’s say my channel
“guix-bimsb” depends on “guix-past”.  How will users of “guix-bimsb”
authenticate the commits of “guix-past” when they don’t know about
“guix-past” (they only care about “guix-bimsb”), and don’t explicitly
add introduction information to their channels file?

Is there something that the authors of “guix-bimsb” can do to not only
indicate the dependency on “guix-past”, but also to attach introduction
information?  Will the format of the “.guix-channel” need to be
adjusted?
Ludovic Courtès June 30, 2020, 8:28 p.m. UTC | #2
Hi Ricardo,

Ricardo Wurmus <rekado@elephly.net> skribis:

> I looked through the changes and while I don’t fully understand the need
> for adding the introduction to the provenance data, it looks good to
> me.  Thank you!

Overall the idea is that a channel spec should always come with its
introduction; together they identify the channel and thus should not be
separated.

Adding the introduction to the provenance data allows ‘guix describe’ to
show the introduction, to ensure it’s not lost in transit.

Does that make sense?

> One thing that I worry about is authentication of channels that are
> added as dependencies of user-selected channels.  Let’s say my channel
> “guix-bimsb” depends on “guix-past”.  How will users of “guix-bimsb”
> authenticate the commits of “guix-past” when they don’t know about
> “guix-past” (they only care about “guix-bimsb”), and don’t explicitly
> add introduction information to their channels file?
>
> Is there something that the authors of “guix-bimsb” can do to not only
> indicate the dependency on “guix-past”, but also to attach introduction
> information?  Will the format of the “.guix-channel” need to be
> adjusted?

That’s a very good question and I had completely overlooked it.

With this patch set, someone pulling guix-bimsb would just end up
pulling guix-past unauthenticated; there’s not even a warning.

(There’s currently a warning in (guix channels), but only when pulling
an unauthenticated 'guix channel.  It’s perhaps too early to have that
warning enabled for all channels.  WDYT?)

So yes, I suppose we would need to extend the ‘.guix-channel’ format for
dependencies.  Luckily it should be quite simply because that format is
extensible; older Guix versions would ignore the ‘introduction’ field.
It would look something like this:

     (channel
      (version 0)
      (dependencies
       (channel
        (name some-collection)
        (url "https://example.org/first-collection.git")
        (introduction (channel-introduction
                        (version 0)
                        (commit "…")
                        (signer "…"))))
       (channel
        (name some-other-collection)
        (url "https://example.org/second-collection.git")
        (branch "testing"))))   ;not an authenticated channel

It does mean that a channel can indirectly trick you into turning off
authentication for a dependent channel.  But I think that’s within the
expectations for channels: when you choose a channel, you trust it
enough to run its code.

WDYT?

Thanks for reviewing!

Ludo’.
Simon Tournier July 1, 2020, 8:51 a.m. UTC | #3
Hi,

On Tue, 30 Jun 2020 at 22:28, Ludovic Courtès <ludo@gnu.org> wrote:

>> One thing that I worry about is authentication of channels that are
>> added as dependencies of user-selected channels.  Let’s say my channel
>> “guix-bimsb” depends on “guix-past”.  How will users of “guix-bimsb”
>> authenticate the commits of “guix-past” when they don’t know about
>> “guix-past” (they only care about “guix-bimsb”), and don’t explicitly
>> add introduction information to their channels file?
>>
>> Is there something that the authors of “guix-bimsb” can do to not only
>> indicate the dependency on “guix-past”, but also to attach introduction
>> information?  Will the format of the “.guix-channel” need to be
>> adjusted?
>
> That’s a very good question and I had completely overlooked it.

Héhé, yet I had the same question one month ago. :-)

--8<---------------cut here---------------start------------->8---
> The question about recursive still applies. ;-)
> Currently, if the local channel file points to a channel A which
> contains the file '.guix-channel' which points to another channel B,
> then when one runs "guix pull" well the channel A will be pulled and
> then the channel B, even if this channel B is not explicit in the
> initial local channel.  (Even, there is bug about recursive implicit
> pulls, see http://issues.guix.gnu.org/issue/41069; well another
> story.)
>What happens for such situation?

Nothing special, I guess: each channel would be authenticated (or not,if
it’s an unsigned channel).  I think it’s completely orthogonal.
--8<---------------cut here---------------end--------------->8---

http://issues.guix.gnu.org/issue/22883#75


> With this patch set, someone pulling guix-bimsb would just end up
> pulling guix-past unauthenticated; there’s not even a warning.
>
> (There’s currently a warning in (guix channels), but only when pulling
> an unauthenticated 'guix channel.  It’s perhaps too early to have that
> warning enabled for all channels.  WDYT?)

Enable the warning appears to me a good idea because this dependency is
like "doing something I am not necessary aware in my back".

For example, the first time I pulled the channel "guix-bimsb-non-free" which
depends on "guix-bimsb", it took me some time to understand why
"guix-bimsb" was pulled twice and once with a name I do not have in my
local channels.scm file.  Anyway.

> So yes, I suppose we would need to extend the ‘.guix-channel’ format for
> dependencies.  Luckily it should be quite simply because that format is
> extensible; older Guix versions would ignore the ‘introduction’ field.
> It would look something like this:
>
>      (channel
>       (version 0)
>       (dependencies
>        (channel
>         (name some-collection)
>         (url "https://example.org/first-collection.git")
>         (introduction (channel-introduction
>                         (version 0)
>                         (commit "…")
>                         (signer "…"))))
>        (channel
>         (name some-other-collection)
>         (url "https://example.org/second-collection.git")
>         (branch "testing"))))   ;not an authenticated channel
>
> It does mean that a channel can indirectly trick you into turning off
> authentication for a dependent channel.  But I think that’s within the
> expectations for channels: when you choose a channel, you trust it
> enough to run its code.

Sound good to me.

When I choose a channel, I trust the people enough to run their code.
But I do not trust the URL which serves it.  I mean, it is the point of
all this new authentication mechanism, isn't it?

However, I agree.  Channel should stay easy to fork and add something
(then maybe send a pull-request) without going in all the GPG signature
dance and/or running the options --allow-downgrades or
--disable-authentication (I do not remember the exact name).


Cheers,
simon
Ludovic Courtès July 1, 2020, 12:12 p.m. UTC | #4
Hi Simon,

zimoun <zimon.toutoune@gmail.com> skribis:

> On Tue, 30 Jun 2020 at 22:28, Ludovic Courtès <ludo@gnu.org> wrote:
>
>>> One thing that I worry about is authentication of channels that are
>>> added as dependencies of user-selected channels.  Let’s say my channel
>>> “guix-bimsb” depends on “guix-past”.  How will users of “guix-bimsb”
>>> authenticate the commits of “guix-past” when they don’t know about
>>> “guix-past” (they only care about “guix-bimsb”), and don’t explicitly
>>> add introduction information to their channels file?
>>>
>>> Is there something that the authors of “guix-bimsb” can do to not only
>>> indicate the dependency on “guix-past”, but also to attach introduction
>>> information?  Will the format of the “.guix-channel” need to be
>>> adjusted?
>>
>> That’s a very good question and I had completely overlooked it.
>
> Héhé, yet I had the same question one month ago. :-)

Oh I’m sorry, I think I misunderstood your question back then!

>> With this patch set, someone pulling guix-bimsb would just end up
>> pulling guix-past unauthenticated; there’s not even a warning.
>>
>> (There’s currently a warning in (guix channels), but only when pulling
>> an unauthenticated 'guix channel.  It’s perhaps too early to have that
>> warning enabled for all channels.  WDYT?)
>
> Enable the warning appears to me a good idea because this dependency is
> like "doing something I am not necessary aware in my back".

I’m talking about the warning that says “this channel is
unauthenticated”, which is mostly orthogonal to the discussion at hand.
The reason I said it’s perhaps too early to enable it is that people
haven’t had a chance to make their channel “authenticable” yet.

>> So yes, I suppose we would need to extend the ‘.guix-channel’ format for
>> dependencies.  Luckily it should be quite simply because that format is
>> extensible; older Guix versions would ignore the ‘introduction’ field.
>> It would look something like this:
>>
>>      (channel
>>       (version 0)
>>       (dependencies
>>        (channel
>>         (name some-collection)
>>         (url "https://example.org/first-collection.git")
>>         (introduction (channel-introduction
>>                         (version 0)
>>                         (commit "…")
>>                         (signer "…"))))
>>        (channel
>>         (name some-other-collection)
>>         (url "https://example.org/second-collection.git")
>>         (branch "testing"))))   ;not an authenticated channel
>>
>> It does mean that a channel can indirectly trick you into turning off
>> authentication for a dependent channel.  But I think that’s within the
>> expectations for channels: when you choose a channel, you trust it
>> enough to run its code.
>
> Sound good to me.

Alright, I’ll do that as a followup.

Thanks!

Ludo’.
Ricardo Wurmus July 1, 2020, 12:25 p.m. UTC | #5
Ludovic Courtès <ludo@gnu.org> writes:

> So yes, I suppose we would need to extend the ‘.guix-channel’ format for
> dependencies.  Luckily it should be quite simply because that format is
> extensible; older Guix versions would ignore the ‘introduction’ field.
> It would look something like this:
>
>      (channel
>       (version 0)
>       (dependencies
>        (channel
>         (name some-collection)
>         (url "https://example.org/first-collection.git")
>         (introduction (channel-introduction
>                         (version 0)
>                         (commit "…")
>                         (signer "…"))))
>        (channel
>         (name some-other-collection)
>         (url "https://example.org/second-collection.git")
>         (branch "testing"))))   ;not an authenticated channel
>
> It does mean that a channel can indirectly trick you into turning off
> authentication for a dependent channel.  But I think that’s within the
> expectations for channels: when you choose a channel, you trust it
> enough to run its code.
>
> WDYT?

This sounds reasonable.  I agree that you’ve got to trust the channel
authors anyway, so allowing them to provide the introduction is fair.
Simon Tournier July 1, 2020, 12:49 p.m. UTC | #6
On Wed, 01 Jul 2020 at 14:12, Ludovic Courtès <ludo@gnu.org> wrote:

> Oh I’m sorry, I think I misunderstood your question back then!

My poor English does not help either. :-)

>>> With this patch set, someone pulling guix-bimsb would just end up
>>> pulling guix-past unauthenticated; there’s not even a warning.
>>>
>>> (There’s currently a warning in (guix channels), but only when pulling
>>> an unauthenticated 'guix channel.  It’s perhaps too early to have that
>>> warning enabled for all channels.  WDYT?)
>>
>> Enable the warning appears to me a good idea because this dependency is
>> like "doing something I am not necessary aware in my back".
>
> I’m talking about the warning that says “this channel is
> unauthenticated”, which is mostly orthogonal to the discussion at hand.
> The reason I said it’s perhaps too early to enable it is that people
> haven’t had a chance to make their channel “authenticable” yet.

Well, the possible scenarii are: when pulling guix-bimsb which ends up
to pull guix-past:

 1- unauthenticated guix-bimsb and unauthenticated guix-past
 2- authenticated guix-bimsb and unauthenticated guix-past
 3- unauthenticated guix-bimsb and authenticated guix-past
 4- authenticated guix-bimsb and authenticated guix-past

The #1 and #4 do not deserve a warning.
The point #3 neither and even the authentication of guix-past should be
turned off, at least now.

The point #2 requires a warning.  Because if I am pulling a
authenticated channel, I expect that all the code it pulls is
authenticated which will not be the case, so IMHO it deserves a
warning.

Then it is up to the guix-bimsb channel to add an introduction for the
dependency using the format you described.

Cheers,
simon
Ludovic Courtès July 1, 2020, 5:05 p.m. UTC | #7
zimoun <zimon.toutoune@gmail.com> skribis:

> On Wed, 01 Jul 2020 at 14:12, Ludovic Courtès <ludo@gnu.org> wrote:

[...]

>> I’m talking about the warning that says “this channel is
>> unauthenticated”, which is mostly orthogonal to the discussion at hand.
>> The reason I said it’s perhaps too early to enable it is that people
>> haven’t had a chance to make their channel “authenticable” yet.

To be clear, I’m referring to this:

  https://git.savannah.gnu.org/cgit/guix.git/tree/guix/channels.scm?id=9f5f3932debc72a57a830fc6ca5ab980f6db4941#n406

> Well, the possible scenarii are: when pulling guix-bimsb which ends up
> to pull guix-past:
>
>  1- unauthenticated guix-bimsb and unauthenticated guix-past
>  2- authenticated guix-bimsb and unauthenticated guix-past
>  3- unauthenticated guix-bimsb and authenticated guix-past
>  4- authenticated guix-bimsb and authenticated guix-past
>
> The #1 and #4 do not deserve a warning.
> The point #3 neither and even the authentication of guix-past should be
> turned off, at least now.
>
> The point #2 requires a warning.  Because if I am pulling a
> authenticated channel, I expect that all the code it pulls is
> authenticated which will not be the case, so IMHO it deserves a
> warning.
>
> Then it is up to the guix-bimsb channel to add an introduction for the
> dependency using the format you described.

Exactly.  I agree that #2 is problematic, but if the authors of
guix-bimsb are aware that guix-past can also be authenticated, then I
think it’s their responsibility to update their ‘.guix-channel’
dependencies accordingly.

Ludo’.
Ludovic Courtès July 1, 2020, 9:50 p.m. UTC | #8
Hi,

Ricardo Wurmus <rekado@elephly.net> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> So yes, I suppose we would need to extend the ‘.guix-channel’ format for
>> dependencies.  Luckily it should be quite simply because that format is
>> extensible; older Guix versions would ignore the ‘introduction’ field.
>> It would look something like this:
>>
>>      (channel
>>       (version 0)
>>       (dependencies
>>        (channel
>>         (name some-collection)
>>         (url "https://example.org/first-collection.git")
>>         (introduction (channel-introduction
>>                         (version 0)
>>                         (commit "…")
>>                         (signer "…"))))
>>        (channel
>>         (name some-other-collection)
>>         (url "https://example.org/second-collection.git")
>>         (branch "testing"))))   ;not an authenticated channel
>>
>> It does mean that a channel can indirectly trick you into turning off
>> authentication for a dependent channel.  But I think that’s within the
>> expectations for channels: when you choose a channel, you trust it
>> enough to run its code.
>>
>> WDYT?
>
> This sounds reasonable.  I agree that you’ve got to trust the channel
> authors anyway, so allowing them to provide the introduction is fair.

I went ahead and did that:

  https://git.savannah.gnu.org/cgit/guix.git/commit/?id=d774c7b1218a3cc20079b19812da119f9ed26b54

Let me know what you think!

The whole series is now pushed:

  d774c7b121 channels: Dependencies listed in '.guix-channel' can have an introduction.
  884df77640 channels: Properly diagnose test failure.
  eb5cf39e66 services: provenance: Save channel introductions.
  6d39f0cb77 guix describe: Display channel introductions and add 'channels-sans-intro'.
  471550c28c channels: Save and interpret 'introduction' field in provenance data.
  22a9699257 channels: Remove 'signature' from <channel-introduction>.
  8b7d982e6a channels: Make channel introductions public.
  6577682a6c channels: Add 'openpgp-fingerprint->bytevector'.

Thanks for your feedback,
Ludo’.
diff mbox series

Patch

diff --git a/gnu/services.scm b/gnu/services.scm
index 27e5558231..f6dc56d940 100644
--- a/gnu/services.scm
+++ b/gnu/services.scm
@@ -31,6 +31,7 @@ 
   #:use-module (guix sets)
   #:use-module (guix ui)
   #:use-module ((guix utils) #:select (source-properties->location))
+  #:autoload   (guix openpgp) (openpgp-format-fingerprint)
   #:use-module (guix modules)
   #:use-module (gnu packages base)
   #:use-module (gnu packages bash)
@@ -392,14 +393,31 @@  by the initrd once the root file system is mounted.")))
 (define (channel->code channel)
   "Return code to build CHANNEL, ready to be dropped in a 'channels.scm'
 file."
-  `(channel (name ',(channel-name channel))
-            (url ,(channel-url channel))
-            (branch ,(channel-branch channel))
-            (commit ,(channel-commit channel))))
+  ;; Since the 'introduction' field is backward-incompatible, and since it's
+  ;; optional when using the "official" 'guix channel, include it if and only
+  ;; if we're referring to a different channel.
+  (let ((intro (and (not (equal? (list channel) %default-channels))
+                    (channel-introduction channel))))
+    `(channel (name ',(channel-name channel))
+              (url ,(channel-url channel))
+              (branch ,(channel-branch channel))
+              (commit ,(channel-commit channel))
+              ,@(if intro
+                    `((introduction
+                       (make-channel-introduction
+                        ,(channel-introduction-first-signed-commit intro)
+                        (openpgp-fingerprint
+                         ,(openpgp-format-fingerprint
+                           (channel-introduction-first-commit-signer
+                            intro))))))
+                    '()))))
 
 (define (channel->sexp channel)
   "Return an sexp describing CHANNEL.  The sexp is _not_ code and is meant to
 be parsed by tools; it's potentially more future-proof than code."
+  ;; TODO: Add CHANNEL's introduction.  Currently we can't do that because
+  ;; older 'guix system describe' expect exactly name/url/branch/commit
+  ;; without any additional fields.
   `(channel (name ,(channel-name channel))
             (url ,(channel-url channel))
             (branch ,(channel-branch channel))
diff --git a/guix/scripts/system.scm b/guix/scripts/system.scm
index 212b49f008..cfefe8a8a8 100644
--- a/guix/scripts/system.scm
+++ b/guix/scripts/system.scm
@@ -452,7 +452,9 @@  list of services."
     (('channel ('name name)
                ('url url)
                ('branch branch)
-               ('commit commit))
+               ('commit commit)
+               rest ...)
+     ;; XXX: In the future REST may include a channel introduction.
      (channel (name name) (url url)
               (branch branch) (commit commit)))))