Skip to content
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

feat(client): allow connecting to IPv6 link-local addresses #1937

Closed
wants to merge 2 commits into from

Conversation

ComputerDruid
Copy link
Contributor

No description provided.

@ComputerDruid
Copy link
Contributor Author

Note: requires hyperium/http#343 to work (patch in Cargo.toml)

@@ -22,7 +22,7 @@ use crate::common::{Future, Never, Pin, Poll, Unpin, task};
/// Resolve a hostname to a set of IP addresses.
pub trait Resolve: Unpin {
/// The set of IP addresses to try to connect to.
type Addrs: Iterator<Item=IpAddr>;
type Addrs: Iterator<Item=SocketAddr>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is modifying a public trait, so it'd be really nice if this could be considered for 0.13 inclusion, otherwise we might need to wait for the next minor version to not break users.

src/client/connect/dns.rs Show resolved Hide resolved
src/client/connect/dns.rs Outdated Show resolved Hide resolved
@ComputerDruid
Copy link
Contributor Author

Addressed the review comments, so this is ready for review again.

Uploaded as a separate commit, I can of course squash/rebase if desired.

@seanmonstar
Copy link
Member

I'm not up-to-speed on IPv6 zone IDs. It identifies a specific port? Or, why is this changing Resolve to return a SocketAddr instead of an IpAddr?

@ComputerDruid
Copy link
Contributor Author

It returns a SocketAddr because it needs SocketAddrV6.scope_id to be set correctly. https://doc.rust-lang.org/std/net/struct.SocketAddrV6.html#method.scope_id , which is used to determine which network interface to use to make the connection. Since generally all interfaces have fe80::/64 link-local addresses, and link-local addresses are non-routable, there needs to be some disambiguation.

The only standard way I found that can turn a zone_id (on Linux, that's the interface name) into a scope_id (on Linux, the interface index) is getaddrinfo.

Some relevant info I found in man pages while I was digging: http://man7.org/linux/man-pages/man7/ipv6.7.html

sin6_scope_id is an ID depending on the scope of the address. It is new in Linux 2.4. Linux supports it only for link-local addresses, in that case sin6_scope_id contains the interface index (see netdevice(7))

The interface index can also be found in /proc/net/if_inet6 (where it's the second column). I don't use that in this CL since I'd rather use standard APIs, and getaddrinfo seems to do the trick.

);

assert_eq!(addr.map(|a| format!("{:?}", a)), None);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need a test to make sure clients don't pass the scope to servers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean a test in this PR that hyper remembers to strip it somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, at least in the HOST header (not sure if we need to do this elsewhere). Specifically in the HOST header (but anywhere else we might be sending the URL to a remote server). Otherwise, this could potentially open up some attack surfaces. I think rfc7404 speaks more about this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, wrong rfc. The right one is rfc6874, which states in section 4:

An HTTP client, proxy, or other intermediary MUST remove any ZoneID attached to an outgoing URI, as it has only local significance at the sending host.

@seanmonstar
Copy link
Member

It returns a SocketAddr because it needs SocketAddrV6.scope_id to be set correctly. https://doc.rust-lang.org/std/net/struct.SocketAddrV6.html#method.scope_id , which is used to determine which network interface to use to make the connection.

I see, thanks for explaining! So, does this mean IPv6 addresses essentially allow encoding HttpConnector::set_local_addresses in a per-destination way?

@seanmonstar
Copy link
Member

Since this has a breaking change, it'd need to be merged before 0.13 is released (target is early next week). I still don't have my head around it entirely, but it sounds like this extra info cannot exist in an IpAddr, only a SocketAddr?

@seanmonstar seanmonstar modified the milestone: 0.13 Dec 6, 2019
@seanmonstar seanmonstar added the B-breaking-change Blocked: this is an "API breaking change". label Dec 10, 2019
@a0145
Copy link

a0145 commented Jan 17, 2020

Is this change still on the table? Without it, it doesn't seem possible to use a link-local address with hyper with more than one link-local nic.

I still don't have my head around it entirely, but it sounds like this extra info cannot exist in an IpAddr, only a SocketAddr?

Per the rust implementation of SocketAddrV6, the scope id is only contained in there. It's not necessarily IP info but "connection setup" info which describes which interface to use for the connection, which is necessary when multiple link-local interfaces are present.

I see, thanks for explaining! So, does this mean IPv6 addresses essentially allow encoding HttpConnector::set_local_addresses in a per-destination way?

If there were an equivalent HttpConnector::set_local_interface(iface_id), technically that is what this notation does per destination. By specifying the zone/interface, it's possible to select which interface to use for the connection which is necessary to disambiguate where you want to connect to.

Ref: https://tools.ietf.org/html/rfc4007#section-11
Ref: https://en.wikipedia.org/wiki/Link-local_address#IPv6

@seanmonstar
Copy link
Member

Is this change still on the table?

I'm still open to the change, it was mentioned by contributors they wanted to add tests. Also, at this point, it'd be a breaking change to the "resolver" API, so either a way to provide it without breakage is needed, or this needs to wait till the next major version.

If there were an equivalent HttpConnector::set_local_interface(iface_id), technically that is what this notation does per destination.

Thanks, I understand now. Sounds useful!

@seanmonstar seanmonstar added this to the 0.14 milestone Oct 9, 2020
seanmonstar added a commit that referenced this pull request Nov 26, 2020
The DNS resolver part of `HttpConnector` used to require resolving to
`IpAddr`s, and this changes it so that they resolve to `SocketAddr`s.
The main benefit here is allowing for resolvers to set the IPv6 zone ID
when resolving, but it also just more closely matches
`std::net::ToSocketAddrs`.

Closes #1937

BREAKING CHANGE: Custom resolvers used with `HttpConnector` must change
  to resolving to an iterator of `SocketAddr`s instead of `IpAddr`s.
@seanmonstar
Copy link
Member

So, as part of the breaking changes in 0.14, I'm pulling part of this change in #2346: the resolvers should return SocketAddrs instead of IpAddrs. I didn't want to pull in percent_decoding to decode a tiny part of the authority, but I believe a custom resolver can handle that if needed.

seanmonstar added a commit that referenced this pull request Dec 3, 2020
The DNS resolver part of `HttpConnector` used to require resolving to
`IpAddr`s, and this changes it so that they resolve to `SocketAddr`s.
The main benefit here is allowing for resolvers to set the IPv6 zone ID
when resolving, but it also just more closely matches
`std::net::ToSocketAddrs`.

Closes #1937

BREAKING CHANGE: Custom resolvers used with `HttpConnector` must change
  to resolving to an iterator of `SocketAddr`s instead of `IpAddr`s.
BenxiangGe pushed a commit to BenxiangGe/hyper that referenced this pull request Jul 26, 2021
…#2346)

The DNS resolver part of `HttpConnector` used to require resolving to
`IpAddr`s, and this changes it so that they resolve to `SocketAddr`s.
The main benefit here is allowing for resolvers to set the IPv6 zone ID
when resolving, but it also just more closely matches
`std::net::ToSocketAddrs`.

Closes hyperium#1937

BREAKING CHANGE: Custom resolvers used with `HttpConnector` must change
  to resolving to an iterator of `SocketAddr`s instead of `IpAddr`s.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-breaking-change Blocked: this is an "API breaking change".
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants