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

Commit to not supporting IPv4-in-IPv6 addresses #86335

Merged
merged 1 commit into from
Aug 3, 2021

Conversation

CDirkx
Copy link
Contributor

@CDirkx CDirkx commented Jun 15, 2021

Stabilization of the ip feature has for a long time been blocked on the question of whether Rust should support handling "IPv4-in-IPv6" addresses: should the various Ipv6Address property methods take IPv4-mapped or IPv4-compatible addresses into account. See also the IPv4-in-IPv6 Address Support issue #85609 and #69772 which originally asked the question.

Overview

In the recent PR #85655 I proposed changing is_loopback to take IPv4-mapped addresses into account, so ::ffff:127.0.0.1 would be recognized as a looback address. However, due to the points that came up in that PR, I alternatively propose the following: Keeping the current behaviour and commit to not assigning any special meaning for IPv4-in-IPv6 addresses, other than what the standards prescribe. This would apply to the stable method is_loopback, but also to currently unstable methods like is_global and is_documentation and any future methods. This is implemented in this PR as a change in documentation, specifically the following section:

Both types of addresses are not assigned any special meaning by this implementation, other than what the relevant standards prescribe. This means that an address like ::ffff:127.0.0.1, while representing an IPv4 loopback address, is not itself an IPv6 loopback address; only ::1 is. To handle these so called "IPv4-in-IPv6" addresses, they have to first be converted to their canonical IPv4 address.

Discussion

In the discussion for or against supporting IPv4-in-IPv6 addresses the question what would be least surprising for users of other languages has come up several times. At first it seemed most big other languages supported IPv4-in-IPv6 addresses (or at least considered ::ffff:127.0.0.1 a loopback address). However after further investigation it appears that supporting IPv4-in-IPv6 addresses comes down to how a language represents addresses. .Net and Go do not have a separate type for IPv4 or IPv6 addresses, and do consider ::ffff:127.0.0.1 a loopback address. Java and Python, which do have separate types, do not consider ::ffff:127.0.0.1 a loopback address. Seeing as Rust has the separate Ipv6Addr type, it would make sense to also not support IPv4-in-IPv6 addresses. Note that this focuses on IPv4-mapped addresses, no other language handles IPv4-compatible addresses.

Another issue that was raised is how useful supporting these IPv4-in-IPv6 addresses would be in practice. Again with the example of ::ffff:127.0.0.1, considering it a loopback address isn't too useful as to use it with most of the socket APIs it has to be converted to an IPv4 address anyway. From that perspective it would be better to instead provide better ways for doing this conversion like stabilizing to_ipv4_mapped or introducing a to_canonical method.

A point in favour of not supporting IPv4-in-IPv6 addresses is that that is the behaviour Rust has always had, and that supporting it would require changing already stable functions like is_loopback. This also keeps the documentation of these functions simpler, as we only have to refer to the relevant definitions in the IPv6 specification.

Decision

To make progress on the ip feature, a decision needs to be made on whether or not to support IPv4-in-IPv6 addresses.
There are several options:

There are more options, like supporting IPv4-in-IPv6 addresses in IpAddr methods instead, but to my knowledge those haven't been seriously argued for by anyone.

There is currently an FCP ongoing on PR #85655. I would ask the libs team for an alternative FCP on this PR as well, which if completed means the rejection of PR #85655, and the decision to commit to not supporting IPv4-in-IPv6 addresses.

If anyone feels there is not enough evidence yet to make the decision for or against supporting IPv4-in-IPv6 addresses, let me know and I'll do whatever I can to resolve it.

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 15, 2021
@rustbot rustbot added A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 15, 2021
Comment on lines +1244 to +1250
/// Returns [`true`] if this is the [loopback address] (`::1`),
/// as defined in [IETF RFC 4291 section 2.5.3].
///
/// This property is defined in [IETF RFC 4291].
/// Contrary to IPv4, in IPv6 there is only one loopback address.
///
/// [IETF RFC 4291]: https://tools.ietf.org/html/rfc4291
/// [loopback address]: Ipv6Addr::LOCALHOST
/// [IETF RFC 4291 section 2.5.3]: https://tools.ietf.org/html/rfc4291#section-2.5.3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should methods like is_loopback explicitly mention that they don't consider IPv4-mapped and IPv4-compatible addresses? That is what is proposed as an alternative in #69772, but I'm worried that adding that information to almost every method on Ipv6Addr would just be noise.

Copy link
Member

Choose a reason for hiding this comment

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

This looks good to me as written. I don't think every method needs to repeat it.

@the8472
Copy link
Member

the8472 commented Jun 15, 2021

From that perspective it would be better to instead provide better ways for doing this conversion like stabilizing to_ipv4_mapped or introducing a to_canonical method.

If this PR gets accepted I'll submit a followup PR to fill that gap.

@yaahc
Copy link
Member

yaahc commented Jun 21, 2021

Since this is a stable API commitment I feel that we should FCP it, even though it's only changing docs.

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Jun 21, 2021

Team member @yaahc has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jun 21, 2021
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 9, 2021
@rfcbot
Copy link

rfcbot commented Jul 21, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jul 21, 2021
@yaahc
Copy link
Member

yaahc commented Jul 21, 2021

Checking off sfackler's checkbox since he's left the libs team.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jul 31, 2021
@rfcbot
Copy link

rfcbot commented Jul 31, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

the8472 added a commit to the8472/rust that referenced this pull request Aug 2, 2021
…them

This simplifies checking common properties in an address-family-agnostic
way since since rust-lang#86335 commits to not checking IPv4 semantics
of IPv4-mapped addresses in the `Ipv6Addr` property methods.
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks @CDirkx! This looks good to me.

@dtolnay
Copy link
Member

dtolnay commented Aug 2, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Aug 2, 2021

📌 Commit 1f2480b has been approved by dtolnay

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 2, 2021
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 2, 2021
@bors
Copy link
Contributor

bors commented Aug 3, 2021

⌛ Testing commit 1f2480b with merge 810b926...

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 3, 2021
Add convenience method for handling ipv4-mapped addresses by canonicalizing them

This simplifies checking common properties in an address-family-agnostic
way since rust-lang#86335 commits to not checking IPv4 semantics
of IPv4-mapped addresses in the `Ipv6Addr` property methods.
@bors
Copy link
Contributor

bors commented Aug 3, 2021

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing 810b926 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 3, 2021
@bors bors merged commit 810b926 into rust-lang:master Aug 3, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 3, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 3, 2021
Add convenience method for handling ipv4-mapped addresses by canonicalizing them

This simplifies checking common properties in an address-family-agnostic
way since rust-lang#86335 commits to not checking IPv4 semantics
of IPv4-mapped addresses in the `Ipv6Addr` property methods.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 3, 2021
Add convenience method for handling ipv4-mapped addresses by canonicalizing them

This simplifies checking common properties in an address-family-agnostic
way since rust-lang#86335 commits to not checking IPv4 semantics
of IPv4-mapped addresses in the `Ipv6Addr` property methods.
@apiraino
Copy link
Contributor

apiraino commented Aug 5, 2021

@rustbot label -to-announce

@rustbot rustbot removed the to-announce Announce this issue on triage meeting label Aug 5, 2021
@typetetris
Copy link

This was very surprising for me. It would be great, if the documentation of IpAddr::is_loopback would mention it doesn't consider mapped IPv4-mapped to be loopback addresses.

If you bind a server to [::]:<some port> to bind for IPv4 and IPv6 (OS feature) and connect to it via 127.0.0.1 the connection will not be considered to be on a loopback device. In my experience localhost is mapped to 127.0.0.1. There are few issues about this, so maybe it is rare, but I would guess, this will hit most people trying to detect local connections?

@the8472
Copy link
Member

the8472 commented Sep 13, 2022

there's the unstable to_canonical which handles it, the example even covers localhost checking.

@typetetris
Copy link

Why not mention it in the comment about is_loopback?

Something like

`is_loopback` doesn't detect IPv4 loopback addresses represented as IPv4-mapped IPv6 addresses.

To detect those, use `to_canonical().is_loopback()`.

@the8472
Copy link
Member

the8472 commented Sep 13, 2022

The issue isn't specific to is_loopback though, it affects all ipv4-mapped addresses, so a more general note on IpAddr itself may be better.

Anyway, this is a closed PR. Only few people will read it. You should file an issue or open a PR with improved docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.