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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃摑 [Proposal]: Add Helpers for Internal IP Ranges #2930

Open
3 tasks done
xEricL opened this issue Mar 23, 2024 · 4 comments
Open
3 tasks done

馃摑 [Proposal]: Add Helpers for Internal IP Ranges #2930

xEricL opened this issue Mar 23, 2024 · 4 comments

Comments

@xEricL
Copy link

xEricL commented Mar 23, 2024

Feature Proposal Description

Currently, adding loopback, link-local, and private network addresses to the fiber.Config.TrustedProxies list requires us to manually add those ranges. Since web apps are commonly deployed behind reverse proxies, it would be helpful to have a simple way of adding these ranges without needing to search them up.

When configuring trusted proxies in Echo framework, the setup is a bit different:

e := echo.New()
_, myProxyRange, _ := net.ParseCIDR("173.245.48.0/20")
e.IPExtractor = echo.ExtractIPFromXFFHeader(
	echo.TrustLoopback(true), // e.g. ipv4 start with 127. 
	echo.TrustLinkLocal(true), // e.g. ipv4 start with 169.254
	echo.TrustPrivateNet(true), // e.g. ipv4 start with 10. or 192.168
	echo.TrustIPRange(myProxyRange),
)

In Fiber, an equivalent setup would look something like

app := fiber.New(fiber.Config{
	ProxyHeader:             fiber.HeaderXForwardedFor,
	EnableTrustedProxyCheck: true,
	TrustedProxies: []string{
		"127.0.0.0/8",    // Loopback addresses
		"169.254.0.0/16", // Link-Local addresses
		"fe80::/10",
		"192.168.0.0/16", // Private Network addresses
		"172.16.0.0/12",
		"10.0.0.0/8",
		"fc00::/7",
		"173.245.48.0/20", // My custom range
	},
})

Although I prefer Fiber's method of using an array of strings, it would be nice to have constants like:

// Fiber helpers.go
// Note: This is not an exhaustive list.
const (
        ...
	IPv4Loopback      = "127.0.0.0/8"
	IPv4LinkLocal     = "169.254.0.0/16"
	IPv4PrivateSmall  = "192.168.0.0/16"
	IPv4PrivateMedium = "172.16.0.0/12"
	IPv4PrivateLarge  = "10.0.0.0/8"
	IPv6Loopback      = "::1/128"
	IPv6LinkLocal     = "fe80::/10"
	IPv6PrivateNet    = "fc00::/7"
)

This would allow developers to use

app := fiber.New(fiber.Config{
	ProxyHeader:             fiber.HeaderXForwardedFor,
	EnableTrustedProxyCheck: true,
	TrustedProxies: []string{
		fiber.IPv4Loopback,
		fiber.IPv4LinkLocal,
		fiber.IPv4PrivateSmall,
		fiber.IPv4PrivateMedium,
		fiber.IPv4PrivateLarge,
		fiber.IPv6Loopback,
		fiber.IPv6LinkLocal,
		fiber.IPv6PrivateNet,
		"173.245.48.0/20",  // My custom range
	},
})

They don't necessarily need to be constants. We could add a new config option to fiber.Config instead:

app := fiber.New(fiber.Config{
	ProxyHeader:             fiber.HeaderXForwardedFor,
	EnableTrustedProxyCheck: true,
	TrustInternalIPs: true, // default to false
	TrustedProxies: []string{
		"173.245.48.0/20",  // My custom range
	},
})

This would be far easier, but at the cost of allowing developers to cherry pick individual ranges. Those cases are probably rare though, which might be why Echo's helper functions for these ranges default to true.

Alignment with Express API

Express.js allows developers to set trusted proxy settings using pre-defined subnets:

// loopback - 127.0.0.1/8, ::1/128
// linklocal - 169.254.0.0/16, fe80::/10
// uniquelocal - 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, fc00::/7
app.set('trust proxy', ['loopback', 'linklocal', 'uniquelocal']) 

HTTP RFC Standards Compliance

n/a (this is a quality of life improvement)

API Stability

IP address ranges don't change.

Feature Examples

(Listed above)

Checklist:

  • I agree to follow Fiber's Code of Conduct.
  • I have searched for existing issues that describe my proposal before opening this one.
  • I understand that a proposal that does not meet these guidelines may be closed without explanation.
Copy link

welcome bot commented Mar 23, 2024

Thanks for opening your first issue here! 馃帀 Be sure to follow the issue template! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@gaby
Copy link
Member

gaby commented Mar 24, 2024

@xEricL I'm looking into this. I also found an issue, the more ranges you add to that list the longer a request takes.

@xEricL
Copy link
Author

xEricL commented Mar 25, 2024

@gaby Thank you for taking an interest in this.

I just want to clarify that the IP ranges I provided was not a complete list.

There are also:

- IPv4 Broadcast 255.255.255.255
- IPv4 Multicast 224.0.0.0/4
- IPv6 Multicast ff00::/8
- IPv6 Loopback ::1/128

There are a lot more ranges defined in RFC6890, RFC4291 and RFC4193.

What are your thoughts on simply adding another option to fiber.Config, such as TrustInternalIPs: true?

net/ip has nice helper functions for detecting address types, such as ip.IsPrivate() (added in Go 1.16).

ctx.go defines a method isLocalHost. We could implement a similar method to check for internal IPs:

// Note: These are the same ranges enabled by default when configuring an IP extractor in Echo.
// https://github.com/labstack/echo/blob/master/ip.go#L174
func (*DefaultCtx) isInternalHost(ip net.IP) bool {
	return ip.IsLoopback() || ip.IsPrivate() || ip.IsLinkLocalUnicast()
}

and then use it in IsProxyTrusted():

func (c *DefaultCtx) IsProxyTrusted() bool {
	...
        ip := c.fasthttp.RemoteIP()

	if c.app.config.TrustInternalIPs && c.isInternalHost(ip) { return true }

	if _, trusted := c.app.config.trustedProxiesMap[ip.String()]; trusted {
		return true
	}
	...
}

This seems to be a much better approach than my initial idea of adding constants for each individual range. I didn't realize there were so many to consider when I first opened this request. It might even have better performance compared to adding the equivalent IP ranges to the TrustedProxies list.

@ReneWerner87 ReneWerner87 added this to the v3 milestone Mar 25, 2024
@gaby
Copy link
Member

gaby commented Mar 25, 2024

@xEricL Yeah, that approach makes sense, let me do some benchmarks. Ibwant to make sure lookup times are consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

3 participants