Add -4 and -6 flags #1802

jsumners · 2023-01-07T15:10:00Z

If completed, this PR will resolve #1801. At the moment I'm opening a draft PR to gauge interest in the overall proposal.

(outdated lint errors)

I do not understand these lint errors:

cmd/flags.go:12: File is not `gci`-ed with --skip-generated -s standard,default (gci)
                        Name: "ipv4only",
cmd/flags.go:14: File is not `gci`-ed with --skip-generated -s standard,default (gci)
                        Usage: "Use IPv4 only. This flag is ignored if ipv6only is also specified.",
cmd/flags.go:17: File is not `gci`-ed with --skip-generated -s standard,default (gci)
                        Name: "ipv6only",
cmd/flags.go:19: File is not `gci`-ed with --skip-generated -s standard,default (gci)
                        Usage: "Use IPv6 only. This flag is ignored if ipv4only is also specified.",

I do not know what "gci-ed" means.

I also don't understand this one because recursiveNameservers is also a global:

challenge/dns01/nameserver.go:31:5: networkStack is a global variable (gochecknoglobals)
var networkStack = "both"

I'm open to this not being a global if I can get some guidance on how it should be passed in from the CLI flags processing down to where the DNS query is actually invoked.

Items that remain:

Update

lego/cmd/setup_challenges.go

Line 44 in e982d5a

func setupHTTPProvider(ctx *cli.Context) challenge.Provider {
Update

lego/cmd/setup_challenges.go

Line 86 in e982d5a

func setupTLSProvider(ctx *cli.Context) challenge.Provider {

dmke · 2023-01-07T18:54:51Z

challenge/dns01/nameserver.go

+// networkStack is used to define which IP stack will be used. The default is
+// both IPv4 and IPv6. Set to "ipv4" for IPv4 only, and "ipv6" for IPv6 only.
+var networkStack = "both"


I believe an enum would be better here:

Suggested change

// networkStack is used to define which IP stack will be used. The default is

// both IPv4 and IPv6. Set to "ipv4" for IPv4 only, and "ipv6" for IPv6 only.

var networkStack = "both"

type networkStack int

const (

DefaultNetworkStack networkStack = iota

IPv4Only

IPv6Only

)

// currentNetworkStack is used to define which IP stack will be used. The default is

// both IPv4 and IPv6. Set to IPv4Only or IPv6Only to select either version.

var currentNetworkStack = DefaultNetworkStack

I'm not a fan of a Go "enum": it's not a real enum, it's just a weak list of values.

@ldez what do you want to see?

As this PR currently only updates the dns01 package (and leaves http01 and tlsalpn01 as a TODO), maybe we can find a way to pass the stack selection to dns01.NewChallenge, http01.NewChallenge and tlsalpn01.NewChallenge as an option?

Looking at dns01.NewChallenge, it already accepts a vararg for options:

lego/challenge/resolver/solver_manager.go

Lines 51 to 54 in 1cad41d

func (c *SolverManager) SetDNS01Provider(p challenge.Provider, opts ...dns01.ChallengeOption) error {

c.solvers[challenge.DNS01] = dns01.NewChallenge(c.core, validate, p, opts...)

return nil

}

Maybe the networkStack can move into the cmd package (in whatever form), and passing the selection as option to the challenge solver can be done (centrally) in setupChallenges?

lego/cmd/setup_challenges.go

Lines 25 to 26 in 1cad41d

if ctx.Bool("http") {

err := client.Challenge.SetHTTP01Provider(setupHTTPProvider(ctx))

(and leaves http01 and tlsalpn01 as a TODO)

It does not leave them as todo items. This PR is in draft because I wanted guidance prior to actually working on the full solution. At this point I'm getting conflicting feedback and would like a clear direction. I need this feature and am willing to put in the work, but I don't want to flail around to get it done.

Sorry for the miscommunication, English isn't my first language. I didn't mean "left as TODO for others", but "for this to work, we need to think of http01 and tlsalpn01 as well".

jsumners · 2023-01-16T21:31:41Z

I know that that the code in the dns challenge is working as intended as I was able to test it in the situation where I need the patch via a local build.

I didn't receive any further feedback on how we should avoid the enum, so I continued with that construct. I do not believe I am missing any places where these switches would apply, but I don't know the code base that well. Please point out if I have missed any.

dmke · 2023-01-18T10:08:03Z

I'll try to review this on the weekend, I'm currently a bit constrained on time.

jsumners · 2023-01-18T21:51:28Z

😕 This test runs cleanly locally https://github.com/go-acme/lego/actions/runs/3934066967/jobs/6757026356#step:5:278

jsumners · 2023-01-18T23:11:37Z

Yeah, I think these tests are flaky. The same suite failed after my last push, but this time on a different domain name after passing the domain name that failed in the prior run.

https://github.com/go-acme/lego/actions/runs/3953224067/jobs/6769527173#step:5:279

ldez · 2023-01-18T23:21:58Z

I don't think those tests are flaky, because I have the same problem locally.

Can you also lint the code? (make checks)

Can you run make locally?

jsumners · 2023-01-19T19:14:05Z

I don't think those tests are flaky, because I have the same problem locally.

The issue was running that subtest in isolation worked, but failed when run with all other subtests. In other words, my added test needed to have a t.Cleanup added to restore the expected state for that subtest.

Can you also lint the code? (make checks)

Can you run make locally?

These should be passing now.

By setting the port to 0, we defer the actual port selection to the OS (and thus we're guaranteed to use a free port). This eliminates an external dependency (jsumners/go-getport), which tried to do the same, but has a race condition between it returning an unused socket address, and us using that address in our tests. When instructing dns.Server to use port 0, the source for the local port depends on the protocol: for UDP this is (*dns.Server).PacketConn, and for TCP we find it in (*dns.Server).Listener.

- The miekg/dns package has, similar to net/http, a dns.HandlerFunc helper, which turns a plain function into a handler. This eliminates a type. - For static v4 addresses, net.IPv4() is better than net.ParseIP(), as parsing is not needed.

In general, require.NoError() should be preferred to assert.NoError(), especially if further assertions follow after that. The stretchr/testify/assert package does not call (*testing.T).FailNow() when the assertion fails (and stop the test), which can lead to pretty unreadable error message, e.g. when the rest of the test code panics.

Introducing a new parameter to an exported (public) function changes the API and requires a major version bump. Remember that Lego is used as library, e.g. as part of the Traefic app proxy. It is possible to preserve the API by extending the interface on the ProviderServer structs though.

…rface Exporting the network stack type allow the consumer to create custom, but likely disfunct values, which might cause panics when used. This changes the API to use dedicated setter functions instead, and reuses the same names accross the dns01, http01 and tlsalpn01 packages: - SetIPv4Only() - SetIPv6Only() - SetDualStack()

dmke · 2023-01-22T00:52:38Z

@jsumners: I've added a bunch of review commits.

The commit messages contain some reasoning, but to reiterate my major changes:

Removal of your go-getport package. I do appreciate the effort, but that code is actually racy: After calling getport.GetPort() and before server.ListenAndServe() the OS is allowed and able to reassign the detected port to another process.

We can remove that dependency and a bunch of code by directly using port 0, and retrieve the actual port directly from either the server.Listener (TCP) or server.PacketConn (UDP).
Keeping the API stable. Lego is used as library (notably in the Traefic project), so we can't easily change the exported (public) API. Doing so requires more than a new CLI option :)

Luckily, we can extend the API without much problem, which is related to my third point.
Better hiding of the network stack options. @ldez doesn't particularly like iota-constants, and I've mentioned a problem with public types used for internal states (consumers could easily define their own, custom, defunct values, and cause panics in Lego this way).

The solution I've come up with is to extend the interface and provide distinct functions to set the network stack (SetIPv4Only, SetIPv6Only, SetDualStack). While I welcome bike-shedding for the names, I will point out that all challenge packages use the same names.

jsumners · 2023-01-25T14:33:26Z

Removal of your go-getport package. I do appreciate the effort, but that code is actually racy: After calling getport.GetPort() and before server.ListenAndServe() the OS is allowed and able to reassign the detected port to another process.

That is explicitly noted in the docs: https://github.com/jsumners/go-getport/blob/ef4d1d0da783a1930d39b868161a94609ab08c71/getport.go#L55-L58. It also has no bearing in these tests. In CI, they are isolated. Running locally, it's extremely unlikely that anything will acquire a port in the nanoseconds between finding the open port and the using it for a test. Still, 🤷‍♂️.

3. Better hiding of the network stack options. @ldez doesn't particularly like iota-constants, and I've mentioned a problem with public types used for internal states

I received no further feedback on which direction to take after asking for it. So I went with the only feedback that had been provided.

3. The solution I've come up with is to extend the interface and provide distinct functions to set the network stack (SetIPv4Only, SetIPv6Only, SetDualStack).

This is fine by me.

Seems like net.DialContext on Windows produces a different error message... I'll look into that tomorrow.

I don't have a Windows environment for testing. All I know is that my last commit was passing with the GitHub CI.

Regarding that statement about "my last commit". I'm rather put off by changes being made directly to the branch. I think they could have all been review comments with some suggestions (item 6 in https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request#adding-line-comments-to-a-pull-request) along the way.

I feel like at this point the PR is yours @dmke. I still want to see this added, as the functionality has solved a problem out of my control (my ISP having a bad IPv6 implementation) and likely solves several other issues as stated in the original proposal. So I can provide feedback if desired.

dmke · 2023-01-25T17:33:24Z

Seems like net.DialContext on Windows produces a different error message... I'll look into that tomorrow.

I don't have a Windows environment for testing. All I know is that my last commit was passing with the GitHub CI.

The failure was introduced here:

 func sendDNSQuery(m *dns.Msg, ns string) (*dns.Msg, error) {
-	udp := &dns.Client{Net: "udp", Timeout: dnsTimeout}
+	network := getNetwork("udp")
+	udp := &dns.Client{Net: network, Timeout: dnsTimeout}
 	in, _, err := udp.Exchange(m, ns)
 
-	if in != nil && in.Truncated {
-		tcp := &dns.Client{Net: "tcp", Timeout: dnsTimeout}
+	network = getNetwork("tcp")
+	// We can encounter a net.OpError if the nameserver is not listening
+	// on UDP at all, i.e. net.Dial could not make a connection.
+	_, isOpErr := err.(*net.OpError)
+	if (in != nil && in.Truncated) || isOpErr {
+		tcp := &dns.Client{Net: network, Timeout: dnsTimeout}
 		// If the TCP request succeeds, the err will reset to nil
 		in, _, err = tcp.Exchange(m, ns)
 	}

(You've changed the tests for this here; for that commit I forgot to approve a CI run though.)

The issue is that the TCP-retry-branch is now taken in more cases, as the previous error ("read udp") also is a net.OpError. Windows has an optimized syscall, ConnectEx, for TCP connections, hence the different error message (the fault location, for reference).

Tackling this is also a bit cumbersome, as a CI run takes forever to complete, and I haven't had a chance to revive my local Windows VM (I've also felt under the whether the last few days, so my capacity for patience to let Windows do its thing was much reduced).

I think they could have all been review comments with some suggestions
[...]
I feel like at this point the PR is yours @dmke.

Sorry about that, I didn't want to scare you off. I've started a review the normal way, but it quickly turned into a wild mess of cross-comment-references. Also, review suggestions can only be applied to changed lines, not arbitrary places.

It turned out to be much cleaner to leave review commits and show the changes one-by-one, in separated contexts. In the end the commits will be squashed anyway, so the outcome would be the same.

There's a small annoyance between GOOS=linux and GOOS=windows for net.DialContext when a TCP connection fails, due to the naming of the syscalls involved: On Linux, connect(2) is used, which can cause ECONNREFUSED, while the Windows equivalent is the ConnectEx syscall, which can cause WSAECONNREFUSED. The error generated by nameservers.FindZoneByFqdnCustom and FindPrimaryNsByFqdnCustom may hence contain a different error trance (one containing "connect:", the other "connectex:"). This commit tries to address that, by using a simple regular expression to match either error message.

dmke · 2023-01-25T23:59:43Z

I've spent the last 7 hours in my Windows VM to get the tests green again. Turns out I hate Windows Defender with a passion.

I guess on the CI, Defender is active as well (log from here), and slows down operations considerably:

panic: test timed out after 10m0s
...
goroutine 232 [IO wait]:
internal/poll.runtime_pollWait(0x14f3a5a4ed8, 0x72)
	C:/hostedtoolcache/windows/go/1.18.10/x64/src/runtime/netpoll.go:302 +0x89
internal/poll.(*pollDesc).wait(0x10734ae?, 0x1153ab6?, 0x0)
	C:/hostedtoolcache/windows/go/1.18.10/x64/src/internal/poll/fd_poll_runtime.go:83 +0x32
internal/poll.execIO(0xc0003a3b98, 0x1228d10)
	C:/hostedtoolcache/windows/go/1.18.10/x64/src/internal/poll/fd_windows.go:175 +0xe5
internal/poll.(*FD).Read(0xc0003a3b80, {0xc0003b2000, 0x1000, 0x1000})
	C:/hostedtoolcache/windows/go/1.18.10/x64/src/internal/poll/fd_windows.go:441 +0x25f
net.(*netFD).Read(0xc0003a3b80, {0xc0003b2000?, 0xc0003a3c59?, 0x1292a98?})
	C:/hostedtoolcache/windows/go/1.18.10/x64/src/net/fd_posix.go:55 +0x29
net.(*conn).Read(0xc000006518, {0xc0003b2000?, 0xc0003b2000?, 0x0?})
	C:/hostedtoolcache/windows/go/1.18.10/x64/src/net/net.go:183 +0x45
github.com/miekg/dns.(*Conn).Read(0xc0002dbdc0, {0xc0003b2000, 0x1000, 0x1000})
	C:/Users/runneradmin/go/pkg/mod/github.com/miekg/[email protected]/client.go:340 +0x162
github.com/miekg/dns.(*Conn).ReadMsgHeader(0xc0002dbdc0, 0x0)
	C:/Users/runneradmin/go/pkg/mod/github.com/miekg/[email protected]/client.go:304 +0xb7
github.com/miekg/dns.(*Conn).ReadMsg(0xc0002dbdc0)
	C:/Users/runneradmin/go/pkg/mod/github.com/miekg/[email protected]/client.go:269 +0x27
github.com/miekg/dns.(*Client).exchangeContext(0xc0002e5f80, {0x128fdc8, 0xc00009a0b8}, 0xc000358900, 0xc0002dbdc0)
	C:/Users/runneradmin/go/pkg/mod/github.com/miekg/[email protected]/client.go:246 +0x445
github.com/miekg/dns.(*Client).exchangeWithConnContext(0x0?, {0x128fdc8?, 0xc00009a0b8?}, 0x0?, 0x0?)
	C:/Users/runneradmin/go/pkg/mod/github.com/miekg/[email protected]/client.go:195 +0x1d7
github.com/miekg/dns.(*Client).ExchangeWithConn(0xc0002e5f80?, 0x128fdc8?, 0xc00009a0b8?)
	C:/Users/runneradmin/go/pkg/mod/github.com/miekg/[email protected]/client.go:190 +0x30
github.com/miekg/dns.(*Client).Exchange(0x128d620?, 0xc00038ea00?, {0x11f218c?, 0xc000006510?})
	C:/Users/runneradmin/go/pkg/mod/github.com/miekg/[email protected]/client.go:170 +0x10e
github.com/go-acme/lego/v4/challenge/dns01.sendDNSQuery(0x11f6850?, {0x11f218c, 0x5})
	D:/a/lego/lego/challenge/dns01/nameserver.go:255 +0x136
github.com/go-acme/lego/v4/challenge/dns01.dnsQuery({0x11f6850?, 0x10?}, 0x20?, {0x14917c0, 0x3, 0xc00036e0f0?}, 0xf8?)
	D:/a/lego/lego/challenge/dns01/nameserver.go:232 +0xaa
github.com/go-acme/lego/v4/challenge/dns01.fetchSoaByFqdn({0x11f6844, 0x10}, {0x14917c0, 0x3, 0x3})
	D:/a/lego/lego/challenge/dns01/nameserver.go:177 +0x14e
github.com/go-acme/lego/v4/challenge/dns01.lookupSoaByFqdn({0x11f6844, 0x10}, {0x14917c0, 0x3, 0x3})
	D:/a/lego/lego/challenge/dns01/nameserver.go:160 +0x14f
github.com/go-acme/lego/v4/challenge/dns01.FindPrimaryNsByFqdnCustom({0x11f6844?, 0x13fdcf8?}, {0x14917c0?, 0xf?, 0x12ea146?})
	D:/a/lego/lego/challenge/dns01/nameserver.go:128 +0x2d
github.com/go-acme/lego/v4/challenge/dns01.TestFindPrimaryNsByFqdnCustom.func1(0x0?)
	D:/a/lego/lego/challenge/dns01/nameserver_test.go:282 +0x65
testing.tRunner(0xc000372000, 0xc00032b410)
	C:/hostedtoolcache/windows/go/1.18.10/x64/src/testing/testing.go:1439 +0x102
created by testing.(*T).Run
	C:/hostedtoolcache/windows/go/1.18.10/x64/src/testing/testing.go:1486 +0x35f
FAIL	github.com/go-acme/lego/v4/challenge/dns01	600.045s

In nameserver.go:255, we're calling (*dns.Client).Exchange, with client.Net == "udp" (i.e. before the TCP/ConnectEx retry-branch) and a timeout set to 20s. Clearly this timeout does not have an effect.

I'm baffled. This part of the code didn't change, how come it stopped working?

@ldez, do you have an idea?

(I'm frustrated enough to consider proposing to downgrade Windows support for Lego to "experimental", and drop Windows from the CI workflow altogether... Maybe I should sleep on this for a bit before doing so :/)

jsumners · 2023-01-26T04:40:19Z

I've spent the last 7 hours in my Windows VM to get the tests green again.

😨

jsumners · 2023-02-01T20:59:17Z

What can I do to help get this to the finish line?

ldez · 2023-02-02T00:44:10Z

I will spend some time on that in the following days

dmke · 2023-02-02T08:39:17Z

I'm currently investigating as too why Windows performs abysmally poor, and seeking ways to improve that. However, that's going very slowly...

@dmke