Code Monkey home page Code Monkey logo

Comments (9)

marcbrevoort-cyberhive avatar marcbrevoort-cyberhive commented on May 24, 2024 1

I've ultimately resorted to wrapping the DNS call in a function that shuts down the call if it doesn't return in a timely fashion, broadly following this example but also allowing the wrapper to store the result in an Arc<Mutex<Option<Result<DnsResponse, ClientError>>>>. On timeout, there is no result, and this None is handled by raising a timeout exception. Otherwise the Result<DnsResponse, ClientError> is returned.

The benefit is that it doesn't touch Hickory, and when the update lands, in principle it should just keep working - so both fixing the lack of timeout in Hickory and removing the wrapper once Hickory is fixed can happen in slow time.

While this workaround works, it doesn't actually solve the Sync client hang, so I'll leave this issue open.

from hickory-dns.

bluejekyll avatar bluejekyll commented on May 24, 2024

Maybe we need some better defaults here, but could you try setting a timeout with this method when you construct your connection for the Client? https://docs.rs/trust-dns-client/0.23.0/trust_dns_client/udp/struct.UdpClientConnection.html#method.with_timeout

from hickory-dns.

marcbrevoort-cyberhive avatar marcbrevoort-cyberhive commented on May 24, 2024

Actually the UdpClientConnection will not do - while I can make it do lookups with UdpClientConnection, WireShark shows it is not secured. I have looked for a timeout function in HttpsClientConnection but cannot find it. Is there a way I'm unaware of that allows me to do DNS over HTTPS and also set the maximum timeout to something non-infinite? Rust reports that with_timeout is not a method of HttpsClientConnection.

My current working (0.22) prototype code for a DNS lookup looks something like this -

use std::net::Ipv4Addr;
use std::str::FromStr;
use trust_dns_client::client::{SyncClient};

use tokio::runtime::Runtime;
use std::sync::Arc;
use trust_dns_client::https::HttpsClientConnection;
use trust_dns_client::op::DnsResponse;
use trust_dns_client::rr::{DNSClass, Name, RData, Record, RecordType};
use rustls::{ClientConfig, OwnedTrustAnchor, RootCertStore};
use trust_dns_proto::iocompat::AsyncIoTokioAsStd;
use trust_dns_client::client::Client;

pub fn main() {

    let name_server = "8.8.8.8:443".parse().unwrap();
    let dns_name = "example.com".to_string();

    let mut root_store = RootCertStore::empty();
    root_store.add_server_trust_anchors(webpki_roots::TLS_SERVER_ROOTS.0.iter().map(|ta| {
        OwnedTrustAnchor::from_subject_spki_name_constraints(
            ta.subject,
            ta.spki,
            ta.name_constraints,
        )
    }));

    let client_config = ClientConfig::builder()
        .with_safe_default_cipher_suites()
        .with_safe_default_kx_groups()
        .with_safe_default_protocol_versions()
        .unwrap()
        .with_root_certificates(root_store)
        .with_no_client_auth();

    let shared_client_config = std::sync::Arc::new(client_config);
    // "string" below is type of phantom data, if used
    let conn:HttpsClientConnection<AsyncIoTokioAsStd<tokio::net::TcpStream>> = HttpsClientConnection::new(name_server, "dns.google".to_string(), shared_client_config);

    let client = SyncClient::new(conn);
    let name = Name::from_ascii(&dns_name).unwrap();
    let dns_class = DNSClass::IN;
    let record_type = RecordType::A;

    let response = client.query(&name, dns_class, record_type);
    match response {
        Ok(answer) => {
            println!("ok={:?}", answer);
        }
        Err(e) => {
            println!("err Resp={:?}", e);

        }
    }
}

Note: This also works as 0.23, but requires these settings to be changed in Cargo.toml to avoid compile errors:

- rustls = { version = "0.20.9" }
+ rustls = { version = "0.21.7" }

- trust-dns-proto = { version = "0.22.0" }
+ trust-dns-proto = { version = "0.23.0" }

- trust-dns-client = { version="0.22.0", default-features = false,features=["dns-over-https", "dns-over-https-rustls"]}
+ trust-dns-client = { version="0.23.0", default-features = false,features=["dns-over-https", "dns-over-https-rustls"]}

from hickory-dns.

marcbrevoort-cyberhive avatar marcbrevoort-cyberhive commented on May 24, 2024

Have upgraded to Hickory (0.24) now. I'm still at a loss about how to set a timeout for HttpsClientConnection.
The documentation states

Use with hickory_client::client::Client impls

but I don't understand in what way this is intended (an example might be useful there). Any insights please?

from hickory-dns.

bluejekyll avatar bluejekyll commented on May 24, 2024

Hm, I was just taking a look at the http interface, and you're right, there's not timeout option to pass through on the configuration. This seems like a missing option. I had thought we had an interface like the UDP variant above, but it appears not. We should definitely add one.

from hickory-dns.

marcbrevoort-cyberhive avatar marcbrevoort-cyberhive commented on May 24, 2024

We should definitely add [with_timeout].

May I ask if there's a known timeline for this yet?

from hickory-dns.

bluejekyll avatar bluejekyll commented on May 24, 2024

I'm not sure that anyone has stepped forward to offer a patch for this at this point. I was just looking through the code, and while it would be easy to add the TCP parameters here: https://github.com/hickory-dns/hickory-dns/blob/main/crates/proto/src/h2/h2_client_stream.rs#L325C26-L325C43 and probably here: https://github.com/hickory-dns/hickory-dns/blob/main/crates/resolver/src/name_server/connection_provider.rs#L355C22-L355C22

you can see the timeout example in the standard TCP impl here: https://github.com/hickory-dns/hickory-dns/blob/main/crates/resolver/src/name_server/connection_provider.rs#L303

If we want to implement timeouts on individual operations, I think that will take more effort, and I'd recommend that we rewrite the H2 client stream into modern async/await syntax before starting to do that. Also, it looks like we need timeouts on h2, h3, and quic. I think this logic is baked into the tcp and tls impls in the same way with the DnsMultiplexer implementation, but since h2, h3, and quic have multiplexing baked in, that interface wasn't necessary for them which is probably why timeouts aren't as robust there right now. That's also probably worth reviewing to see if it offers any good methods for implementing these timeouts.

from hickory-dns.

marcbrevoort-cyberhive avatar marcbrevoort-cyberhive commented on May 24, 2024

Can I ask how the timeout is actually enforced - is it by blocks such as these?

(udp_client_stream.rs)

        S::Time::timeout::<Pin<Box<dyn Future<Output = Result<DnsResponse, ProtoError>> + Send>>>(
            self.timeout,
            Box::pin(async move {
                let socket: S = NextRandomUdpSocket::new_with_closure(&addr, creator).await?;
                send_serial_message_inner(message, message_id, verifier, socket, recv_buf_size)
                    .await
            }),
        )
        .into()

(tcp_stream.rs)

    async fn connect_with_future<F: Future<Output = Result<S, io::Error>> + Send + 'static>(
        future: F,
        name_server: SocketAddr,
        timeout: Duration,
        outbound_messages: StreamReceiver,
    ) -> Result<Self, io::Error> {
        S::Time::timeout(timeout, future)
            .map(move |tcp_stream: Result<Result<S, io::Error>, _>| {
                tcp_stream
                    .and_then(|tcp_stream| tcp_stream)
                    .map(|tcp_stream| {
                        debug!("TCP connection established to: {}", name_server);
                        Self {
                            socket: tcp_stream,
                            outbound_messages,
                            send_state: None,
                            read_state: ReadTcpState::LenBytes {
                                pos: 0,
                                bytes: [0u8; 2],
                            },
                            peer_addr: name_server,
                        }
                    })
            })
            .await
    }

from hickory-dns.

bluejekyll avatar bluejekyll commented on May 24, 2024

Yes, I think in many cases that is the case. We rely on the socket being dropped to cleanup and close all the connections. I'd have to look at all the implementations to remember how and where we did all of that. A bunch of that is older code that I haven't looked at in a while.

from hickory-dns.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.