Fix for ipv6 link local with scope

[jader-eero]

Fix for ipv6 link local with scope. If you try to connect passing address like "ipv6%wlan0" it will use

public SNIHostName(String hostname) {
        super(0, (hostname = IDN.toASCII((String)Objects.requireNonNull(hostname, "Server name value of host_name cannot be null"), 2)).getBytes(StandardCharsets.US_ASCII));
        this.hostname = hostname;
        this.checkHostName();
    }

Which raises hostname is not valid because this method doesn't accept non ASCII characters. With the change it will use

public SNIHostName(byte[] encoded) {
        super(0, encoded);

        try {
            CharsetDecoder decoder = StandardCharsets.UTF_8.newDecoder().onMalformedInput(CodingErrorAction.REPORT).onUnmappableCharacter(CodingErrorAction.REPORT);
            this.hostname = IDN.toASCII(decoder.decode(ByteBuffer.wrap(encoded)).toString());
        } catch (CharacterCodingException | RuntimeException var3) {
            throw new IllegalArgumentException("The encoded server name value is invalid", var3);
        }

        this.checkHostName();
    }

Now scoped ipv6 link is supported. 😄

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: jader-eero / name: Jader Alcântara (1b08fdc)

[ejona86]

"ipv6%wlan0" is all-ASCII. What is the non-ASCII character?

SNI should not be used with IP addresses. It is domain names only. It sounds like instead we may need logic to detect IP addresses and ignore them.

[jader-eero]

Using SNI_HOST_NAME.newInstance(string) raises The input does not conform to the STD 3 ASCII rules.. I'm not familiar with the grpc library to do what you are suggesting, but the current proposal solves the issue.

[ejona86]

Looks like that's talking about https://unicode.org/reports/tr46/#STD3_Rules or similar. So it is the % sign causing the error. It looks like you are forcing it to go through punycoding processing instead. I honestly don't know at what level punycode should be applied, but it is still the inappropriate behavior in your case. In your case SNI should not be used at all.

Easiest immediate fix I see is to skip the invoke() if InetAddresses.isInetAddress() returns true.

[jader-eero]

Using InetAddresses.isInetAddress returns false in my case, the hostname value is fe80::2ab:48ff:fe1f:402d.

[ejona86]

I added this to a test and it passed:

assertEquals(true, com.google.common.net.InetAddresses.isInetAddress("fe80::2ab:48ff:fe1f:402d%wlan0"));

It works without the %wlan0 as well.

[jader-eero]

Sorry the hostname value has brackets it's [fe80::2ab:48ff:fe1f:402d] that's why is returning false.

[jader-eero]

The brackets are needed due this issue, I think. #4278

[ejona86]

We could just throw https://guava.dev/releases/snapshot/api/docs/com/google/common/net/HostAndPort.html at this.

Although that seems a bit out-of-place. I think we might should use HostAndPort here instead of URI:

grpc-java/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java

Lines 767 to 774 in d4fa0ec

	String getOverridenHost() {
	URI uri = GrpcUtil.authorityToUri(defaultAuthority);
	if (uri.getHost() != null) {
	return uri.getHost();
	}
	return defaultAuthority;
	}

[ejona86]

Ugh, no. That would break the hostname verifier. It looks like we should be passing host+port to hostname verifier or something. I'm fine with just throwing HostAndPort in the TLS code and we kick the can down the road.

[jader-eero]

I didn't get the suggestion.

[ejona86]

Combine both methods: InetAddresses.isInetAddress(HostAndPort.fromString(host).getHost())

[jader-eero]

I'll do more test and let you know if everything went well soon.

[jader-eero]

It worked well. Thank you for the suggestion. About the checking failing, what do I need to do?

[ejona86]

I'm not all that confident that we understand how the SET_HOSTNAME path works, but this works and the SET_HOSTNAME code path may just become a noop.

[ejona86]

I think we'll ignore the lack of test that the codecov/patch status is complaining about. We could add a test to Http2OkHttpTest, but it might break in some environments. Last time we tried (many years ago) it was a pain, and I think I recall more recent cases where even IPv6 loopback is not available. It is possible to cook up an Assumes to skip the test when IPv6 is unavailable, but seems annoying. (Maybe try to bind to [::1]:0 and if we can't then skip the test?)

[jader-eero]

How long does it take to merge and have a new release with the fix?

[ejona86]

This will be backported to the release scheduled for next week.