Repository navigation
fix(workloadapi): accept IPv6 literals in tcp Workload API addresses - #481
Conversation
maxlambrecht
left a comment
maxlambrecht
left a comment
There was a problem hiding this comment.
Thanks for fixing this. There’s one validation issue to address before merging.
| // URI.getHost() returns IPv6 literals enclosed in square brackets (e.g. '[::1]') | ||
| boolean ipValid; | ||
| ipValid = InetAddresses.isInetAddress(parsedAddress.getHost()); | ||
| ipValid = InetAddresses.isUriInetAddress(parsedAddress.getHost()); |
There was a problem hiding this comment.
isUriInetAddress() ignores the zone suffix during validation, so this also accepts scoped addresses. The original host then reaches gRPC unchanged.
For example, tcp://[fe80::1%25lo0]:8000 passes this check, but Java interprets the interface name as 25lo0, and resolution fails with no such interface.
The Workload Endpoint spec references RFC 3986, which excludes zone IDs. I’d keep this change limited to ordinary IPv6 literals and reject hosts containing %:
| ipValid = InetAddresses.isUriInetAddress(parsedAddress.getHost()); | |
| String host = parsedAddress.getHost(); | |
| boolean ipValid = host.indexOf('%') < 0 && InetAddresses.isUriInetAddress(host); |
There was a problem hiding this comment.
Good catch, hosts containing % are now rejected before the isUriInetAddress check, as you suggested. b34da7d
| Arguments.of("tcp://john:doe@1.2.3.4:5/path", "Workload endpoint tcp socket URI must not include user info: tcp://john:doe@1.2.3.4:5/path"), | ||
| Arguments.of("tcp://foo:9000", "Workload endpoint tcp socket URI host component must be an IP:port: tcp://foo:9000"), | ||
| Arguments.of("tcp://1.2.3.4", "Workload endpoint tcp socket URI host component must include a port: tcp://1.2.3.4") | ||
| Arguments.of("tcp://[foo]:9000", "Workload endpoint socket is not a valid URI: tcp://[foo]:9000"), |
There was a problem hiding this comment.
Could you also add rejected cases for tcp://[fe80::1%eth0]:8000 and tcp://[fe80::1%25eth0]:8000 to cover the scope restriction?
The [foo] case is useful malformed-input coverage, but java.net.URI rejects it before it reaches the changed check. An accepted tcp://[::ffff:127.0.0.1]:8000 case would also be useful coverage for IPv4-mapped IPv6.
There was a problem hiding this comment.
Added both zone ID cases as rejected and tcp://[::ffff:127.0.0.1]:8000 as accepted. I kept the [foo] case as malformed input coverage.
The SPIFFE Workload Endpoint standard requires the host of a tcp://
socket address to be an IP address, which includes IPv6. An IPv6 literal
in a URI has to be written in square brackets (tcp://[::1]:8000), and
java.net.URI.getHost() returns it with the brackets kept ("[::1]").
Address.validateTcpAddress passed that value to Guava's
InetAddresses.isInetAddress, which does not accept brackets. So every
IPv6 address was rejected with "host component must be an IP:port", and
DefaultWorkloadApiClient, DefaultX509Source, DefaultJwtSource and the
helper could not connect to an agent listening on IPv6. (Before
commons-validator was removed, its InetAddressValidator rejected
bracketed hosts in the same way.)
Use InetAddresses.isUriInetAddress instead. It accepts IPv4 literals and
bracketed IPv6 literals, which is exactly what URI.getHost() returns, and
still rejects hostnames. The gRPC channel factories already pass the
bracketed host to NettyChannelBuilder.forAddress, which resolves it
correctly, so no change is needed there.
AddressTest gains valid IPv6 cases (loopback and a global address) and
an IPv6 address without a port, which is still rejected.
Signed-off-by: NopeCharon <115794254+NopeCharon@users.noreply.github.com>
InetAddresses.isUriInetAddress ignores a zone suffix, so a host such as [fe80::1%eth0] passed validation and was handed to gRPC unchanged, where the scope could be misread (e.g. %25eth0 as interface "25eth0"). RFC 3986, which the Workload Endpoint standard references, does not allow zone IDs, so reject hosts that contain '%'. AddressTest gains rejected cases for zone IDs and an accepted case for an IPv4-mapped IPv6 address. Signed-off-by: NopeCharon <115794254+NopeCharon@users.noreply.github.com>
2c17327 to
b34da7d
Compare
What
Accept IPv6 literals in
tcp://Workload API addresses, e.g.tcp://[::1]:8000.Address.validateTcpAddressnow validates the host with Guava'sInetAddresses.isUriInetAddressinstead ofisInetAddress.Fixes #478
Why
URI.getHost()returns IPv6 hosts with their brackets ("[::1]"), whichisInetAddressrejects, so every IPv6 TCP address failed withhost component must be an IP:port. The Workload Endpoint standard only requires an IP address, and go-spiffe accepts these addresses.isUriInetAddressaccepts exactly whatURI.getHost()returns (IPv4 and bracketed IPv6) and still rejects hostnames. The channel factories already pass the bracketed host toNettyChannelBuilder.forAddress, which handles it, so validation is the only change needed.How tested
AddressTest: new valid casestcp://[::1]:8000andtcp://[2001:db8::1]:8000, plustcp://[::1](missing port, still rejected) andtcp://[foo]:9000(invalid, still rejected). The valid cases fail without this change.FakeWorkloadApion a TCP port and createdDefaultWorkloadApiClientwithtcp://[::1]:<port>.fetchX509Context()returned the SVID with the fix. Without it,newClientfailed with theSocketEndpointAddressExceptionabove../gradlew :java-spiffe-core:buildon JDK 17.