Clean up old data validation code and remove ignoreValidity#186
Conversation
…ion_alternate_vchinn04 Certificate verification alternate flow
This reverts commit dd86af7.
tianyuan129
left a comment
tianyuan129
left a comment
There was a problem hiding this comment.
LGTM. Actually it's more comprehensive than I initially thought! Huge thanks to the contribution. Just a comment here. If you think it's unnecessary I can just go merge.
|
|
||
| // Check if certificate is valid | ||
| if sec.CertIsExpired(data) { | ||
| return ndn.ErrInvalidValue{Item: "certificate expiry"} |
There was a problem hiding this comment.
I suggest we make a warning here instead.
There was a problem hiding this comment.
I think removing this check can potentially cause the trust schema Suggest to suggest an expired signing key, this could be prevented by adding an expiry check in Suggest which makes sense to have (it doesn't currently), I'm not sure if the lack of this check would lead to other issues in the future though.
There was a problem hiding this comment.
How about this, we keep this rejection, but meanwhile in Suggest() we add the ValidityPeriod check?
There was a problem hiding this comment.
Does this mean we won't store any expired certificate for validation caching? (the CertCache also removes expired or nearly expired certificate)
There was a problem hiding this comment.
Sorry I mean downgrade this rejection to a warning, meanwhile let Suggest() consider ValidityPeriod.
At some time points, we do need a function to purge expired certs from keychain....
There was a problem hiding this comment.
Actually I was mistaken and just didn't notice the check for validity period in Suggest() somehow, I think this can be merged
The keychain has a function to delete certs which could be used by applications to do purging, agree there probably should be one that iterates through all of them though
3 participants
No description provided.