• Bug#1059150: No longer works with signing subkeys

    From Guillem Jover@1:229/2 to Steve McIntyre on Thu Dec 21 00:10:02 2023
    XPost: linux.debian.bugs.dist
    From: [email protected]

    Hi!

    On Wed, 2023-12-20 at 15:30:24 +0000, Steve McIntyre wrote:
    Package: debsig-verify
    Version: 0.23+b2
    Severity: important
    Tags: patch

    Updating our derived distro from bullseye to bookworm, we've moved on
    from 0.23 to 0.28. We're using subkeys for signing our debs, and that
    no longer works. I can see that the change you've made to no longer
    fall back if a fingerprint doesn't match (849d9633ebf809398c848821c603148ae0470278) has broken this.

    Ouch, I've been increasingly unhappy with the whole policy thing,
    because it was not functioning as documented, fixing it to do so has
    broken multiple use cases, it seems like unnecessary complexity and in
    a way trying to reimplement some of the checks that should be done by
    the OpenPGP implementation, and it is getting in the way of adding
    other OpenPGP backends due to the insistence of tying the signature
    issuer fingerprint with the policy to apply, which means the primary certificate fingerprint cannot be used as would perhaps be usually
    expected.

    I recorded part of this in the TODO, and I had in mind asking you
    about how you use this as part of the redesign work, but I'll leave
    that for a later point. :)

    Here's a patch that I've added locally on top of 0.28 to also attempt
    to match subkey fingerprints. This passes tests here and makes subkeys
    work for us again.

    Thanks for tracking this and providing a patch!

    diff --git a/src/openpgp-gpg.c b/src/openpgp-gpg.c
    index 4c29b7f..97ec3a4 100644
    --- a/src/openpgp-gpg.c
    +++ b/src/openpgp-gpg.c
    @@ -241,6 +242,7 @@ gpg_getKeyID(const char *keyring, const char *match_id)
    continue;
    if (strcmp(uid, match_id) != 0) {
    free(uid);
    + state = KEYID_SUB;
    continue;
    }
    free(uid);

    I think the problem with this is that if the first uid does not match,
    then it will then switch to looking for a new fingerprint line, which
    might then omit some valid uids.

    I've prepared a change based on this at:

    https://git.hadrons.org/cgit/debian/dpkg/debsig-verify.git/log/?h=pu/openpgp-subkey

    With the assumption that one would define the policy and keyrings
    paths based on the subkey fingerprint and not the primary public
    certificate fingerprint, because otherwise some of the other matches
    cannot easily match, such as uid-based ones. But wanted to check with
    you whether that's the case before merging. Otherwise I can try to see
    how to support all the various cases.

    (For example I think the current code might break if the public
    certificate can sign, but it has a signing subkey too, and the
    signature issuer is the public certificate. :/)

    Thanks,
    Guillem

    --- SoupGate-Win32 v1.05
    * Origin: you cannot sedate... all the things you hate (1:229/2)
  • From Steve McIntyre@1:229/2 to All on Wed Dec 20 16:40:01 2023
    XPost: linux.debian.bugs.dist
    From: [email protected]

    This is a multi-part MIME message sent by reportbug.


    Package: debsig-verify
    Version: 0.23+b2
    Severity: important
    Tags: patch

    Hey Guillem,

    Updating our derived distro from bullseye to bookworm, we've moved on
    from 0.23 to 0.28. We're using subkeys for signing our debs, and that
    no longer works. I can see that the change you've made to no longer
    fall back if a fingerprint doesn't match (849d9633ebf809398c848821c603148ae0470278) has broken this.

    Here's a patch that I've added locally on top of 0.28 to also attempt
    to match subkey fingerprints. This passes tests here and makes subkeys
    work for us again.

    Cheers,

    Steve


    -- System Information:
    Debian Release: 11.8
    APT prefers oldstable-updates
    APT policy: (500, 'oldstable-updates'), (500, 'oldstable-security'), (500, 'oldoldstable'), (500, 'oldstable')
    Architecture: amd64 (x86_64)
    Foreign Architectures: i386

    Kernel: Linux 5.10.0-26-amd64 (SMP w/8 CPU threads)
    Kernel taint flags: TAINT_OOT_MODULE, TAINT_UNSIGNED_MODULE
    Locale: LANG=en_GB.UTF-8, LC_CTYPE=en_GB.UTF-8 (charmap=UTF-8), LANGUAGE=en_GB:en
    Shell: /bin/sh linked to /usr/bin/dash
    Init: systemd (via /run/systemd/system)
    LSM: AppArmor: enabled

    Versions of packages debsig-verify depends on:
    ii gnupg 2.2.27-2+deb11u2
    ii gpg 2.2.27-2+deb11u2
    ii libc6 2.31-13+deb11u7
    ii libexpat1 2.2.10-2+deb11u5

    debsig-verify recommends no packages.

    Versions of packages debsig-verify suggests:
    ii debian-keyring 2021.09.25
    ii debsigs 0.1.25

    -- no debconf information

    diff --git a/src/openpgp-gpg.c b/src/openpgp-gpg.c
    index 4c29b7f..97ec3a4 100644
    --- a/src/openpgp-gpg.c
    +++ b/src/openpgp-gpg.c
    @@ -115,6 +115,7 @@ enum keyid_state {
    KEYID_FPR,
    KEYID_UID,
    KEYID_SIG,
    + KEYID_SUB,
    };

    enum colon_fields {
    @@ -221,7 +222,7 @@ gpg_getKeyID(const char *keyring, const char *match_id)

    /* Certificate found. */
    state = KEYID_PUB;
    - } else if (state == KEYID_PUB) {
    + } else if (state == KEYID_PUB || state == KEYID_SUB) {
    if (!match_prefix(buf, "fpr:"))
    continue;
    fpr = get_colon_field(buf, COLON_FIELD_FPR_ID);
    @@ -241,6 +242,7 @@ gpg_getKeyID(const char *keyring, const char *match_id)
    continue;
    if (strcmp(uid, match_id) != 0) {
    free(uid);
    + state = KEYID_SUB;
    continue;
    }
    free(uid);

    --- SoupGate-Win32 v1.05
    * Origin: you cannot sedate... all the things you hate (1:229/2)
  • From Guillem Jover@1:229/2 to Guillem Jover on Thu Mar 7 04:30:01 2024
    XPost: linux.debian.bugs.dist
    From: [email protected]

    Hi!

    On Wed, 2023-12-20 at 23:59:31 +0100, Guillem Jover wrote:
    On Wed, 2023-12-20 at 15:30:24 +0000, Steve McIntyre wrote:
    diff --git a/src/openpgp-gpg.c b/src/openpgp-gpg.c
    index 4c29b7f..97ec3a4 100644
    --- a/src/openpgp-gpg.c
    +++ b/src/openpgp-gpg.c
    @@ -241,6 +242,7 @@ gpg_getKeyID(const char *keyring, const char *match_id)
    continue;
    if (strcmp(uid, match_id) != 0) {
    free(uid);
    + state = KEYID_SUB;
    continue;
    }
    free(uid);

    I think the problem with this is that if the first uid does not match,
    then it will then switch to looking for a new fingerprint line, which
    might then omit some valid uids.

    I've prepared a change based on this at:

    https://git.hadrons.org/cgit/debian/dpkg/debsig-verify.git/log/?h=pu/openpgp-subkey

    With the assumption that one would define the policy and keyrings
    paths based on the subkey fingerprint and not the primary public
    certificate fingerprint, because otherwise some of the other matches
    cannot easily match, such as uid-based ones. But wanted to check with
    you whether that's the case before merging. Otherwise I can try to see
    how to support all the various cases.

    I assume you have had no time to look into this, but I'd like to make
    sure the above branch fixes your issue before merging, and potentially preparing a backport for stable. :)

    Thanks,
    Guillem

    --- SoupGate-Win32 v1.05
    * Origin: you cannot sedate... all the things you hate (1:229/2)
  • From Steve McIntyre@1:229/2 to Guillem Jover on Wed Mar 20 18:30:01 2024
    XPost: linux.debian.bugs.dist
    From: [email protected]

    Hi Guillem,

    Sorry, I've been swamped with other stuff then ill for the last week
    or so. Looking now...

    On Thu, Mar 07, 2024 at 04:22:08AM +0100, Guillem Jover wrote:
    Hi!

    On Wed, 2023-12-20 at 23:59:31 +0100, Guillem Jover wrote:
    On Wed, 2023-12-20 at 15:30:24 +0000, Steve McIntyre wrote:
    diff --git a/src/openpgp-gpg.c b/src/openpgp-gpg.c
    index 4c29b7f..97ec3a4 100644
    --- a/src/openpgp-gpg.c
    +++ b/src/openpgp-gpg.c
    @@ -241,6 +242,7 @@ gpg_getKeyID(const char *keyring, const char *match_id)
    continue;
    if (strcmp(uid, match_id) != 0) {
    free(uid);
    + state = KEYID_SUB;
    continue;
    }
    free(uid);

    I think the problem with this is that if the first uid does not match,
    then it will then switch to looking for a new fingerprint line, which
    might then omit some valid uids.

    I've prepared a change based on this at:

    https://git.hadrons.org/cgit/debian/dpkg/debsig-verify.git/log/?h=pu/openpgp-subkey

    With the assumption that one would define the policy and keyrings
    paths based on the subkey fingerprint and not the primary public
    certificate fingerprint, because otherwise some of the other matches
    cannot easily match, such as uid-based ones. But wanted to check with
    you whether that's the case before merging. Otherwise I can try to see
    how to support all the various cases.

    I assume you have had no time to look into this, but I'd like to make
    sure the above branch fixes your issue before merging, and potentially >preparing a backport for stable. :)

    Thanks,
    Guillem

    --
    Steve McIntyre, Cambridge, UK. [email protected] "Further comment on how I feel about IBM will appear once I've worked out
    whether they're being malicious or incompetent. Capital letters are forecast."
    Matthew Garrett, http://www.livejournal.com/users/mjg59/30675.html

    --- SoupGate-Win32 v1.05
    * Origin: you cannot sedate... all the things you hate (1:229/2)
  • From Steve McIntyre@1:229/2 to Steve McIntyre on Wed Mar 20 19:10:01 2024
    XPost: linux.debian.bugs.dist
    From: [email protected]

    On Wed, Mar 20, 2024 at 05:18:08PM +0000, Steve McIntyre wrote:
    Hi Guillem,

    Sorry, I've been swamped with other stuff then ill for the last week
    or so. Looking now...

    And I can confirm that your changes work here for our system too.

    Thanks!

    --
    Steve McIntyre, Cambridge, UK. [email protected] "Every time you use Tcl, God kills a kitten." -- Malcolm Ray

    --- SoupGate-Win32 v1.05
    * Origin: you cannot sedate... all the things you hate (1:229/2)
  • From Steve McIntyre@1:229/2 to Guillem Jover on Wed Mar 20 20:10:01 2024
    XPost: linux.debian.bugs.dist
    From: [email protected]

    On Wed, Dec 20, 2023 at 11:59:31PM +0100, Guillem Jover wrote:
    Hi!

    On Wed, 2023-12-20 at 15:30:24 +0000, Steve McIntyre wrote:
    Package: debsig-verify
    Version: 0.23+b2
    Severity: important
    Tags: patch

    Updating our derived distro from bullseye to bookworm, we've moved on
    from 0.23 to 0.28. We're using subkeys for signing our debs, and that
    no longer works. I can see that the change you've made to no longer
    fall back if a fingerprint doesn't match
    (849d9633ebf809398c848821c603148ae0470278) has broken this.

    Ouch, I've been increasingly unhappy with the whole policy thing,
    because it was not functioning as documented, fixing it to do so has
    broken multiple use cases, it seems like unnecessary complexity and in
    a way trying to reimplement some of the checks that should be done by
    the OpenPGP implementation, and it is getting in the way of adding
    other OpenPGP backends due to the insistence of tying the signature
    issuer fingerprint with the policy to apply, which means the primary >certificate fingerprint cannot be used as would perhaps be usually
    expected.

    Nod. To make everything work reliably here for all cases, we're now
    making 4 copies of the policy directory for every key we might use,
    using both the long keyid and the full fingerprint for each of the
    master key and the signing subkey. Then we're including a keyring with
    all of the keys in each of those policy directories. It's not
    wonderful... :-/

    I recorded part of this in the TODO, and I had in mind asking you
    about how you use this as part of the redesign work, but I'll leave
    that for a later point. :)

    ACK. :-)

    So, I'm curious...

    Debsig-verify does seem to be really quite over-complicated, at least
    for our use case. Wouldn't it be much simpler to just have a keyring
    per origin, and then (maybe) a system config file to state which
    origin(s) are needed. The policy definition files don't seem to add
    any value here. IMHO.

    It would also be lovely if the design was less restricted by
    GnuPG. (Yes, I know!) A real problem for me is that debsig-verify
    wants to see *every* signature accounted for when verifying a
    package. This is opposite to the behaviour of gpgv, which is more like
    what we were inititally expecting / hoping for. We're signing packages
    with a rolling range of N keys for our releases, similar to Debian's Release.gpg setup, and now we have to include 4*N policy directories
    for debsig-verify, and our keyring files all have to include *all* the
    keys.

    So, I'd be tempted by something easier to follow:

    * config to say which keyring(s) to use, and (maybe) some config to
    say "need minimum N valid sigs"

    * keyring(s) including key(s)

    * when validating signatures, verify each of them individually rather
    than expecting GnuPG to DTRT. I think we both know how well that
    works *grin*. If enough valid sigs are detected, we're good. If
    not, fail.

    Does that sound reasonable? What am I missing?

    --
    Steve McIntyre, Cambridge, UK. [email protected] "I suspect most samba developers are already technically insane... Of
    course, since many of them are Australians, you can't tell." -- Linus Torvalds

    --- SoupGate-Win32 v1.05
    * Origin: you cannot sedate... all the things you hate (1:229/2)
  • From Guillem Jover@1:229/2 to Steve McIntyre on Fri Mar 22 12:30:02 2024
    XPost: linux.debian.bugs.dist
    From: [email protected]

    Hi!

    On Wed, 2024-03-20 at 18:00:30 +0000, Steve McIntyre wrote:
    On Wed, Mar 20, 2024 at 05:18:08PM +0000, Steve McIntyre wrote:
    Sorry, I've been swamped with other stuff then ill for the last week
    or so. Looking now...

    No worries, hope you are doing well now! :)

    And I can confirm that your changes work here for our system too.

    Perfect, thank you! I'll be rechecking the change and merging and
    uploading during the weekend. Then will prepare a stable update.

    Thanks,
    Guillem

    --- SoupGate-Win32 v1.05
    * Origin: you cannot sedate... all the things you hate (1:229/2)
  • From Guillem Jover@1:229/2 to Steve McIntyre on Fri Mar 22 13:20:01 2024
    XPost: linux.debian.bugs.dist
    From: [email protected]

    Hi!

    On Wed, 2024-03-20 at 19:05:59 +0000, Steve McIntyre wrote:
    On Wed, Dec 20, 2023 at 11:59:31PM +0100, Guillem Jover wrote:
    On Wed, 2023-12-20 at 15:30:24 +0000, Steve McIntyre wrote:
    Package: debsig-verify
    Version: 0.23+b2
    Severity: important
    Tags: patch

    Ouch, I've been increasingly unhappy with the whole policy thing,
    because it was not functioning as documented, fixing it to do so has
    broken multiple use cases, it seems like unnecessary complexity and in
    a way trying to reimplement some of the checks that should be done by
    the OpenPGP implementation, and it is getting in the way of adding
    other OpenPGP backends due to the insistence of tying the signature
    issuer fingerprint with the policy to apply, which means the primary >certificate fingerprint cannot be used as would perhaps be usually >expected.

    Nod. To make everything work reliably here for all cases, we're now
    making 4 copies of the policy directory for every key we might use,
    using both the long keyid and the full fingerprint for each of the
    master key and the signing subkey. Then we're including a keyring with
    all of the keys in each of those policy directories. It's not
    wonderful... :-/

    Ugh. :/ I'd expect that if the keys are new-ish (and they have a
    fingerprint packet), and the package providing the policy does not
    need to cater to old debsig-verify packages then just using the
    fingerprint path would be enough though? In any case yeah, this is
    all very nasty.

    I recorded part of this in the TODO, and I had in mind asking you
    about how you use this as part of the redesign work, but I'll leave
    that for a later point. :)

    ACK. :-)

    So, I'm curious...

    Debsig-verify does seem to be really quite over-complicated, at least
    for our use case. Wouldn't it be much simpler to just have a keyring
    per origin, and then (maybe) a system config file to state which
    origin(s) are needed. The policy definition files don't seem to add
    any value here. IMHO.

    I don't know what was the thinking in the original design and
    implementation choices TBH, the only thing I can go by is the
    specification and design documents from that time. AFAIR this was
    designed before (or at a similar time) as the repo signing, so there
    was probably not much experience in this area. And perhaps also to
    be flexible for unexpected ways people could end up using this.

    Also when pondering about how to redesign and simply this, my other
    fear is that I've had zero visibility over how people are using this
    (if at all) except four your invaluable input!

    Currently my thinking is that multiple origins and the role-based
    signing ideas are not bad, and that some kind of stacking would be
    interesting to support, as you might want to sign .debs taken from
    somewhere else, or to record the provenance steps where the packages
    have gone through (but perhaps no one is using that for example, or
    would ever do, dunno :).

    As part of this redesign my main motivation would be to be able to
    integrate this directly into dpkg-deb (so no new external libraries,
    where in the past I even played with switching from XML to JSON, but
    that's still unnecessary complexity and dependencies), and being able
    to use SOP as the primary OpenPGP interface (probably also gpgv in
    addition).

    So from the TODO (slightly edited now):

    * Redesign policies:
    - Do not require XML.
    - Do not require fetching the fingerprint for signatures and keys.
    - Use the origin name as entry point, and role names to refer to keyrings.
    - Use filesystem as policy declaration? For example:
    <policy-dir>/keyrings/<vendor>/origin.pgp
    <policy-dir>/keyrings/<vendor>/role-maint.pgp
    <policy-dir>/keyrings/<vendor>/role-uploader.pgp
    <policy-dir>/keyrings/<vendor>/role-builder.pgp
    - Use a deb822 file for a policy file to denote optional/required/reject?

    I'm not even sure we might need a deb822 file, perhaps if that part is needed/wanted at all it could even be a few text files with just one
    line each containing a fingerprint. Say:

    <policy-dir>/keyrings/<vendor>/policy/optional
    <policy-dir>/keyrings/<vendor>/policy/reject
    <policy-dir>/keyrings/<vendor>/policy/required

    or similar. And then the match would be based on the vendor, not the fingerprint, which then should make key rotation, and the OpenPGP
    verification (even using SOP) easier to implement, to deploy and to
    reason about, I think.

    It would also be lovely if the design was less restricted by
    GnuPG. (Yes, I know!) A real problem for me is that debsig-verify
    wants to see *every* signature accounted for when verifying a
    package. This is opposite to the behaviour of gpgv, which is more like
    what we were inititally expecting / hoping for. We're signing packages
    with a rolling range of N keys for our releases, similar to Debian's Release.gpg setup, and now we have to include 4*N policy directories
    for debsig-verify, and our keyring files all have to include *all* the
    keys.

    Yes, the current policy seems all backwards to me too.

    So, I'd be tempted by something easier to follow:

    * config to say which keyring(s) to use, and (maybe) some config to
    say "need minimum N valid sigs"

    I think the above would cover part of this. I guess for the minimum
    valid sigs we could add a new file (or config option if going for a
    simple config file, or a deb822 policy file or similar). Say:

    <policy-dir>/keyrings/<vendor>/policy/min-signatures

    containing say "3", or whatever.

    * keyring(s) including key(s)

    * when validating signatures, verify each of them individually rather
    than expecting GnuPG to DTRT. I think we both know how well that

    [continued in next message]

    --- SoupGate-Win32 v1.05
    * Origin: you cannot sedate... all the things you hate (1:229/2)