Skip to content
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

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

Closed
wants to merge 1 commit into from

Conversation

sholsapp
Copy link
Contributor

This change adds a number of features to the X509Store class in order to
allow use of certificate revocation lists (and more) in verification
contexts.

Fixes #256

@sholsapp
Copy link
Contributor Author

Hey all, I submitted the PR mostly to get the testing/coverage goodness and to give others a chance to submit feedback before I spend too much longer on this. I'll ad some comments to parts I specifically have questions about.

a certificate. To carry out the actual verification process, see
:py:class:`X509StoreContext`.
"""

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all of the flags below are tested. What do folks prefer I do here: 1) expose all of the flags below and let people use them even though they're not tested or 2) only expose the flags that have been specifically tested?

@sholsapp
Copy link
Contributor Author

Also, it appears that I need the latest version of cryptography for this PR to work. How can I do that?

@reaperhulk
Copy link
Member

@sholsapp The Travis CI builds run against cryptography master in the allowed failures section. They abruptly error, which is usually a sign of a memory management problem: https://travis-ci.org/pyca/pyopenssl/jobs/67910194

@sholsapp
Copy link
Contributor Author

@reaperhulk That's what I thought re: the building against master. I'll find some time to see if I can hunt down a memory error. Thanks for the tip.

@sholsapp
Copy link
Contributor Author

I deleted my last comment couple comments and I think this patch is a red herring.

I'm finding that building and testing pyOpenSSL against head of cryptography, even without this patch, is failing at the same location. I narrowed it down to failing on OpenSSL/test/test_ssl.py:361 when instantiating Context(method).

Can someone else reproduce?

@reaperhulk
Copy link
Member

@sholsapp Yes I can reproduce. Will have to investigate what's going on tomorrow though.

@sholsapp
Copy link
Contributor Author

Thanks @reaperhulk. If I find anything I'll speak up.

@reaperhulk
Copy link
Member

@sholsapp calling SSLv2_method is causing an immediate segfault in master. This does not happen in 0.9.1. Not sure why right now.

"""
Sign the CRL.

Signing a CRL enables clients to assosciate the CRL itself with an
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo in 'associate'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, will fix.

@reaperhulk
Copy link
Member

As an update, this segfault is a bug in cryptography master related to conditional names not being stripped. See: pyca/cryptography#2061

@sholsapp
Copy link
Contributor Author

Thanks for tracking that down @reaperhulk! 😄

@reaperhulk
Copy link
Member

pyca/cryptography#2061 has been fixed. I restarted your builds and it passes on py26 and py27 but there are some issue with the PR on py3.

@sholsapp
Copy link
Contributor Author

sholsapp commented Aug 8, 2015

Thanks @reaperhulk I'll check it out soon.

@sholsapp sholsapp force-pushed the x509storecontext-crl branch 3 times, most recently from 1f4aab1 to 5c16491 Compare August 22, 2015 15:37
@codecov-io
Copy link

Current coverage is 92.46%

Merging #281 into master will decrease coverage by -0.10% as of 23d58e8

@@            master    #281   diff @@
======================================
  Files           14      14       
  Stmts         5365    5452    +87
  Branches       486     493     +7
  Methods          0       0       
======================================
+ Hit           4966    5041    +75
- Partial        157     163     +6
- Missed         242     248     +6

Review entire Coverage Diff as of 23d58e8

Powered by Codecov. Updated on successful CI builds.

@sholsapp sholsapp force-pushed the x509storecontext-crl branch 2 times, most recently from 9075124 to dbef1c7 Compare August 22, 2015 15:57
This change adds a number of features to the X509Store class in order to
allow use of certificate revocation lists (and more) in verification
contexts.

Fixes pyca#256
@sholsapp sholsapp force-pushed the x509storecontext-crl branch from dbef1c7 to 94107a4 Compare August 22, 2015 16:01
@hynek
Copy link
Contributor

hynek commented Aug 25, 2015

FYI, we usually don’t look at PRs unless codecov/commit is 100% (and not 86.36%). IOW the checks must be all green.

@sholsapp
Copy link
Contributor Author

@hynek sure thing, that's great that we're at 100% code coverage. I'll get to this soonish.

@hynek
Copy link
Contributor

hynek commented Aug 26, 2015

JFTR, we're not at 100% project-wide; but we require new patches to be fully covered so we catch up one day. :)

@dsully
Copy link

dsully commented Jun 2, 2016

@sholsapp - can you rebase this, and let's see where the code coverage is at? Likely only minor changes needed.

@hynek
Copy link
Contributor

hynek commented Jun 2, 2016

Please also do #478 on it. That should pretty much solve all coverage problems.

@dsully
Copy link

dsully commented Jun 4, 2016

I have a working patch based on @sholsapp's change, rebased to HEAD & using _openssl_assert().

What's the best way to get that change into this PR? Or do I need a new PR by it's nature?

@alex
Copy link
Member

alex commented Jun 4, 2016

New PR is probably the easiest approach.

On Fri, Jun 3, 2016 at 8:48 PM, Dan Sully [email protected] wrote:

I have a working patch based on @sholsapp https://github.com/sholsapp's
change, rebased to HEAD & using _openssl_assert().

What's the best way to get that change into this PR? Or do I need a new PR
by it's nature?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#281 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAADBFEvyVy7Kl10XVrLb0E8uectnwstks5qIMtvgaJpZM4FJU_P
.

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: D1B3 ADC0 E023 8CA6

@dsully
Copy link

dsully commented Jun 4, 2016

Ok, #483 is good to go, passed Travis.

@alex alex closed this Jun 5, 2016
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

X509StoreContext should accept CRLs
7 participants