acme/autocert: make host of TLS certificate to be obtained always Punycode #85

aofei · 2019-04-13T09:19:25Z

According to the RFC 5280, section 4.2.1.6:

...
When the subjectAltName extension contains a domain name system
label, the domain name MUST be stored in the dNSName (an IA5String).
The name MUST be in the "preferred name syntax", as specified by
Section 3.5 of [RFC1034] and as modified by Section 2.1 of
[RFC1123]. Note that while uppercase and lowercase letters are
allowed in domain names, no significance is attached to the case.
...

And the RFC 1034, section 3.5:

...
Note that while upper and lower case letters are allowed in domain
names, no significance is attached to the case. That is, two names with
the same spelling but different case are to be treated as if identical.
...

We should return the same TLS certificate for both example.com and EXAMPLE.COM. But the autocert.Manager.GetCertificate treats the two as totally different, it signs and returns two different TLS certificates.

Moreover, now the autocert.Manager.GetCertificate and the autocert.HostWhitelist can only handle Punycode IDN. If the client sends a Unicode IDN to autocert.Manager.GetCertificate (cURL is doing this), the "Invalid character in DNS name" error will be triggered.

This PR corrects these problems by converting the host of the TLS certificate to be obtained to Punycode via idna.Lookup.ToASCII.

…ercase

gopherbot · 2019-04-13T09:20:19Z

This PR (HEAD: 7b1b4fe) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/crypto/+/171997 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-04-13T09:21:00Z

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/171997.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-04-13T19:33:06Z

Message from Brad Fitzpatrick:

Patch Set 1: Run-TryBot+1

Could you add a test?

What's the ClientHello.ServerName for an IDN/punycode domain name? Is it Unicode here or punycode?

Please don’t reply on this GitHub thread. Visit golang.org/cl/171997.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-04-13T19:33:48Z

Message from Gobot Gobot:

Patch Set 1:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=7687650f

Please don’t reply on this GitHub thread. Visit golang.org/cl/171997.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-04-13T19:38:10Z

Message from Gobot Gobot:

Patch Set 1: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/171997.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-04-14T03:22:46Z

This PR (HEAD: a22ba07) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/crypto/+/171997 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-04-14T03:30:09Z

Message from Aofei Sheng:

Patch Set 2:

Patch Set 1: Run-TryBot+1

Could you add a test?

What's the ClientHello.ServerName for an IDN/punycode domain name? Is it Unicode here or punycode?

Hi Brad,

I think the ClientHelloInfo.ServerName is always Unicode. I have an IDN myself, I used it to test this patch, nothing unexpected happens.

Please don’t reply on this GitHub thread. Visit golang.org/cl/171997.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-04-14T04:57:05Z

Message from Aofei Sheng:

Patch Set 2:

Patch Set 2:

Patch Set 1: Run-TryBot+1

Could you add a test?

What's the ClientHello.ServerName for an IDN/punycode domain name? Is it Unicode here or punycode?

Hi Brad,

I think the ClientHelloInfo.ServerName is always Unicode. I have an IDN myself, I used it to test this patch, nothing unexpected happens.

Sorry, I didn't test enough, I just sent the request via cURL (it's Unicode here). When I sent the request through the browser (Safari), I got a Punycode ClientHelloInfo.ServerName. It seems that the browser has made an implicit conversion. But I think this is not the current patch should be worried about, it's safe to lowercase the Punycode hosts too, just like the idna.ToUnicode can only convert the lowercase Punycode hosts.

And BTW, I'm wondering, should we do something special for this kind of bare request (no implicit conversion to host) from cURL? Maybe an explicit conversion? Because if I send a request with an IDN via cURL now, I will get the "Invalid character in DNS name" error.

Please don’t reply on this GitHub thread. Visit golang.org/cl/171997.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-04-14T06:12:17Z

Message from Aofei Sheng:

Patch Set 2:

Patch Set 2:

Patch Set 2:

Patch Set 1: Run-TryBot+1

Could you add a test?

What's the ClientHello.ServerName for an IDN/punycode domain name? Is it Unicode here or punycode?

Hi Brad,

I think the ClientHelloInfo.ServerName is always Unicode. I have an IDN myself, I used it to test this patch, nothing unexpected happens.

Sorry, I didn't test enough, I just sent the request via cURL (it's Unicode here). When I sent the request through the browser (Safari), I got a Punycode ClientHelloInfo.ServerName. It seems that the browser has made an implicit conversion. But I think this is not the current patch should be worried about, it's safe to lowercase the Punycode hosts too, just like the idna.ToUnicode can only convert the lowercase Punycode hosts.

And BTW, I'm wondering, should we do something special for this kind of bare request (no implicit conversion to host) from cURL? Maybe an explicit conversion? Because if I send a request with an IDN via cURL now, I will get the "Invalid character in DNS name" error.

It can be like this:

name, err := idna.ToASCII(strings.ToLower(hello.ServerName))
if err != nil {
	return nil, err
}

If the hello.ServerName is already ASCII, then the idna.ToASCII will not do anything.

Please don’t reply on this GitHub thread. Visit golang.org/cl/171997.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-04-14T11:30:32Z

Message from Aofei Sheng:

Patch Set 2:

Patch Set 2:

Patch Set 2:

Patch Set 2:

Patch Set 1: Run-TryBot+1

Could you add a test?

What's the ClientHello.ServerName for an IDN/punycode domain name? Is it Unicode here or punycode?

Hi Brad,

I think the ClientHelloInfo.ServerName is always Unicode. I have an IDN myself, I used it to test this patch, nothing unexpected happens.

Sorry, I didn't test enough, I just sent the request via cURL (it's Unicode here). When I sent the request through the browser (Safari), I got a Punycode ClientHelloInfo.ServerName. It seems that the browser has made an implicit conversion. But I think this is not the current patch should be worried about, it's safe to lowercase the Punycode hosts too, just like the idna.ToUnicode can only convert the lowercase Punycode hosts.

And BTW, I'm wondering, should we do something special for this kind of bare request (no implicit conversion to host) from cURL? Maybe an explicit conversion? Because if I send a request with an IDN via cURL now, I will get the "Invalid character in DNS name" error.

It can be like this:
name, err := idna.ToASCII(strings.ToLower(hello.ServerName))
if err != nil {
	return nil, err
}
If the hello.ServerName is already ASCII, then the idna.ToASCII will not do anything.

Or this:

name, err := idna.Lookup.ToASCII(hello.ServerName) // It also handles mixedcase hosts well
if err != nil {
	return nil, err
}

I also found the "σςΣ" problem described in here. So if we want to use the idna.Profile.ToASCII, then we should to use the idna.Lookup.ToASCII instead of the idna.Punycode.ToASCII. After my test, the idna.Punycode.ToASCII can't handle the "σςΣ" problem very well.

However, it seems that the performance of the idna.Profile.ToASCII is not very good. If we use it here, then every handshake will execute it, will it have too much impact on the overall performance? If you think it can be used here and there are no other problems, then I'll push the new code.

Seems that there is a time difference between us. Anyway, I'll stay up late because of some other things tonight, so I'm waiting for your reply. :-)

Please don’t reply on this GitHub thread. Visit golang.org/cl/171997.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-04-14T13:38:35Z

This PR (HEAD: 4e9f76b) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/crypto/+/171997 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-04-14T13:42:55Z

Message from Aofei Sheng:

Patch Set 3:

I just pushed the new code so that you can understand my ideas more easily. :-)

Please don’t reply on this GitHub thread. Visit golang.org/cl/171997.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-04-14T17:24:21Z

This PR (HEAD: 6e606d9) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/crypto/+/171997 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-04-15T03:24:59Z

This PR (HEAD: 381032f) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/crypto/+/171997 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-04-22T10:22:36Z

Message from Aofei Sheng:

Patch Set 7:

Any ideas?

Please don’t reply on this GitHub thread. Visit golang.org/cl/171997.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-04-22T16:56:00Z

Message from Brad Fitzpatrick:

Patch Set 7: Run-TryBot+1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/171997.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-04-22T16:56:01Z

Message from Gobot Gobot:

Patch Set 7:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=6b486319

Please don’t reply on this GitHub thread. Visit golang.org/cl/171997.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-04-22T16:58:17Z

Message from Gobot Gobot:

Patch Set 7: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/171997.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-04-22T17:01:33Z

This PR (HEAD: 6c12694) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/crypto/+/171997 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-04-22T17:04:11Z

Message from Aofei Sheng:

Patch Set 8:

Patch Set 7: Run-TryBot+1

(1 comment)

Typo has been corrected.

Please don’t reply on this GitHub thread. Visit golang.org/cl/171997.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-04-22T17:09:29Z

Message from Aofei Sheng:

Patch Set 8:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/171997.
After addressing review feedback, remember to publish your drafts!

…ycode According to the [RFC 5280, section 4.2.1.6](https://tools.ietf.org/html/rfc5280#section-4.2.1.6): > ... > When the subjectAltName extension contains a domain name system > label, the domain name MUST be stored in the dNSName (an IA5String). > The name MUST be in the "preferred name syntax", as specified by > Section 3.5 of [RFC1034] and as modified by Section 2.1 of > [RFC1123]. Note that while uppercase and lowercase letters are > allowed in domain names, no significance is attached to the case. > ... And the [RFC 1034, section 3.5](https://tools.ietf.org/html/rfc1034#section-3.5): > ... > Note that while upper and lower case letters are allowed in domain > names, no significance is attached to the case. That is, two names with > the same spelling but different case are to be treated as if identical. > ... We should return the same TLS certificate for both `example.com` and `EXAMPLE.COM`. But the `autocert.Manager.GetCertificate` treats the two as totally different, it signs and returns two different TLS certificates. Moreover, now the `autocert.Manager.GetCertificate` and the `autocert.HostWhitelist` can only handle Punycode IDN. If the client sends a Unicode IDN to `autocert.Manager.GetCertificate` (cURL is doing this), the "Invalid character in DNS name" error will be triggered. This PR corrects these problems by converting the host of the TLS certificate to be obtained to Punycode via `idna.Lookup.ToASCII`. Change-Id: I993821b3a6ae532a53772e2db00524479ef111af GitHub-Last-Rev: 6c12694 GitHub-Pull-Request: #85 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/171997 Reviewed-by: Brad Fitzpatrick <[email protected]>

gopherbot · 2019-04-22T17:19:59Z

This PR is being closed because golang.org/cl/171997 has been merged.

…ycode According to the [RFC 5280, section 4.2.1.6](https://tools.ietf.org/html/rfc5280#section-4.2.1.6): > ... > When the subjectAltName extension contains a domain name system > label, the domain name MUST be stored in the dNSName (an IA5String). > The name MUST be in the "preferred name syntax", as specified by > Section 3.5 of [RFC1034] and as modified by Section 2.1 of > [RFC1123]. Note that while uppercase and lowercase letters are > allowed in domain names, no significance is attached to the case. > ... And the [RFC 1034, section 3.5](https://tools.ietf.org/html/rfc1034#section-3.5): > ... > Note that while upper and lower case letters are allowed in domain > names, no significance is attached to the case. That is, two names with > the same spelling but different case are to be treated as if identical. > ... We should return the same TLS certificate for both `example.com` and `EXAMPLE.COM`. But the `autocert.Manager.GetCertificate` treats the two as totally different, it signs and returns two different TLS certificates. Moreover, now the `autocert.Manager.GetCertificate` and the `autocert.HostWhitelist` can only handle Punycode IDN. If the client sends a Unicode IDN to `autocert.Manager.GetCertificate` (cURL is doing this), the "Invalid character in DNS name" error will be triggered. This PR corrects these problems by converting the host of the TLS certificate to be obtained to Punycode via `idna.Lookup.ToASCII`. Change-Id: I993821b3a6ae532a53772e2db00524479ef111af GitHub-Last-Rev: 6c12694574b6ac71a920da8a8bdb130c0b6e0272 GitHub-Pull-Request: golang/crypto#85 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/171997 Reviewed-by: Brad Fitzpatrick <[email protected]>

…ycode According to the [RFC 5280, section 4.2.1.6](https://tools.ietf.org/html/rfc5280#section-4.2.1.6): > ... > When the subjectAltName extension contains a domain name system > label, the domain name MUST be stored in the dNSName (an IA5String). > The name MUST be in the "preferred name syntax", as specified by > Section 3.5 of [RFC1034] and as modified by Section 2.1 of > [RFC1123]. Note that while uppercase and lowercase letters are > allowed in domain names, no significance is attached to the case. > ... And the [RFC 1034, section 3.5](https://tools.ietf.org/html/rfc1034#section-3.5): > ... > Note that while upper and lower case letters are allowed in domain > names, no significance is attached to the case. That is, two names with > the same spelling but different case are to be treated as if identical. > ... We should return the same TLS certificate for both `example.com` and `EXAMPLE.COM`. But the `autocert.Manager.GetCertificate` treats the two as totally different, it signs and returns two different TLS certificates. Moreover, now the `autocert.Manager.GetCertificate` and the `autocert.HostWhitelist` can only handle Punycode IDN. If the client sends a Unicode IDN to `autocert.Manager.GetCertificate` (cURL is doing this), the "Invalid character in DNS name" error will be triggered. This PR corrects these problems by converting the host of the TLS certificate to be obtained to Punycode via `idna.Lookup.ToASCII`. Change-Id: I993821b3a6ae532a53772e2db00524479ef111af GitHub-Last-Rev: 6c12694 GitHub-Pull-Request: golang#85 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/171997 Reviewed-by: Brad Fitzpatrick <[email protected]>

…ycode According to the [RFC 5280, section 4.2.1.6](https://tools.ietf.org/html/rfc5280#section-4.2.1.6): > ... > When the subjectAltName extension contains a domain name system > label, the domain name MUST be stored in the dNSName (an IA5String). > The name MUST be in the "preferred name syntax", as specified by > Section 3.5 of [RFC1034] and as modified by Section 2.1 of > [RFC1123]. Note that while uppercase and lowercase letters are > allowed in domain names, no significance is attached to the case. > ... And the [RFC 1034, section 3.5](https://tools.ietf.org/html/rfc1034#section-3.5): > ... > Note that while upper and lower case letters are allowed in domain > names, no significance is attached to the case. That is, two names with > the same spelling but different case are to be treated as if identical. > ... We should return the same TLS certificate for both `example.com` and `EXAMPLE.COM`. But the `autocert.Manager.GetCertificate` treats the two as totally different, it signs and returns two different TLS certificates. Moreover, now the `autocert.Manager.GetCertificate` and the `autocert.HostWhitelist` can only handle Punycode IDN. If the client sends a Unicode IDN to `autocert.Manager.GetCertificate` (cURL is doing this), the "Invalid character in DNS name" error will be triggered. This PR corrects these problems by converting the host of the TLS certificate to be obtained to Punycode via `idna.Lookup.ToASCII`. Change-Id: I993821b3a6ae532a53772e2db00524479ef111af GitHub-Last-Rev: 6c12694574b6ac71a920da8a8bdb130c0b6e0272 GitHub-Pull-Request: golang/crypto#85 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/171997 Reviewed-by: Brad Fitzpatrick <[email protected]>

acme/autocert: make host of TLS certificate to be obtained always low…

7b1b4fe

…ercase

googlebot added the cla: yes label Apr 13, 2019

aofei added 2 commits April 14, 2019 11:16

acme/autocert: add tests for obtaining mixedcase certificates

329d5cd

acme/autocert: update docs of HostWhitelist

a22ba07

acme/autocert: add support for RFC 3492 to Manager.GetCertificate

4e9f76b

aofei changed the title ~~acme/autocert: make host of TLS certificate to be obtained always lowercase~~ acme/autocert: make host of TLS certificate to be obtained always Punycode Apr 15, 2019

acme/autocert: correct typo in comments

6c12694

gopherbot closed this Apr 22, 2019

acme/autocert: make host of TLS certificate to be obtained always Punycode #85

acme/autocert: make host of TLS certificate to be obtained always Punycode #85

Uh oh!

Conversation

aofei commented Apr 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gopherbot commented Apr 13, 2019

Uh oh!

gopherbot commented Apr 13, 2019

Uh oh!

gopherbot commented Apr 13, 2019

Uh oh!

gopherbot commented Apr 13, 2019

Uh oh!

gopherbot commented Apr 13, 2019

Uh oh!

gopherbot commented Apr 14, 2019

Uh oh!

gopherbot commented Apr 14, 2019

Uh oh!

gopherbot commented Apr 14, 2019

Uh oh!

gopherbot commented Apr 14, 2019

Uh oh!

gopherbot commented Apr 14, 2019

Uh oh!

gopherbot commented Apr 14, 2019

Uh oh!

gopherbot commented Apr 14, 2019

Uh oh!

gopherbot commented Apr 14, 2019

Uh oh!

gopherbot commented Apr 15, 2019

Uh oh!

gopherbot commented Apr 22, 2019

Uh oh!

gopherbot commented Apr 22, 2019

Uh oh!

gopherbot commented Apr 22, 2019

Uh oh!

gopherbot commented Apr 22, 2019

Uh oh!

gopherbot commented Apr 22, 2019

Uh oh!

gopherbot commented Apr 22, 2019

Uh oh!

gopherbot commented Apr 22, 2019

Uh oh!

gopherbot commented Apr 22, 2019

Uh oh!

Uh oh!

aofei commented Apr 13, 2019 •

edited

Loading