variant of verify_callback with additional context #29
Conversation
src/tls_stream.rs
Outdated
| /// successful. The Ok() variant indicates a successful validation while the Err() variant | ||
| /// contains the errorcode returned from the internal verification process. | ||
| /// The validated certificate, is accessible through the second argument of the closure. | ||
| pub fn verify_callback2<F>(&mut self, callback: F) -> &mut Builder |
There was a problem hiding this comment.
I don't think we've cut a release with verify_callback so we can just update this in place.
src/cert_chain.rs
Outdated
| None | ||
| } | ||
|
|
||
| pub fn chains(&self) -> Vec<CertChain> { |
There was a problem hiding this comment.
This should probably return an iterator rather than a vec.
| None | ||
| } | ||
|
|
||
| pub fn get_chain(&self, index :usize) -> Option<CertChain> { |
There was a problem hiding this comment.
We'll want some way to know how many chains there are if we have this, and the implementation shouldn't be a linear scan.
There was a problem hiding this comment.
Can we cut the get_ prefix off of this?
src/tls_stream.rs
Outdated
| impl CertValidationResult { | ||
|
|
||
| /// Returns the certificate that failed validation if applicable | ||
| pub fn get_failed_certificate(&self) -> Option<CertContext> { |
There was a problem hiding this comment.
Getter functions typically omit the get_ prefix.
|
Attempted to make the changes SFackler suggested. Thanks for the review |
|
@steffengy[github.com] @sfackler[github.com] I'm not sure why this check is failing, but I don't think it's due to my code change. |
|
It's just a nightly regression so that currently GCC will not be found. (for MINGW) So you can safely ignore it, in the next few days it'll fix itself. |
|
Hey guys, is there anything I need to do to push this PR forward? I'd really like to get this merge and have a release cut so I can use this with hyper-native-tls in a project :) |
| @@ -32,7 +33,7 @@ lazy_static! { | |||
| #[derive(Default)] | |||
| pub struct Builder { | |||
| domain: Option<Vec<u16>>, | |||
| verify_callback: Option<Arc<Fn(io::Result<()>, &CertChain) -> io::Result<()>>>, | |||
| verify_callback: Option<Arc<Fn(CertValidationResult) -> io::Result<()>>>, | |||
There was a problem hiding this comment.
This needs to be passed a reference, not a result by value or it'll have access to a bunch of dangling pointers once the callback returns.
src/tls_stream.rs
Outdated
| res :io::Result<()>, | ||
| chain_index :i32, | ||
| element_index :i32, | ||
| extra_policy_status :Option<winapi::CERT_CHAIN_POLICY_STATUS>, |
There was a problem hiding this comment.
Looks like this is unused?
src/tls_stream.rs
Outdated
| self.chain.final_chain() | ||
| } | ||
|
|
||
| pub fn result(&self) -> &io::Result<()> { |
There was a problem hiding this comment.
io::Error isn't Clone, so we'll want this to return the Result by value so that it can then be returned from the closure - we should probably store the error code directly and construct the Error on each call.
src/tls_stream.rs
Outdated
| res :io::Result<()>, | ||
| chain_index :i32, | ||
| element_index :i32, | ||
| extra_policy_status : (winapi::LPCSTR, *const winapi::c_void), |
There was a problem hiding this comment.
Can we get rid of these fields if they aren't used?
There was a problem hiding this comment.
steffengy suggested that it might be useful long-term, so I put them in and made them usable. It would be an API breaking change to add them later, but if they don't seem useful in this context I will remove them.
There was a problem hiding this comment.
I don't think it'd be breaking - they're not publicly accessible, right?
There was a problem hiding this comment.
To clarify:
I only meant to describe which struct fields could potentially be relevant for us.
Those particular fields might be relevant for some use case in the future
(when we would access them and expose them without breaking BC).
Since we currently aren't even aware of a use case,
there's no need to keep them around.
|
Hey @sfackler I have put up another revision. I appreciate your attention here, as we are somewhat blocked on getting these changes merged in and incorporated into the project. |
|
LGTM, thanks! I won't be able to publish until I get home to my Windows box tonight, but you should be able to unblock yourself by doing a package replacement: http://doc.crates.io/manifest.html#the-replace-section. |
|
@sfackler |
|
Published as 0.1.6. |
This enables the workfow where an individual needs user confirmation for a failed certificate during verification callback.