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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
