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

x/net/idna: add pre-defined profile for non-transitional IDNA2008 #47510

Closed
favonia opened this issue Aug 3, 2021 · 19 comments
Closed

x/net/idna: add pre-defined profile for non-transitional IDNA2008 #47510

favonia opened this issue Aug 3, 2021 · 19 comments

Comments

@favonia
Copy link

@favonia favonia commented Aug 3, 2021

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

$ go version
go version go1.16.6 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//.cache/go-build"
GOENV="/home/
/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home//go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/
/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.6"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
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-build***=/tmp/go-build -gno-record-gcc-switches"

What did you do?

https://play.golang.org/p/zSGcE1no8yN

package main

import (
	"fmt"

	"golang.org/x/net/idna"
)

func main() {
	eszett, _ := idna.Lookup.ToASCII("ß")
	fmt.Printf("ß => %s", eszett)
}

What did you expect to see?

I wish that ß was kept as ß as in Firefox and Safari.

ß => ß

I am currently using the idna package to develop DNS-related tools (https://github.com/favonia/cloudflare-ddns), and I hope there is a predefined profile that uses non-transitional IDNA2008 processing. Despite the warnings that the actual Lookup profile could evolve over time, I understand that current software could rely on its specific behavior. Therefore, I propose adding a new profile IDNA2008Lookup or NontransitionalLookup (or some other good name---I am not a native English speaker anyways) as follows:

IDNA2008Lookup = &Profile{options{
	transitional: false,
	useSTD3Rules: true,
	checkHyphens: true,
	checkJoiners: true,
	trie:         trie,
	fromPuny:     validateFromPunycode,
	mapping:      validateAndMap,
	bidirule:     bidirule.ValidString,
}}

Alternatively, the idna can provide new methods to create new profiles from the existing ones. It would then be trivial to create such a profile from existing Lookup profile (especially after the bugfix https://go-review.googlesource.com/c/text/+/317729/ is merged). Here is the pseudocode demonstrating the idea:

func main() {
	eszett, _ := idna.Lookup.Derive(idna.Transitional(false)).ToASCII("ß")
	fmt.Printf("ß => %s", eszett)
}

What did you see instead?

ß is mapped to ss, as in Chrome/Chromium.

ß => ss

The Lookup profile currently implements what's called transitional processing in Unicode TS #46. It is the current behavior of Chrome/Chromium, though there is an open issue discussing a possible switch.

Related Issues

This is closely related to #46001, which proposed to change the default behavior of net/http. I like #46001 very much, but chose to propose something more conservative in case there's any objection to #46001. This proposal merely adds a new helpful definition to facilitate non-transitional processing. Whether #46001 should be accepted or in general whether the standard library and other libraries should change their behaviors is out of the scope of this issue, though those changes would probably benefit from this proposal. I strongly believe the Go language should make it easy to write software using non-transitional IDNA2008 processing even if the standard library sticks to the current behavior.

Further Justification

It is true that one can already define desirable profiles by carefully combining the correct set of options. I found the construction very unintuitive and strongly prefer a predefined profile ready to use.

Implementation Details and Auxiliary Changes

Introducing a new profile should be trivial. In fact, the above quoted code is almost all the necessary changes (modulo documentation and testing). Relatedly, the comment It is used by most browsers when resolving domain names. for Transitional is no longer accurate and should also be changed, in my opinion.

@seankhliao seankhliao changed the title x/net/idna: consider adding a pre-defined profile for non-transitional IDNA2008 proposal: x/net/idna: add pre-defined profile for non-transitional IDNA2008 Aug 3, 2021
@gopherbot gopherbot added this to the Proposal milestone Aug 3, 2021
@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Aug 3, 2021

Loading

@favonia
Copy link
Author

@favonia favonia commented Aug 3, 2021

Updates: I added the Playground link and the pseudocode demonstrating the alternative approach.

Loading

@neild
Copy link
Contributor

@neild neild commented Aug 4, 2021

Is this different than the following?

IDNA2000Lookup = idna.New(
  idna.MapForLookup(),
  idna.BidiRule(),
)

Loading

@favonia
Copy link
Author

@favonia favonia commented Aug 4, 2021

Is this different than the following?

IDNA2000Lookup = idna.New(
  idna.MapForLookup(),
  idna.BidiRule(),
)

Technically, it's the same. However, in terms of library user experience, it is quite different. I am confident in answering your question only because I have read through the entire file. I still remember that when I first checked the idna package I was very confused by the 10 different options where some seem to imply the others. For example, it was not clear to me that MapForLookup is a superset of ValidateLabels until I checked the source code. In fact, the documentation gave me the opposite impression that ValidateLabels could have done more than MapForLookup. It is also strange that I could not specifically turn off BidiRule.

A ready-to-use profile for IDNA2008 would save my past self lots of time, and this proposal is from the perspective of a new user of this library. I feel pre-defined profiles for common cases would encourage correct usage of the library and have values on their own even if, after knowing all the bits of this library, they could be defined in terms of current options. This is what I meant by "unintuitive" in my proposal. I would never know some combination is correct until I read the source code.

If I may, I really wanted to replace the Profile with bit flags and clearly define those composite options as bitwise-ors of other flags. However, I decided to maintain maximum backward compatibility in this proposal. If the maintainers of this library are in favor of such a radical reform (e.g., type Profile int), I will then agree that a user should be able to figure out the correct combinations by themselves with the new interface.

Loading

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Aug 4, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Aug 4, 2021

Perhaps we should put @neild's example explicitly into the docs?

Loading

@neild
Copy link
Contributor

@neild neild commented Aug 4, 2021

Perhaps the docs should specify exactly how each profile is constructed.

// Lookup is the recommended profile for looking up domain names, according
// to Section 5 of RFC 5891. The exact configuration of this profile may
// change over time.
//
// The current configuration is equivalent to:
//   idna.NewProfile(
//     idna.MapForLookup(),
//     idna.BidiRule(),
//     idna.Transitional(true),
//   )
Lookup *Profile = lookup

Or we could change the var definition, at the expense of needing to initialize the profile at init time rather than statically:

Lookup = New(
  idna.MapForLookup(),
  idna.BidiRule(),
  idna.Transitional(true),
)

Loading

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

@rsc rsc commented Aug 4, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

Loading

@neild
Copy link
Contributor

@neild neild commented Aug 4, 2021

I have no strong opinion on whether we should add a predefined nontransitional lookup profile, but if we do I suggest NontransitionalLookup or LookupNontransitional as the name. IDNA2008Lookup isn't correct, because the idna package as a whole implements IDNA2008.

Loading

@favonia
Copy link
Author

@favonia favonia commented Aug 4, 2021

Perhaps the docs should specify exactly how each profile is constructed.

Thank you, I think I can be satisfied with this (especially after https://go-review.googlesource.com/c/text/+/317729/ is merged) because it clearly shows how I can create non-transitional versions of existing profiles. I am also happy with any of suggested names for the new profile (if we decide to add a new one).

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Aug 11, 2021

It's unclear how #46001 will play out, but the fact that it's a question suggests that having the table available from an easy-to-call function (instead of having to copy an example) is probably worth doing.

(A function with an internal sync.Once seems better than a global var, to avoid init-time overhead for unused things.)

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Aug 18, 2021

Looking at the definition of Profile, it has multiple methods we'd have to expose if we didn't expose the Profile itself.
And it only contains plain data, so we should be able to write a literal that the linker would drop if unused.
So exporting the Profile seems fine.

The docs for Lookup say it may change from time to time. Is it time to change it to non-transitional?
If we resolve that #46001 should change, then maybe we should implement that by changing idna.Lookup here?

We could still define idna.Transitional and idna.NonTransitional for people who want an explicit choice.

It's also unclear whether Display, Registration, and Punycode need to change at the same time.
Any ideas?

/cc @mpvl

Loading

@neild
Copy link
Contributor

@neild neild commented Aug 18, 2021

The docs for Lookup say it may change from time to time. Is it time to change it to non-transitional?

If #46001 resolves in favor of using nontransitional processing in net/http, we should do that by changing idna.Lookup.

I do not believe Display, Registration, and Punycode need to change. Transitional processing only affects the lookup profile. (The docs for idna.Transitional state: "This option is only meaningful if combined with MapForLookup.")

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Aug 25, 2021

OK, sounds like this is blocked on #46001.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Sep 8, 2021

#46001 is a likely accept, so I suppose this one is too (changing the default idna.Lookup).

Loading

@rsc rsc moved this from Active to Likely Accept in Proposals Sep 8, 2021
@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

Loading

@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

Loading

@rsc rsc moved this from Likely Accept to Accepted in Proposals Sep 15, 2021
@rsc rsc changed the title proposal: x/net/idna: add pre-defined profile for non-transitional IDNA2008 x/net/idna: add pre-defined profile for non-transitional IDNA2008 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
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 29, 2021

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

Loading

gopherbot pushed a commit to golang/text that referenced this issue Nov 1, 2021
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

@gopherbot gopherbot commented Nov 1, 2021

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

Loading

gopherbot pushed a commit to golang/net that referenced this issue Nov 4, 2021
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>
@TimothyGu
Copy link
Contributor

@TimothyGu TimothyGu commented Nov 25, 2021

N.B., this has been fixed in 77c473f for Go 1.18, which changes the pre-defined Lookup profile to be non-transitional.

Loading

@favonia favonia closed this Nov 25, 2021
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
6 participants