New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the ability to set a custom verification time on X509Store #567

Merged
merged 13 commits into from Nov 22, 2016

Conversation

Projects
None yet
4 participants
@tsileo
Contributor

tsileo commented Oct 24, 2016

This PR add a set_time methods on OpenSSL.crypto.X509Store.

This new methods:

  • create a X509_VERIFY_PARAM
  • "set" the given time (a datetime.datetime instance)
  • and finally "set" the param on the X509Store

One things I'm worried about is if the X509_VERIFY_PARAM should be "gc"ed manually? or it will be cleaned when the X509Store will be "gc"ed?

I chose to implement the method on X509Store because it's more convenient to set the time only once on the store, and then verify multiple certificates (instead of having to set it in each X509StoreContext).

Thanks!

@codecov-io

This comment has been minimized.

codecov-io commented Oct 24, 2016

Current coverage is 95.65% (diff: 100%)

Merging #567 into master will increase coverage by 0.18%

@@             master       #567   diff @@
==========================================
  Files            16         16          
  Lines          5610       5614     +4   
  Methods           0          0          
  Messages          0          0          
  Branches        408        403     -5   
==========================================
+ Hits           5356       5370    +14   
+ Misses          176        167     -9   
+ Partials         78         77     -1   

Powered by Codecov. Last update 67666e2...a0bdd7a

tsileo added some commits Oct 24, 2016

:param datetime vfy_time: The verification time to set on this store.
:return: ``None`` if the verification time were successfully set.
"""
param = _lib.X509_VERIFY_PARAM_new()

This comment has been minimized.

@reaperhulk

reaperhulk Nov 6, 2016

Member

You'll need to add a param = _ffi.gc(param, X509_VERIFY_PARAM_free) to this. X509_STORE_set1_param increfs it so without a corresponding free to decref after creation it will leak.

This comment has been minimized.

@tsileo

tsileo Nov 6, 2016

Contributor

Thanks for your feedback!

When I try:

from OpenSSL._util import lib
lib.X509_VERIFY_PARAM_free

I got this exception on my system: AttributeError: 'module' object has no attribute 'X509_VERIFY_PARAM_free'

It looks like X509_VERIFY_PARAM_free is not exported, I should do a PR on cryptography to export it? or something is wrong on my side?

Thanks!

This comment has been minimized.

@reaperhulk

reaperhulk Nov 6, 2016

Member

Nope, you're quite right it isn't currently exported in cryptography. Please do submit a PR there!

@tsileo

This comment has been minimized.

Contributor

tsileo commented Nov 6, 2016

I've made the change, I'll push once the cryptography PR has been merged (to make the CI works).

@reaperhulk

This comment has been minimized.

Member

reaperhulk commented Nov 6, 2016

We won't be able to merge this until after we do a cryptography release with the new binding (and this PR will then need to be updated to reflect the new minimum version required), but now we can see if it works against cryptography master :)

@reaperhulk

This comment has been minimized.

Member

reaperhulk commented Nov 6, 2016

I am unfamiliar with the state of docs for X509Store, does this function need to be documented outside of the docstring @hynek?

.. versionadded: 16.3.0
:param datetime vfy_time: The verification time to set on this store.
:return: ``None`` if the verification time were successfully set.

This comment has been minimized.

@reaperhulk

reaperhulk Nov 6, 2016

Member

s/were/was

@hynek

This comment has been minimized.

Contributor

hynek commented Nov 6, 2016

On phone but it's gonna need an automethod line somewhere.

@tsileo

This comment has been minimized.

Contributor

tsileo commented Nov 6, 2016

@reaperhulk no problem I get it. I'll follow the "Sixteenth Release" milestone of cryptography.

Let me know what changes are needed for the doc, I checked doc/api/crypto.rst and with autoclass:: X509Store and :members: I though it would be OK with doc, unless it need to be displayed somewhere else.

@tsileo

This comment has been minimized.

Contributor

tsileo commented Nov 6, 2016

The build fails because it's using cryptography==1.5.3, it passes locally with cryptography master.

I will wait till the new cryptography version is released.

@reaperhulk

This comment has been minimized.

Member

reaperhulk commented Nov 22, 2016

@tsileo cryptography 1.6 is now out :)

@tsileo

This comment has been minimized.

Contributor

tsileo commented Nov 22, 2016

Thanks for the heads-up!

How should I update the doc to reflect the new minimum version required for cryptography? I can't find any example.

And I think it should be OK for the doc due to the autoclass::X509Store but let me know.

Any other change I need to make?

@reaperhulk

This comment has been minimized.

Member

reaperhulk commented Nov 22, 2016

Probably something like "bumped minimum cryptography version to 1.6", but hynek may have a preference. Technically this PR is fine @hynek so if you've got any stylistic nits speak now :)

@hynek

This comment has been minimized.

Contributor

hynek commented Nov 22, 2016

Before we get into stylistic questions, the new minimum version has to be applied to setup.py/tox.ini.

@tsileo

This comment has been minimized.

Contributor

tsileo commented Nov 22, 2016

Done! Let me know if there's something else I need to do :)

tox.ini Outdated
@@ -9,7 +9,7 @@ deps =
coverage>=4.2
pytest>=3.0.1
cryptographyMaster: git+https://github.com/pyca/cryptography.git
cryptographyMinimum: cryptography<1.4
cryptographyMinimum: cryptography==1.6

This comment has been minimized.

@hynek

hynek Nov 22, 2016

Contributor

This got to be <1.7 so we always test against the latest 1.6 release.

@@ -1518,6 +1518,20 @@ def set_flags(self, flags):
"""
_openssl_assert(_lib.X509_STORE_set_flags(self._store, flags) != 0)
def set_time(self, vfy_time):
"""Set verification time to this store.

This comment has been minimized.

@hynek

hynek Nov 22, 2016

Contributor

""" on own lines please.

@@ -3740,6 +3740,27 @@ def test_modification_pre_verify(self):
store_ctx.set_store(store_good)
self.assertEqual(store_ctx.verify_certificate(), None)
def test_verify_with_time(self):
"""
:func:`verify_certificate` raises error when the verification time is

This comment has been minimized.

@hynek

hynek Nov 22, 2016

Contributor

I know the code base is inconsistent in this regard, but please don't use type prefixes like :func: in test doc strings. They are never rendered and there’s no way to verify them at all.

@hynek

This comment has been minimized.

Contributor

hynek commented Nov 22, 2016

As for documenting minimum cryptography version I’m not sure either. Until now, we’ve never really documented it in the CHANGELOG…

@tsileo

This comment has been minimized.

Contributor

tsileo commented Nov 22, 2016

@hynek I just pushed the style tweaks and updated the minimum cryptography requirements.

@@ -1514,10 +1514,24 @@ def set_flags(self, flags):
:param int flags: The verification flags to set on this store.
See :class:`X509StoreFlags` for available constants.
:return: ``None`` if the verification flags were successfully set.

This comment has been minimized.

@hynek

hynek Nov 22, 2016

Contributor

heh you removed the wrong one but I see the qualification is everywhere in this class so just re-add this one and let the other be for now

This comment has been minimized.

@tsileo

tsileo Nov 22, 2016

Contributor

Oops, I was checking if it was present in other methods. I just re-added it.

@@ -1518,6 +1518,21 @@ def set_flags(self, flags):
"""
_openssl_assert(_lib.X509_STORE_set_flags(self._store, flags) != 0)
def set_time(self, vfy_time):
"""
Set verification time to this store.

This comment has been minimized.

@hynek

hynek Nov 22, 2016

Contributor

Final thing: could you add a few more lines about the what that method is good for? Is that grammar even correct? Honest question. :)

This comment has been minimized.

@tsileo

tsileo Nov 22, 2016

Contributor

No problem. Not sure it was correct. It's updated.

@hynek

hynek approved these changes Nov 22, 2016

@hynek

This comment has been minimized.

Contributor

hynek commented Nov 22, 2016

flake8 is failing

@tsileo

This comment has been minimized.

Contributor

tsileo commented Nov 22, 2016

Sorry about that, I fixed the line too long (my config is set at 120 :p)

seems resolved

@hynek hynek merged commit e15e60a into pyca:master Nov 22, 2016

3 checks passed

codecov/patch 100% of diff hit (target 95.47%)
Details
codecov/project 95.65% (+0.18%) compared to 67666e2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hynek

This comment has been minimized.

Contributor

hynek commented Nov 22, 2016

thanks!

aszlig added a commit to NixOS/nixpkgs that referenced this pull request Jun 21, 2017

pyopenssl: 16.2.0 -> 17.0.0 and fix tests
Upstream changes:

 * Added OpenSSL.X509Store.set_time() to set a custom verification time
   when verifying certificate chains. pyca/pyopenssl#567
 * Added a collection of functions for working with OCSP stapling. None
   of these functions make it possible to validate OCSP assertions, only
   to staple them into the handshake and to retrieve the stapled
   assertion if provided. Users will need to write their own code to
   handle OCSP assertions. We specifically added:
   Context.set_ocsp_server_callback, Context.set_ocsp_client_callback,
   and Connection.request_ocsp. pyca/pyopenssl#580
 * Changed the SSL module's memory allocation policy to avoid zeroing
   memory it allocates when unnecessary. This reduces CPU usage and
   memory allocation time by an amount proportional to the size of the
   allocation. For applications that process a lot of TLS data or that
   use very lage allocations this can provide considerable performance
   improvements. pyca/pyopenssl#578
 * Automatically set SSL_CTX_set_ecdh_auto() on OpenSSL.SSL.Context.
   pyca/pyopenssl#575
 * Fix empty exceptions from OpenSSL.crypto.load_privatekey().
   pyca/pyopenssl#581

The full upstream changelog can be found at:

https://pyopenssl.readthedocs.io/en/17.0.0/changelog.html

I've also added a patch from pyca/pyopenssl#637 in order to fix the
tests, which was the main reason for the version bump because that patch
won't apply for 16.2.0.

According to the upstream changelog there should be no
backwards-incompatible changes, but I've tested building against some of
the packages depending on pyopenssl anyway. Regardless of this, the
build for pyopenssl fails right now anyway, so the worst that could
happen via this commit would be that we break something that's already
broken.

Signed-off-by: aszlig <aszlig@redmoonstudios.org>

peterhoeg added a commit to peterhoeg/nixpkgs that referenced this pull request Jun 22, 2017

pyopenssl: 16.2.0 -> 17.0.0 and fix tests
Upstream changes:

 * Added OpenSSL.X509Store.set_time() to set a custom verification time
   when verifying certificate chains. pyca/pyopenssl#567
 * Added a collection of functions for working with OCSP stapling. None
   of these functions make it possible to validate OCSP assertions, only
   to staple them into the handshake and to retrieve the stapled
   assertion if provided. Users will need to write their own code to
   handle OCSP assertions. We specifically added:
   Context.set_ocsp_server_callback, Context.set_ocsp_client_callback,
   and Connection.request_ocsp. pyca/pyopenssl#580
 * Changed the SSL module's memory allocation policy to avoid zeroing
   memory it allocates when unnecessary. This reduces CPU usage and
   memory allocation time by an amount proportional to the size of the
   allocation. For applications that process a lot of TLS data or that
   use very lage allocations this can provide considerable performance
   improvements. pyca/pyopenssl#578
 * Automatically set SSL_CTX_set_ecdh_auto() on OpenSSL.SSL.Context.
   pyca/pyopenssl#575
 * Fix empty exceptions from OpenSSL.crypto.load_privatekey().
   pyca/pyopenssl#581

The full upstream changelog can be found at:

https://pyopenssl.readthedocs.io/en/17.0.0/changelog.html

I've also added a patch from pyca/pyopenssl#637 in order to fix the
tests, which was the main reason for the version bump because that patch
won't apply for 16.2.0.

According to the upstream changelog there should be no
backwards-incompatible changes, but I've tested building against some of
the packages depending on pyopenssl anyway. Regardless of this, the
build for pyopenssl fails right now anyway, so the worst that could
happen via this commit would be that we break something that's already
broken.

Signed-off-by: aszlig <aszlig@redmoonstudios.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment