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

check crl/ocsp against *now* by default instead of context.moment #7

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kak-bo-che
Copy link

  • adjust moment date for revocation checks
  • handle unauthorized ocsp response
  • optionally check for csrl/ocsp next_update
  • Added failure fields to SoftFailError
  • Added Tests related to CRL check and nextUpdate missing
    @wbond this is a new PR based on a previous one, that is cleaner to see what is going on.

* adjust moment date for revocation checks
* handle unauthorized ocsp response
* optionally check for csrl/ocsp next_update
* Added failure fields to SoftFailError
* Added Tests related to CRL check and nextUpdate missing
@codecov
Copy link

codecov bot commented May 28, 2017

Codecov Report

Merging #7 into master will increase coverage by 1.05%.
The diff coverage is 92.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #7      +/-   ##
==========================================
+ Coverage   81.46%   82.52%   +1.05%     
==========================================
  Files          12       12              
  Lines        1430     1465      +35     
==========================================
+ Hits         1165     1209      +44     
+ Misses        265      256       -9
Impacted Files Coverage Δ
certvalidator/errors.py 100% <100%> (ø) ⬆️
certvalidator/ocsp_client.py 76.08% <50%> (-1.19%) ⬇️
certvalidator/context.py 79.22% <89.47%> (+3.96%) ⬆️
certvalidator/validate.py 87.08% <95.12%> (+0.32%) ⬆️
certvalidator/crl_client.py 59.61% <0%> (+9.61%) ⬆️

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 4383a4b...f5754c2. Read the comment docs.

@pkill37
Copy link

pkill37 commented Dec 20, 2017

I think this might be a fix for my issue in #9. Can we merge this @wbond?

@mttcpr
Copy link

mttcpr commented Dec 20, 2017

If I'm reading your changes correctly, you've created a validation mode that isn't described in the standards. Point in time validation requires valid revocation data from that point in time, not some future time where the revocation status may have changed. The obvious case is where a certificate in the path expired and the revocation entry was removed from the CRL. A less obvious case is the signer could have been on hold at the time of signing and subsequently removed from hold. My take is if you are generating signatures that need to be point in time validated in the future, those signatures should carry the revocation data with them. Acrobat is an example of an app that does this.

@kak-bo-che
Copy link
Author

@mttcpr The target application for my point in time validation is Microsoft Authenticode. Which is essentially a CMS/PKCS#7 validation with a counter signing signature. The counter signature establishes the point in time which the file signature was known to be valid and the signing chain was within its period of validity. Certificate revocation is therefore checked years after the certificate itself has expired, due to the counter signature. This situation does occur when for instance the private key was stolen.

@stevemk14ebr
Copy link

+1 please revisit this

@wbond
Copy link
Owner

wbond commented May 4, 2020

Unfortunately I don't currently have the bandwidth to work on this package.

@stevemk14ebr
Copy link

would you be able/willing to assign a new maintainer so that these PRs can be closed?

@wbond
Copy link
Owner

wbond commented May 4, 2020

Not at this point - no one has shown an understanding of the topic and/or codebase. No one has ever submitted more than one PR.

You are certainly welcome to fork, rename and distribute as you see fit, as long as you respect the license.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants