Skip to content

Placed SSL/TLS dependencies behind a default feature flag #124

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,12 @@ is-it-maintained-open-issues = { repository = "jonhoo/rust-imap" }
name = "imap"
path = "src/lib.rs"

[features]
default = ["ssl"]
ssl = ["native-tls"]

[dependencies]
native-tls = "0.2.2"
native-tls = {version = "0.2.2", optional = true}
regex = "1.0"
bufstream = "0.1"
imap-proto = "0.7"
Expand Down
1 change: 1 addition & 0 deletions examples/basic.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
extern crate imap;
#[cfg(feature = "ssl")]
extern crate native_tls;

fn main() {
Expand Down
4 changes: 3 additions & 1 deletion examples/gmail_oauth2.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
extern crate base64;
extern crate imap;
#[cfg(feature = "ssl")]
extern crate native_tls;

#[cfg(feature = "ssl")]
use native_tls::TlsConnector;

struct GmailOAuth2 {
Expand All @@ -20,6 +21,7 @@ impl imap::Authenticator for GmailOAuth2 {
}
}

#[cfg(feature = "ssl")]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably better here to list the example in Cargo.toml with required-features = ["ssl"] (assuming that works).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know about required-features. Seems like that's a much better solution, thanks.

fn main() {
let gmail_auth = GmailOAuth2 {
user: String::from("[email protected]"),
Expand Down
3 changes: 3 additions & 0 deletions src/client.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use base64;
use bufstream::BufStream;
#[cfg(feature = "ssl")]
use native_tls::{TlsConnector, TlsStream};
use nom;
use std::collections::HashSet;
Expand Down Expand Up @@ -161,6 +162,7 @@ pub fn connect_insecure<A: ToSocketAddrs>(addr: A) -> Result<Client<TcpStream>>
/// let client = imap::connect(("imap.example.org", 993), "imap.example.org", &tls).unwrap();
/// # }
/// ```
#[cfg(feature = "ssl")]
pub fn connect<A: ToSocketAddrs, S: AsRef<str>>(
addr: A,
domain: S,
Expand All @@ -185,6 +187,7 @@ impl Client<TcpStream> {
/// This will upgrade an IMAP client from using a regular TCP connection to use TLS.
///
/// The domain parameter is required to perform hostname verification.
#[cfg(feature = "ssl")]
pub fn secure<S: AsRef<str>>(
mut self,
domain: S,
Expand Down
12 changes: 12 additions & 0 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ use std::string::FromUtf8Error;
use base64::DecodeError;
use bufstream::IntoInnerError as BufError;
use imap_proto::Response;
#[cfg(feature = "ssl")]
use native_tls::Error as TlsError;
#[cfg(feature = "ssl")]
use native_tls::HandshakeError as TlsHandshakeError;

/// A convenience wrapper around `Result` for `imap::Error`.
Expand All @@ -22,8 +24,10 @@ pub enum Error {
/// An `io::Error` that occurred while trying to read or write to a network stream.
Io(IoError),
/// An error from the `native_tls` library during the TLS handshake.
#[cfg(feature = "ssl")]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and some of the other changes) would make the ssl feature non-additive, which causes problems. I'm not entirely sure that there's a way to work around this :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this is a tricky one to solve cleanly, especially without any breaking changes (split the enum into subset/superset relationship).
Perhaps it is appropriate though to allow this as the current API of the crate makes it a conscious decision to create a secure or insecure session - it would not be possible anyway to switch from not having the ssl feature to having it without also having to adapt other parts of a user's code.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately crate authors do not have the option of choosing. Since cargo assumes that features are additive, it will automatically pick the maximal set of features for any dependency. For example if a and b both depended on imap, one with the feature and one without, cargo would only compile imap once with the feature and then compile both a and b against that, at which point b would fail to compile if it ever tried to match over the errors for example. That's far from ideal..

TlsHandshake(TlsHandshakeError<TcpStream>),
/// An error from the `native_tls` library while managing the socket.
#[cfg(feature = "ssl")]
Tls(TlsError),
/// A BAD response from the IMAP server.
Bad(String),
Expand Down Expand Up @@ -58,12 +62,14 @@ impl<T> From<BufError<T>> for Error {
}
}

#[cfg(feature = "ssl")]
impl From<TlsHandshakeError<TcpStream>> for Error {
fn from(err: TlsHandshakeError<TcpStream>) -> Error {
Error::TlsHandshake(err)
}
}

#[cfg(feature = "ssl")]
impl From<TlsError> for Error {
fn from(err: TlsError) -> Error {
Error::Tls(err)
Expand All @@ -80,7 +86,9 @@ impl fmt::Display for Error {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match *self {
Error::Io(ref e) => fmt::Display::fmt(e, f),
#[cfg(feature = "ssl")]
Error::Tls(ref e) => fmt::Display::fmt(e, f),
#[cfg(feature = "ssl")]
Error::TlsHandshake(ref e) => fmt::Display::fmt(e, f),
Error::Validate(ref e) => fmt::Display::fmt(e, f),
Error::No(ref data) | Error::Bad(ref data) => {
Expand All @@ -95,7 +103,9 @@ impl StdError for Error {
fn description(&self) -> &str {
match *self {
Error::Io(ref e) => e.description(),
#[cfg(feature = "ssl")]
Error::Tls(ref e) => e.description(),
#[cfg(feature = "ssl")]
Error::TlsHandshake(ref e) => e.description(),
Error::Parse(ref e) => e.description(),
Error::Validate(ref e) => e.description(),
Expand All @@ -109,7 +119,9 @@ impl StdError for Error {
fn cause(&self) -> Option<&StdError> {
match *self {
Error::Io(ref e) => Some(e),
#[cfg(feature = "ssl")]
Error::Tls(ref e) => Some(e),
#[cfg(feature = "ssl")]
Error::TlsHandshake(ref e) => Some(e),
Error::Parse(ParseError::DataNotUtf8(ref e)) => Some(e),
_ => None,
Expand Down
2 changes: 2 additions & 0 deletions src/extensions/idle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

use client::Session;
use error::{Error, Result};
#[cfg(feature = "ssl")]
use native_tls::TlsStream;
use std::io::{self, Read, Write};
use std::net::TcpStream;
Expand Down Expand Up @@ -164,6 +165,7 @@ impl<'a> SetReadTimeout for TcpStream {
}
}

#[cfg(feature = "ssl")]
impl<'a> SetReadTimeout for TlsStream<TcpStream> {
fn set_read_timeout(&mut self, timeout: Option<Duration>) -> Result<()> {
self.get_ref().set_read_timeout(timeout).map_err(Error::Io)
Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
extern crate base64;
extern crate bufstream;
extern crate imap_proto;
#[cfg(feature = "ssl")]
extern crate native_tls;
extern crate nom;
extern crate regex;
Expand Down
6 changes: 6 additions & 0 deletions tests/imap_integration.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,21 @@
extern crate imap;
extern crate lettre;
extern crate lettre_email;
#[cfg(feature = "ssl")]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's probably fine for native-tls to always be included in dev-dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good pickup, that makes sense.

extern crate native_tls;

use lettre::Transport;
use std::net::TcpStream;

#[cfg(feature = "ssl")]
fn tls() -> native_tls::TlsConnector {
native_tls::TlsConnector::builder()
.danger_accept_invalid_certs(true)
.build()
.unwrap()
}

#[cfg(feature = "ssl")]
fn session(user: &str) -> imap::Session<native_tls::TlsStream<TcpStream>> {
let mut s = imap::connect("127.0.0.1:3993", "imap.example.com", &tls())
.unwrap()
Expand All @@ -22,6 +25,7 @@ fn session(user: &str) -> imap::Session<native_tls::TlsStream<TcpStream>> {
s
}

#[cfg(feature = "ssl")]
fn smtp(user: &str) -> lettre::SmtpTransport {
let creds = lettre::smtp::authentication::Credentials::new(user.to_string(), user.to_string());
lettre::SmtpClient::new(
Expand All @@ -43,6 +47,7 @@ fn connect_insecure() {

#[test]
#[ignore]
#[cfg(feature = "ssl")]
fn connect_insecure_then_secure() {
// ignored because of https://github.com/greenmail-mail-test/greenmail/issues/135
imap::connect_insecure("127.0.0.1:3143")
Expand All @@ -52,6 +57,7 @@ fn connect_insecure_then_secure() {
}

#[test]
#[cfg(feature = "ssl")]
fn connect_secure() {
imap::connect("127.0.0.1:3993", "imap.example.com", &tls()).unwrap();
}
Expand Down