TlsStream::certificate_chain, ChainIterator type and Certificate::public_key_info_der #117
Conversation
|
Also I would like to expose hashing algorithms (md5, sha256, sha384 and sha512) from rust-native-tls. Would this be agreeable? It goes a bit further then the stated goal of this project but it's very useful and quite commonly needed for anyone using this upstream. |
|
I have been thinking a bit about adding a |
src/imp/schannel.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub struct TlsStream<S>(tls_stream::TlsStream<S>, Option<CertStore>); |
There was a problem hiding this comment.
I'd use a struct here with named fields to make things a bit more clear.
src/imp/schannel.rs
Outdated
| if self.1.is_none() { | ||
| match self.0.peer_certificate() { | ||
| Ok(cert) => { | ||
| self.1 = cert.cert_store(); |
There was a problem hiding this comment.
Why do we need to cache the cert store?
There was a problem hiding this comment.
I could not find a better solution. CertStore once you call certs is borrowed. You can't drop it and you can't put it in the iterator as it will be a self referenced struct.
There was a problem hiding this comment.
It seems like things would be easier to work with if we just returned a Result<Vec<Certificate>>. I don't think we're gaining all that much by not buffing the certs up again.
| if let Some(trust) = self.trust.as_ref() { | ||
| let pos = self.pos; | ||
| self.pos += 1; | ||
| return trust.certificate_at_index(pos as _).map(Certificate); |
There was a problem hiding this comment.
This needs to stop at the end of the chain, right?
There was a problem hiding this comment.
Trust will return None once it is over limit.
src/imp/security_framework.rs
Outdated
| let trust = match self.stream.context().peer_trust2()? { | ||
| Some(trust) => trust, | ||
| None => { | ||
| return Ok(ChainIterator { |
There was a problem hiding this comment.
This can be restructured a bit to avoid having to have a separate early return:
let trust = match self.stream.context().peer_trust2()? {
Some(trust) => {
trust.evaluate()?;
Some(trust)
}
None => None
};
...
| @@ -630,6 +647,11 @@ impl<S: io::Read + io::Write> TlsStream<S> { | |||
| Ok(self.0.peer_certificate()?.map(Certificate)) | |||
| } | |||
|
|
|||
| /// Returns an iterator over certificate chain, if available. | |||
| pub fn certificate_chain(&mut self) -> Result<ChainIterator<S>> { | |||
There was a problem hiding this comment.
Should we distinguish the "no chain present" case here by returning something like Result<Option<ChainIterator>>?
There was a problem hiding this comment.
I would prefer to create an empty iterator that just returns None immediately. That is what happens in the downstream crates. Result<Option> is a pretty annoying API.
|
Should we add an intermediate |
|
Not sure what the API would be for the PublicKey type. schannel-rs looks pretty sparse in this area. Since checks are passing right now, is there anything else to do with regards to this? |
|
Can I do anything more or is this not something you want merged? |
|
Having access to the |
This patch makes certificate pinning possible on top of rust-native-tls. What is required is access to the entire certificate chain and subjectPublicKeyInfo part of a certificate.
This is not ready to be merged yet, as I am waiting for downstream schannel and rust-security-framework patches to be merged.
I've created this PR now to resolve any blockers in the meantime.