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

net: LookupNS does not resolve CNAME on windows #8492

Closed
egonelbre opened this issue Aug 7, 2014 · 21 comments

Comments

Projects
None yet
4 participants
@egonelbre
Copy link
Contributor

commented Aug 7, 2014

go version devel +d50ba39e60ab Thu Aug 07 13:34:30 2014 +0400 windows/amd64

http://play.golang.org/p/sMfTegpPDk

1. run net.LookupNS("www.golang.com")
2. it does not find the NS records on Windows

It should have returned:
&{Host:ns2.google.com.}
&{Host:ns3.google.com.}
&{Host:ns4.google.com.}
&{Host:ns1.google.com.}

It seems that windows syscall.DnsQuery is not resolving the CNAME-s automatically. This
also could be my local Windows problem, so someone should check whether it behaves
similarly on other computers.

This affects all the other windows lookup*() functions as well.

I also noticed that the Windows lookupNS functions contain:

  for p := r; p != nil && p.Type == syscall.DNS_TYPE_NS; p = p.Next {

Which could abort before finding the actual record. As an example when the server
decides to return [CNAME, A, NS], but I'm not sure whether it is a valid result for
DnsQuery. (ps. this is not why the resolving in the first example)
@egonelbre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2014

Comment 1:

Actually noticed that the result is not being returned because of the for loop issue. I
did some bad analysis that made me conclude otherwise.
Also a better test would be to search net.LookupMX("transport-testing.nist.gov").
@egonelbre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2014

Comment 2:

I made a proof of concept fix: https://golang.org/cl/122200043/
It uses the same principle to take CNAME into account as OpenDNS (see
https://github.com/opendns/dynamicipupdate/blob/master/windows/src/DnsQuery.cpp).
@egonelbre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2014

Comment 3:

Regarding the OpenDNS approach, I'm not quite sure whether the check for
   if (cursor->Flags.S.Section != DnsSectionAnswer) {
and checking:
   if (DnsNameCompare(cursor->pName, name)) {
and handling:
   } else if (cursor->wType == DNS_TYPE_CNAME) {
       name = cursor->Data.CNAME.pNameHost;
Also can a DNS server (DnsQuery) return the results in arbitrary order, e.g. query for
MX d1 returns [d1 CNAME d2, d3 MX mail, d2 CNAME d3]? Or alternatively return irrelevant
in the query?
@egonelbre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2014

Comment 4:

Regarding the OpenDNS approach, I'm not quite sure whether the check for
   if (cursor->Flags.S.Section != DnsSectionAnswer) {
and checking:
   if (DnsNameCompare(cursor->pName, name)) {
and handling:
   } else if (cursor->wType == DNS_TYPE_CNAME) {
       name = cursor->Data.CNAME.pNameHost;
is sufficient/necessary.
I guess it depends whether a DNS server (DnsQuery) can return the results in arbitrary
order, e.g. query for MX d1 returns [d1 CNAME d2, d3 MX mail, d2 CNAME d3]. Or
alternatively return irrelevant values in the query, e.g. query for MX d1 returns [d2 MX
mail].
@alexbrainman

This comment has been minimized.

Copy link
Member

commented Aug 8, 2014

Comment 5:

egonelbre,
Thank you for the issue. Fix is good, but I think we should fix our tests first - tests
let us down - tests didn't complain about our problem. I think we should test more
hosts, the ones that fail. I also think we should use "nslookup -querytype=ns
www.golang.com" command to fetch results during the test - this way we can actually
verify that all our results are correct. Perhaps we should create new windows specific
tests to keep non-windows tests simple (I don't see how non-windows code could be
incorrect here). Would you like to do all that as part of your fix?
Alex

Labels changed: added repo-main, os-windows, release-go1.4.

Owner changed to @alexbrainman.

Status changed to Accepted.

@egonelbre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2014

Comment 6:

I'm not sure whether the inclusion of "nslookup" would be a good approach, it seems that
net tests try to get rid of third-party dependencies. Also if tested, then it should be
tested for all platforms, although it's less likely it's incorrect on other platforms.
Also, adding a call to nslookup would slow the tests down.
Currently I simply replaced the LookupNS("gmail.com") test with
LookupNS("www.golang.com"). I believe it should be sufficient, but if not, I can add
more comprehensive tests.
@gopherbot

This comment has been minimized.

Copy link

commented Aug 8, 2014

Comment 7:

CL https://golang.org/cl/122200043 mentions this issue.
@alexbrainman

This comment has been minimized.

Copy link
Member

commented Aug 8, 2014

Comment 8:

I am happy to hear other test suggestions (not nslookup). But I think we need better
tests then what we have now. None of current tests fail - so why are we changing code
then? You're also changing many lookup functions, why you only changing LookupNS test
only? Please, add tests for all others that fail now but will get PASSed once your
package changes are applied.
Like I said earlier, I would just use nslookup to test everything properly. You can try
to use nslookup if available and works. Do not fail the test, if you don't find nslookup
or if it fails. I am not sure about using nslookup on non-windows platforms. Surely it
won't work on plan9.
None of these tests run by the builder - builder only runs "go test -short". So new
tests won't slow anyone down, but just people who care about this code.
Also please don't remove old tests (like gmail.com), maybe create a little table of
hostnames you would like to test, and test them in a loop. I would leave current tests
alone, and write all new tests with nslookup to run on windows only in a new test file.
If it is too much for you, I will do it myself. I need proper tests before I would agree
we are making correct changes.
Alex
@egonelbre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2014

Comment 9:

Understood, I'll make a separate CL for the windows tests using nslookup.
Currently my main question would be, which hostnames to use. Would ["mail.golang.com",
"gmail.com"] be sufficient? Or should there also be a third server that as more than 1
CNAME redirect in it? Currently I wasn't able to find one.
Instead of nslookup, it could use things in dnsmsg.go to do the queries in pure Go. Of
course that would be much larger change and probably out of my competence.
@egonelbre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2014

Comment 10:

https://golang.org/cl/121450043
I wasn't able to figure out what to query to break LookupAddr and LookupSRV.
@alexbrainman

This comment has been minimized.

Copy link
Member

commented Aug 9, 2014

Comment 11:

I applied your new tests from https://golang.org/cl/121450043 to the current
tip, and they PASS (even without any net package changes). Why?
Alex
@egonelbre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2014

Comment 12:

I could get technical, but the main problem was me being stupid and not properly testing
things, sorry about that. PTAL
@alexbrainman

This comment has been minimized.

Copy link
Member

commented Aug 9, 2014

Comment 13:

Yes that FAILS now. But unfortunately, I think my nslookup output is different then
yours:
gmail.com       MX preference = 5, mail exchanger = gmail-smtp-in.l.google.com
gmail.com       MX preference = 30, mail exchanger = alt3.gmail-smtp-in.l.google.com
gmail.com       MX preference = 10, mail exchanger = alt1.gmail-smtp-in.l.google.com
gmail.com       MX preference = 40, mail exchanger = alt4.gmail-smtp-in.l.google.com
gmail.com       MX preference = 20, mail exchanger = alt2.gmail-smtp-in.l.google.com
I am using windows 7 here. Maybe my nslookup idea is not so good after all ...
Alex
@egonelbre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2014

Comment 14:

I'm getting the same result; the gmail.com version doesn't use CNAME-s so it should work
with the current version. It simply cross-validates that both get exactly the same
result as nslookup.
The results for both nslookup and the windows API calls should be the same for a given
computer, most of the time. Of course network changes and things can go down, so there
will be false negatives some times.
@alexbrainman

This comment has been minimized.

Copy link
Member

commented Aug 9, 2014

Comment 15:

I tried nslookup on my windows xp, and it looks the same as #13. Are you sure you use
windows nslookup (not unix). What windows version you're running?
Alex
@alexbrainman

This comment has been minimized.

Copy link
Member

commented Aug 9, 2014

Comment 16:

I get this:
--- FAIL: TestLookupMX (1.19s)
        lookup_windows_test.go:45: different results mail.golang.com:   exp:[]
got:[{"Host":"aspmx.l.google.com.","Pref":1},{"Host":"alt1.aspmx.l.google.com.",
"Pref":2},{"Host":"alt2.aspmx.l.google.com.","Pref":2},{"Host":"alt3.aspmx.l.goo
gle.com.","Pref":2}]
        lookup_windows_test.go:45: different results gmail.com: exp:[]  got:[{"H
ost":"gmail-smtp-in.l.google.com.","Pref":5},{"Host":"alt1.gmail-smtp-in.l.googl
e.com.","Pref":10},{"Host":"alt2.gmail-smtp-in.l.google.com.","Pref":20},{"Host"
:"alt3.gmail-smtp-in.l.google.com.","Pref":30},{"Host":"alt4.gmail-smtp-in.l.goo
gle.com.","Pref":40}]
=== RUN TestLookupNS
--- FAIL: TestLookupNS (1.50s)
        lookup_windows_test.go:69: different results "mail.golang.com": exp:[{"H
ost":"ns1.google.com"},{"Host":"ns2.google.com"},{"Host":"ns3.google.com"},{"Hos
t":"ns4.google.com"}]   got:[0xc0820036b0 0xc082003650 0xc082003670 0xc082003690
]
        lookup_windows_test.go:69: different results "gmail.com":       exp:[{"H
ost":"ns1.google.com"},{"Host":"ns2.google.com"},{"Host":"ns3.google.com"},{"Hos
t":"ns4.google.com"}]   got:[0xc0820034d0 0xc082003590 0xc0820035b0 0xc082003390
]
I think your regex does not match my nslookup output.
Alex
@egonelbre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2014

Comment 17:

I'm using Windows 7, but I noticed that I have dig installed and it contains nslookup.
C:\Windows>which nslookup
C:\Programs\dig\bin\nslookup.EXE
c:\Programs\Go\src\pkg\net>nslookup -querytype=mx gmail.com
Server:         192.168.1.254
Address:        192.168.1.254#53
Non-authoritative answer:
gmail.com       mail exchanger = 30 alt3.gmail-smtp-in.l.google.com.
gmail.com       mail exchanger = 40 alt4.gmail-smtp-in.l.google.com.
gmail.com       mail exchanger = 10 alt1.gmail-smtp-in.l.google.com.
gmail.com       mail exchanger = 20 alt2.gmail-smtp-in.l.google.com.
gmail.com       mail exchanger = 5 gmail-smtp-in.l.google.com.
I guess using nslookup path could be hardcoded to c:\Windows\system32\nslookup.exe, and
fail the tests if it's not there?
@alexbrainman

This comment has been minimized.

Copy link
Member

commented Aug 9, 2014

Comment 18:

Yes, mine lives in c:\Windows\system32\nslookup.exe. And I don't have which. Perhaps you
have cygwin installed or something. See, if you can remove C:\Programs\dig\bin from your
PATH and try again. Do you have C:\Programs\dig\bin before c:\Windows\system32 in your
PATH?
Alex
@egonelbre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2014

Comment 19:

It seems the MX and TXT syntax are different, but only MX records needed fixing. It
should now pass with both nslookup-s.
@alexbrainman

This comment has been minimized.

Copy link
Member

commented Aug 9, 2014

Comment 20:

Yes, tests work now. Can you, please, combine your both CLs into one. That is what we
always do, when find new bug. Write a new test (reproducing this bug) that FAILs, fix
the test by changing package source accordingly, then submit all changes in one CL. I
will try and do proper review of your CL tomorrow or Monday. Thank you.
Alex
@alexbrainman

This comment has been minimized.

Copy link
Member

commented Aug 15, 2014

Comment 21:

This issue was closed by revision a18a360.

Status changed to Fixed.

@rsc rsc added this to the Go1.4 milestone Apr 14, 2015

@rsc rsc removed the release-go1.4 label Apr 14, 2015

@golang golang locked and limited conversation to collaborators Jun 25, 2016

wheatman added a commit to wheatman/go-akaros that referenced this issue Jun 25, 2018

net: fix CNAME resolving on Windows
Fixes golang#8492

LGTM=alex.brainman
R=golang-codereviews, alex.brainman
CC=golang-codereviews
https://golang.org/cl/122200043

wheatman added a commit to wheatman/go-akaros that referenced this issue Jul 9, 2018

net: fix CNAME resolving on Windows
Fixes golang#8492

LGTM=alex.brainman
R=golang-codereviews, alex.brainman
CC=golang-codereviews
https://golang.org/cl/122200043

This issue was closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.