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

Fix custom resolver implementation for Windows #255

Merged
merged 9 commits into from
Jul 29, 2019

Conversation

adferrand
Copy link
Contributor

@adferrand adferrand commented Jul 22, 2019

Pebble accepts a --dnsserver flag, that allows to select a custom DNS resolver to use when validating the ACME challenges. This option is critical to make the certbot integration tests work in particular.

Current implementation is to set the net.DefaultResolver on a new net.Resolver instance with a custom Dial property to consume the value from --dnsserver. This allows to transparently resolve any host using this custom DNS resolver from a pure-Go implementation of the DNS resolution API.

Sadly this approach does not work on Windows, and the --dnsserver has no effect on how resolution is done, and it is always the OS mechanism that is used.

One can reveal this behavior by running the following piece of code on Linux and on Windows:

// test.go
package main

import (
	"context"
	"fmt"
	"net"
)

func main() {
	resolver := &net.Resolver{
		PreferGo: true,
		Dial: func(ctx context.Context, _, _ string) (net.Conn, error) {
			d := net.Dialer{}
			return d.DialContext(ctx, "udp", "4.3.2.1:404")
		},
	}
	net.DefaultResolver = resolver
	ctx, cancelfunc := context.WithTimeout(context.Background(), 30)
	defer cancelfunc()
	_, err := resolver.LookupTXT(ctx, "stupid.entry.net")

	fmt.Printf("Error is: %s\n", err)
}

This piece of code tries to resolve a non-existent domain on a non-existent DNS server as IP 4.3.2.1:404.

On Linux, you will get the following error:

Error is: lookup stupid.entry.net on 127.0.0.53:53: dial udp 4.3.2.1:404: i/o timeout

That indicates that the system tried to reach the non-existent DNS server, and get back a timeout exception on it.

However on Windows you will get:

Error is: lookup stupid.entry.net: dnsquery: DNS name does not exist.

This indicates that the system ignored the dummy DNS server address, contacted the OS DNS resolver, that responded that the DNS name does not exist.

One can see also the reason for this behavior on Windows on the net godoc, https://godoc.org/net, in particular this line in the module introduction:

On Windows, the resolver always uses C library functions, such as GetAddrInfo and DnsQuery.

In fact, the pure Go DNS resolver is not applicable on Windows, the OS DNS resolver will be used, ignoring any customization.

Several relevant discussions, in particular a proposal (not developed yet) to make the pure Go DNS resolver available on Windows:

To fix this, this PR makes Pebble switch to a different logic:

  • if -dnsserver is not set, use the default API to resolve the names
  • if -dnsserver is set, use a dedicated DNS client, to avoid to use the OS one both on Linux and Windows

The DNS client is https://github.com/miekg/dns, a highly used and supported DNS library.

With these modifications, integrations tests on Certbot are running correctly both on Linux and Windows.

@cpu
Copy link
Contributor

cpu commented Jul 22, 2019

@adferrand Thanks for the detailed summary of the problem and a proposed PR 👍 It's a shame that the language differs in behaviour across platforms in this way.

The DNS client is https://github.com/miekg/dns, a highly used and supported DNS library.

In the past I resisted adding this dependency to Pebble. Not because of any concerns about quality (we use this dependency in both Boulder and the challtestsrv) but with the hopes we could keep the project as small and self-contained as possible.

That said, it's already there transitively by way of github.com/letsencrypt/challtestsrv and it's clearly required for Windows support. I'm happy to change positions on this and accept the new dependency for those reasons. 👍

Copy link
Contributor

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @adferrand 👍

va/va.go Outdated Show resolved Hide resolved
va/va.go Outdated Show resolved Hide resolved
va/va.go Show resolved Hide resolved
va/va.go Show resolved Hide resolved
va/va.go Outdated Show resolved Hide resolved
va/va.go Show resolved Hide resolved
va/va.go Outdated Show resolved Hide resolved
@adferrand
Copy link
Contributor Author

Also note that as soon as a new version of Go allows to use the native pure Go resolver, I am willing to make a new PR to revert this one.

va/va.go Outdated Show resolved Hide resolved
adferrand and others added 2 commits July 22, 2019 16:00
Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>
va/va.go Outdated Show resolved Hide resolved
va/va.go Outdated Show resolved Hide resolved
va/va.go Outdated Show resolved Hide resolved
va/va.go Show resolved Hide resolved
va/va.go Show resolved Hide resolved
va/va.go Outdated Show resolved Hide resolved
adferrand and others added 2 commits July 26, 2019 10:33
Co-Authored-By: Jacob Hoffman-Andrews <github@hoffman-andrews.com>
va/va.go Outdated Show resolved Hide resolved
@adferrand
Copy link
Contributor Author

Ok @jsha, I improved the error handling thanks to your comments. It is true that for a newcomer in Go, the error handling in this language is quite disturbing at the beginning ^^

Copy link
Contributor

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Thanks @adferrand

@jsha jsha merged commit ce13973 into letsencrypt:master Jul 29, 2019
@jsha
Copy link
Contributor

jsha commented Jul 29, 2019

Thanks for the PR, and for the excellent and thorough description of the problem and the solution!

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.

4 participants