Allow to add additional root certificates with native-tls #74
Conversation
src/streams.rs
Outdated
| let mut connector_builder = TlsConnector::builder(); | ||
| connector_builder.danger_accept_invalid_certs(info.base_settings.accept_invalid_certs); | ||
| connector_builder.danger_accept_invalid_hostnames(info.base_settings.accept_invalid_hostnames); | ||
| for cert in info.base_settings.root_certificates.iter() { |
There was a problem hiding this comment.
I think the .iter() call could be replaced by just doing for cert in &info.base_settings.root_certificates?
There was a problem hiding this comment.
The compiler doesn't seems to like it:
error[E0507]: cannot move out of `info.base_settings.root_certificates` which is behind a shared reference
--> src/streams.rs:97:21
|
97 | for cert in info.base_settings.root_certificates {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| move occurs because `info.base_settings.root_certificates` has type `std::vec::Vec<certificate::Certificate>`, which does not implement the `Copy` trait
| help: consider iterating over a slice of the `Vec<_>`'s content: `&info.base_settings.root_certificates`
There was a problem hiding this comment.
You need the & sigil in front to indicate that you want a reference and not move out of the struct field, i.e. in &info.base_settings.root_certificates instead of in info.base_settings.root_certificates.
src/certificate.rs
Outdated
| pub inner_cert: native_tls::Certificate, | ||
| } | ||
|
|
||
| impl fmt::Debug for Certificate { |
There was a problem hiding this comment.
I am not sure if the debug output is worth the additional module and new type wrapper compared to e.g. just using the existing SkipDebug wrapper? Especially so since we start to expose this in our API as per the base settings member.
If we decide to keep it, I would suggest using a tuple type like pub struct Certificate(pub native_tls::Certificate) and dropping the From impl as we never need to convert this in generic context.
There was a problem hiding this comment.
Especially so since we start to expose this in our API as per the base settings member.
It's not exposed. The public add_root_certificate methods accepts a native_tls::Certificate.
There was a problem hiding this comment.
It's not exposed. The public add_root_certificate methods accepts a native_tls::Certificate.
Indeed. I was mistakenly thinking about the pub root_certificates: Vec<Certificate> field in BaseSettings but that type itself is not exposed in the API.
Personally, I would still prefer to just use SkipDebug there to keep this reasonably simple.
There was a problem hiding this comment.
Sure. I just didn't knew there was a SkipDebug trait in the first place. I've just amended the PR so it uses it instead of the new Certificate module/type.
There was a problem hiding this comment.
Looks much nicer to me now! (Totally a nit, but I guess SkipDebug<Vec<Certificate>> would be nicer as it would avoid having to spell out the wrapper every time an item is added and the resulting debug output will contain only one ellipsis instead of one for each certificate. SkipDebug also has a similar From impl as Certificate had.)
There was a problem hiding this comment.
Indeed, that's better this way. PR updated.
|
@sbstp Looks good to me. |
|
Looks good to me as well. |
|
I'll release this in a bit |
|
Thank you, it's out in 0.15.0 |
Fix #71