factor out function to dump crl #368

ddcc · 2015-10-13T17:51:48Z

No description provided.

reaperhulk · 2015-10-13T17:58:15Z

OpenSSL/crypto.py

+            "FILETYPE_TEXT")
+
+    assert ret == 1
+    return _bio_to_string(bio)



Two blank lines here for PEP8.

reaperhulk · 2015-10-13T17:58:21Z

OpenSSL/crypto.py

@@ -2576,6 +2560,30 @@ def verify(cert, signature, data, digest):
    if verify_result != 1:
        _raise_current_error()



Two blank lines here for PEP8.

codecov-io · 2015-10-13T18:22:25Z

Current coverage is `93.16%`

Merging #368 into master will increase coverage by +0.05% as of 5b0f479

@@            master    #368   diff @@
======================================
  Files           14      14       
  Stmts         5347    5353     +6
  Branches       461     460     -1
  Methods          0       0       
======================================
+ Hit           4979    4987     +8
+ Partial        144     143     -1
+ Missed         224     223     -1

Review entire Coverage Diff as of 5b0f479

Powered by Codecov. Updated on successful CI builds.

reaperhulk · 2015-10-13T21:59:21Z

So what's the goal of refactoring this out? Is it the intent to have dump_crl be part of the public API? If so, this needs docs (and a changelog entry).

Otherwise the core code looks fine to me, but I don't have any opinions on whether this should be a public API or not.

ddcc · 2015-10-14T00:29:52Z

Yeah, I'd like to add it as part of the API, since most other objects support both loading and dumping. For my use case, I'd also like to be able to export CRL's that are within PKCS#7 data.

hynek · 2015-10-16T09:31:45Z

OpenSSL/test/test_crypto.py

+        """
+        crl = load_crl(FILETYPE_PEM, crlData)
+        buf = dump_crl(FILETYPE_PEM, crl)
+        self.assertEqual(crlData, buf)


please assert crlData == buf

hynek · 2015-10-16T09:32:43Z

doc/api/crypto.rst

@@ -90,6 +90,11 @@ Private keys
 Certificate revocation lists
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

+.. py:function:: dump_crl(type, crl)


please use autodoc here. currently the docstring is more verbose than the documentation. :)

ddcc · 2015-10-18T00:20:23Z

@hynek Ok, I think I have made the changes you mentioned. Never used autodoc before, so let me know if it needs to be changed.

hynek · 2015-10-18T05:55:35Z

Looks good

just:

add respective data types (buffer is not a data type…) using Sphinx directives
surround the constants with double ` to make them fixed width
the Returns string doesn’t have to be that verbose
finish sentences with a period. :)

bonus points if you do it also to the remaining three CRL-related functions (completely optional).

thanks!

hynek · 2015-10-18T15:36:33Z

OpenSSL/test/test_crypto.py

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

+    def test_dump_crl(self):
+        """
+        Dump a known CRL and ensure it is output correctly.


https://jml.io/pages/test-docstrings.html Please

hynek · 2015-10-21T10:55:51Z

(please note that I didn’t see that you’ve added commits; please bump the ticket once you feel that your branch is ready for review)

Getting closer!

please rebase
Check that the dumped CRL matches the original input. should be just The dumped CRL matches the original input.

ddcc · 2015-10-21T14:05:37Z

Ok, done.

hynek · 2015-10-21T15:14:26Z

The failures are unrelated. thanks!

Changes: 16.0.0 (2016-03-19) ------------------- This is the first release under full stewardship of PyCA. We have made *many* changes to make local development more pleasing. The test suite now passes both on Linux and OS X with OpenSSL 0.9.8, 1.0.1, and 1.0.2. It has been moved to `py.test <https://pytest.org/>`_, all CI test runs are part of `tox <https://testrun.org/tox/>`_ and the source code has been made fully `flake8 <https://flake8.readthedocs.org/>`_ compliant. We hope to have lowered the barrier for contributions significantly but are open to hear about any remaining frustrations. Backward-incompatible changes: ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - Python 3.2 support has been dropped. It never had significant real world usage and has been dropped by our main dependency ``cryptography``. Affected users should upgrade to Python 3.3 or later. Deprecations: ^^^^^^^^^^^^^ - The support for EGD has been removed. The only affected function ``OpenSSL.rand.egd()`` now uses ``os.urandom()`` to seed the internal PRNG instead. Please see `pyca/cryptography#1636 <https://github.com/pyca/cryptography/pull/1636>`_ for more background information on this decision. In accordance with our backward compatibility policy ``OpenSSL.rand.egd()`` will be *removed* no sooner than a year from the release of 16.0.0. Please note that you should `use urandom <http://sockpuppet.org/blog/2014/02/25/safely-generate-random-numbers/>`_ for all your secure random number needs. - Python 2.6 support has been deprecated. Our main dependency ``cryptography`` deprecated 2.6 in version 0.9 (2015-05-14) with no time table for actually dropping it. pyOpenSSL will drop Python 2.6 support once ``cryptography`` does. Changes: ^^^^^^^^ - Fixed ``OpenSSL.SSL.Context.set_session_id``, ``OpenSSL.SSL.Connection.renegotiate``, ``OpenSSL.SSL.Connection.renegotiate_pending``, and ``OpenSSL.SSL.Context.load_client_ca``. They were lacking an implementation since 0.14. `#422 <https://github.com/pyca/pyopenssl/pull/422>`_ - Fixed segmentation fault when using keys larger than 4096-bit to sign data. `#428 <https://github.com/pyca/pyopenssl/pull/428>`_ - Fixed ``AttributeError`` when ``OpenSSL.SSL.Connection.get_app_data()`` was called before setting any app data. `#304 <https://github.com/pyca/pyopenssl/pull/304>`_ - Added ``OpenSSL.crypto.dump_publickey()`` to dump ``OpenSSL.crypto.PKey`` objects that represent public keys, and ``OpenSSL.crypto.load_publickey()`` to load such objects from serialized representations. `#382 <https://github.com/pyca/pyopenssl/pull/382>`_ - Added ``OpenSSL.crypto.dump_crl()`` to dump a certificate revocation list out to a string buffer. `#368 <https://github.com/pyca/pyopenssl/pull/368>`_ - Added ``OpenSSL.SSL.Connection.get_state_string()`` using the OpenSSL binding ``state_string_long``. `#358 <https://github.com/pyca/pyopenssl/pull/358>`_ - Added support for the ``socket.MSG_PEEK`` flag to ``OpenSSL.SSL.Connection.recv()`` and ``OpenSSL.SSL.Connection.recv_into()``. `#294 <https://github.com/pyca/pyopenssl/pull/294>`_ - Added ``OpenSSL.SSL.Connection.get_protocol_version()`` and ``OpenSSL.SSL.Connection.get_protocol_version_name()``. `#244 <https://github.com/pyca/pyopenssl/pull/244>`_ - Switched to ``utf8string`` mask by default. OpenSSL formerly defaulted to a ``T61String`` if there were UTF-8 characters present. This was changed to default to ``UTF8String`` in the config around 2005, but the actual code didn't change it until late last year. This will default us to the setting that actually works. To revert this you can call ``OpenSSL.crypto._lib.ASN1_STRING_set_default_mask_asc(b"default")``. `#234 <https://github.com/pyca/pyopenssl/pull/234>`_

reaperhulk reviewed Oct 13, 2015
View changes

OpenSSL/crypto.py Outdated

"FILETYPE_TEXT")

assert ret == 1

return _bio_to_string(bio)

This comment has been minimized.

Sign in to view

reaperhulk Oct 13, 2015
Member

Two blank lines here for PEP8.

reaperhulk reviewed Oct 13, 2015
View changes

OpenSSL/crypto.py Outdated

@@ -2576,6 +2560,30 @@ def verify(cert, signature, data, digest):

if verify_result != 1:

_raise_current_error()

This comment has been minimized.

Sign in to view

reaperhulk Oct 13, 2015
Member

Two blank lines here for PEP8.

ddcc force-pushed the ddcc:crl branch from aedbfa1 to f1554c7 Oct 13, 2015

ddcc force-pushed the ddcc:crl branch from ccd33f7 to 9dc1952 Oct 14, 2015

hynek reviewed Oct 16, 2015
View changes

OpenSSL/test/test_crypto.py Outdated

"""

crl = load_crl(FILETYPE_PEM, crlData)

buf = dump_crl(FILETYPE_PEM, crl)

self.assertEqual(crlData, buf)

This comment has been minimized.

Sign in to view

hynek Oct 16, 2015
Contributor

please assert crlData == buf

hynek reviewed Oct 16, 2015
View changes

ddcc force-pushed the ddcc:crl branch 2 times, most recently from 74a08ff to cca0281 Oct 18, 2015

hynek reviewed Oct 18, 2015
View changes

ddcc force-pushed the ddcc:crl branch 2 times, most recently from b557d5e to aa1a0de Oct 18, 2015

ddcc added 2 commits Oct 13, 2015

factor out function to dump crl

7a7a3e9

fix typo in previous changelog entry

Loading status checks…

c14e2ca

ddcc force-pushed the ddcc:crl branch from aa1a0de to c14e2ca Oct 21, 2015

hynek closed this in 6c5d849 Oct 21, 2015

ddcc deleted the ddcc:crl branch Nov 3, 2015

github-actions bot locked as resolved and limited conversation to collaborators Aug 21, 2020

pyca / pyopenssl

factor out function to dump crl #368

factor out function to dump crl #368

ddcc commented Oct 13, 2015

This comment has been minimized.

This comment has been minimized.

codecov-io commented Oct 13, 2015

reaperhulk commented Oct 13, 2015

ddcc commented Oct 14, 2015

This comment has been minimized.

This comment has been minimized.

ddcc commented Oct 18, 2015

hynek commented Oct 18, 2015

This comment has been minimized.

hynek commented Oct 21, 2015

ddcc commented Oct 21, 2015

hynek commented Oct 21, 2015

		@@ -2576,6 +2560,30 @@ def verify(cert, signature, data, digest):
		if verify_result != 1:
		_raise_current_error()

pyca / pyopenssl

factor out function to dump crl #368

factor out function to dump crl #368

Conversation

ddcc commented Oct 13, 2015

This comment has been minimized.

reaperhulk Oct 13, 2015 Member

This comment has been minimized.

reaperhulk Oct 13, 2015 Member

codecov-io commented Oct 13, 2015

Current coverage is 93.16%

reaperhulk commented Oct 13, 2015

ddcc commented Oct 14, 2015

This comment has been minimized.

hynek Oct 16, 2015 Contributor

This comment has been minimized.

hynek Oct 16, 2015 Contributor

ddcc commented Oct 18, 2015

hynek commented Oct 18, 2015

This comment has been minimized.

hynek Oct 18, 2015 Contributor

hynek commented Oct 21, 2015

ddcc commented Oct 21, 2015

hynek commented Oct 21, 2015

reaperhulk Oct 13, 2015
Member

reaperhulk Oct 13, 2015
Member

Current coverage is `93.16%`

hynek Oct 16, 2015
Contributor

hynek Oct 16, 2015
Contributor

hynek Oct 18, 2015
Contributor