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: LookupCNAME inconsistency on unix systems #59943

Open
mateusz834 opened this issue May 3, 2023 · 4 comments · May be fixed by #57093
Open

net: LookupCNAME inconsistency on unix systems #59943

mateusz834 opened this issue May 3, 2023 · 4 comments · May be fixed by #57093
Assignees
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@mateusz834
Copy link
Member

mateusz834 commented May 3, 2023

So there was a #50101 proposal to make the LookupCNAME consistent between unix/windows, or more concrete to make it send an explicit CNAME query, so that when the last CNAME doesn't have an ending A/AAAA record it returns the CNAME. So the change made the cgo version to use the res_search routines instead of getaddrinfo.

And that leads to some compat breaking changes that were made because of this change and some minor differences between linux/windows.

Current Issues:

  1. when cgo fails the go resolver is being used (even for noSuchHost). This returns completed == false, so a fallback to go happens (lookupCNAME). EDIT: the go resolver will also send, second (unnecessary) CNAME.

    go/src/net/cgo_unix.go

    Lines 296 to 300 in 0d34754

    func cgoLookupCNAME(ctx context.Context, name string) (cname string, err error, completed bool) {
    resources, err := resSearch(ctx, name, int(dnsmessage.TypeCNAME), int(dnsmessage.ClassINET))
    if err != nil {
    return
    }

    go/src/net/lookup_unix.go

    Lines 102 to 110 in 0d34754

    func (r *Resolver) lookupCNAME(ctx context.Context, name string) (string, error) {
    order, conf := systemConf().hostLookupOrder(r, name)
    if order == hostLookupCgo {
    if cname, err, ok := cgoLookupCNAME(ctx, name); ok {
    return cname, err
    }
    }
    return r.goLookupCNAME(ctx, name, order, conf)
    }
  2. Windows returns the last CNAME in a CNAME chain, on unix the first is returned. (Before that change the last was returned, but only when A/AAAA existed, now always the first one (assuming that the CNAMEs are in order)
  3. Unix no longer uses getaddrinfo, so when in nsswitch.conf is different in any way from: hosts: dns it returns the wrong result (different that before that change), because of the fallback to go (Issue 1) this is not really noticeable, because the go resolver handles /etc/hosts aliases correctly. (but for other nss modules it might cause problems (mdns, myhostname, resolve, ....))
  4. Cgo doesn't send also A/AAAA (like the go resolver does), so when removing the fallback to go resolver (Issue 1) then the tests start to fail, because not all domains in tests have CNAME (only A records).
  5. net: LookupNS doesn't chase through CNAMEs #44199 (comment)

I made before a CL 455275 to try address that problems.
The best solution to fix that is to do something like (for cgo resolver):
Try with getaddrinfo, if it doesn't find anything (returns errNoSuchHost) then try with res_search query for CNAME.

CC @ianlancetaylor

@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 3, 2023
@cagedmantis cagedmantis added this to the Backlog milestone May 3, 2023
@cagedmantis
Copy link
Contributor

cc @neild

@mateusz834 mateusz834 linked a pull request May 7, 2023 that will close this issue
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/455275 mentions this issue: net: rework the unified CNAME handling on unix

@ianlancetaylor
Copy link
Contributor

CC @rsc

@mateusz834
Copy link
Member Author

@rsc do you have any opinions/thoughts on this? It would be nice to fix this issue at some point. Considering that you implemented the #50101 change, I would like to hear your opinion on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants