From a0be028340acf1de55426a48ee4e89994f5a4166 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 8 Jun 2016 16:49:08 -0700 Subject: [PATCH 01/11] Add _get_names_from_cert_or_req --- certbot/crypto_util.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/certbot/crypto_util.py b/certbot/crypto_util.py index 6b1b8426c..8640fec1e 100644 --- a/certbot/crypto_util.py +++ b/certbot/crypto_util.py @@ -296,6 +296,18 @@ def get_sans_from_csr(csr, typ=OpenSSL.crypto.FILETYPE_PEM): csr, OpenSSL.crypto.load_certificate_request, typ) +def _get_names_from_cert_or_req(cert_or_req, load_func, typ): + loaded_cert_or_req = _load_cert_or_req(cert_or_req, load_func, typ) + subject = loaded_cert_or_req.get_subject().CN + # pylint: disable=protected-access + sans = acme_crypto_util._pyopenssl_cert_or_req_san(loaded_cert_or_req) + + if subject is None: + return sans + else: + return [subject] + [d for d in sans if d != subject] + + def get_names_from_csr(csr, typ=OpenSSL.crypto.FILETYPE_PEM): """Get a list of domains from a CSR, including the CN if it is set. From ac581951b3926c590ce930230a315d31e91ed0b2 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 8 Jun 2016 16:50:34 -0700 Subject: [PATCH 02/11] Have get_names_from_csr use _get_names_from_cert_or_req --- certbot/crypto_util.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/certbot/crypto_util.py b/certbot/crypto_util.py index 8640fec1e..e8047f086 100644 --- a/certbot/crypto_util.py +++ b/certbot/crypto_util.py @@ -318,13 +318,8 @@ def get_names_from_csr(csr, typ=OpenSSL.crypto.FILETYPE_PEM): :rtype: list """ - loaded_csr = _load_cert_or_req( + return _get_names_from_cert_or_req( csr, OpenSSL.crypto.load_certificate_request, typ) - # Use a set to avoid duplication with CN and Subject Alt Names - domains = set(d for d in (loaded_csr.get_subject().CN,) if d is not None) - # pylint: disable=protected-access - domains.update(acme_crypto_util._pyopenssl_cert_or_req_san(loaded_csr)) - return list(domains) def dump_pyopenssl_chain(chain, filetype=OpenSSL.crypto.FILETYPE_PEM): From 753aea2f3f495eccb3f50cf977c3864dc56a55b6 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 8 Jun 2016 16:53:04 -0700 Subject: [PATCH 03/11] Add get_names_from_cert function --- certbot/crypto_util.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/certbot/crypto_util.py b/certbot/crypto_util.py index e8047f086..f45645de1 100644 --- a/certbot/crypto_util.py +++ b/certbot/crypto_util.py @@ -308,6 +308,20 @@ def _get_names_from_cert_or_req(cert_or_req, load_func, typ): return [subject] + [d for d in sans if d != subject] +def get_names_from_cert(csr, typ=OpenSSL.crypto.FILETYPE_PEM): + """Get a list of domains from a cert, including the CN if it is set. + + :param str cert: Certificate (encoded). + :param typ: `OpenSSL.crypto.FILETYPE_PEM` or `OpenSSL.crypto.FILETYPE_ASN1` + + :returns: A list of domain names. + :rtype: list + + """ + return _get_names_from_cert_or_req( + csr, OpenSSL.crypto.load_certificate, typ) + + def get_names_from_csr(csr, typ=OpenSSL.crypto.FILETYPE_PEM): """Get a list of domains from a CSR, including the CN if it is set. From 8db1b5627c02d4a02890075e1a3ca6f9a3a29d96 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 8 Jun 2016 16:57:56 -0700 Subject: [PATCH 04/11] Add GetNamesFromCertTest --- certbot/tests/crypto_util_test.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/certbot/tests/crypto_util_test.py b/certbot/tests/crypto_util_test.py index fa88e89e7..742d4ec8c 100644 --- a/certbot/tests/crypto_util_test.py +++ b/certbot/tests/crypto_util_test.py @@ -273,6 +273,25 @@ class GetSANsFromCSRTest(unittest.TestCase): [], self._call(test_util.load_vector('csr-nosans.pem'))) +class GetNamesFromCertTest(unittest.TestCase): + """Tests for certbot.crypto_util.get_names_from_cert.""" + + @classmethod + def _call(cls, *args, **kwargs): + from certbot.crypto_util import get_names_from_cert + return get_names_from_cert(*args, **kwargs) + + def test_single(self): + self.assertEqual( + ['example.com'], + self._call(test_util.load_vector('cert.pem'))) + + def test_san(self): + self.assertEqual( + ['example.com', 'www.example.com'], + self._call(test_util.load_vector('cert-san.pem'))) + + class GetNamesFromCSRTest(unittest.TestCase): """Tests for certbot.crypto_util.get_names_from_csr.""" @classmethod From 0a707b64ec0b9a0ee61fe56fc6bcf4b1b9525318 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 8 Jun 2016 16:59:44 -0700 Subject: [PATCH 05/11] Use common_name instead of subject --- certbot/crypto_util.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/certbot/crypto_util.py b/certbot/crypto_util.py index f45645de1..1e831dd8f 100644 --- a/certbot/crypto_util.py +++ b/certbot/crypto_util.py @@ -298,14 +298,14 @@ def get_sans_from_csr(csr, typ=OpenSSL.crypto.FILETYPE_PEM): def _get_names_from_cert_or_req(cert_or_req, load_func, typ): loaded_cert_or_req = _load_cert_or_req(cert_or_req, load_func, typ) - subject = loaded_cert_or_req.get_subject().CN + common_name = loaded_cert_or_req.get_subject().CN # pylint: disable=protected-access sans = acme_crypto_util._pyopenssl_cert_or_req_san(loaded_cert_or_req) - if subject is None: + if common_name is None: return sans else: - return [subject] + [d for d in sans if d != subject] + return [common_name] + [d for d in sans if d != common_name] def get_names_from_cert(csr, typ=OpenSSL.crypto.FILETYPE_PEM): From 2c803eff6ab161079128dca16f8efd8154049ac1 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 8 Jun 2016 17:01:54 -0700 Subject: [PATCH 06/11] Use get_names_from_cert in storage.py --- certbot/storage.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/storage.py b/certbot/storage.py index b0c8245d3..60886e306 100644 --- a/certbot/storage.py +++ b/certbot/storage.py @@ -616,7 +616,7 @@ class RenewableCert(object): # pylint: disable=too-many-instance-attributes if target is None: raise errors.CertStorageError("could not find cert file") with open(target) as f: - return crypto_util.get_sans_from_cert(f.read()) + return crypto_util.get_names_from_cert(f.read()) def autodeployment_is_enabled(self): """Is automatic deployment enabled for this cert? From 02cdb5db0ef2e035a19b86ca0b462a0a38e50694 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 9 Jun 2016 16:03:32 -0700 Subject: [PATCH 07/11] Add cert with not alphabetically first CN --- certbot/tests/testdata/cert-5sans.pem | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 certbot/tests/testdata/cert-5sans.pem diff --git a/certbot/tests/testdata/cert-5sans.pem b/certbot/tests/testdata/cert-5sans.pem new file mode 100644 index 000000000..5de7cc6cb --- /dev/null +++ b/certbot/tests/testdata/cert-5sans.pem @@ -0,0 +1,16 @@ +-----BEGIN CERTIFICATE----- +MIICkTCCAjugAwIBAgIJAJNbfABWQ8bbMA0GCSqGSIb3DQEBCwUAMHkxCzAJBgNV +BAYTAlVTMRMwEQYDVQQIDApDYWxpZm9ybmlhMRYwFAYDVQQHDA1TYW4gRnJhbmNp +c2NvMScwJQYDVQQKDB5FbGVjdHJvbmljIEZyb250aWVyIEZvdW5kYXRpb24xFDAS +BgNVBAMMC2V4YW1wbGUuY29tMB4XDTE2MDYwOTIzMDEzNloXDTE2MDcwOTIzMDEz +NloweTELMAkGA1UEBhMCVVMxEzARBgNVBAgMCkNhbGlmb3JuaWExFjAUBgNVBAcM +DVNhbiBGcmFuY2lzY28xJzAlBgNVBAoMHkVsZWN0cm9uaWMgRnJvbnRpZXIgRm91 +bmRhdGlvbjEUMBIGA1UEAwwLZXhhbXBsZS5jb20wXDANBgkqhkiG9w0BAQEFAANL +ADBIAkEArHVztFHtH92ucFJD/N/HW9AsdRsUuHUBBBDlHwNlRd3fp580rv2+6QWE +30cWgdmJS86ObRz6lUTor4R0T+3C5QIDAQABo4GlMIGiMB0GA1UdDgQWBBQmz8jt +S9eUsuQlA1gkjwTAdNWXijAfBgNVHSMEGDAWgBQmz8jtS9eUsuQlA1gkjwTAdNWX +ijAMBgNVHRMEBTADAQH/MFIGA1UdEQRLMEmCDWEuZXhhbXBsZS5jb22CDWIuZXhh +bXBsZS5jb22CDWMuZXhhbXBsZS5jb22CDWQuZXhhbXBsZS5jb22CC2V4YW1wbGUu +Y29tMA0GCSqGSIb3DQEBCwUAA0EAVXmZxB+IJdgFvY2InOYeytTD1QmouDZRtj/T +H/HIpSdsfO7qr4d/ZprI2IhLRxp2S4BiU5Qc5HUkeADcpNd06A== +-----END CERTIFICATE----- From 4f99cc7b2a2f1eb4eb1820e75d909551eea2d1c7 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 9 Jun 2016 17:43:05 -0700 Subject: [PATCH 08/11] Add _write_out_kind method --- certbot/tests/storage_test.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/certbot/tests/storage_test.py b/certbot/tests/storage_test.py index 0c88d3d55..0579c9f1c 100644 --- a/certbot/tests/storage_test.py +++ b/certbot/tests/storage_test.py @@ -84,6 +84,16 @@ class BaseRenewableCertTest(unittest.TestCase): def tearDown(self): shutil.rmtree(self.tempdir) + def _write_out_kind(self, kind, ver, value=None): + link = getattr(self.test_rc, kind) + if os.path.lexists(link): + os.unlink(link) + os.symlink(os.path.join(os.path.pardir, os.path.pardir, "archive", + "example.org", "{0}{1}.pem".format(kind, ver)), + link) + with open(link, "w") as f: + f.write(kind if value is None else value) + def _write_out_ex_kinds(self): for kind in ALL_FOUR: where = getattr(self.test_rc, kind) From 562802bfd0443f14cd928ee41a4ae73b9453ce39 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 9 Jun 2016 17:44:33 -0700 Subject: [PATCH 09/11] Refactor common symlink writing code --- certbot/tests/storage_test.py | 124 ++++++---------------------------- 1 file changed, 19 insertions(+), 105 deletions(-) diff --git a/certbot/tests/storage_test.py b/certbot/tests/storage_test.py index 0579c9f1c..44b881fd9 100644 --- a/certbot/tests/storage_test.py +++ b/certbot/tests/storage_test.py @@ -96,16 +96,8 @@ class BaseRenewableCertTest(unittest.TestCase): def _write_out_ex_kinds(self): for kind in ALL_FOUR: - where = getattr(self.test_rc, kind) - os.symlink(os.path.join("..", "..", "archive", "example.org", - "{0}12.pem".format(kind)), where) - with open(where, "w") as f: - f.write(kind) - os.unlink(where) - os.symlink(os.path.join("..", "..", "archive", "example.org", - "{0}11.pem".format(kind)), where) - with open(where, "w") as f: - f.write(kind) + self._write_out_kind(kind, 12) + self._write_out_kind(kind, 11) class RenewableCertTests(BaseRenewableCertTest): @@ -214,10 +206,7 @@ class RenewableCertTests(BaseRenewableCertTest): def test_current_target(self): # Relative path logic - os.symlink(os.path.join("..", "..", "archive", "example.org", - "cert17.pem"), self.test_rc.cert) - with open(self.test_rc.cert, "w") as f: - f.write("cert") + self._write_out_kind("cert", 17) self.assertTrue(os.path.samefile(self.test_rc.current_target("cert"), os.path.join(self.tempdir, "archive", "example.org", @@ -235,12 +224,8 @@ class RenewableCertTests(BaseRenewableCertTest): def test_current_version(self): for ver in (1, 5, 10, 20): - os.symlink(os.path.join("..", "..", "archive", "example.org", - "cert{0}.pem".format(ver)), - self.test_rc.cert) - with open(self.test_rc.cert, "w") as f: - f.write("cert") - os.unlink(self.test_rc.cert) + self._write_out_kind("cert", ver) + os.unlink(self.test_rc.cert) os.symlink(os.path.join("..", "..", "archive", "example.org", "cert10.pem"), self.test_rc.cert) self.assertEqual(self.test_rc.current_version("cert"), 10) @@ -251,61 +236,30 @@ class RenewableCertTests(BaseRenewableCertTest): def test_latest_and_next_versions(self): for ver in xrange(1, 6): for kind in ALL_FOUR: - where = getattr(self.test_rc, kind) - if os.path.islink(where): - os.unlink(where) - os.symlink(os.path.join("..", "..", "archive", "example.org", - "{0}{1}.pem".format(kind, ver)), where) - with open(where, "w") as f: - f.write(kind) + self._write_out_kind(kind, ver) self.assertEqual(self.test_rc.latest_common_version(), 5) self.assertEqual(self.test_rc.next_free_version(), 6) # Having one kind of file of a later version doesn't change the # result - os.unlink(self.test_rc.privkey) - os.symlink(os.path.join("..", "..", "archive", "example.org", - "privkey7.pem"), self.test_rc.privkey) - with open(self.test_rc.privkey, "w") as f: - f.write("privkey") + self._write_out_kind("privkey", 7) self.assertEqual(self.test_rc.latest_common_version(), 5) # ... although it does change the next free version self.assertEqual(self.test_rc.next_free_version(), 8) # Nor does having three out of four change the result - os.unlink(self.test_rc.cert) - os.symlink(os.path.join("..", "..", "archive", "example.org", - "cert7.pem"), self.test_rc.cert) - with open(self.test_rc.cert, "w") as f: - f.write("cert") - os.unlink(self.test_rc.fullchain) - os.symlink(os.path.join("..", "..", "archive", "example.org", - "fullchain7.pem"), self.test_rc.fullchain) - with open(self.test_rc.fullchain, "w") as f: - f.write("fullchain") + self._write_out_kind("cert", 7) + self._write_out_kind("fullchain", 7) self.assertEqual(self.test_rc.latest_common_version(), 5) # If we have everything from a much later version, it does change # the result - ver = 17 for kind in ALL_FOUR: - where = getattr(self.test_rc, kind) - if os.path.islink(where): - os.unlink(where) - os.symlink(os.path.join("..", "..", "archive", "example.org", - "{0}{1}.pem".format(kind, ver)), where) - with open(where, "w") as f: - f.write(kind) + self._write_out_kind(kind, 17) self.assertEqual(self.test_rc.latest_common_version(), 17) self.assertEqual(self.test_rc.next_free_version(), 18) def test_update_link_to(self): for ver in xrange(1, 6): for kind in ALL_FOUR: - where = getattr(self.test_rc, kind) - if os.path.islink(where): - os.unlink(where) - os.symlink(os.path.join("..", "..", "archive", "example.org", - "{0}{1}.pem".format(kind, ver)), where) - with open(where, "w") as f: - f.write(kind) + self._write_out_kind(kind, ver) self.assertEqual(ver, self.test_rc.current_version(kind)) # pylint: disable=protected-access self.test_rc._update_link_to("cert", 3) @@ -322,10 +276,7 @@ class RenewableCertTests(BaseRenewableCertTest): "chain3000.pem") def test_version(self): - os.symlink(os.path.join("..", "..", "archive", "example.org", - "cert12.pem"), self.test_rc.cert) - with open(self.test_rc.cert, "w") as f: - f.write("cert") + self._write_out_kind("cert", 12) # TODO: We should probably test that the directory is still the # same, but it's tricky because we can get an absolute # path out when we put a relative path in. @@ -335,13 +286,7 @@ class RenewableCertTests(BaseRenewableCertTest): def test_update_all_links_to_success(self): for ver in xrange(1, 6): for kind in ALL_FOUR: - where = getattr(self.test_rc, kind) - if os.path.islink(where): - os.unlink(where) - os.symlink(os.path.join("..", "..", "archive", "example.org", - "{0}{1}.pem".format(kind, ver)), where) - with open(where, "w") as f: - f.write(kind) + self._write_out_kind(kind, ver) self.assertEqual(ver, self.test_rc.current_version(kind)) self.assertEqual(self.test_rc.latest_common_version(), 5) for ver in xrange(1, 6): @@ -386,13 +331,7 @@ class RenewableCertTests(BaseRenewableCertTest): def test_has_pending_deployment(self): for ver in xrange(1, 6): for kind in ALL_FOUR: - where = getattr(self.test_rc, kind) - if os.path.islink(where): - os.unlink(where) - os.symlink(os.path.join("..", "..", "archive", "example.org", - "{0}{1}.pem".format(kind, ver)), where) - with open(where, "w") as f: - f.write(kind) + self._write_out_kind(kind, ver) self.assertEqual(ver, self.test_rc.current_version(kind)) for ver in xrange(1, 6): self.test_rc.update_all_links_to(ver) @@ -405,21 +344,12 @@ class RenewableCertTests(BaseRenewableCertTest): def test_names(self): # Trying the current version - test_cert = test_util.load_vector("cert-san.pem") - os.symlink(os.path.join("..", "..", "archive", "example.org", - "cert12.pem"), self.test_rc.cert) - with open(self.test_rc.cert, "w") as f: - f.write(test_cert) + self._write_out_kind("cert", 12, test_util.load_vector("cert-san.pem")) self.assertEqual(self.test_rc.names(), ["example.com", "www.example.com"]) # Trying a non-current version - test_cert = test_util.load_vector("cert.pem") - os.unlink(self.test_rc.cert) - os.symlink(os.path.join("..", "..", "archive", "example.org", - "cert15.pem"), self.test_rc.cert) - with open(self.test_rc.cert, "w") as f: - f.write(test_cert) + self._write_out_kind("cert", 15, test_util.load_vector("cert.pem")) self.assertEqual(self.test_rc.names(12), ["example.com", "www.example.com"]) @@ -490,13 +420,7 @@ class RenewableCertTests(BaseRenewableCertTest): # No pending deployment for ver in xrange(1, 6): for kind in ALL_FOUR: - where = getattr(self.test_rc, kind) - if os.path.islink(where): - os.unlink(where) - os.symlink(os.path.join("..", "..", "archive", "example.org", - "{0}{1}.pem".format(kind, ver)), where) - with open(where, "w") as f: - f.write(kind) + self._write_out_kind(kind, ver) self.assertFalse(self.test_rc.should_autodeploy()) def test_autorenewal_is_enabled(self): @@ -517,11 +441,7 @@ class RenewableCertTests(BaseRenewableCertTest): self.assertFalse(self.test_rc.should_autorenew()) self.test_rc.configuration["autorenew"] = "1" for kind in ALL_FOUR: - where = getattr(self.test_rc, kind) - os.symlink(os.path.join("..", "..", "archive", "example.org", - "{0}12.pem".format(kind)), where) - with open(where, "w") as f: - f.write(kind) + self._write_out_kind(kind, 12) # Mandatory renewal on the basis of OCSP revocation mock_ocsp.return_value = True self.assertTrue(self.test_rc.should_autorenew()) @@ -535,13 +455,7 @@ class RenewableCertTests(BaseRenewableCertTest): for ver in xrange(1, 6): for kind in ALL_FOUR: - where = getattr(self.test_rc, kind) - if os.path.islink(where): - os.unlink(where) - os.symlink(os.path.join("..", "..", "archive", "example.org", - "{0}{1}.pem".format(kind, ver)), where) - with open(where, "w") as f: - f.write(kind) + self._write_out_kind(kind, ver) self.test_rc.update_all_links_to(3) self.assertEqual( 6, self.test_rc.save_successor(3, "new cert", None, From bb1d2c0a1faf7d58249e73fdbd6ea832335755b3 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 9 Jun 2016 17:49:44 -0700 Subject: [PATCH 10/11] Test common name is listed first in storage.py --- certbot/tests/storage_test.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/certbot/tests/storage_test.py b/certbot/tests/storage_test.py index 44b881fd9..0d907eca3 100644 --- a/certbot/tests/storage_test.py +++ b/certbot/tests/storage_test.py @@ -353,6 +353,13 @@ class RenewableCertTests(BaseRenewableCertTest): self.assertEqual(self.test_rc.names(12), ["example.com", "www.example.com"]) + # Testing common name is listed first + self._write_out_kind( + "cert", 12, test_util.load_vector("cert-5sans.pem")) + self.assertEqual( + self.test_rc.names(12), + ["example.com"] + ["{0}.example.com".format(c) for c in "abcd"]) + # Trying missing cert os.unlink(self.test_rc.cert) self.assertRaises(errors.CertStorageError, self.test_rc.names) From 07cf34284eabf129ce0fae76d79939d38a8bbcdb Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 9 Jun 2016 17:55:46 -0700 Subject: [PATCH 11/11] Add test_common_name_sans_order --- certbot/tests/crypto_util_test.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/certbot/tests/crypto_util_test.py b/certbot/tests/crypto_util_test.py index 742d4ec8c..5a592bbb1 100644 --- a/certbot/tests/crypto_util_test.py +++ b/certbot/tests/crypto_util_test.py @@ -291,6 +291,13 @@ class GetNamesFromCertTest(unittest.TestCase): ['example.com', 'www.example.com'], self._call(test_util.load_vector('cert-san.pem'))) + def test_common_name_sans_order(self): + # Tests that the common name comes first + # followed by the SANS in alphabetical order + self.assertEqual( + ['example.com'] + ['{0}.example.com'.format(c) for c in 'abcd'], + self._call(test_util.load_vector('cert-5sans.pem'))) + class GetNamesFromCSRTest(unittest.TestCase): """Tests for certbot.crypto_util.get_names_from_csr."""