Refactor TLS API #388
Refactor TLS API #388
Conversation
Refactor the API for SSL/TLS configuration. This accounts for the fact that the TLS engine can vary (as rustls is now an option) and corrects some confusing parts of the API that don't always make sense.
Codecov ReportBase: 52.99% // Head: 52.42% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #388 +/- ##
==========================================
- Coverage 52.99% 52.42% -0.57%
==========================================
Files 56 59 +3
Lines 5831 5951 +120
==========================================
+ Hits 3090 3120 +30
- Misses 2741 2831 +90
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| /// )) | ||
| /// let response = Request::get("https://badssl.com") | ||
| /// .tls_config(TlsConfig::builder() | ||
| /// .danger_accept_invalid_certs(true) |
There was a problem hiding this comment.
I'm thinking about adding enable alike API:
.danger_accept_invalid_certs()Or add API like accept_invalid_certs() but under inside feature insecure_tls?
|
|
||
| // If an empty list is provided, reset to default. Otherwise build up a | ||
| // string in curl format containing the cipher names. | ||
| if let Some(first) = iter.next() { |
There was a problem hiding this comment.
What's the different with ciphers.into_iter().join(":")?
There was a problem hiding this comment.
When an empty list is provided, we're simply not setting any ciphers, and allowing the TLS engine to choose whichever ciphers they think is best. If we actually set it to an empty list though, some TLS engines will interpret this as, "No ciphers are allowed" and basically reject all connections.
There was a problem hiding this comment.
How about:
self.ciphers = Some(ciphers.into_iter().join(":")).filter(|v|!v.is_empty());Looks nicer to me.
src/tls/mod.rs
Outdated
| /// Disables all server certificate validation. | ||
| /// | ||
| /// By default this is enabled. |
There was a problem hiding this comment.
To my understanding: This functions is used to enable invalid certs. And it's disabled by default.
|
|
||
| #[derive(Clone, Debug)] | ||
| enum StoreImpl { | ||
| NoOp, |
There was a problem hiding this comment.
We need docs for the difference between NoOp and Unset.
| #[test] | ||
| #[cfg_attr(not(feature = "online-tests"), ignore)] | ||
| fn accept_expired_cert() { | ||
| Request::get("https://expired.badssl.com") |
There was a problem hiding this comment.
Oh, nice. It's the first time for me to know this website. Lessons learnt.
Refactor the API for SSL/TLS configuration. This accounts for the fact that the TLS engine can vary (as rustls is now an option) and corrects some confusing parts of the API that don't always make sense.