-
-
Notifications
You must be signed in to change notification settings - Fork 764
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
Add support for X509_VERIFY_PARAM_set_time and X509_VERIFY_PARAM_set_… #1710
Add support for X509_VERIFY_PARAM_set_time and X509_VERIFY_PARAM_set_… #1710
Conversation
f699099
to
b5fabee
Compare
a237506
to
969f955
Compare
openssl/src/x509/tests.rs
Outdated
assert!(context | ||
.init(&store, &cert, &chain, |c| c.verify_cert()) | ||
.unwrap()); | ||
assert!(context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something, but why are you calling assert twice with the same arguments? I'd also be much happier with these tests if they also checked a failing cert or depth (you can do it in the same function).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test_verify_cert in this file calls it twice, I assumed that was best practice. On the failing side, I do checking failing certs with respect to set_time in test_verify_param_set_time_fails_verification. For set_depth, I don't believe there are any test certs in the repo with a 3-deep chain. What is best practice for creating such a chain? Should I just add it to the repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test_verify_cert
calling it twice is somewhere between a fluke of history and a test about cleanup, in this case you only need one. Yes, just create a new certificate and throw it in the directory with the rest of them (openssl/test
), I think this change would be fine without that test but if you're willing to do that it would be nice to have the coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the double call, and added new certificates into the repository to create a 3-long chain, which is then used in the set_depth tests to check failure
449bd45
to
9ad8458
Compare
9ad8458
to
808b951
Compare
@sfackler Anything else I need to do for this PR? |
Thanks! Sorry for the delay. |
…depth