Fix issues with certvalidator

[TheDoctor0]

There are some minor issues with certvalidator that need to be addressed:

1. ValueError: allow_fetching must be False when moment is specified

It does not allow to specify moment when allow_fetching is enabled (wbond/certvalidator#27).
I added a test to reproduce this. Fix is really simple, do not pass timestamp to ValidationContext if allow_fetching is set to True.

2. AttributeError: 'Void' object has no attribute 'human_friendly'

The verification in test_jameslth_revoked raises an exception, but it's not the one that it should be (wbond/certvalidator#26).
By default [email protected] is being installed - released 29 July 2016.
This error been fixed in wbond/certvalidator@80119e8 - commit from 9 May 2017.
I changed requirements.txt to force installation directly from GitHub.

[ralphje]

For item 2, I agree this is an issue with the library we're using, though I don't think this is an issue that hinders us (the error stems from the generator of the error message, so it's an exception anyway. I'm fiercely against pinning releases on GitHub commits as this doesn't work for setup.py and is unsuitable for installations that only want to download from PyPI. Therefore I would like you to remove this from the PR.

For 1 I agree that this limitation is unnecessarily imposedd by the imported library, but are we running into any issues if someone is not setting the timestamp but forcing CRL checks (eg. for countersignatures)? Or is this better suited as a documentation thing? I can also imagine that there are some issues with revoked certificates after or before issuing of the (counter)signature and I'm not sure how to deal with those.

[TheDoctor0]

1. This has been actually solved in only PR in certvalidator repository that is open since May 2017. In conversation here the author of this library clearly states that he don't have time to maintain it and even encourages to fork it. So probably it won't be merged and there won't be any official release anytime soon.

2. In my use-case (static malware classification) it is extremely important to get a more precise reason why the signature is not valid and that's how I discovered this error with revocation reason in the first place. Of course, if you want to limit sources of dependencies to PyPI only, I will revert this change for PR and use my fork anyway.

[ralphje]

I think that the solution you proposed for item number 1 is wrong: you are not passing the timestamp when we want to verify against the CRL. That means that the certificate will be invalid when it's after its expiry date, so we will invalidate certificates that are correctly timestamped to a date within its validity.

Simply put, combining timestamped certificates with CRLs cannot be easily done. This is also mentioned in the PR you mentioned, and the example put there (when a certificate is expired it does not necessarily mean that a revoked certificate is still in the CRL) is a great one as to why I'm not accepting this PR at this time.

We should first figure out how Microsoft proposes handling CRLs and such, as this can get quite convoluted: Are revoked certificates used for timestamped signatures to be considered revoked regardless of whether it was revoked before or after the timestamp? Do we trust CRLs to include revoked certificates that are also expired? Et cetera.

For item number 2 I would propose you install the correct dependency in your environment directly (through requirements.txt) as signify does not prohibit you from installing other versions.

I'm open to proposals to use other similar projects that offer functionality that can be used by signify to correctly verify certificates with timestamps. You are free to re-open the PR when you have any additional remarks.

[MrCrumbs]

@ralphje Question - the way it is now, when I try to verify a PE with a timestamp, with allow_fetching=True, we get the ValueError: allow_fetching must be False when moment is specified error. So I thought to myself, let's combine allow_fetching=True with allow_countersignature_errors=True, so I can at least try to get the revocation status, without worrying about the timestamp countersignature validity. However, this doesn't work because of the order of the validation process - i.e. the verification process fails because the original certificate is expired (and we're not checking the countersignature that extends its validity). Is it possible to change the sequence of events to check the CRL first? This way I can know that the certificate was revoked, and I don't really care about the timestamp/countersignature.

[ralphje]

I don't think that changing the sequence of events would make a large difference. We could add an option to ignore the countersignature in the verify function though, which could be a solution for this issue, though the main point still remains.

Is that something that would help?

[MrCrumbs]

That's actually a great idea which would help us in our scenarios (even though, as you said, it doesn't solve the general issue). We would greatly appreciate adding such an option to the verify function.
(It might help @TheDoctor0 as well.. ?)