Enable use of CRL (and more) in verify context. #483

dsully · 2016-06-04T01:22:23Z

This is a rebased change from #281 by @sholsapp, which changes by me to be _openssl_assert() compatible.

A local py.test --cov looks good, and all tests pass.

codecov-io · 2016-06-04T01:37:00Z

Current coverage is 88.36%

Merging #483 into master will increase coverage by 0.19%

@@             master       #483   diff @@
==========================================
  Files             7          7          
  Lines          2059       2096    +37   
  Methods           0          0          
  Messages          0          0          
  Branches        367        367          
==========================================
+ Hits           1815       1852    +37   
  Misses          130        130          
  Partials        114        114

Powered by Codecov. Last updated by 40d448f...f38ea82

hynek · 2016-06-04T18:11:15Z

src/OpenSSL/crypto.py

+
+    An X.509 store, being only a description, cannot be used by itself to
+    verify a certificate. To carry out the actual verification process, see
+    :py:class:`X509StoreContext`.


no :py: prefixes in new code please

How would you prefer this change? Just to "X509StoreContext" ?

Just

:class:`X509StoreContext`

unless i’m missing something?

hynek · 2016-06-04T18:12:11Z

src/OpenSSL/crypto.py

+    ALLOW_PROXY_CERTS = _lib.X509_V_FLAG_ALLOW_PROXY_CERTS
+    POLICY_CHECK = _lib.X509_V_FLAG_POLICY_CHECK
+    EXPLICIT_POLICY = _lib.X509_V_FLAG_EXPLICIT_POLICY
+    # FLAG_INHIBIT_ANY = _lib.X509_V_FLAG_FLAG_INHIBIT_ANY


what are these commented out lines about?

Please see the original diff, #281 - I can remove them if you'd like.

I added a comment from the #281 thread.

after talking to Paul: kill the commented out and also the comment above. if users ask for it we can still add them conditionally (i.e. getattr() style).

hynek · 2016-06-04T18:13:59Z

src/OpenSSL/crypto.py

@@ -1452,6 +1477,46 @@ def add_cert(self, cert):
        if not result:
            _raise_current_error()


since you’re touching the code around here you could _openssl_assert-ize this as well :)

hynek · 2016-06-04T18:14:41Z

src/OpenSSL/crypto.py

+        the associated flags are configured to check certificate revocation
+        lists.
+
+        .. versionadded:: 0.17


there is no such thing as pyOpenSSL 0.17. The next release will be 16.1.0.

hynek · 2016-06-04T18:15:20Z

src/OpenSSL/crypto.py

+        .. versionadded:: 0.17
+
+        :param CRL crl: The certificate revocation list to add to this store.
+        :return: :py:data:`None` if the certificate revocation list was added


just

``None``

is fine

hynek · 2016-06-04T18:18:08Z

src/OpenSSL/crypto.py

+          suitable CRL must be added to the store otherwise an error will be
+          raised.
+
+        .. versionadded:: 0.17


there is no such thing as pyOpenSSL 0.17. The next release will be 16.1.0.

hynek · 2016-06-04T18:18:35Z

src/OpenSSL/crypto.py

+        .. versionadded:: 0.17
+
+        :param int flags: The verification flags to set on this store.
+        :return: :py:data:`None` if the verification flags were


just

``None``

is fine

hynek · 2016-06-04T18:18:54Z

src/OpenSSL/crypto.py

-      verification parameters and certificate revocation lists.
+    An X.509 store context is used to carry out the actual verification process
+    of a certificate in a described context. For describing such a context, see
+    :py:class:`X509Store`.


no :py: prefixes in new code please

hynek · 2016-06-04T18:20:00Z

src/OpenSSL/crypto.py


-        :param cert: The certificate used to sign the CRL.
-        :type cert: :py:class:`X509`
+        .. versionadded:: 0.17


there is no such thing as pyOpenSSL 0.17. The next release will be 16.1.0.

hynek · 2016-06-04T18:20:29Z

src/OpenSSL/crypto.py


-        :param key: The key used to sign the CRL.
-        :type key: :py:class:`PKey`
+        :return: :py:class:`X509Name`


no :py: prefixes in new code please

hynek · 2016-06-04T18:22:05Z

src/OpenSSL/crypto.py


-        :param int days: The number of days until the next update of this CRL.
+        .. versionadded:: 0.17


there is no such thing as pyOpenSSL 0.17. The next release will be 16.1.0.

hynek · 2016-06-04T18:22:23Z

src/OpenSSL/crypto.py

+        .. versionadded:: 0.17
+
+        :param int version: The version of the CRL.
+        :return: :py:const:`None`


just

``None``

is fine

hynek · 2016-06-04T18:22:38Z

src/OpenSSL/crypto.py

+            YYYYMMDDhhmmss+hhmm
+            YYYYMMDDhhmmss-hhmm
+
+        .. versionadded:: 0.17


there is no such thing as pyOpenSSL 0.17. The next release will be 16.1.0.

hynek · 2016-06-04T18:22:47Z

src/OpenSSL/crypto.py

+        .. versionadded:: 0.17
+
+        :param bytes when: A timestamp string.
+        :return: :py:const:`None`


just

``None``

is fine

hynek · 2016-06-04T18:23:13Z

src/OpenSSL/crypto.py

+            YYYYMMDDhhmmss+hhmm
+            YYYYMMDDhhmmss-hhmm
+
+        .. versionadded:: 0.17


there is no such thing as pyOpenSSL 0.17. The next release will be 16.1.0.

hynek · 2016-06-04T18:23:27Z

src/OpenSSL/crypto.py

+        .. versionadded:: 0.17
+
+        :param bytes when: A timestamp string.
+        :return: :py:const:`None`


just

``None``

is fine

hynek · 2016-06-04T18:24:48Z

src/OpenSSL/crypto.py

+        This method implicitly sets the issuer's name based on the issuer
+        certificate and private key used to sign the CRL.
+
+        .. versionadded:: 0.17


there is no such thing as pyOpenSSL 0.17. The next release will be 16.1.0.

hynek · 2016-06-04T18:26:09Z

src/OpenSSL/crypto.py

        :param bytes digest: The name of the message digest to use (eg
            ``b"sha1"``).
-
        :return: :py:data:`bytes`


please make this an

:rtype: bytes

and it’s fine to have an empty line between the params and the return docs.

hynek · 2016-06-04T18:26:40Z

src/OpenSSL/crypto.py

        :return: :py:data:`bytes`
        """
+
+        # TODO: fix this function to use functionality added in version 0.16.


i’m not sure what this means? also there’s no 0.16.

hynek · 2016-06-04T18:28:08Z

tests/test_crypto.py

@@ -3389,6 +3398,14 @@ def test_load_crl_bad_data(self):
        """
        self.assertRaises(Error, load_crl, FILETYPE_PEM, b"hello, world")

+    def test_get_issuer(self):
+        """
+        Load a known CRL and inspect its issuer's common name.


Please write test docstrings as what the expected behavior that you are testing, not about what you do: https://jml.io/pages/test-docstrings.html

hynek · 2016-06-04T18:33:03Z

tests/test_crypto.py

+        Create a CRL.
+
+        :param list[X509] certs: A list of certificates to revoke.
+        :return: :py:class:`CRL`


no :py: prefixes in new code please

hynek · 2016-06-04T18:34:16Z

tests/test_crypto.py

+
+    def test_verify_with_revoked(self):
+        """
+        :py:obj:`verify_certificate` raises error when an intermediate


that should be a :func:, no?

hynek · 2016-06-04T18:36:20Z

tests/test_crypto.py

+
+    def test_verify_with_missing_crl(self):
+        """
+        :py:obj:`verify_certificate` raises error when an intermediate


:func: again, although it’s okay to not markup function names in tests at all. it’s not like we run autodoc on it…

hynek · 2016-06-04T18:37:04Z

tests/test_crypto.py


 class X509StoreContextTests(TestCase):
    """
    Tests for :py:obj:`OpenSSL.crypto.X509StoreContext`.
    """
    root_cert = load_certificate(FILETYPE_PEM, root_cert_pem)
+    root_key = load_privatekey(FILETYPE_PEM, root_key_pem)


it doesn’t seem like you use these attributes?

reaperhulk · 2016-06-04T19:40:14Z

src/OpenSSL/crypto.py

@@ -1902,9 +1955,6 @@ class CRL(object):
    """

    def __init__(self):
-        """
-        Create a new empty certificate revocation list.
-        """


I suspect @hynek would prefer this be left in?

meh, can die

Ok, removing.

reaperhulk · 2016-06-04T19:51:19Z

src/OpenSSL/crypto.py

+        """
+        return self._set_boundary_time(_lib.X509_CRL_get_nextUpdate, when)
+
+    def sign(self, issuer_cert, issuer_key, digest='sha1'):


Since this is a new API and there's no backwards compatibility concern I'd prefer this to not have a default arg for digest.

hynek · 2016-06-04T23:32:45Z

doc/api/crypto.rst

+--------------------
+
+.. autoclass:: X509StoreFlags
+               :members:


this sadly doesn’t seem to work :'( gotta enumerate them I’m afraid?

hynek · 2016-06-04T23:35:34Z

src/OpenSSL/crypto.py

@@ -1425,9 +1426,35 @@ def get_extension(self, index):
 X509Type = X509


+class X509StoreFlags(Enum):
+    """ Flags for X509 verification


""" please on single lines

more explanation please?

crosslink with set_flags (and backlink too)

add something like “See the OpenSSL docs for the meaning.” and link some words to that URL

dsully · 2016-06-05T00:01:30Z

That Travis failure appears to be unrelated & flakey, as it's doing a time based check.

hynek · 2016-06-05T00:13:33Z

tests/test_crypto.py

+        store.add_crl(root_crl)
+        store.add_crl(intermediate_crl)
+        store.set_flags(
+            X509StoreFlags.CRL_CHECK.value |


ok this is gross, I’m sorry. make X509Store flags a regular class. (just subclass object, nothing else special). none of the three of us thought about this .value crap. sorry again.

Yeaah... that's the worst part about Enum. I think everyone wishes it wasn't that way.

hynek · 2016-06-05T00:27:29Z

src/OpenSSL/crypto.py

@@ -1890,7 +1946,7 @@ def get_rev_date(self):
        Get the revocation timestamp.

        :return: The timestamp of the revocation, as ASN.1 GENERALIZEDTIME.
-        :rtype: :py:class:`bytes`
+        :rtype: :class:`bytes`


just “bytes” is fine for rtype

hynek · 2016-06-05T00:29:28Z

Almost there! Since you did find/replace: could you get rid of all the

:const:`None`

in favor of

``None``

?

I have some more smaller things, but I don’t want to torture you any further. :)

hynek · 2016-06-05T00:38:26Z

src/OpenSSL/crypto.py

+        _openssl_assert(_lib.X509_STORE_add_crl(self._store, crl._crl) != 0)
+
+    def set_flags(self, flags):
+        """


this docstring needs to point to the new flags class in some sane manner. Maybe within :param int flags?

hynek · 2016-06-05T00:39:11Z

src/OpenSSL/crypto.py

    :param X509 certificate: The certificate to be verified.
+


no empty line please

hynek · 2016-06-05T00:43:15Z

Actually I know! :) We need to sort out the TODO comment. I will merge once it’s resolved. :)

dsully · 2016-06-05T00:45:35Z

@hynek - I removed the TODO comment, as @sholsapp originally added it, and I can't get ahold of him right now to determine what he meant. I'll let him address it in a later PR. Does that work?

hynek · 2016-06-05T00:49:45Z

you haven’t pushed and yep wfm

hynek · 2016-06-05T00:56:44Z

src/OpenSSL/crypto.py

+
+        :param X509 issuer_cert: The issuer's certificate.
+        :param PKey issuer_key: The issuer's private key.
+        :param str digest: The digest method to sign the CRL with.


ok sorry one last one: we don't do str in pyOpenSSL. This has to be bytes.

hynek · 2016-06-05T01:05:34Z

🍻

hynek · 2016-06-05T01:06:24Z

Thank you so much to everyone for their patience. pyOpenSSL is slightly less terrible once again!

sholsapp · 2016-06-06T16:43:29Z

Awesomeee! :) 🍻

Add patch that makes tests on NetBSD progress further. But then there's a segfault. See pyca/pyopenssl#596 16.2.0 (2016-10-15) ------------------- Changes: ^^^^^^^^ - Fixed compatibility errors with OpenSSL 1.1.0. - Fixed an issue that caused failures with subinterpreters and embedded Pythons. `#552 <https://github.com/pyca/pyopenssl/pull/552>`_ 16.1.0 (2016-08-26) ------------------- Deprecations: ^^^^^^^^^^^^^ - Dropped support for OpenSSL 0.9.8. Changes: ^^^^^^^^ - Fix memory leak in ``OpenSSL.crypto.dump_privatekey()`` with ``FILETYPE_TEXT``. `#496 <https://github.com/pyca/pyopenssl/pull/496>`_ - Enable use of CRL (and more) in verify context. `#483 <https://github.com/pyca/pyopenssl/pull/483>`_ - ``OpenSSL.crypto.PKey`` can now be constructed from ``cryptography`` objects and also exported as such. `#439 <https://github.com/pyca/pyopenssl/pull/439>`_ - Support newer versions of ``cryptography`` which use opaque structs for OpenSSL 1.1.0 compatibility.

sholsapp and others added 2 commits Jun 4, 2016

Enable use of CRL (and more) in verify context.

Loading status checks…

f38ea82

flake8 fixes.

Loading status checks…

45d3447

dsully mentioned this pull request Jun 4, 2016

Enable use of CRL (and more) in verify context #281

Closed