search

wbond / certvalidator

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

Confusing behavior when allow_fetching is True #20

Open atmenta opened 4 years ago

atmenta commented 4 years ago

The docstring of context.ValidationContext states that if the value of the allow_fetching parameter is True "and certificates contain the location of a CRL or OCSP responder, an HTTP request will be made to obtain information for revocation checking". The word "allow" seems to indicate that this HTTP request is merely an additional way of obtaining information about revocation status. Moreover, also the following comment (especially the word "also") suggests that CRL or OCSP data requested via HTTP are not the only source of revocation status information if allow_fetching is True:

https://github.com/wbond/certvalidator/blob/5bc5c390c1955195507c23db91b8926bb03f7385/certvalidator/context.py#L87-L90

The docstring of the crls and ocsps parameters explain that pre-fetched/cached CRL and/or OCSP data can be passed to ValidationContext.

Putting all together, it seems that setting the allow_fetching parameter to True may result in fetching CRL or OCSP data in addition to checking pre-fetched/cached data.

Contrary to this expectation, if ValidationContext._allow_fetching is True, pre-fetched/cached CRL and OCSP data (passed by the crls and ocsps parameters) are completely ignored. (See the crls and ocsps properties and the retrieve_crls and retrieve_ocsps methods of ValidationContext.) If that is the intended behavior, it should be clarified in the documentation. Otherwise - and I would except this more intuitive - the two sources of revocation information should be merged when this is appropriate.

wbond commented 4 years ago

I do believe the wording in the comment is incorrect - there should be no "also".

It's been a while since I've worked on this project, so I don't have everything off of the top of my head. That said, if allow fetching was True, and the fetch failed, but there was a cached CRL, I would probably expect it to use the cached CRL. Which I don't believe it currently does. It appears to only used the cached CRL/OCSP info when fetching is disabled.

atmenta commented 4 years ago

I do believe the wording in the comment is incorrect - there should be no "also".

It's been a while since I've worked on this project, so I don't have everything off of the top of my head. That said, if allow fetching was True, and the fetch failed, but there was a cached CRL, I would probably expect it to use the cached CRL. Which I don't believe it currently does. It appears to only used the cached CRL/OCSP info when fetching is disabled.

Then we seem to agree on the expected behavior. Should I try to fix it and provide a pull request?