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/http: use nontransitional IDNA processing #46001

Open
TimothyGu opened this issue May 6, 2021 · 21 comments
Open

net/http: use nontransitional IDNA processing #46001

TimothyGu opened this issue May 6, 2021 · 21 comments

Comments

@TimothyGu
Copy link

@TimothyGu TimothyGu commented May 6, 2021

What version of Go are you using (go version)?

$ go version
go version go1.16.3 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/timothy-gu/.cache/go-build"
GOENV="/home/timothy-gu/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/timothy-gu/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/timothy-gu/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16.3"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/tmp/gourl/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build343371698=/tmp/go-build -gno-record-gcc-switches"

What did you do?

package main

import (
	"fmt"
	"net/http"
)

func main() {
	resp, _ := http.Get("http://straße.de/")
	fmt.Println("straße", resp.Header.Get("Content-Length"))

	resp, _ = http.Get("http://strasse.de/")
	fmt.Println("strasse", resp.Header.Get("Content-Length"))

	resp, _ = http.Get("http://xn--strae-oqa.de/")
	fmt.Println("xn--strae-oqa", resp.Header.Get("Content-Length"))
}

What did you expect to see?

straße.de should get resolved to xn--strae-oqa per IDNA Nontransitional processing and DENIC announcement. This is the case on Firefox and Safari when you type the URL into the address bar. It is also the behavior of curl, wget, Node.js, and the browser WHATWG URL Standard.

straße 33560
strasse 6637
xn--strae-oqa 33560

What did you see instead?

straße.de got resolved instead to strasse.de. This is the current behavior in Chrome, and from what Ryan Sleevi implied in https://crbug.com/694157, other pieces of Google software as well. (The same behavior is in Python as well, but that's a bit unfair, since Python's IDNA encoding is effectively frozen to IDNA 2003.)

straße 6637
strasse 6637
xn--strae-oqa 33560

Discussion

Which way to go here is ultimately a policy change. The arguments for keeping transitional processing are:

  • Maintains strict compatibility with previous versions of Go
  • Maintains compatibility with Chrome, and Google products in general

The arguments for moving to nontransitional processing are:

  • Aligns with the rest of the world
  • Aligns with web standards

It appears to me that implementing things from web standards and first principles is part of the Go ethos, which would lean in favor of using nontransitional processing. However, maintaining compatibility is also a powerful motivation.

@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented May 8, 2021

@networkimprov
Copy link

@networkimprov networkimprov commented May 16, 2021

@ianlancetaylor possible proposal?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 17, 2021

I'll let net/http maintainers comment.

@networkimprov
Copy link

@networkimprov networkimprov commented May 17, 2021

@neild
Copy link
Contributor

@neild neild commented Aug 6, 2021

Empirical testing of navigating to http://straße.de/ in various browsers (version whatever was installed on the machine closest to me at the time):

  • Chrome: strasse.de
  • Safari: xn--strae-oqa.de
  • Firefox: xn--strae-oqa.de
  • Edge: strasse.de

My most-convenient Linux box has curl 7.74.0 with IDNA support and resolves http://straße.de/ to xn--strae-oqa.de. I also found CVE-2016-8625 for libcurl, describing the use of transitional processing in libcurl before version 7.51.0 as a security vulnerability.

There appears to be no consistency in browser behavior.

However, given that:

  • the .de registry has been issuing domains containing ß for about a decade
  • many browsers (and client libraries like libcurl) do use nontransitional processing
  • nontransitional processing is the desired end state

it seems reasonable to me to change the "golang.org/x/net/idna".Lookup profile default to use nontransitional processing.

Marking this as a proposal.

cc @FiloSottile for possible security considerations.

@neild neild changed the title net/http: consider using nontransitional IDNA processing proposal: net/http: use nontransitional IDNA processing Aug 6, 2021
@gopherbot gopherbot added this to the Proposal milestone Aug 6, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Aug 6, 2021
@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Aug 7, 2021

I wouldn’t go as far as calling the current behavior a security vulnerability, but unexpected and spec-divergent behavior is a security liability, so I generally support the change.

This fits in a pattern of implementation choices that haven’t kept up with the changing ecosystem, to the point of becoming dangerously idiosyncratic (like most recently semicolons in URL queries). We don’t have an exception for them in the Compatibility Promise, but maybe we should discuss adding one to ensure the Go standard library doesn’t become riddled with legacy surprising behaviors.

By the way, if we make this change, we should document not only the new behavior but also how it will change and will be kept up to date.

@rsc
Copy link
Contributor

@rsc rsc commented Aug 8, 2021

What is Chrome's rationale for not making this change?

@favonia
Copy link

@favonia favonia commented Aug 8, 2021

What is Chrome's rationale for not making this change?

FYI: There have been multiple issues on Chromium over the years about transitional v.s. non-transitional processings. The latest issue (that was not merged, blocked, or closed) is https://bugs.chromium.org/p/chromium/issues/detail?id=694157 There was an older one closed as WONTFIX: https://bugs.chromium.org/p/chromium/issues/detail?id=505262

@neild
Copy link
Contributor

@neild neild commented Aug 8, 2021

I have not been able to find a conclusive rationale for Chrome not making this change. That doesn't mean there isn't one, of course, just that I can't find it.

So far as I can tell, the primary concern that led to the introduction of transitional processing is that the case-insensitivity of domain names introduces an ambiguity: Uppercase ß is SS, so uppercasing straße.de and strasse.de ambiguously produce STRASSE.DE.

Of course, the current situation is that lowercase straße.de produces ambiguous results depending on which HTTP client you use, so avoiding ambiguity entirely does not presently appear to be an option.

By the way, if we make this change, we should document not only the new behavior but also how it will change and will be kept up to date.

We currently document that the idna.Lookup and idna.Display profiles may change over time, so I believe that a change to nontransitional processing would be within the scope of our documented compatibility promises.

I don't know what a good policy for future changes would be. It seems to me that the lack of a clearly correct policy has led to the current fragmentation between browsers. A simple and clear one for us would be to defer the decision to some other party--e.g., always do what Chrome does by default.

@rsc rsc moved this from Incoming to Active in Proposals Aug 11, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Aug 11, 2021

Agreeing with Chrome carries a lot of weight with me.
If Chrome is using this behavior, it can't be that much of a security hole.

@domenic
Copy link

@domenic domenic commented Aug 15, 2021

In case it helps, the Chrome team considers the current behavior (and any other misalignments with the URL Standard) to be a bug. You can track our meta-bug for this effort at https://bugs.chromium.org/p/chromium/issues/detail?id=660384.

@rsc
Copy link
Contributor

@rsc rsc commented Aug 15, 2021

Considering it a bug and considering it appropriate to change are clearly two different things, and it appears that for now at least Chrome does not consider it appropriate to change. The specific issue for nontransitional IDNA seems to be https://bugs.chromium.org/p/chromium/issues/detail?id=694157.

I'm still reluctant to make the change before Chrome. There must be a reason they haven't done it yet.

@domenic
Copy link

@domenic domenic commented Aug 15, 2021

The reason is engineering resources. (Perhaps I wasn't clear; I'm a member of the Chrome team.)

@rsc
Copy link
Contributor

@rsc rsc commented Aug 18, 2021

The reason is engineering resources. (Perhaps I wasn't clear; I'm a member of the Chrome team.)

Understood, but why is the change costly in engineering resources? It should be something like a 1-line change, no?
I'm trying to get at what the underlying impact or cost is that is keeping Chrome from doing this.
Presumably there is worry about changing behavior in various cases, subtle bugs, and so on?

@rsc
Copy link
Contributor

@rsc rsc commented Aug 25, 2021

@domenic is there a planned Chrome milestone when a conversion will happen?
Are there any concerns about breaking users by making the change?

@domenic
Copy link

@domenic domenic commented Aug 25, 2021

There is no planned milestone; as I said this is not something we're devoting engineering resources to.

There's no significant concern about the change since we would be matching Safari and the spec.

@rsc
Copy link
Contributor

@rsc rsc commented Sep 1, 2021

Thanks @domenic. If there's no significant concern then maybe we should just make the change.
Any objections to doing that?

@rsc
Copy link
Contributor

@rsc rsc commented Sep 8, 2021

This sounds like a likely accept, implemented by also accepting and implementing #47510 (changing the default).

@rsc
Copy link
Contributor

@rsc rsc commented Sep 8, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals Sep 8, 2021
@rsc rsc moved this from Likely Accept to Accepted in Proposals Sep 15, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Sep 15, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: net/http: use nontransitional IDNA processing net/http: use nontransitional IDNA processing Sep 15, 2021
@rsc rsc removed this from the Proposal milestone Sep 15, 2021
@rsc rsc added this to the Backlog milestone Sep 15, 2021
@TimothyGu
Copy link
Author

@TimothyGu TimothyGu commented Oct 25, 2021

This is blocked by #30940. We cannot properly choose between transitional/nontransitional right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants