Address pull request comments

Signed-off-by: Jeremy Cline <jeremy@jcline.org>

fedmsg/crypto/utils.py

@@ -103,11 +103,15 @@ def load_certificates(ca_location, crl_location=None, invalidate_cache=False):
     """
     Load the CA certificate and CRL, caching it for future use.
 
+    .. note::
+        Providing the location of the CA and CRL as an HTTPS URL is deprecated
+        and will be removed in a future release.
+
     Args:
         ca_location (str): The location of the Certificate Authority certificate. This should
             be the absolute path to a PEM-encoded file. It can also be an HTTPS url, but this
             is deprecated and will be removed in a future release.
-        ca_location (str): The location of the Certificate Revocation List. This should
+        crl_location (str): The location of the Certificate Revocation List. This should
             be the absolute path to a PEM-encoded file. It can also be an HTTPS url, but
             this is deprecated and will be removed in a future release.
         invalidate_cache (bool): Whether or not to invalidate the certificate cache.
@@ -150,7 +154,7 @@ def _load_certificate(location):
             ASCII encoding.
 
     Returns:
-        str:
+        str: The PEM-encoded certificate as a unicode string.
 
     Raises:
         requests.exception.RequestException: Any exception requests could raise.

fedmsg/crypto/x509.py

@@ -145,7 +145,7 @@ def fail(reason):
     try:
         ca_certificate, crl = utils.load_certificates(ca_location, crl_location)
         os.write(fd, ca_certificate.encode('ascii'))
-        os.close(fd)
+        os.fsync(fd)
         ctx = m2ext.SSL.Context()
         ctx.load_verify_locations(cafile=cafile)
         if not ctx.validate_certificate(cert):
@@ -161,15 +161,17 @@ def fail(reason):
         _log.error(str(e))
         return False
     finally:
+        os.close(fd)
         os.remove(cafile)
 
     if crl:
-        fd, crlfile = tempfile.mkstemp(text=True)
-        os.write(fd, crl.encode('ascii'))
-        os.close(fd)
         try:
+            fd, crlfile = tempfile.mkstemp(text=True)
+            os.write(fd, crl.encode('ascii'))
+            os.fsync(fd)
             crl = M2Crypto.X509.load_crl(crlfile)
         finally:
+            os.close(fd)
             os.remove(crlfile)
         # FIXME -- We need to check that the CRL is signed by our own CA.
         # See https://bugzilla.osafoundation.org/show_bug.cgi?id=12954#c2

fedmsg/tests/crypto/test_utils.py

@@ -122,10 +122,9 @@ def test_remote_cert(self):
 
         with mock.patch.dict('fedmsg.crypto.utils._cached_certificates', clear=True):
             ca, crl = utils.load_certificates(location)
-
             self.assertEqual((expected_cert, None), utils._cached_certificates[location])
-            self.assertEqual(expected_cert, ca)
-            self.assertTrue(crl is None)
+        self.assertEqual(expected_cert, ca)
+        self.assertTrue(crl is None)
 
     @mock.patch('fedmsg.crypto.utils._load_certificate')
     def test_valid_cache(self, mock_load_cert):
@@ -134,7 +133,6 @@ def test_valid_cache(self, mock_load_cert):
 
         with mock.patch.dict('fedmsg.crypto.utils._cached_certificates', {'/crt': ('crt', None)}):
             ca, crl = utils.load_certificates(location)
-
         self.assertEqual('crt', ca)
         self.assertTrue(crl is None)
         self.assertEqual(0, mock_load_cert.call_count)
@@ -147,8 +145,7 @@ def test_invalidate_cache(self, mock_load_cert):
 
         with mock.patch.dict('fedmsg.crypto.utils._cached_certificates', {'/crt': ('crt', None)}):
             ca, crl = utils.load_certificates(location, invalidate_cache=True)
-
             self.assertEqual(('fresh_ca', None), utils._cached_certificates[location])
-            self.assertEqual('fresh_ca', ca)
-            self.assertTrue(crl is None)
-            mock_load_cert.called_once_with('/crt')
+        self.assertEqual('fresh_ca', ca)
+        self.assertTrue(crl is None)
+        mock_load_cert.called_once_with('/crt')