net/ip: ParsePrefix with IPv6 zone should return an error

[mdlayher]

EDIT 2022-03-29: Go is documented to comply with https://www.rfc-editor.org/rfc/rfc4291.html#section-2.3 which makes no mention of IPv6 prefix + zone syntax.

Leaving the rest of this message alone for historical context.

---

What version of Go are you using (go version)?

Go 1.18, tip

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

linux/amd64, but n/a.

What did you do?

Attempted to parse a link-local IPv6 address prefix with zone: fe80::%2/64
https://go.dev/play/p/EeZwl0aah3q

What did you expect to see?

• net.ParseCIDR: no error, valid *net.IPNet with zone
• netip.ParsePrefix: no error, valid *netip.Prefix with zone

What did you see instead?

• net.ParseCIDR: error: invalid CIDR address: fe80::%2/64
• netip.ParsePrefix: no error, fe80::/64 returned with zone stripped

---

This was brought to my attention by a blog and associated thread on reddit:

https://old.reddit.com/r/golang/comments/tkf9rm/a_tiny_flaw_in_gos_netip_design/
https://adam-p.ca/blog/2022/03/go-netip-flaw/

While working on inet.af/netaddr which ultimately became net/netip, I don't believe we were able to find any evidence of an IPv6 prefix with associated zone. However, that evidence turned up today while I was doing further research:

RFC 4007: IPv6 Scoped Address Architecture, March 2005
https://datatracker.ietf.org/doc/html/rfc4007#section-11.7

[11.7] Combinations of Delimiter Characters

   There are other kinds of delimiter characters defined for IPv6
   addresses.  In this subsection, we describe how they should be
   combined with the format for non-global addresses.

   The IPv6 addressing architecture [[1](https://datatracker.ietf.org/doc/html/rfc4007#ref-1)] also defines the syntax of IPv6
   prefixes.  If the address portion of a prefix is non-global and its
   scope zone should be disambiguated, the address portion SHOULD be in
   the format.  For example, a link-local prefix fe80::/64 on the second
   link can be represented as follows:

            fe80::%2/64

Note that the prefix fe80::%2/64 should be considered valid, and thus both the current behaviors of net.ParseCIDR and netip.ParsePrefix are incorrect. I believe this was an oversight on my part during the development of inet.af/netaddr, as I was the primary user of IPv6 link-local addresses with zones. However, I have not run into a need to use IPv6 prefixes with zones as of yet.

Either way, I believe both functions should be updated to handle this case as appropriate.

In addition, RFC6874, Section 1 (https://datatracker.ietf.org/doc/html/rfc6874#section-1) says:

It should be noted that
   zone identifiers have purely local meaning within the node in which
   they are defined, often being the same as IPv6 interface names.  They
   are completely meaningless for any other node.  Today, they are
   meaningful only when attached to addresses with less than global
   scope, but it is possible that other uses might be defined in the
   future.

I don't believe the status quo has changed in the 9 years since that RFC, and would suggest that we do one of the following:

1. parse prefix zones for any type of IPv6 address prefix (link-local is the only current "less than global scope" user, but zones could also be used for other address types later)
2. parse prefix zones for only link-local IPv6 address prefixes; that is, if ip.IsLinkLocalUnicast() == true or IP is contained in fe80::/10

EDIT: as a data point, since net/netip already accepts the following, even given the RFC text above, I am in favor of option 1.

fmt.Println(netip.MustParseAddr("2001:db8::1%eth0"))
// 2001:db8::1%eth0

I do think this should be fixed for link-local addresses at a minimum though. If we agree this should be done, I'm happy to fix this in both net and net/netip.

/cc @bradfitz @danderson @josharian @ianlancetaylor @neild

[adam-p]

Thanks for creating this @mdlayher, and I'm glad you found an actual reference to prefixes with zones.

As a kind of user story, here's what led me to bothered by the current behaviour:

I'm writing a library to help people extract the correct client IP from X-Forwarded-For and other headers (because right now it's being done wrong way too often). One of the strategies for doing that is to configure a list of trusted IP ranges containing the IPs inhabited by the trusted reverse proxies. The IPs in the XFF headers are searched from the rightmost, looking for the first one that isn't contained by a trusted prefix. (This is similar to what Nginx's X-Real-IP module does.)

It's written using net types rather than netip. I was considering switching to the latter when @mdlayher randomly told me about the prefix behaviour difference. I realized that if I naively switched from net.IPNet to netip.Prefix I would introduce a bug: users would be able to "successfully" configure prefixes with zones -- whereas before those prefixes would produce errors -- and then the prefixes would silently fail to contain any IPs with zones.

I ended up adding an explicit check for zones and returning an error, to help my future self from introducing the bug.

So the current behaviour adds some burden for those of us writing libraries that use netip.Prefix and then exposing a different interface to it. We don't know beforehand what our users might throw at us. We either need to add explicit zone checks (like I did) or add a documentation caveat about zones causing silent failures (like netip.Prefix.Contains does).

I know that the current behaviour is working as documented, but (as I said in the blog post) I think a lot of people simply won't see the documentation. Because:

1. The "Prefixes strip zones" warning is only on Prefix.Contains and not on Prefix or ParsePrefix. Kind of easy to miss.
2. The API is very simple and most of us have a reasonably good idea of what it means for a prefix/CIDR/range to "contain" and IP. So we just won't look at the doc.
3. If we've used net.CIDR, then we're even less likely to think we need to read the doc for the analogous facilities in netip. And then we'll introduce quiet bugs.

(FWIW, I prefer @mdlayher's "parse prefix zones for any type of IPv6 address prefix" option.)

[danderson]

I'm torn on this. My main worry is that using zones for anything higher level than very low-level networking configuration (e.g. IPv6 autoconfiguration, DHCPv6, dynamic routing protocols) is almost certainly a mistake on the part of the person using zones. In the original IPv6 designs, zones were going to be a much more generally useful thing, but these days their only use is to disambiguate link-local communication, and in turn link-local communication should only be used as a stepping stone towards configuring globally-scoped IPv6.

So, when we designed netaddr, we tried to find a legitimate use for scoped prefixes, and concluded that the only beneficiaries would be malicious inputs trying to confuse code that wasn't written with scope-awareness. That's how we ended up with the scope stripping behavior for prefixes. We had to keep zones for single IPs, since that is definitely both necessary and useful for things like CoreRAD, which have to wrestle with link-local communication. (Unfortunately, IMO, because I'd have loved to lose zones entirely)

The thing I'm torn on is whether the use cases in this bug are a legitimately useful/necessary use of zones in prefixes, or if we're arguing about supporting a hypothetical edge case for completeness, at the cost of making mainstream uses of the package harder.

In particular, I would like us to not consider the existence of an RFC as proof that something is necessary or useful. Especially in IPv6, which has gone through decades of redesign and refinement, there are a lot of RFCs out there that encourage what we now know to be bad ideas. So, an RFC alone should not be our guide as to how APIs should behave, because if we did that, we'd end up with a lot of really whacky behavior all over the place.

Currently, our only example of wanting to use zones in prefixes is a library for use in HTTP, where I would argue zones should never be appearing in the first place. What's the failure mode of the current behavior? IIUC, it causes false negatives, that is, source IPs that should be trusted end up not being trusted, because of the zone mismatch. Is that correct? If that's the extent of the impact, then my preference would be to treat this as a documentation issue, and make it more explicit that netip Prefixes cannot have zone specifiers.

If there is a compelling reason to force all users of netip.Prefix to care about zone specifiers, then I think I would reluctantly advocate for consistency and allowing them for all prefixes, rather than hardcoding an exception for fe80::/10. "Zones suck and you have to always deal with them" is an easier thing to explain in documentation, and to discover in tests, than "zones will sometimes cause you got shoot yourself in the foot, but only in edge cases you may not think about as a beginner user of netip".

On top of all the above, we'll have to consider the Go 1 compatibility promise: now that a release exists in which Prefixes strip zones, I don't think we can silently change the behavior of existing prefix constructors. So, if we do want to support that, do we end up having to create WithZone constructor equivalents for all the current ways in which you can obtain a Prefix? That would mean duplicating or backwards-compatibly augmenting the semantics of the following API surface:

• Addr.Prefix(bits)
• MustParsePrefix(s)
• ParsePrefix(s)
• Prefix.Masked()
• Prefix.Overlaps(other)

[adam-p]

Another variation on not supporting zones that I would prefer to the current beahviour: Have ParsePrefix return an error if there's a zone present.

This is the same as the net.ParseCIDR behaviour, but may still be unpalatable as it's a behaviour change for ParsePrefix.

[neild]

Silently dropping zones seems like the worst of all worlds: Incorrect and surprising.

While having ParsePrefix return an error if a zone is present is a behavior change, the current behavior is documented as parsing RFC 4291 notation and RFC 4291 does not (unless I'm missing something) allow for zones in prefixes. Therefore I think it's reasonable to say that accepting prefixes with zones is simply a bug, and ParsePrefix should return an error for this condition just as ParseCIDR does.

[mdlayher]

https://www.rfc-editor.org/rfc/rfc4291.html#section-2.3

Interesting, I suppose I had never bothered to check the docs for ParseCIDR since I've been pretty intimately familiar with its use. You're correct: section 2.3 here makes no mention of IPv6 zones at all and simiply says:

... An IPv6 address prefix is represented by the notation:

      ipv6-address/prefix-length

   where

      ipv6-address    is an IPv6 address in any of the notations listed
                      in [Section 2.2](https://www.rfc-editor.org/rfc/rfc4291.html#section-2.2).

      prefix-length   is a decimal value specifying how many of the
                      leftmost contiguous bits of the address comprise
                      the prefix.

Since updating netip.ParsePrefix makes the behavior consistent with net.ParseCIDR, I'm good with making that change. I will send a CL.

[gopherbot]

Change https://go.dev/cl/396299 mentions this issue: net/netip: return an error from ParsePrefix with IPv6 zone input