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

Add IDN TLD support #2278

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@patf
Contributor

patf commented Oct 22, 2016

publicsuffix-go's Find expects suffixes to be in unicode rather than punycode, causing IDN TLDs to be rejected. Convert domains to unicode prior to checking whether the domain ends in an ICANN suffix.

Fixes #2277.

Add IDN TLD support
publicsuffix-go's Find expects suffixes to be in unicode rather
than punycode, causing IDN TLDs to be rejected. Convert domains
to unicode prior to checking whether the domain ends in an ICANN
suffix.
@patf

This comment has been minimized.

Show comment
Hide comment
@patf

patf Oct 23, 2016

Contributor

Further testing revealed that the rate-limiting code in ra.go could be affected by this too. domainsForRateLimiting correctly handles punycoded IDN suffixes with one label:

domainsForRateLimiting([]string{"example.xn--fiqs8s"}) == "example.xn--fiqs8s"

However, it fails for suffixes with multiple IDN labels (.xn--o1ach.xn--90a3ac or упр.срб being a suffix like co.uk):

domainsForRateLimiting([]string{"example.xn--o1ach.xn--90a3ac"}) == "xn--o1ach.xn--90a3ac"

(This is not too bad, since there aren't exactly too many multi-label IDN suffixes on the list and the only effect would be that rate limiting scope is applied too strictly, but we might as well get it right everywhere.)

Converting to unicode would fix this issue as well (though we'd have to convert back to punycode for storage in order for the rate-limit identifier to remain backwards-compatible), but I'm wondering whether handling this conversion (and preserving the encoding for the return values, I guess?) should be done in publicsuffix-go rather than the calling code. @weppos: Any thoughts on this?

Contributor

patf commented Oct 23, 2016

Further testing revealed that the rate-limiting code in ra.go could be affected by this too. domainsForRateLimiting correctly handles punycoded IDN suffixes with one label:

domainsForRateLimiting([]string{"example.xn--fiqs8s"}) == "example.xn--fiqs8s"

However, it fails for suffixes with multiple IDN labels (.xn--o1ach.xn--90a3ac or упр.срб being a suffix like co.uk):

domainsForRateLimiting([]string{"example.xn--o1ach.xn--90a3ac"}) == "xn--o1ach.xn--90a3ac"

(This is not too bad, since there aren't exactly too many multi-label IDN suffixes on the list and the only effect would be that rate limiting scope is applied too strictly, but we might as well get it right everywhere.)

Converting to unicode would fix this issue as well (though we'd have to convert back to punycode for storage in order for the rate-limit identifier to remain backwards-compatible), but I'm wondering whether handling this conversion (and preserving the encoding for the return values, I guess?) should be done in publicsuffix-go rather than the calling code. @weppos: Any thoughts on this?

@wangqiliang wangqiliang referenced this pull request Oct 23, 2016

Closed

IDN support #331

@wensheng

This comment has been minimized.

Show comment
Hide comment
@wensheng

wensheng Oct 23, 2016

Please merge this.

I was excited by the announcement of IDN support. But when I tried it, it give me:

The request message was malformed :: Name does not end in a public suffix

thx

wensheng commented Oct 23, 2016

Please merge this.

I was excited by the announcement of IDN support. But when I tried it, it give me:

The request message was malformed :: Name does not end in a public suffix

thx

@jsha

This comment has been minimized.

Show comment
Hide comment
@jsha

jsha Oct 24, 2016

Contributor

My preference would be for publicsuffix-go to accept A-labels rather than U-labels. That way we don't have to convert back and forth.

Contributor

jsha commented Oct 24, 2016

My preference would be for publicsuffix-go to accept A-labels rather than U-labels. That way we don't have to convert back and forth.

@kachkaev

This comment has been minimized.

Show comment
Hide comment
@kachkaev

kachkaev Oct 24, 2016

I agree with @jsha. U-labels are less likely to be supported by other tools, so it is more reasonable to define host names with ASCII characters inside ENV variables and configs. If Letsencrypt expects Unicode, some developers will end up having the same thing defined twice.

kachkaev commented Oct 24, 2016

I agree with @jsha. U-labels are less likely to be supported by other tools, so it is more reasonable to define host names with ASCII characters inside ENV variables and configs. If Letsencrypt expects Unicode, some developers will end up having the same thing defined twice.

@jsha jsha referenced this pull request Oct 24, 2016

Closed

Accept A-labels #31

@jsha

This comment has been minimized.

Show comment
Hide comment
@jsha

jsha Oct 25, 2016

Contributor

@patf regarding the domainsForRateLimiting issue, to clarify for other readers xn--o1ach.xn--90a3ac (упр.срб) is a public suffix, so the second example should return example.xn--o1ach.xn--90a3ac.

Contributor

jsha commented Oct 25, 2016

@patf regarding the domainsForRateLimiting issue, to clarify for other readers xn--o1ach.xn--90a3ac (упр.срб) is a public suffix, so the second example should return example.xn--o1ach.xn--90a3ac.

@cpu

This comment has been minimized.

Show comment
Hide comment
@cpu

cpu Nov 21, 2016

Member

Hi @patf - thanks for putting this together. I really appreciate that you took the time to arrive with a solution in addition to a problem. You can't beat that! ❤️

That said I think we're all in agreement that upstream support is better than special-casing in Boulder. I'm going to close this PR and would love if you could give #2339 a 🔍

Member

cpu commented Nov 21, 2016

Hi @patf - thanks for putting this together. I really appreciate that you took the time to arrive with a solution in addition to a problem. You can't beat that! ❤️

That said I think we're all in agreement that upstream support is better than special-casing in Boulder. I'm going to close this PR and would love if you could give #2339 a 🔍

@cpu cpu closed this Nov 21, 2016

@cpu cpu removed the status/blocked/dep label Nov 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment