Use cryptography and pyOpenSSL instead of M2Crypto #421

jeremycline · 2017-06-16T15:24:10Z

M2Crypto is Python 2 only and is in maintenance mode. Although pyOpenSSL
is also in maintenance mode, it supports Python 3 and relies on
cryptography for its OpenSSL bindings. Currently the only thing
cryptography is missing (that pyOpenSSL has) is certificate chain
validation. Once that's available in cryptography, we can drop
pyOpenSSL.

Unfortunately, we still need to support M2Crypto because the current
versions of cryptography and pyOpenSSL in EL7 is below 1.6 and 16.1.0
respectively.

This patch uses cryptography and pyOpenSSL if available and falls back
to M2Crypto.

fixes #411

Signed-off-by: Jeremy Cline [email protected]

codecov-io · 2017-06-16T15:33:28Z

Codecov Report

Merging #421 into develop will increase coverage by 7.59%.
The diff coverage is 86.13%.

@@             Coverage Diff             @@
##           develop     #421      +/-   ##
===========================================
+ Coverage    47.15%   54.75%   +7.59%     
===========================================
  Files           33       33              
  Lines         2059     2009      -50     
  Branches       331      312      -19     
===========================================
+ Hits           971     1100     +129     
+ Misses         993      812     -181     
- Partials        95       97       +2

Impacted Files	Coverage Δ
fedmsg/crypto/__init__.py	`64.06% <100%> (-0.65%)`	⬇️
fedmsg/crypto/x509_ng.py	`100% <100%> (+100%)`	⬆️
fedmsg/crypto/utils.py	`61.53% <66.66%> (ø)`
fedmsg/crypto/x509.py	`80.45% <78.04%> (+31.33%)`	⬆️
fedmsg/utils.py
fedmsg/commands/__init__.py
fedmsg/consumers/__init__.py
fedmsg/tests/commands/__init__.py	`43.1% <0%> (ø)`
fedmsg/tests/consumers/__init__.py	`72.09% <0%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5aad9af...0c7aee2. Read the comment docs.

This PEP-8s nearly all of fedmsg except for the stuff touched by fedora-infra#421. Signed-off-by: Jeremy Cline <[email protected]>

bowlofeggs

This looks reasonable to me. Note however that I lack familiarity with the cryptography and pyopenssl libraries, so I am not able to comment on whether they were used correctly or not. Thus, I recommend getting a second opinion from someone who is familiar with those APIs before merging. On the other hand, I did like the tests you wrote and they helped give me confidence in the code ☺

bowlofeggs · 2017-06-17T15:19:10Z

fedmsg/crypto/utils.py

+        signer (str): The Common Name of the certificate used to sign the message.
+
+    Returns:
+        bool: True of the policy defined in the settings allows the signer to send


True if the policy…

bowlofeggs · 2017-06-17T15:19:42Z

fedmsg/crypto/utils.py

+            # whitelisted to send on this topic in our config policy.
+            return True
+        else:
+            # We have a policy for this topic and $homeboy isn't on the list.


bowlofeggs · 2017-06-17T15:20:40Z

fedmsg/crypto/utils.py

+            # routing_policy for the topic of this message.. but we don't
+            # really care.
+            _log.info('No routing policy defined for "{t}" and routing_nitpicky '
+                      'is False so the message is being treated as authorized.'.format(t=topic))


This might make sense at warning level.

bowlofeggs · 2017-06-17T15:21:56Z

fedmsg/crypto/utils.py

+def load_remote_cert(location, cache, cache_expiry, tries=0, **config):
+    """ Get a fresh copy from fp.o/fedmsg/crl.pem if ours is getting stale.
+
+    Return the local filename.


I recommend documenting the arguments and the return type.

bowlofeggs · 2017-06-17T15:56:34Z

fedmsg/crypto/utils.py

+        (cache_expiry and time.time() - modtime > cache_expiry)
+    ):
+        try:
+            response = requests.get(location)


Maybe put a timeout on this call to get().

bowlofeggs · 2017-06-17T16:06:26Z

fedmsg/crypto/x509_ng.py

-    except M2Crypto.RSA.RSAError as e:
-        return fail(str(e))
+        _validate_signing_cert(ca_certificate, certificate, crl)
+    except X509StoreContextError as e:


I presume this step will catch expired certificates and non-properly signed certificates?

Yup, it'll raise for that as well as revoked certificates and expired CRLs

bowlofeggs · 2017-06-17T16:09:01Z

fedmsg/tests/crypto/test_x509.py

@@ -0,0 +1,216 @@
+# This file is part of fedmsg.
+# Copyright (C) 2012 - 2014 Red Hat, Inc.


bowlofeggs · 2017-06-17T16:12:38Z

fedmsg/tests/crypto/test_x509.py

+        self.config['routing_policy'] = {'mytopic': ['shell-app01.phx2.fedoraproject.org']}
+        self.config['routing_nitpicky'] = True
+        signed = self.sign({'topic': 'mytopic', 'message': 'so secure'}, **self.config)
+        self.assertTrue(self.validate(signed, **self.config))


How about a test_signed_by_expired_cert and test_signed_by_expired_ca (bad name, but what I mean is the signing cert is signed by an expired CA) as well?

Good idea! I have made it so, Number One.

bowlofeggs · 2017-06-17T16:13:16Z

fedmsg/tests/crypto/test_x509.py

+
+
+@skipIf(not (_m2crypto or _cryptography), "Neither M2Crypto nor Cryptography available")
+class X509BaseTests(TestCase):


I like this pattern of making a superclass to get the same tests run with the different implementations. Nice!

bowlofeggs · 2017-06-17T16:14:04Z

fedmsg/tests/crypto/test_x509.py

+            'ca_cert_cache': os.path.join(SSLDIR, 'ca.crt'),
+            'ca_cert_cache_expiry': 1497618475,  # Stop fedmsg overwriting my CA, See Issue 420
+
+            'crl_location': "http://threebean.org/fedmsg-tests/crl.pem",


Do these tests work offline? If not, I see that you have a fedmsg/tests/test_certs/keys/crl.pem that you might could use instead…

They do, but only because I set the cache expiry to be ~47 years. There's no way to have the cert not downloaded from the Internet eventually and it defaults to the Fedora CA and CRL so even if you don't provide a URL it'll happen (all of which I want to change, I just haven't decided how yet).

M2Crypto is Python 2 only and is in maintenance mode. Although pyOpenSSL is _also_ in maintenance mode, it supports Python 3 and relies on cryptography for its OpenSSL bindings. Currently the only thing cryptography is missing (that pyOpenSSL has) is certificate chain validation. Once that's available in cryptography, we can drop pyOpenSSL. Unfortunately, we still need to support M2Crypto because the current versions of cryptography and pyOpenSSL in EL7 is below 1.6 and 16.1.0 respectively. This patch uses cryptography and pyOpenSSL if available and falls back to M2Crypto. fixes fedora-infra#411 Signed-off-by: Jeremy Cline <[email protected]>

Signed-off-by: Jeremy Cline <[email protected]>

Prior to this commit, it was required that a CRL was specified. This was fine with the old M2Crypto code that didn't properly check the CRL for expiration, but the new crytography-based code will fail if the CRL is expired. This allows the definition of a CRL in the configuration to be optional so users don't have to keep a fresh CRL that they may not actually be using available. Signed-off-by: Jeremy Cline <[email protected]>

Thanks bowlofeggs for the suggestion! Signed-off-by: Jeremy Cline <[email protected]>

puiterwijk

Overall this looks good, with some future suggestions.

puiterwijk · 2017-06-28T18:30:16Z

fedmsg/crypto/utils.py

+
+    Return the local filename.
+
+    .. note:: This is not a public API and is subject to change.


Maybe start the name with _ then?

puiterwijk · 2017-06-28T18:33:18Z

fedmsg/crypto/x509.py

+def _disabled_sign(*args, **kwargs):
+    """A fallback function that emits a warning when no crypto is being used."""
+    warnings.warn('Message signing is disabled because "cryptography" and '
+                  '"pyopenssl" or "m2crypto" are not available.')


Perhaps add parentheses to indicate the and/or relations?

puiterwijk · 2017-06-28T18:38:01Z

fedmsg/crypto/x509_ng.py

        asymmetric.padding.PKCS1v15(),
-        hashes.SHA1()
+        hashes.SHA1(),


While it's fine right now, it would be great if we could start migrating to new hashes, like sha256/sha512.
Maybe add an indicator which is used before the next version, so we can switch easier in the future?

I considered this, but I didn't want to make that change alongside all the other stuff that's already happening. I thought I filed an issue, but it seems like I hadn't, so I filed #438

That makes perfect sense. This was just an idea now that you got me rereading the code :).

puiterwijk · 2017-06-28T18:45:47Z

fedmsg/crypto/x509_ng.py

-    # Load our policy from the config dict.
+    # Step 4, check that the certificate is permitted to emit messages for the
+    # topic.
+    common_name = crypto_certificate.subject.get_attributes_for_oid(x509.oid.NameOID.COMMON_NAME)


You should not use the common name ideally, this is being deprecated by the CA/Browser Forum baseline requirements.
While we are not reliant on those, I think ideally we should follow their move to the Subject Alternative Name.
This does mean there may be multiple records, and we might need to check all SAN values in the authorization step.
For the time being, for compatibility, we might add the Common Name to the list of SANs for checking purposes though and issue a warning if the CN was not in any SAN values.

Yeah, I wanted to do this, but the M2Crypto version is only using the CN. I will definitely look into moving both of them to SAN (#439).

The concrete sentence about Common Name from the CA/Browser Baselines requirements v1.4.5 (which is currently the latest stable, source https://cabforum.org/wp-content/uploads/CA-Browser-Forum-BR-1.4.5.pdf):

Certificate Field: subject:commonName (OID 2.5.4.3) Required/Optional: Deprecated (Discouraged, but not prohibited)

puiterwijk · 2017-06-28T18:46:25Z

fedmsg/crypto/x509_ng.py

    routing_policy = config.get('routing_policy', {})
+    nitpicky = config.get('routing_nitpicky', False)
+    return utils.validate_policy(


An idea would be a return any(...) over all the SANs.

puiterwijk · 2017-06-28T18:49:49Z

fedmsg/tests/crypto/test_x509.py

+            'ca_cert_cache': os.path.join(SSLDIR, 'ca.crt'),
+            'ca_cert_cache_expiry': 1497618475,  # Stop fedmsg overwriting my CA, See Issue 420
+
+            'crl_location': "http://threebean.org/fedmsg-tests/crl.pem",


Ideally we would not depend on this website... Maybe use a file:// ?

Most unfortunately, the way the code currently works it doesn't support file://. I set the cache expiry to be sometime in the mid 2030s so it should never actually try to fetch from that URL. I plan to re-write all the cert fetching code in the near future.

Signed-off-by: Jeremy Cline <[email protected]>

jeremycline requested a review from puiterwijk June 16, 2017 15:39

jeremycline added a commit to jeremycline/fedmsg that referenced this pull request Jun 16, 2017

PEP-8 nearly all of fedmsg

3cb2871

This PEP-8s nearly all of fedmsg except for the stuff touched by fedora-infra#421. Signed-off-by: Jeremy Cline <[email protected]>

jeremycline mentioned this pull request Jun 16, 2017

PEP-8 nearly all of fedmsg #422

Merged

jeremycline added a commit to jeremycline/fedmsg that referenced this pull request Jun 16, 2017

PEP-8 nearly all of fedmsg

7fe5111

This PEP-8s nearly all of fedmsg except for the stuff touched by fedora-infra#421. Signed-off-by: Jeremy Cline <[email protected]>

bowlofeggs approved these changes Jun 17, 2017

View reviewed changes

jeremycline added 5 commits June 19, 2017 09:38

Start linting the crypto package

bdca6d9

Signed-off-by: Jeremy Cline <[email protected]>

Add docblock load_remote_cert

1de335b

Signed-off-by: Jeremy Cline <[email protected]>

Add a test for signatures by expired CAs

9ba315f

Thanks bowlofeggs for the suggestion! Signed-off-by: Jeremy Cline <[email protected]>

jeremycline force-pushed the crypto-fallback branch from 22a78f5 to 9ba315f Compare June 19, 2017 16:13

jeremycline requested a review from ralphbean June 19, 2017 16:34

glensc mentioned this pull request Jun 21, 2017

fedmsg-tail pretty args not pretty at all #429

Open

puiterwijk approved these changes Jun 28, 2017

View reviewed changes

puiterwijk added the crypto label Jun 28, 2017

Mark load_remote_cert as private

0c7aee2

Signed-off-by: Jeremy Cline <[email protected]>

jeremycline merged commit f530afb into fedora-infra:develop Jun 30, 2017

jeremycline deleted the crypto-fallback branch June 30, 2017 13:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use cryptography and pyOpenSSL instead of M2Crypto #421

Use cryptography and pyOpenSSL instead of M2Crypto #421

jeremycline commented Jun 16, 2017

codecov-io commented Jun 16, 2017 •

edited

Loading

bowlofeggs left a comment

bowlofeggs Jun 17, 2017

bowlofeggs Jun 17, 2017

bowlofeggs Jun 17, 2017

bowlofeggs Jun 17, 2017

bowlofeggs Jun 17, 2017

bowlofeggs Jun 17, 2017

jeremycline Jun 19, 2017

bowlofeggs Jun 17, 2017

bowlofeggs Jun 17, 2017

jeremycline Jun 19, 2017

bowlofeggs Jun 17, 2017

bowlofeggs Jun 17, 2017

jeremycline Jun 19, 2017

puiterwijk left a comment

puiterwijk Jun 28, 2017

puiterwijk Jun 28, 2017

puiterwijk Jun 28, 2017

jeremycline Jun 28, 2017

puiterwijk Jun 28, 2017

puiterwijk Jun 28, 2017

jeremycline Jun 28, 2017

puiterwijk Jun 28, 2017 •

edited

Loading

puiterwijk Jun 28, 2017

puiterwijk Jun 28, 2017

jeremycline Jun 28, 2017

		@@ -0,0 +1,216 @@
		# This file is part of fedmsg.
		# Copyright (C) 2012 - 2014 Red Hat, Inc.



		@skipIf(not (_m2crypto or _cryptography), "Neither M2Crypto nor Cryptography available")
		class X509BaseTests(TestCase):


		Return the local filename.

		.. note:: This is not a public API and is subject to change.

Use cryptography and pyOpenSSL instead of M2Crypto #421

Use cryptography and pyOpenSSL instead of M2Crypto #421

Conversation

jeremycline commented Jun 16, 2017

codecov-io commented Jun 16, 2017 • edited Loading

Codecov Report

bowlofeggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puiterwijk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puiterwijk Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 16, 2017 •

edited

Loading

puiterwijk Jun 28, 2017 •

edited

Loading