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.

Sign up for GitHub

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

Jump to bottom

net/http: use nontransitional IDNA processing #46001

Closed
TimothyGu opened this issue May 6, 2021 · 26 comments
Closed

net/http: use nontransitional IDNA processing #46001

TimothyGu opened this issue May 6, 2021 · 26 comments
Labels
FrozenDueToAge Proposal Proposal-Accepted
Projects
Proposals (old)
Milestone
Go1.18

Comments

@TimothyGu
Copy link
Contributor

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 seankhliao added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 8, 2021
@seankhliao
Copy link
Member

cc @bradfitz @rsc @empijei

@networkimprov
Copy link

@ianlancetaylor possible proposal?

@ianlancetaylor
Copy link
Contributor

I'll let net/http maintainers comment.

@networkimprov
Copy link

cc @fraenkel @neild

@favonia favonia mentioned this issue Aug 3, 2021
Closed
@neild
Copy link
Contributor

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
@gopherbot gopherbot added Proposal and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Aug 6, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Aug 6, 2021
@FiloSottile
Copy link
Contributor

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 commented Aug 8, 2021

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

@favonia
Copy link
Contributor

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 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 (old) Aug 11, 2021
@rsc
Copy link
Contributor

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.

@rsc rsc mentioned this issue Aug 11, 2021
Open
@domenic
Copy link

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 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 commented Aug 15, 2021 •
edited

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

@rsc
Copy link
Contributor

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 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 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 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 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 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 (old) Sep 8, 2021
@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Sep 15, 2021
@rsc
Copy link
Contributor

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 modified the milestones: Proposal, Backlog Sep 15, 2021
@TimothyGu
Copy link
Contributor Author

TimothyGu commented Oct 25, 2021 •
edited

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

@gopherbot
Copy link

Change https://golang.org/cl/359634 mentions this issue: internal/export/idna: use nontransitional processing in Go 1.18

gopherbot pushed a commit to golang/text that referenced this issue Nov 1, 2021
@neild
internal/export/idna: use nontransitional processing in Go 1.18
835dae6 
Updates golang/go#46001
Updates golang/go#47510

Change-Id: I1e978a3c6230abfd0b1aaab0c7343b33dda1ba64
Reviewed-on: https://go-review.googlesource.com/c/text/+/359634
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Timothy Gu <timothygu99@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/360381 mentions this issue: idna: update from x/text

gopherbot pushed a commit to golang/net that referenced this issue Nov 4, 2021
@neild
idna: update from x/text
ce13745 
CL 359634: use nontransitional processing in Go 1.18
CL 317731: fix int32 overflows

Updates golang/go#46001
Updates golang/go#47510
Updates golang/go#28233

Change-Id: I2d9cdf5caa44f7c9b2834a256e7464b6edaae9ef
Reviewed-on: https://go-review.googlesource.com/c/net/+/360381
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@neild neild modified the milestones: Backlog, Go1.18 Nov 4, 2021
@TimothyGu
Copy link
Contributor Author

This has been fixed in 77c473f.

@gopherbot
Copy link

Change https://golang.org/cl/366955 mentions this issue: doc/go1.18: add net/http IDNA change

@gopherbot
Copy link

Change https://go.dev/cl/389955 mentions this issue: _content/doc/go1.18: mention net/http IDNA change

gopherbot pushed a commit to golang/website that referenced this issue Mar 4, 2022
@ianlancetaylor
_content/doc/go1.18: mention net/http IDNA change
e52779f 
Based on CL 366955 originally by Timothy Gu.

For golang/go#46001
For golang/go#47694

Change-Id: Ide7711680d651c4cbbb6da13ab33b67cf5e26758
Reviewed-on: https://go-review.googlesource.com/c/website/+/389955
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Trust: DO NOT USE <iant@google.com>
xhit pushed a commit to xhit/text that referenced this issue Oct 10, 2022
@neild @xhit
internal/export/idna: use nontransitional processing in Go 1.18
dcedd34 
Updates golang/go#46001
Updates golang/go#47510

Change-Id: I1e978a3c6230abfd0b1aaab0c7343b33dda1ba64
Reviewed-on: https://go-review.googlesource.com/c/text/+/359634
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Timothy Gu <timothygu99@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
passionSeven added a commit to passionSeven/website that referenced this issue Oct 18, 2022
@passionSeven
_content/doc/go1.18: mention net/http IDNA change
e858d00 
Based on CL 366955 originally by Timothy Gu.

For golang/go#46001
For golang/go#47694

Change-Id: Ide7711680d651c4cbbb6da13ab33b67cf5e26758
Reviewed-on: https://go-review.googlesource.com/c/website/+/389955
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Trust: DO NOT USE <iant@google.com>
@golang golang locked and limited conversation to collaborators Mar 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal Proposal-Accepted
Projects
No open projects
Proposals (old)
Accepted
Milestone
Go1.18
Development

No branches or pull requests
10 participants
@neild @rsc @networkimprov @domenic @FiloSottile @favonia @TimothyGu @ianlancetaylor @gopherbot @seankhliao