From 6ae357d393978566133a91aa1d4207b4f67da2c1 Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Thu, 1 Nov 2018 20:16:39 +0000 Subject: [PATCH] Revert doh (#800) * Revert "Require URLs for DOH addresses (#684)" This reverts commit 8ccae88257a8cec016f407e36517a0861e33dda8. * Revert "WIP: DNS-over-HTTPS support for Client.Exchange API (#671)" This reverts commit 64746df23bbe418a542e5940c2cdf11291134819. Signed-off-by: Miek Gieben * Finish revert of DoH Signed-off-by: Miek Gieben * Add back in the race condition comment Signed-off-by: Miek Gieben --- client.go | 83 +------------------------------------------------- client_test.go | 20 ------------ leak_test.go | 1 - 3 files changed, 1 insertion(+), 103 deletions(-) diff --git a/client.go b/client.go index 63ced2bd..770a946c 100644 --- a/client.go +++ b/client.go @@ -7,11 +7,8 @@ import ( "context" "crypto/tls" "encoding/binary" - "fmt" "io" - "io/ioutil" "net" - "net/http" "strings" "time" ) @@ -19,8 +16,6 @@ import ( const ( dnsTimeout time.Duration = 2 * time.Second tcpIdleTimeout time.Duration = 8 * time.Second - - dohMimeType = "application/dns-message" ) // A Conn represents a connection to a DNS server. @@ -44,7 +39,6 @@ type Client struct { DialTimeout time.Duration // net.DialTimeout, defaults to 2 seconds, or net.Dialer.Timeout if expiring earlier - overridden by Timeout when that value is non-zero ReadTimeout time.Duration // net.Conn.SetReadTimeout value for connections, defaults to 2 seconds - overridden by Timeout when that value is non-zero WriteTimeout time.Duration // net.Conn.SetWriteTimeout value for connections, defaults to 2 seconds - overridden by Timeout when that value is non-zero - HTTPClient *http.Client // The http.Client to use for DNS-over-HTTPS TsigSecret map[string]string // secret(s) for Tsig map[], zonename must be in canonical form (lowercase, fqdn, see RFC 4034 Section 6.2) SingleInflight bool // if true suppress multiple outstanding queries for the same Qname, Qtype and Qclass group singleflight @@ -132,11 +126,6 @@ func (c *Client) Dial(address string) (conn *Conn, err error) { // attribute appropriately func (c *Client) Exchange(m *Msg, address string) (r *Msg, rtt time.Duration, err error) { if !c.SingleInflight { - if c.Net == "https" { - // TODO(tmthrgd): pipe timeouts into exchangeDOH - return c.exchangeDOH(context.TODO(), m, address) - } - return c.exchange(m, address) } @@ -149,11 +138,6 @@ func (c *Client) Exchange(m *Msg, address string) (r *Msg, rtt time.Duration, er cl = cl1 } r, rtt, err, shared := c.group.Do(m.Question[0].Name+t+cl, func() (*Msg, time.Duration, error) { - if c.Net == "https" { - // TODO(tmthrgd): pipe timeouts into exchangeDOH - return c.exchangeDOH(context.TODO(), m, address) - } - return c.exchange(m, address) }) if r != nil && shared { @@ -199,67 +183,6 @@ func (c *Client) exchange(m *Msg, a string) (r *Msg, rtt time.Duration, err erro return r, rtt, err } -func (c *Client) exchangeDOH(ctx context.Context, m *Msg, a string) (r *Msg, rtt time.Duration, err error) { - p, err := m.Pack() - if err != nil { - return nil, 0, err - } - - req, err := http.NewRequest(http.MethodPost, a, bytes.NewReader(p)) - if err != nil { - return nil, 0, err - } - - req.Header.Set("Content-Type", dohMimeType) - req.Header.Set("Accept", dohMimeType) - - hc := http.DefaultClient - if c.HTTPClient != nil { - hc = c.HTTPClient - } - - if ctx != context.Background() && ctx != context.TODO() { - req = req.WithContext(ctx) - } - - t := time.Now() - - resp, err := hc.Do(req) - if err != nil { - return nil, 0, err - } - defer closeHTTPBody(resp.Body) - - if resp.StatusCode != http.StatusOK { - return nil, 0, fmt.Errorf("dns: server returned HTTP %d error: %q", resp.StatusCode, resp.Status) - } - - if ct := resp.Header.Get("Content-Type"); ct != dohMimeType { - return nil, 0, fmt.Errorf("dns: unexpected Content-Type %q; expected %q", ct, dohMimeType) - } - - p, err = ioutil.ReadAll(resp.Body) - if err != nil { - return nil, 0, err - } - - rtt = time.Since(t) - - r = new(Msg) - if err := r.Unpack(p); err != nil { - return r, 0, err - } - - // TODO: TSIG? Is it even supported over DoH? - - return r, rtt, nil -} - -func closeHTTPBody(r io.ReadCloser) error { - io.Copy(ioutil.Discard, io.LimitReader(r, 8<<20)) - return r.Close() -} - // ReadMsg reads a message from the connection co. // If the received message contains a TSIG record the transaction signature // is verified. This method always tries to return the message, however if an @@ -559,10 +482,6 @@ func DialTimeoutWithTLS(network, address string, tlsConfig *tls.Config, timeout // context, if present. If there is both a context deadline and a configured // timeout on the client, the earliest of the two takes effect. func (c *Client) ExchangeContext(ctx context.Context, m *Msg, a string) (r *Msg, rtt time.Duration, err error) { - if !c.SingleInflight && c.Net == "https" { - return c.exchangeDOH(ctx, m, a) - } - var timeout time.Duration if deadline, ok := ctx.Deadline(); !ok { timeout = 0 @@ -571,7 +490,7 @@ func (c *Client) ExchangeContext(ctx context.Context, m *Msg, a string) (r *Msg, } // not passing the context to the underlying calls, as the API does not support // context. For timeouts you should set up Client.Dialer and call Client.Exchange. - // TODO(tmthrgd): this is a race condition + // TODO(tmthrgd,miekg): this is a race condition. c.Dialer = &net.Dialer{Timeout: timeout} return c.Exchange(m, a) } diff --git a/client_test.go b/client_test.go index 63a90afd..6b1d787b 100644 --- a/client_test.go +++ b/client_test.go @@ -589,23 +589,3 @@ func TestConcurrentExchanges(t *testing.T) { } } } - -func TestDoHExchange(t *testing.T) { - const addrstr = "https://dns.cloudflare.com/dns-query" - - m := new(Msg) - m.SetQuestion("miek.nl.", TypeSOA) - - cl := &Client{Net: "https"} - - r, _, err := cl.Exchange(m, addrstr) - if err != nil { - t.Fatalf("failed to exchange: %v", err) - } - - if r == nil || r.Rcode != RcodeSuccess { - t.Errorf("failed to get an valid answer\n%v", r) - } - - // TODO: proper tests for this -} diff --git a/leak_test.go b/leak_test.go index ff83ac74..af37011d 100644 --- a/leak_test.go +++ b/leak_test.go @@ -29,7 +29,6 @@ func interestingGoroutines() (gs []string) { strings.Contains(stack, "closeWriteAndWait") || strings.Contains(stack, "testing.Main(") || strings.Contains(stack, "testing.(*T).Run(") || - strings.Contains(stack, "created by net/http.(*http2Transport).newClientConn") || // These only show up with GOTRACEBACK=2; Issue 5005 (comment 28) strings.Contains(stack, "runtime.goexit") || strings.Contains(stack, "created by runtime.gc") ||