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

Improved verify #1779

Merged
merged 11 commits into from
Oct 4, 2018
Merged

Improved verify #1779

merged 11 commits into from
Oct 4, 2018

Conversation

explody
Copy link
Contributor

@explody explody commented Sep 27, 2018

Redoing this completely since I broke the last one.

This PR addresses a few issues in the CLI verification process, and #1079

  • Stacked exceptions were yielding lots of tracebacks
    Rather than using exceptions for non-error failure states, I've updated the code to use return values (False = revoked, True = good, None = unknown). Exceptions were left in place where they are for unexpected errors (parse errors, http errors, etc.)

  • Verifying a large number of certificates was causing slowdowns as the code was pulling the same CRL over and over for certs with the same CRL.
    I implemented an extremely simple cache so that an individual CRL is only downloaded once. This works fine for the CLI where the code is loaded fresh each time but it could be a problem if the verify methods are used by the running web app, as the cache has no update/expire feature. Currently, verification is only in use by the CLI code.

  • Better logging
    Added debug logging output in case anyone needs to troubleshoot the process. Also, parse the cert in verify() and pass it to crl_verify() and ocsp_cerify() so they both have access to cert data for logging purposes.

Tests, flake8 and docstrings have been updated since the previous PR and master is merged in.

@explody
Copy link
Contributor Author

explody commented Sep 27, 2018

Build appears to be failing in test-cli, I don't think it's related to anything in this PR. test-python locally passes fine.

@castrapel
Copy link
Contributor

This appears to be an issue with lint. Try running flake8. The specific error in Travis-CI is:

--> Linting Python files
PYFLAKES_NODOCTEST=1 flake8 lemur
lemur/tests/test_verify.py:16:53: E711 comparison to None should be 'if cond is None:'
make: *** [lint-python] Error 1

@coveralls
Copy link

coveralls commented Oct 2, 2018

Coverage Status

Coverage decreased (-0.08%) to 60.831% when pulling 79033f4 on explody:improved_verify into eb7eb6a on Netflix:master.

Copy link
Contributor

@castrapel castrapel left a comment

Choose a reason for hiding this comment

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

Looks good! Will merge. Thank you

@castrapel castrapel merged commit 3185272 into Netflix:master Oct 4, 2018
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.

3 participants