launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> 
> === modified file 'lib/lp/archivepublisher/tests/test_uefi.py'
> --- lib/lp/archivepublisher/tests/test_uefi.py	2016-02-05 16:51:12 +0000
> +++ lib/lp/archivepublisher/tests/test_uefi.py	2016-04-27 10:58:39 +0000
> @@ -17,12 +17,27 @@
>  from lp.testing import TestCase
>  from lp.testing.fakemethod import FakeMethod
>  
> +class FakeMethodExecuteCmd(FakeMethod):

I suspect lint will complain that you want two blank lines on either side of this class.

This whole thing is fairly ugly.  I would propose a slightly different approach.  We want to test that the proper command is executed by the sign and genUefiKeys methods, and we also want to be able to run larger parts of this code without actually executing subprocesses.  So I'd split the tests into two parts.  Firstly, you'd have unit tests each of which just calls one of the sign or genUefiKeys methods in some appropriate environment in isolation.  These could use something like this to mock subprocess.call:

  fake_call = FakeMethod()
  with self.useFixture(MonkeyPatch("subprocess.call", fake_call)):
      # code under test
  # assert things about fake_call

Secondly, you'd have tests for the things that you expect to call sign and/or genUefiKeys.  For these, you can just directly replace sign/genUefiKeys with FakeMethods that have the side-effects you need; so presumably just an unadorned FakeMethod in the case of sign, and a subclass that touches upload.key and upload.cert in the case of genUefiKeys.  You can then get rid of UefiUpload.execute_cmd.

> +    """A fake command executer."""
> +    def __call__(self, *args, **kwargs):
> +        super(FakeMethodExecuteCmd, self).__call__(*args, **kwargs)
> +
> +        cmdl = args[0]
> +
> +        # keygen command for UEFI keys:
> +        if (cmdl[0] == 'openssl' and
> +            cmdl[8] == '-keyout' and cmdl[9].startswith('/tmp/') and
> +            cmdl[10] == '-out' and cmdl[11].startswith('/tmp/')):
> +            
> +            write_file(cmdl[9], "")
> +            write_file(cmdl[11], "")
>  
>  class FakeConfig:
>      """A fake publisher configuration."""
>      def __init__(self, archiveroot, uefiroot):
>          self.archiveroot = archiveroot
>          self.uefiroot = uefiroot
> +        self.uefiautokey = False
>  
>  
>  class TestUefi(TestCase):
> @@ -37,11 +52,44 @@
>          old_umask = os.umask(0o022)
>          self.addCleanup(os.umask, old_umask)
>  
> -    def setUpKeyAndCert(self):
> +    def setUpAutoKey(self):
> +        self.pubconf.uefiautokey = True
> +
> +    def setUpKeyAndCert(self, create=True):
>          self.key = os.path.join(self.uefi_dir, "uefi.key")
>          self.cert = os.path.join(self.uefi_dir, "uefi.crt")
> -        write_file(self.key, "")
> -        write_file(self.cert, "")
> +        if create:
> +            write_file(self.key, "")
> +            write_file(self.cert, "")
> +
> +    def validateKeyAndCert(self):
> +        if os.path.exists(self.key) and os.path.exists(self.cert):
> +            return True
> +        else:
> +            return False

Normally just "return boolean_expression" rather than "if boolean_expression: return True; else: return False".

But all these validate* methods should instead be assert*, and should directly assert the thing they're testing rather than returning a boolean for the caller to assert.  So, for example:

  def assertKeyAndCertExist(self):
      self.assertTrue(os.path.exists(self.key))
      self.assertTrue(os.path.exists(self.cert))

(Note also the split into multiple assertions, which produces clearer failure messages if only one part goes wrong.)

> +
> +    def validateCmdUefiKeygen(self, call):
> +        args = call[0][0]
> +
> +        archive_name = os.path.basename(self.pubconf.archiveroot)
> +        owner_name   = os.path.basename(os.path.dirname(self.pubconf.archiveroot))
> +        common_name  = '/CN=PPA ' + owner_name + ' ' + archive_name + '/'
> +        
> +        cmd_gen  = ['openssl', 'req', '-new', '-x509', '-newkey', 'rsa:2048',
> +                    '-subj', common_name,
> +                    '-keyout', self.key, '-out', self.cert,
> +                    '-days', '3650', '-nodes', '-sha256']

LP style would normally be more like:

  expected_args = [
      'openssl', 'req', '-new', ...,
      '-days', '3650', '-nodes', '-sha256',
      ]

> +        return args == cmd_gen
> +
> +    def validateCmdSbsign(self, call, filename):
> +        args = call[0][0]
> +
> +        if len(args) >= 6 and args[5].startswith('/'):
> +            args[5] = os.path.basename(args[5])

The startswith test is probably unnecessary here.  But this all simplifies if you refactor the tests as I proposed earlier; in that case you'd be testing the behaviour of the sign method given a particular argument in one test (in which case no directory mangling is expected to take place), and testing that the sign method is called with a particular argument in another test (in which case you do need the directory mangling, but it should be more straightforwardly constructed).

> +
> +        cmd_sign = ['sbsign', '--key', self.key, '--cert', self.cert, filename ]
> +
> +        return args == cmd_sign
>  
>      def openArchive(self, loader_type, version, arch):
>          self.path = os.path.join(
> 
> === modified file 'lib/lp/archivepublisher/uefi.py'
> --- lib/lp/archivepublisher/uefi.py	2013-07-25 16:45:20 +0000
> +++ lib/lp/archivepublisher/uefi.py	2016-04-27 10:58:39 +0000
> @@ -102,13 +95,71 @@
>                  if filename.endswith(".efi"):
>                      yield os.path.join(dirpath, filename)
>  
> +    def genUefiKeys(self):
> +        old_mask = os.umask(0o022)
> +        try:
> +            directory = os.path.dirname(self.key)
> +            if not os.path.exists(directory):
> +                os.makedirs(directory)
> +
> +            # XXX: pull out the PPA owner and name to seed key CN
> +            archive_name = os.path.basename(self.archiveroot)
> +            owner_name   = os.path.basename(os.path.dirname(directory))
> +            common_name  = '/CN=PPA ' + owner_name + ' ' + archive_name + '/'
> +
> +            os.umask(0o077)

I don't totally understand the umask manipulations here.  You're setting umask 022 to create the containing directory, then umask 077 while executing openssl, then restoring the umask at the end.  I guess this is a direct translation of the current wiki instructions for webops.  Those have to deal with whatever random environment might be in the operator's shell, hence the initial call, but that seems unnecessary in Launchpad and ArchiveSigningKey.exportSecretKey doesn't specify a particular umask for creating the directory.  Keeping the umask around the key generation makes sense, of course.

> +            cmdl = [ 'openssl', 'req', '-new', '-x509', '-newkey', 'rsa:2048',
> +                     '-subj', common_name, '-keyout', self.key,
> +                     '-out', self.cert, '-days', '3650', '-nodes', '-sha256' ]
> +            self.execute_cmd(cmdl)
> +
> +            if os.path.exists(self.cert):
> +                os.chmod(self.cert, 0o644)
> +
> +        finally:
> +            os.umask(old_mask)
> +
> +    def getUefiKeys(self):
> +        """Validate and return the uefi key and cert for encryption."""
> +
> +        if self.key and self.cert:
> +            # If neither of the key files exists then attempt to
> +            # generate them.
> +            if (self.autokey and not os.path.exists(self.key)
> +                and not os.path.exists(self.cert)):
> +                self.genUefiKeys()
> +                
> +            # If we have keys, but cannot read them they are dead to us.
> +            if not os.access(self.key, os.R_OK):
> +                if self.logger is not None:
> +                    self.logger.warning(
> +                        "UEFI private key %s not readable" % self.key)
> +                self.key = None
> +            if not os.access(self.cert, os.R_OK):
> +                if self.logger is not None:
> +                    self.logger.warning(
> +                        "UEFI certificate %s not readable" % self.cert)
> +                self.cert = None
> +
> +        return (self.key, self.cert)
> +
>      def getSigningCommand(self, image):
>          """Return the command used to sign an image."""
> -        return ["sbsign", "--key", self.key, "--cert", self.cert, image]
> +
> +        (key, cert) = self.getUefiKeys()
> +        if not key or not cert:
> +            return None
> +
> +        return ["sbsign", "--key", key, "--cert", cert, image]
> +
> +    def execute_cmd(self, cmdl):
> +        """Execute a signing command."""
> +        return subprocess.call(cmdl)
>  
>      def sign(self, image):
>          """Sign an image."""
> -        if subprocess.call(self.getSigningCommand(image)) != 0:
> +        cmdl = self.getSigningCommand(image)
> +        if cmdl == None or self.execute_cmd(cmdl) != 0:
>              # Just log this rather than failing, since custom upload errors
>              # tend to make the publisher rather upset.
>              if self.logger is not None:


-- 
https://code.launchpad.net/~apw/launchpad/uefi-auto-key/+merge/293087
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.