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.

Sign up for GitHub

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

Jump to bottom

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

Closed
3 tasks done
xEricL opened this issue Mar 23, 2024 · 4 comments · Fixed by #3170
Closed
3 tasks done

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

xEricL opened this issue Mar 23, 2024 · 4 comments · Fixed by #3170
Labels
✏️ Feature 📝 Proposal v3
Milestone
v3

Comments

@xEricL
Copy link
Contributor

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.
@welcome Welcome
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.

@gaby gaby mentioned this issue Mar 25, 2024
Merged
2 tasks
@xEricL
Copy link
Contributor 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.

@xEricL xEricL mentioned this issue Sep 15, 2024
Closed
14 tasks
@xEricL xEricL mentioned this issue Oct 15, 2024
Merged
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
✏️ Feature 📝 Proposal v3
Projects
v3
Status: Done
Milestone
v3
3 participants
@gaby @ReneWerner87 @xEricL