launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> === modified file 'lib/lp/archivepublisher/signing.py'
> --- lib/lp/archivepublisher/signing.py	2018-05-06 08:52:34 +0000
> +++ lib/lp/archivepublisher/signing.py	2018-08-03 14:17:19 +0000
> @@ -242,6 +242,41 @@
>          cmdl = ["sbsign", "--key", key, "--cert", cert, image]
>          return self.callLog("UEFI signing", cmdl)
>  
> +    openssl_config_opal = """
> +        [ req ]
> +        default_bits = 4096
> +        distinguished_name = req_distinguished_name
> +        prompt = no
> +        string_mask = utf8only
> +        x509_extensions = myexts
> +
> +        [ req_distinguished_name ]
> +        CN = {common_name}
> +
> +        [ myexts ]
> +        basicConstraints=critical,CA:FALSE
> +        keyUsage=digitalSignature
> +        subjectKeyIdentifier=hash
> +        authorityKeyIdentifier=keyidb

Was the change from "keyid" to "keyidb" here deliberate?  It looks like a mistake.

> +        """
> +
> +    openssl_config_kmod = openssl_config_opal + """
> +        # 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)
> +
> +        return textwrap.dedent(genkey_tmpl.format(common_name=common_name))

I'd mildly prefer to have the textwrap.dedent be done at the points where openssl_config_opal and openssl_config_kmod are declared, rather than at the point where they're used.

> +
>      def generatePemX509Pair(self, key_type, pem_filename, x509_filename):
>          """Generate new pem/x509 key pairs."""
>          directory = os.path.dirname(pem_filename)
> 
> === modified file 'lib/lp/archivepublisher/tests/test_signing.py'
> --- lib/lp/archivepublisher/tests/test_signing.py	2018-05-06 08:52:34 +0000
> +++ lib/lp/archivepublisher/tests/test_signing.py	2018-08-03 14:17:19 +0000
> @@ -555,6 +556,21 @@
>              ]
>          self.assertEqual(expected_cmd, args)
>  
> +    def test_correct_kmod_openssl_config(self):
> +        # Check that calling generateOpensslConfig() will return an appropriate
> +        # openssl configuration.
> +        upload = SigningUpload()
> +        text = upload.generateOpensslConfig('Kmod', 'something-unique')
> +
> +        cn_re = re.compile(r'\bCN\s*=\s*something-unique\b')
> +        eku_re = re.compile(
> +            r'\bextendedKeyUsage\s*=\s*'
> +            r'codeSigning,1.3.6.1.4.1.2312.16.1.2\s*\b')
> +
> +        self.assertTrue('[ req ]' in text)

Slight preference for this instead:

  self.assertIn('[ req ]', text)

(Same in test_correct_opal_openssl_config.)

> +        self.assertIsNotNone(cn_re.search(text))
> +        self.assertIsNotNone(eku_re.search(text))
> +
>      def test_correct_kmod_signing_command_executed(self):
>          # Check that calling signKmod() will generate the expected command
>          # when appropriate keys are present.
> @@ -621,6 +637,18 @@
>              ]
>          self.assertEqual(expected_cmd, args)
>  
> +    def test_correct_opal_openssl_config(self):
> +        # Check that calling generateOpensslConfig() will return an appropriate
> +        # openssl configuration.
> +        upload = SigningUpload()
> +        text = upload.generateOpensslConfig('Opal', 'something-unique')
> +
> +        cn_re = re.compile(r'\bCN\s*=\s*something-unique\b')
> +
> +        self.assertTrue('[ req ]' in text)
> +        self.assertIsNotNone(cn_re.search(text))
> +        self.assertFalse('extendedKeyUsage' in text)

self.assertNotIn

> +
>      def test_correct_opal_signing_command_executed(self):
>          # Check that calling signOpal() will generate the expected command
>          # when appropriate keys are present.


-- 
https://code.launchpad.net/~apw/launchpad/signing-kmod-extended-key-usage/+merge/352305
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.