Add support for IDNA #640
Add support for IDNA #640
Conversation
roidelapluie
force-pushed
the
dina
branch
2 times, most recently
from
a5f9121 to
474f012
Compare
prober/utils.go
Outdated
| @@ -119,3 +126,14 @@ func ipHash(ip net.IP) float64 { | |||
| h.Write(ip) | |||
| return float64(h.Sum32()) | |||
| } | |||
|
|
|||
| func internationalizeDNSTarget(logger log.Logger, target string) (string, error) { | |||
| idnaTarget, err := idna.ToASCII(target) | |||
There was a problem hiding this comment.
Should we use the lookup profile here rather than the raw punycode?
There was a problem hiding this comment.
Updated to use idna.Lookup.ToASCII
|
CI doesn't seem happy, it's timing out. |
|
Yes, locally as well |
|
I have fixed the test |
|
|
||
| func internationalizeDNSDomain(logger log.Logger, domain string) string { | ||
| idnaDomain, err := idna.Lookup.ToASCII(domain) | ||
| if err != nil { |
There was a problem hiding this comment.
I don't think we should be silently ignoring errors.
There was a problem hiding this comment.
Somehow we pass :: in some tests, which creates an error disallowed rune U+003A. I need to investigate why, but I was thinking that it would be safe to ignore those errors.
There was a problem hiding this comment.
I see what's going on here.
You have to try to handle the hostname as an IP address (either IPv4 or IPv6), net.ParseIP will do this for you, and if that fails handle it as a hostname and apply a punycode transformation to it.
Basically:
if net.ParseIP(domain) != nil {
return domain
}
idnaDomain, err := idna.Lookup.ToASCII(domain)
...
|
:: is an ipv6 version of 0.0.0.0, so it's a valid IP address. |
Signed-off-by: Julien Pivotto <[email protected]>
| @@ -248,11 +248,13 @@ func ProbeDNS(ctx context.Context, target string, module config.Module, registry | |||
| } | |||
| } | |||
|
|
|||
| queryName := internationalizeDNSDomain(logger, module.DNS.QueryName) | |||
There was a problem hiding this comment.
I'm not sure this is correct, let's leave this out.
|
Ping, this needs a rebase. |
|
@SuperQ I'm not the original author but I gave it a rebase here d6f6a341b4ad7b21886855739a7e9cd292b6e446 and tests seem to pass. Let me know if you need me to open my own MR. |
|
Nevermind, I didn't realise this doesn't affect the http prober. Opened #1192. |
Fixes #626
Signed-off-by: Julien Pivotto [email protected]