Skip to content
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

Factor out tokio-openssl support to new trust_dns_openssl crate. #217

Merged
merged 5 commits into from Oct 2, 2017
Merged

Factor out tokio-openssl support to new trust_dns_openssl crate. #217

merged 5 commits into from Oct 2, 2017

Conversation

briansmith
Copy link
Contributor

No description provided.

Copy link
Member

@bluejekyll bluejekyll left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Couple of minor things.

@@ -217,7 +218,6 @@ fn tls_client_stream_test(server_addr: IpAddr, mtls: bool) {
}

#[allow(unused_variables)]
#[cfg(feature = "tls")]
fn config_mtls(
Copy link
Member

Choose a reason for hiding this comment

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

btw, none of my mtls stuff is working at the moment. The OpenSSL library has an ugly habit of just grabbing STDIN and trying to read for passwds, etc. Never got to the bottom of it. It was before I had rustls that I started this though, so maybe when I fix this, I'll just support rustls. Issue #100

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Does that affect this PR in any way? The main goal here is to eliminate OpenSSL-related crate dependencies outside of this new crate, and to accomplish that, this code needs to be moved, IIUC.

Copy link
Member

Choose a reason for hiding this comment

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

No... I just wanted you to be aware in case any of the methods "appear" to be unused.


# This points to a file in the repository (relative to this Cargo.toml). The
# contents of this file are stored and indexed in the registry.
readme = "README.md"
Copy link
Member

Choose a reason for hiding this comment

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

Does this exist?

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 added it in the new rebased commits.

@@ -1,3 +1,5 @@
#![cfg(feature = "tls")]
Copy link
Member

Choose a reason for hiding this comment

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

Does this just disable the entire test suite? that's nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly.

Don't run TLS tests when the TLS feature is disabled. Don't depend on
crates only needed for TLS when TLS is disabled.

Besides just generally cleaning things up, this is also a step toward
making `cargo test` work out of the box on Windows without OpenSSL
installed.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 88.277% when pulling 293f788 on briansmith:openssl into a12698d on bluejekyll:master.

Move the openssl and tokio-openssl dependencies to trust-dns-openssl.
This is a step towards adapting the remaining server code to work with
other TLS implementations.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 88.315% when pulling 1f14ee5 on briansmith:openssl into a12698d on bluejekyll:master.

@bluejekyll
Copy link
Member

This looks good, shall I merge?

Copy link
Contributor Author

@briansmith briansmith left a comment

Choose a reason for hiding this comment

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

This looks good, shall I merge?

Yes, I don't plan on making any changes to this branch, unless you see anything that looks bad.


/// A Type definition for the TLS stream
pub type TlsClientStream = TcpClientStream<TokioTlsStream<TokioTcpStream>>;

impl TlsClientStream {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This had to be changed because we can't impl TlsClientStream from this crate, since TlsClientStream is an alias to a type defined in another crate now.

@@ -57,81 +57,71 @@ impl TlsIdentityExt for SslContextBuilder {
/// A TlsStream counterpart to the TcpStream which embeds a secure TlsStream
pub type TlsStream = TcpStream<TokioTlsStream<TokioTcpStream>>;

impl TlsStream {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto, we can't impl TlsStream here because TlsStream is an alias to a type defined in another crate.


# This points to a file in the repository (relative to this Cargo.toml). The
# contents of this file are stored and indexed in the registry.
readme = "README.md"
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 added it in the new rebased commits.

@@ -217,7 +218,6 @@ fn tls_client_stream_test(server_addr: IpAddr, mtls: bool) {
}

#[allow(unused_variables)]
#[cfg(feature = "tls")]
fn config_mtls(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Does that affect this PR in any way? The main goal here is to eliminate OpenSSL-related crate dependencies outside of this new crate, and to accomplish that, this code needs to be moved, IIUC.

@@ -1,3 +1,5 @@
#![cfg(feature = "tls")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly.

@bluejekyll bluejekyll merged commit 67a0bf6 into hickory-dns:master Oct 2, 2017
@briansmith briansmith deleted the openssl branch October 30, 2017 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants