Add support for IPv6 scoped addresses (RFC4007) #15263

foxxx0 · 2024-12-10T12:07:28Z

In order to translate interface names in such scoped addresses the
required LibC binding to if_nametoindex() has been added.
This method obviously only works for interfaces (devices) that are
actually present on the system.

The binding for the reverse operation if_indextoname() has also been
added, although its usage is a bit more cumbersome due to LibC::Char*
buffer handling. The necessary buffer length has been placed into the
constant LibC::IF_NAMESIZE, which appears to be 16u8 on unix-like
systems and 257u16 on windows.
This could potentially be reworked via a preprocessor block at
compile-time as indicated by some folks over on discord, I currently do
not know how to achieve that though.

Scoped identifiers are only valid for link-local (fe80::) addresses, e.g. fe80::1%eth0

References:

https://datatracker.ietf.org/doc/html/rfc4007

Fixes #15264 .

straight-shoota · 2024-12-10T12:26:04Z

Could you please open an issue as a feature request first? We ask this to separate high-level discussion of the feature (in the issue discussion) from details of this specific PR (PR discussion). Ref: https://github.com/crystal-lang/crystal/blob/master/CONTRIBUTING.md#using-the-issue-tracker

I'm marking this PR as draft for now.

foxxx0 · 2024-12-10T14:03:34Z

ah well, shame on me.

I've just created #15264 now to discuss the feature itself.

In order to translate interface names in such scoped addresses the required `LibC` binding to `if_nametoindex()` has been added. This method obviously only works for interfaces (devices) that are actually present on the system. The binding for the reverse operation `if_indextoname()` has also been added, although its usage is a bit more cumbersome due to LibC::Char* buffer handling. The necessary buffer length has been placed into the constant `LibC::IF_NAMESIZE`, which appears to be `16u8` on unix-like systems and `257u16` on windows. This could potentially be reworked via a preprocessor block at compile-time as indicated by some folks over on discord, I currently do not know how to achieve that though. Scoped identifiers are only valid for link-local (`fe80::`) addresses, e.g. `fe80::1%eth0` References: - https://datatracker.ietf.org/doc/html/rfc4007 Fixes crystal-lang#15264

foxxx0 · 2025-04-12T09:30:27Z

Alright, here we go.

Not sure why, but something is not happy in the workflow runs in my fork.

AArch64 is always skipped immediately
x86_64-windows-dlls and x86_64-windows-libs fail building libiconv

Apart from that the remaining workflows appear to be happy with it.

Let me know if you'd like more comments in the added code or want something changed.

src/lib_c/x86_64-windows-msvc/c/foobar

src/socket/address.cr

spec/std/socket/address_spec.cr

src/lib_c/x86_64-windows-msvc/c/foobar

src/socket/address.cr

This should never have made it into the previous commit. If allowed, I could squash/fixup to get rid of this noise but as per the contributing guidelines amending/editing and force-pushing branches with active PRs is undesired.

I fully agree with all suggestions, so this commit implements them. As per the contributing guidelines this is a separate commit and not squashed.

foxxx0 · 2025-04-14T23:44:22Z

Thanks for the detailed review and constructive feedback/suggestions.
I've updated the branch/PR accordingly.

Following the contributing guidelines I have done so with separate commits but I'd be happy to rebase+squash/fixup if desired for a "cleaner" history before merging into master.

src/socket/address.cr

This just implicitly assumes that `eth0` in the example got enumerated with interface index `2` by the kernel. Should be sufficient for an example.

straight-shoota

Don't worry about squash/rebase. We do that when merging.
Commit history in the PR is best left as is.

src/socket/address.cr

spec/std/socket/address_spec.cr

Instead of using `#partition('%')` on the input string, which results in additional `String` allocations, we can stop consuming the slice within `#parse_v6_fields?` when we encounter the zone seprator character (`'%'`) and pass that index back to the constructor. However, this change to `#parse_v6_fields?` is a bit more intrusive and changes it return type from a simple `UInt16[8]?` to a `Tuple(UInt16[8]?, Int32?)` to account for the zone separator index. All references to that method and its specs were updated accordingly to adopt this change. Additionally the exception conditions for invalid v6 addresses have been simplified a bit and all of the converted to proper `Socket::Error` exceptions.

This is proposed as an alternate solution for the previous implementation. Returning the index of the zone separator (`'%'`) back to the constructor is a bit cumbersome for subsequent usage. Return the remaining subslice instead makes it much easier to use in the constructor for subsequent parsing and within the exception messages. I definitely prefer this approach over the previous one and consider it an improvement.

src/socket/address.cr

To avoid a breaking change only modify the private overload. Additionally remove unneeded counter variable for tracking the index of the zone separator as its value can be calculated when needed.

As per the discussion there is reasoning behind keeping stdlib integer types a simple `Int32` whenever possible.

foxxx0 · 2025-05-19T11:03:48Z

Thanks for the explanation and dilligence in the reviews, that goes to everyone who has taken part here, much appreciated.

I've pushed two more commits, one to switch the macro to use LibC.has_method? and another one to switch zone_id to an Int32 and allow generic Int as function parameters where applicable.

straight-shoota · 2025-05-19T11:54:44Z

I'm not happy about the Int restrictions. IMO they're wrong. We should not change the existing type restrictions, but we should avoid introducing new ones.

Int includes all integer types, so it's allowed to call the method with Int64::MAX, for example. But it's clear that such a value would never fit. With an Int restriction, there's no compile-time validation. It'll only raise at runtime.
With an Int32 restrictions, however, we still get implicit autocasting for all compatible types. But if the source type is bigger than Int32, we need to cast explicitly at the call site. This can help identify issues at compile time.

IMO input and output types should always be identical, so Int32 both for the parameter type and the return type.

foxxx0 · 2025-05-19T13:54:55Z

That is definitely a good catch. I've adjusted zone_id to Int32 everywhere now.

HertzDevil · 2025-05-19T14:01:03Z

Windows CI is failing, because Win32 presumably does not set errno at all, and the ENOENT you're seeing probably came from another C runtime function call:

If the if_nametoindex function fails and returns zero, it is not possible to determine an error code.

foxxx0 · 2025-05-19T14:35:44Z

Windows CI is failing, because Win32 presumably does not set errno at all, and the ENOENT you're seeing probably came from another C runtime function call:

If the if_nametoindex function fails and returns zero, it is not possible to determine an error code.

Unfortunately I can't seem to find any useful errno documentation for windows w.r.t. if_indextoname() / if_nametoindex().
Errors from if_nametoindex() are already handled regardless of errno via checking zone_id.positive?.
I have now adjusted the error handling for if_indextoname(), since a NULL pointer always indicates an error, regardless of what errno may or may not be present. For platforms with errno support, which include the following to my knowledge:

linux
bsd
darwin
solaris
the Errno.value is inspected and appended to the exception message unless it is Errno::NONE.
The corresponding spec is adjusted accordingly.

src/socket/address.cr

foxxx0 · 2025-05-20T14:32:48Z

Are we good to merge now?

(I'd really like this to be part of the 1.17.0 release 🙂 )

RX14 · 2025-05-22T09:53:00Z

src/socket/address.cr

+    # Scoped/Zoned IPv6 link-local addresses are supported per RFC4007, e.g.
+    # `fe80::abcd%eth0` but will always use their numerical interface index
+    # in the `#inspect` representation. The interface name can be retrieved later
+    # using `#link_local_interface` on the `IPAddress` object.


Sorry for the post-merge comment, this is a fantastic PR I will get a bunch of use out of.

However, isn't it actually #to_s which i more important to document the representation of? #to_s is expected to be used in application code, whereas #inspect is for debugging, and happens to use #to_s here.

Additionally, would it make sense for #inspect to show the interface by name, since performance is a non-issue? With fallback to zone number if #inspect fails.

Improving documentation sounds good for a follow-up 👌

I'm not sure about name vs. id. #to_s is supposed to be a user-friendly format, so that would be an argument to show the name instead of the id there...

Modifying the #to_s output would be a breaking change. There are likely lots of things/applications/libs that occasionally convert an IPv4/IPv6 address to a string and handing them off to whatever. Each of these implementations would then potentially need adjusting if there was suddenly a %<zone_id> suffix as part of the string.

I do think a separate #to_s_rfc4007? method might be useful to get that exact format. With an optional boolean to chose between numerical zone_id and link_local_interface.

I do think a separate #to_s_rfc4007? method might be useful to get that exact format. With an optional boolean to chose between numerical zone_id and link_local_interface.

Sounds like a great idea. I'd suggest using a dedicated named argument in to_s instead though.

Correct me if I'm wrong, but the current state of this PR looks like to_s is modified as well as inspect.

I'd prefer for inspect to by default include the zone, by default with name.

Yes, this PR adds the zone ID to the output of both, #to_s and #inspect (the latter delegates to the former).
We missed adding a spec for that to make it explicit.
Considering https://github.com/crystal-lang/crystal/pull/15263/files#r2103365809 this might have been an unintended change and we should revert that.

I'd prefer for inspect to always include the zone.

Agreed. The information should be there. But should it be ID or name?
Maybe ID because it's sufficient and more succinct?

After taking another look, the #to_s always returned the address accompanied by the port. If one wanted to obtain just the address, there is an #address method, which hasn't been modified by this PR, i.e. it doesn't include any %<zone_id>, neither numerical nor string.

It is indeed true, that we've now implicitly changed the #to_s behavior with a potentially breaking formatting change. This was definitely an oversight as there is also no spec for it.

I can follow up with another PR for adding an optional bool to #to_s that controls whether or not to include the zone_id.

Should we default to always include it or omit it in #to_s to keep previous behavior and only always include it in #inspect?
Furthermore, when including it, do we use numerical ID or interface name?
I'd be fine attempting #link_local_interface and fallback to using numerical @zone_id if it failed. Thoughts?

One more thing worth noting:
Since zone_id can only ever be set (i.e. .positive?) on link-local ipv6 addresses, the format change doesn't really affect anything.
If at all, it actually might fix certain implementations since link-local ipv6 addresses might not even work properly without the zone_id.

But I still think we can make improvements in both the comments/docs, default behavior and obviously also add some specs for that.

Given the effect is only for link-local addresses, I think including the zone ID by default makes sense for both to_s and inspect. However, looking up the interface name has a performance impact so I believe it should only be on by default for inspect.

straight-shoota marked this pull request as draft December 10, 2024 12:26

straight-shoota added kind:feature topic:stdlib:networking labels Dec 10, 2024

foxxx0 mentioned this pull request Dec 10, 2024

IPv6 scoped link-local addresses (RFC4007) #15264

Closed

foxxx0 force-pushed the ipv6ll_scope_rfc4007 branch 3 times, most recently from 12d0b01 to e9e524e Compare April 11, 2025 20:19

foxxx0 force-pushed the ipv6ll_scope_rfc4007 branch from e9e524e to 5b253ee Compare April 12, 2025 08:29

foxxx0 marked this pull request as ready for review April 12, 2025 09:26

straight-shoota requested changes Apr 14, 2025

View reviewed changes

foxxx0 added 2 commits April 15, 2025 01:35

remove totally unnecessary artifact

78a1101

This should never have made it into the previous commit. If allowed, I could squash/fixup to get rid of this noise but as per the contributing guidelines amending/editing and force-pushing branches with active PRs is undesired.

adjust RFC4007 implementation based on review feedback

06d9a06

I fully agree with all suggestions, so this commit implements them. As per the contributing guidelines this is a separate commit and not squashed.

foxxx0 requested a review from straight-shoota April 14, 2025 23:44

Sija reviewed Apr 15, 2025

View reviewed changes

src/socket/address.cr Outdated Show resolved Hide resolved

src/socket/address.cr Outdated Show resolved Hide resolved

src/socket/address.cr Outdated Show resolved Hide resolved

src/socket/address.cr Outdated Show resolved Hide resolved

foxxx0 added 3 commits April 15, 2025 10:04

remove trailing dot from exception messages

280e074

add specs for invalid v4 addrs with zone identifier

cd27591

clarify RFC4007 comment and fix #inspect example

e39b72e

This just implicitly assumes that `eth0` in the example got enumerated with interface index `2` by the kernel. Should be sufficient for an example.

foxxx0 requested a review from Sija April 15, 2025 08:19

straight-shoota reviewed Apr 15, 2025

View reviewed changes

src/socket/address.cr Outdated Show resolved Hide resolved

src/socket/address.cr Outdated Show resolved Hide resolved

src/socket/address.cr Outdated Show resolved Hide resolved

spec/std/socket/address_spec.cr Outdated Show resolved Hide resolved

foxxx0 added 2 commits April 15, 2025 22:37

wrap references to methods in backticks

8f84cd3

foxxx0 requested a review from straight-shoota April 15, 2025 22:01

straight-shoota reviewed Apr 15, 2025

View reviewed changes

src/socket/address.cr Outdated Show resolved Hide resolved

src/socket/address.cr Outdated Show resolved Hide resolved

revert public parse_v6_fields? return type

67a91a7

To avoid a breaking change only modify the private overload. Additionally remove unneeded counter variable for tracking the index of the zone separator as its value can be calculated when needed.

use normal Int32 for zone_id

a16ed4b

As per the discussion there is reasoning behind keeping stdlib integer types a simple `Int32` whenever possible.

foxxx0 requested review from bcardiff and ysbaddaden May 19, 2025 11:03

ysbaddaden approved these changes May 19, 2025

View reviewed changes

restrict zone_id parameter to Int32

72c8b56

foxxx0 added 4 commits May 19, 2025 16:15

handle windows errno appropriately

2227909

rework if_indextoname error handling

1f45a62

only look at errno for supported platforms

6d8f516

adjust specs to match previous commit

5e379df

straight-shoota reviewed May 19, 2025

View reviewed changes

src/socket/address.cr Outdated Show resolved Hide resolved

foxxx0 added 2 commits May 19, 2025 17:47

only skip errno inspection on flag?(:win32)

9109226

fix formatting

21b5b49

straight-shoota changed the title ~~implement IPv6 scoped addresses (RFC4007)~~ Add support for IPv6 scoped addresses (RFC4007) May 19, 2025

straight-shoota approved these changes May 19, 2025

View reviewed changes

ysbaddaden reviewed May 19, 2025

View reviewed changes

src/socket/address.cr Outdated Show resolved Hide resolved

raise exception from errno on non-windows platforms

22675f9

foxxx0 requested a review from ysbaddaden May 19, 2025 18:07

expect proper darwin/bsd errno message for ENXIO

a6f880f

ysbaddaden approved these changes May 20, 2025

View reviewed changes

straight-shoota approved these changes May 20, 2025

View reviewed changes

straight-shoota added this to the 1.17.0 milestone May 20, 2025

straight-shoota merged commit 92048a8 into crystal-lang:master May 22, 2025
34 checks passed

foxxx0 deleted the ipv6ll_scope_rfc4007 branch May 22, 2025 08:24

RX14 reviewed May 22, 2025

View reviewed changes

Uh oh!

Add support for IPv6 scoped addresses (RFC4007) #15263

Add support for IPv6 scoped addresses (RFC4007) #15263

Uh oh!

Conversation

foxxx0 commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

straight-shoota commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

foxxx0 commented Dec 10, 2024

Uh oh!

foxxx0 commented Apr 12, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

foxxx0 commented Apr 14, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

straight-shoota left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

foxxx0 commented May 19, 2025

Uh oh!

straight-shoota commented May 19, 2025

Uh oh!

foxxx0 commented May 19, 2025

Uh oh!

HertzDevil commented May 19, 2025

Uh oh!

foxxx0 commented May 19, 2025

Uh oh!

Uh oh!

Uh oh!

foxxx0 commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

RX14 May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

straight-shoota May 22, 2025

Choose a reason for hiding this comment

Uh oh!

foxxx0 May 22, 2025

Choose a reason for hiding this comment

Uh oh!

Sija May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RX14 May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

straight-shoota May 23, 2025

Choose a reason for hiding this comment

Uh oh!

foxxx0 May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

foxxx0 commented Dec 10, 2024 •

edited

Loading

straight-shoota commented Dec 10, 2024 •

edited

Loading

foxxx0 commented May 20, 2025 •

edited

Loading

RX14 May 22, 2025 •

edited

Loading

Sija May 22, 2025 •

edited

Loading

RX14 May 23, 2025 •

edited

Loading

foxxx0 May 23, 2025 •

edited

Loading

foxxx0 May 23, 2025 •

edited

Loading