wbond / certvalidator

Python library for validating X.509 certificates and paths
MIT License
107 stars 31 forks source link

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

Closed kak-bo-che closed 7 years ago

kak-bo-che commented 7 years ago

This PR is related to Issue #3. For your consideration. I tried to minimally impact the existing code, but I did not any new tests although all of the existing ones continue to work.

wbond commented 7 years ago

Sorry this has been sitting for so long! I do mean to get to it, it is just nuanced enough that I need to make sure I understand the implications, and I haven't had time to work on much open source stuff recently.

codecov[bot] commented 7 years ago

Codecov Report

Merging #4 into master will increase coverage by 1.02%. The diff coverage is 94.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #4      +/-   ##
==========================================
+ Coverage   81.46%   82.49%   +1.02%     
==========================================
  Files          12       12              
  Lines        1430     1457      +27     
==========================================
+ Hits         1165     1202      +37     
+ Misses        265      255      -10
Impacted Files Coverage Δ
certvalidator/version.py 100% <ø> (ø) :arrow_up:
certvalidator/__init__.py 73.13% <100%> (ø) :arrow_up:
certvalidator/_urllib.py 100% <100%> (ø) :arrow_up:
certvalidator/errors.py 100% <100%> (ø) :arrow_up:
certvalidator/crl_client.py 59.61% <71.42%> (+9.61%) :arrow_up:
certvalidator/ocsp_client.py 76.08% <87.5%> (-1.19%) :arrow_down:
certvalidator/context.py 79% <91.66%> (+3.74%) :arrow_up:
certvalidator/validate.py 87.06% <97.56%> (+0.3%) :arrow_up:
... and 3 more

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...e162367. Read the comment docs.

wbond commented 7 years ago

This is looking good. I think the only other thing would be to have some sort of test to make sure this functionality isn't reverted by a future change. Do you think there is some way we could reasonably pull this off?

wbond commented 7 years ago

There are a few tests that are failing, could you get those fixed up so I can review this?

kak-bo-che commented 7 years ago

After making more changes, the tests are still failing. I now realize it is because of a separate issue in asn1crypto. I created a 1 line PR there and if you merge that one these tests should pass.

wbond commented 7 years ago

You can always ping me if you want me to re-trigger the CI for a commit.

kak-bo-che commented 7 years ago

@wbond I'm closing this request because the number of changes and commit log is a mess. I'll create a new PR.