launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve

Looks pretty much OK; just a few nits.

Diff comments:

> === modified file 'lib/lp/archivepublisher/signing.py'
> --- lib/lp/archivepublisher/signing.py	2018-08-03 16:10:41 +0000
> +++ lib/lp/archivepublisher/signing.py	2019-06-03 14:49:08 +0000
> @@ -260,37 +266,35 @@
>          authorityKeyIdentifier=keyid
>          """)
>  
> -    openssl_config_kmod = openssl_config_opal + textwrap.dedent("""
> +    openssl_config_opal = "# OPAL openssl config" + openssl_config_base

Shouldn't this be "# OPAL openssl config\n" + openssl_config_base?  (Also I'd marginally prefer to capitalise OpenSSL in the standard way.)

Ah, so when I reread I find that this isn't technically a problem because openssl_config_base starts with a newline.  But I think it would be clearer for it *not* to start with a newline (put a backslash after the opening '"""') and write the newline explicitly here, as otherwise this is very confusing to read.

Same for openssl_config_kmod and openssl_config_sipl below.

> +
> +    openssl_config_kmod = "# KMOD openssl config" + openssl_config_base + \
> +        textwrap.dedent("""
>          # codeSigning:  specifies that this key is used to sign code.
>          # 1.3.6.1.4.1.2312.16.1.2:  defines this key as used for
>          #   module signing only. See https://lkml.org/lkml/2015/8/26/741.
>          extendedKeyUsage        = codeSigning,1.3.6.1.4.1.2312.16.1.2
>          """)
>  
> -    def generateOpensslConfig(self, key_type, common_name):
> -        if key_type == 'Kmod':
> -            genkey_tmpl = self.openssl_config_kmod
> -        elif key_type == 'Opal':
> -            genkey_tmpl = self.openssl_config_opal
> -        else:
> -            raise ValueError("unknown key_type " + key_type)
> +    openssl_config_sipl = "# SIPL openssl config" + openssl_config_base
> +
> +    def generateOpensslConfig(self, key_type, genkey_tmpl):
> +        # Truncate name to 64 character maximum.
> +        common_name = self.generateKeyCommonName(
> +            self.archive.owner.name, self.archive.name, key_type)
>  
>          return genkey_tmpl.format(common_name=common_name)
>  
> -    def generatePemX509Pair(self, key_type, pem_filename, x509_filename):
> +    def generatePemX509Pair(self, key_type, genkey_text, pem_filename,
> +            x509_filename):
>          """Generate new pem/x509 key pairs."""
>          directory = os.path.dirname(pem_filename)
>          if not os.path.exists(directory):
>              os.makedirs(directory)
>  
> -        # Truncate name to 64 character maximum.
> -        common_name = self.generateKeyCommonName(
> -            self.archive.owner.name, self.archive.name, key_type)
> -
>          old_mask = os.umask(0o077)
>          try:
>              with tempfile.NamedTemporaryFile(suffix='.keygen') as tf:
> -                genkey_text = self.generateOpensslConfig(key_type, common_name)
>                  print(genkey_text, file=tf)
>  
>                  # Close out the underlying file so we know it is complete.
> @@ -346,6 +352,22 @@
>          cmdl = ["kmodsign", "-D", "sha512", pem, cert, image, image + ".sig"]
>          return self.callLog("Opal signing", cmdl)
>  
> +    def generateSiplKeys(self):
> +        """Generate new Sipl Signing Keys for this archive."""
> +        config = self.generateOpensslConfig("Sipl", self.openssl_config_sipl)

Opal has the excuse of plausibly looking like an English word (though it's really an acronym and we should probably capitalise it as such), but I wonder if SIPL should instead be capitalised thus?

> +        self.generatePemX509Pair("Sipl", config, self.sipl_pem, self.sipl_x509)
> +
> +    def signSipl(self, image):
> +        """Attempt to sign a kernel image for Sipl."""
> +        remove_if_exists("%s.sig" % image)
> +        (pem, cert) = self.getKeys('Sipl Kernel', self.generateSiplKeys,
> +            self.sipl_pem, self.sipl_x509)
> +        if not pem or not cert:
> +            return
> +        self.publishPublicKey(cert)
> +        cmdl = ["kmodsign", "-D", "sha512", pem, cert, image, image + ".sig"]

I'm pleasantly surprised to see that this is currently the same invocation as e.g. OPAL.  Do we need to deploy any changes to kmodsign first?

> +        return self.callLog("Sipl signing", cmdl)
> +
>      def convertToTarball(self):
>          """Convert unpacked output to signing tarball."""
>          tarfilename = os.path.join(self.tmpdir, "signed.tar.gz")
> 
> === modified file 'lib/lp/archivepublisher/tests/test_signing.py'
> --- lib/lp/archivepublisher/tests/test_signing.py	2019-05-24 11:10:38 +0000
> +++ lib/lp/archivepublisher/tests/test_signing.py	2019-06-03 14:49:08 +0000
> @@ -559,15 +603,20 @@
>      def test_correct_kmod_openssl_config(self):
>          # Check that calling generateOpensslConfig() will return an appropriate
>          # openssl configuration.
> +        self.setUpPPA()
>          upload = SigningUpload()
> -        text = upload.generateOpensslConfig('Kmod', 'something-unique')
> +        upload.setTargetDirectory(
> +            self.archive, "test_1.0_amd64.tar.gz", "distroseries")
> +        text = upload.generateOpensslConfig('Kmod', upload.openssl_config_kmod)
>  
> -        cn_re = re.compile(r'\bCN\s*=\s*something-unique\b')
> +        id_re = re.compile(r'^# KMOD openssl config\b')

Maybe explicitly check \n rather than \b here (and similarly below), since it's important that the comment is terminated.

> +        cn_re = re.compile(r'\bCN\s*=\s*' + self.testcase_cn[4:-1] + '\s+Kmod')
>          eku_re = re.compile(
>              r'\bextendedKeyUsage\s*=\s*'
>              r'codeSigning,1.3.6.1.4.1.2312.16.1.2\s*\b')
>  
>          self.assertIn('[ req ]', text)
> +        self.assertIsNotNone(id_re.search(text))
>          self.assertIsNotNone(cn_re.search(text))
>          self.assertIsNotNone(eku_re.search(text))
>  


-- 
https://code.launchpad.net/~apw/launchpad/signing-sipl/+merge/368275
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.