[OE-core] [PATCH 3/3] oe.gpg_sign: support obs-signd

[Markus Lehtonen]

Hi,

On Tue, 2016-01-12 at 18:24 +0200, Markus Lehtonen wrote:
> Hi Mark,
> 
> Thank you for your review! Comments below.
> 
> On Mon, 2016-01-11 at 10:33 -0600, Mark Hatle wrote:
> > On 1/11/16 10:13 AM, Markus Lehtonen wrote:
> > > Implement support for remote signing using obs-signd. It is now possible
> > > to sign both RPM packages and package feeds with this method. The user
> > > just needs to set RPM_GPG_BACKEND and/or PACKAGE_FEED_GPG_BACKEND
> > > variables to 'obssign' in the bitbake config. Of course, in addition,
> > > one needs to setup the signing server and the configure the 'sign'
> > > client command on the build host. The *_PASSPHRASE_FILE settings are not
> > > used when the obssign backend is enabled.
> > > 
> > > [YOCTO #8755]
> > > 
> > > Signed-off-by: Markus Lehtonen <markus.lehtonen at linux.intel.com>
> > > ---
> > >  meta/classes/sign_package_feed.bbclass |  5 +++-
> > >  meta/classes/sign_rpm.bbclass          |  5 +++-
> > >  meta/lib/oe/gpg_sign.py                | 48 ++++++++++++++++++++++++++++++++++
> > >  3 files changed, 56 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/meta/classes/sign_package_feed.bbclass b/meta/classes/sign_package_feed.bbclass
> > > index d5df8af..953fa85 100644
> > > --- a/meta/classes/sign_package_feed.bbclass
> > > +++ b/meta/classes/sign_package_feed.bbclass
> > > @@ -24,7 +24,10 @@ PACKAGE_FEED_GPG_BACKEND ?= 'local'
> > >  
> > >  python () {
> > >      # Check sanity of configuration
> > > -    for var in ('PACKAGE_FEED_GPG_NAME', 'PACKAGE_FEED_GPG_PASSPHRASE_FILE'):
> > > +    required = ['PACKAGE_FEED_GPG_NAME']
> > > +    if d.getVar('PACKAGE_FEED_GPG_BACKEND', True) != 'obssign':
> > > +        required.append('PACKAGE_FEED_GPG_PASSPHRASE_FILE')
> > > +    for var in required:
> > >          if not d.getVar(var, True):
> > >              raise_sanity_error("You need to define %s in the config" % var, d)
> > >  
> > > diff --git a/meta/classes/sign_rpm.bbclass b/meta/classes/sign_rpm.bbclass
> > > index 8bcabee..8be1c35 100644
> > > --- a/meta/classes/sign_rpm.bbclass
> > > +++ b/meta/classes/sign_rpm.bbclass
> > > @@ -23,7 +23,10 @@ RPM_GPG_BACKEND ?= 'local'
> > >  
> > >  python () {
> > >      # Check configuration
> > > -    for var in ('RPM_GPG_NAME', 'RPM_GPG_PASSPHRASE_FILE'):
> > > +    required = ['RPM_GPG_NAME']
> > > +    if d.getVar('RPM_GPG_BACKEND', True) != 'obssign':
> > > +        required.append('RPM_GPG_PASSPHRASE_FILE')
> > > +    for var in required:
> > >          if not d.getVar(var, True):
> > >              raise_sanity_error("You need to define %s in the config" % var, d)
> > >  
> > > diff --git a/meta/lib/oe/gpg_sign.py b/meta/lib/oe/gpg_sign.py
> > > index 55abad8..d8ab816 100644
> > > --- a/meta/lib/oe/gpg_sign.py
> > > +++ b/meta/lib/oe/gpg_sign.py
> > > @@ -66,11 +66,59 @@ class LocalSigner(object):
> > >                                        (input_file, output))
> > >  
> > >  
> > > +class ObsSigner(object):
> > > +    """Class for handling signing with obs-signd"""
> > > +    def __init__(self, keyid):
> > > +        self.keyid = keyid
> > > +        self.rpm_bin = bb.utils.which(os.getenv('PATH'), "rpm")
> > > +
> > > +    def export_pubkey(self, output_file):
> > > +        """Export GPG public key to a file"""
> > > +        cmd = "sign -u '%s' -p" % self.keyid
> > > +        status, output = oe.utils.getstatusoutput(cmd)
> > > +        if status:
> > > +            raise bb.build.FuncFailed('Failed to export gpg public key (%s): %s' %
> > > +                                      (self.keyid, output))
> > > +        with open(output_file, 'w') as fobj:
> > > +            fobj.write(output)
> > > +            fobj.write('\n')
> > > +
> > > +    def sign_rpms(self, files):
> > > +        """Sign RPM files"""
> > > +        import pexpect
> > > +
> > > +        # Remove existing signatures
> > > +        cmd = "%s --delsign %s" % (self.rpm_bin, ' '.join(files))
> > 
> > Why are you removing existing signatures?  I believe for many cases this is
> > actually incorrect.
> > 
> > RPM (5) has the ability to have an endless number of signatures within a given
> > package.  The package SHOULD included the internal non-repudiable signature...
> > 
> > (to refresh memory) all RPM 5 packages include an internal non-repudiable
> > signature.  Think of this as an extended md5sum, sha256sum, etc.  It doesn't
> > change that a package is 'authentic' in any way (often the purpose of signatures
> > like what this code is doing), but instead keeps a high reliability way to sign
> > and verify the package is signed properly.
> > 
> > This is used for validation if the system doing the install does not have the
> > public key that the package was signed with.
> > 
> > ... as well as one or more repudiable signatures that can be used to verify that
> > it's "authentic" in some way.  A system could very easily have OSV, OEM, and ISV
> > keys install on them.  You can program RPM in such a way that it will refused to
> > install packages with unknown authentication keys or the non-repudiable key as well.
> > 
> > So, I believe running delsign is wrong.  If the obs-signd can't handle ADDING
> > signatures to packages, then I'd say it is broken and should be fixed in some
> > way -- or at least the signature deletion code should be optional.
> 
> Yes, unfortunately this is currently the limitation of obs-signd. It
> refuses to sign if there are signatures present in the rpm package.
> Using --delsign is "unfortunate" consequence of this and that should've
> probably been described in a comment. Making signature deletion a
> configurable setting is hopefully a decent resolution for now. I will
> send a new version of the patchset later.

Backing up a bit here. I did some quick testing and it seems that RPM5
does not support multiple signatures (anymore?). Doing --addsign seems
to overwrite the existing signatures similarly to --resign. Support for
multiple signatures were removed from RPM4 years ago.

In this light, doing --delsign should be ok. What do you think?


Thanks,
  Markus