switch to Strategy as an interface

README.md

@@ -1,15 +1,16 @@
 [![GoDoc](https://godoc.org/github.com/realclientip/realclientip-go?status.svg)](http://godoc.org/github.com/realclientip/realclientip-go)
-[![Go Playground](https://img.shields.io/badge/Go-playground-%23007d9c?style=flat)](https://go.dev/play/p/Z0jNsEcOCnL)
+[![Go Playground](https://img.shields.io/badge/Go-playground-%23007d9c?style=flat)](https://go.dev/play/p/NUvc6U8KQKI)
 [![Test](https://github.com/realclientip/realclientip-go/actions/workflows/test.yml/badge.svg)](https://github.com/realclientip/realclientip-go/actions/workflows/test.yml)
 ![coverage](https://img.shields.io/badge/coverage-100%25-success?style=flat)
-![license](https://img.shields.io/badge/license-0BSD-important.svg?style=flat)
+[![license](https://img.shields.io/badge/license-0BSD-important.svg?style=flat)](https://choosealicense.com/licenses/0bsd/)
 
 # realclientip-go
 
-`X-Forwarded-For` and other "real" client IP headers are [often used incorrectly](https://adam-p.ca/blog/2022/03/x-forwarded-for/), resulting in bugs and security vulnerabilities. This library is an attempt to create a reference implementation of the correct ways to use such headers.
+`X-Forwarded-For` and other "real" client IP headers are [often used incorrectly][xff-post], resulting in bugs and security vulnerabilities. This library is an attempt to create a reference implementation of the correct ways to use such headers.
 
-This library is written in Go, but the hope is that it will be reimplemented in other languages. Please open an issue if you would like to create such an implementation.
+[xff-post]: https://adam-p.ca/blog/2022/03/x-forwarded-for/
 
+This library is written in Go, but the hope is that it will be reimplemented in other languages. Please open an issue if you would like to create such an implementation.
 
 This library is freely licensed. You may use it as a dependency or copy it or modify it or anything else you want. It has no dependencies, is written in pure Go, and supports Go versions as far back as 1.13.
 
@@ -18,46 +19,62 @@ This library is freely licensed. You may use it as a dependency or copy it or mo
 This library provides strategies for extracting the desired "real" client IP from various headers or from `http.Request.RemoteAddr` (the client socket IP).
 
 ```golang
-clientIPStrategy, err := realclientip.RightmostTrustedCountStrategy("X-Forwarded-For", 1)
+strategy, err := realclientip.NewRightmostTrustedCountStrategy("X-Forwarded-For", 2)
 ...
-clientIP := clientIPStrategy(req.Header, req.RemoteAddr)
+clientIP := strategy.ClientIP(req.Header, req.RemoteAddr)
 ```
 
-Try it out [in the playground](https://go.dev/play/p/Z0jNsEcOCnL).
+Try it out [in the playground](https://go.dev/play/p/NUvc6U8KQKI).
+
+There are a number of different strategies available -- the right one will depend on your network configuration. See the [documentation] to find out what's available and which you should use.
 
-There are a number of different strategies available -- the right one will depend on your network configuration. See the [documentation](https://pkg.go.dev/github.com/realclientip/realclientip-go) to find out what's available and which you should use.
+`ClientIP` is threadsafe for all strategies. The same strategy instance can be used for handling all HTTP requests, for example.
 
-There are some more extensive examples of use in the [`_examples` directory](/_examples/).
+[documentation]: (https://pkg.go.dev/github.com/realclientip/realclientip-go)
+
+There are examples of use in the [documentation] and [`_examples` directory](/_examples/).
 
 ### Strategy failures
 
-The Strategy used must be chosen and tuned for your network configuration. This _should_ result in the Strategy _never_ returning an empty string -- i.e., never failing to find a candidate for the "real" IP. Consequently, getting an empty-string result should be treated as an application error, perhaps even worthy of panicking.
+The strategy used must be chosen and tuned for your network configuration. This _should_ result in the strategy _never_ returning an empty string -- i.e., never failing to find a candidate for the "real" IP. Consequently, getting an empty-string result should be treated as an application error, perhaps even worthy of panicking.
 
-For example, if you have 2 levels of trusted reverse proxies, you would probably use `RightmostTrustedCountStrategy` and it should work every time. If you're directly connected to the internet, you would probably use `RemoteAddrStrategy` or something like `ChainStrategies(LeftmostNonPrivateStrategy(...), RemoteAddrStrategy)` and you will be sure to get a value every time. If you're behind Cloudflare, you would probably use `SingleIPHeaderStrategy("Cf-Connecting-IP")` and it should work every time.
+For example, if you have 2 levels of trusted reverse proxies, you would probably use `RightmostTrustedCountStrategy` and it should work every time. If you're directly connected to the internet, you would probably use `RemoteAddrStrategy` or something like `ChainStrategy(LeftmostNonPrivateStrategy(...), RemoteAddrStrategy)` and you will be sure to get a value every time. If you're behind Cloudflare, you would probably use `SingleIPHeaderStrategy("Cf-Connecting-IP")` and it should work every time.
 
-So if an empty string is returned, it is either because the Strategy choice or configuration is incorrect, or your network configuration has changed. In either case, immediate remediation is required.
+So if an empty string is returned, it is either because the strategy choice or configuration is incorrect or your network configuration has changed. In either case, immediate remediation is required.
 
 ### Headers
 
 Leftmost-ish and rightmost-ish strategies support the `X-Forwarded-For` and `Forwarded` headers.
 
-`SingleIPHeaderStrategy` supports any header containing a single IP address or IP:port. For a list of some common headers, see the [Single-IP Headers wiki page](https://github.com/realclientip/realclientip-go/wiki/Single-IP-Headers).
+`SingleIPHeaderStrategy` supports any header containing a single IP address or IP:port. For a list of some common headers, see the [Single-IP Headers wiki page][single-ip-wiki].
+
+You must choose exactly the correct header for your configuration. Choosing the wrong header can result in failing to get the client IP or falling victim to IP spoofing.
+
+Do not abuse `ChainStrategy` to check multiple headers. There is likely only one header you should be checking, and checking more can leave you vulnerable to IP spoofing.
+
+[single-ip-wiki]: https://github.com/realclientip/realclientip-go/wiki/Single-IP-Headers
 
 #### `Forwarded` header support
 
-Support for the `Forwarded` header should be sufficient for the vast majority of rightmost-ish uses, but it is not complete and doesn't completely adhere  to [RFC 7239](https://datatracker.ietf.org/doc/html/rfc7239). See the [`Test_forwardedHeaderRFCDeviations`](https://github.com/realclientip/realclientip-go/blob/65719ac74acb471001b3049b4270a3cc38920a30/realclientip_test.go#L1895) test for details on deviations.
+Support for the [`Forwarded` header] should be sufficient for the vast majority of rightmost-ish uses, but it is not complete and doesn't completely adhere  to [RFC 7239]. See the [`Test_forwardedHeaderRFCDeviations`] test for details on deviations.
+
+[`Forwarded` header]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Forwarded
+[RFC 7239]: https://datatracker.ietf.org/doc/html/rfc7239
+[`Test_forwardedHeaderRFCDeviations`]: https://github.com/realclientip/realclientip-go/blob/65719ac74acb471001b3049b4270a3cc38920a30/realclientip_test.go#L1895
 
 ### IPv6 zones
 
-IPv6 zone identifiers are retained in the IP address returned by the strategies. [Whether you should keep the zone](https://adam-p.ca/blog/2022/03/strip-ipv6-zone/) depends on your specific use case. As a general rule, if you are not immediately using the IP address (for example, if you are appending it to the `X-Forwarded-For` header and passing it on), then you _should_ include the zone. This allows downstream consumers the option to use it. If your code is the final consumer of the IP address, then keeping the zone will depend on your specific case (for example: if you're logging the IP, then you probably want the zone; if you are rate limiting by IP, then you probably want to discard it).
+IPv6 zone identifiers are retained in the IP address returned by the strategies. [Whether you should keep the zone][strip-zone-post] depends on your specific use case. As a general rule, if you are not immediately using the IP address (for example, if you are appending it to the `X-Forwarded-For` header and passing it on), then you _should_ include the zone. This allows downstream consumers the option to use it. If your code is the final consumer of the IP address, then keeping the zone will depend on your specific case (for example: if you're logging the IP, then you probably want the zone; if you are rate limiting by IP, then you probably want to discard it).
 
 To split the zone off and discard it, you may use `realclientip.SplitHostZone`.
 
+[strip-zone-post]: https://adam-p.ca/blog/2022/03/strip-ipv6-zone/
+
 ### Known IP ranges
 
 There is a copy of [Cloudflare's IP ranges](https://www.cloudflare.com/ips/) under `ranges.Cloudflare`. This can be used with `realclientip.RightmostTrustedRangeStrategy`. We may add more known cloud provider ranges in the future. Contributions are welcome to add new providers or update existing ones.
 
-(It might be preferable to use [provider APIs](https://api.cloudflare.com/#cloudflare-ips-properties) to retrieve the ranges, as they are guaranteed to be up to date.)
+(It might be preferable to use [provider APIs](https://api.cloudflare.com/#cloudflare-ips-properties) to retrieve the ranges, as they are guaranteed to be up-to-date.)
 
 ## Implementation decisions and notes
 
@@ -73,7 +90,7 @@ The values `0.0.0.0` (zero) and `::` (unspecified) are valid IPs, strictly speak
 
 ### Normalizing IPs
 
-All IPs output by the library are first convert to a structure (like `net.IP`) and then stringified. This helps normalize the cases where there are multiple ways of encoding the same IP -- like `192.0.2.1` and `::ffff:192.0.2.1`, and the various zero-collapsed states of IPv6 (`fe80::1` vs `fe80::0:0:0:1`, etc.).
+All IPs output by the library are first converted to a structure (like `net.IP`) and then stringified. This helps normalize the cases where there are multiple ways of encoding the same IP -- like `192.0.2.1` and `::ffff:192.0.2.1`, and the various zero-collapsed states of IPv6 (`fe80::1` vs `fe80::0:0:0:1`, etc.).
 
 ### Input format strictness
 
@@ -82,9 +99,7 @@ Some input is allowed that isn't strictly correct. Some examples:
 * IPv4 with brackets: `[2.2.2.2]:1234`
 * IPv4 with zone: `2.2.2.2%eth0`
 * Non-numeric port values: `2.2.2.2:nope`
-* `Forwarded` header with too much space: `For= 2.2.2.2`
-* `Forwarded` header no quotes around IPv6 values: `For=[2001:db8:cafe::18]:1234`
-* `Forwarded` header components that are invalid (as long as `For=` is also present): `For="2001:db8:cafe::18";Nope=what`
+* Other `Forwarded` header [deviations][`Test_forwardedHeaderRFCDeviations`]
 
 It could be argued that it would be better to be absolutely strict in what is accepted.
 
@@ -94,10 +109,10 @@ As this library aspires to be a "reference implementation", the code is heavily
 
 ### Pre-creating Strategies
 
-Most of the strategies are created by calling a function, like `RightmostTrustedCountStrategy("Forwarded", 2)`. That can make it awkward to create-and-call at the same time, like `RightmostTrustedCountStrategy("Forwarded", 2)(r.Header, r.RemoteAddr)`. We could have instead implemented non-pre-created functions, like `RightmostTrustedCountStrategy("Forwarded", 2, r.Header, r.RemoteAddr)`. The reasons for the way we did it include:
-1. A consistent call signature -- i.e., the `Strategy` type. This enables `ChainStrategies`.
+Strategies are created by calling a constructor, like `NewRightmostTrustedCountStrategy("Forwarded", 2)`. That can make it awkward to create-and-call at the same time, like `NewRightmostTrustedCountStrategy("Forwarded", 2).ClientIP(r.Header, r.RemoteAddr)`. We could have instead implemented non-pre-created functions, like `RightmostTrustedCountStrategy("Forwarded", 2, r.Header, r.RemoteAddr)`. The reasons for the way we did it include:
+1. A consistent interface. This enables `ChainStrategy`. It also enables library users to have code paths that aren't strategy-dependent, in case they want the strategy to be configurable.
 2. Pre-creation allows us to put as much of the invariant processing as possible into the creation step. (Although, in practice, so far, this is only the header name canonicalization.)
-3. No error return is required from the strategies. (Although they can -- but should not -- return empty string.) All error-prone processing is done in the pre-creation.
+3. No error return is required from the strategy `ClientIP` calls. (Although they can -- but should not -- return empty string.) All error-prone processing is done in the pre-creation.
 
 An alternative approach could be using functions like:
 
@@ -110,6 +125,19 @@ _, ip, err := RightmostTrustedRangeStrategy("Forward", 2, r.Header, r.RemoteAddr
 
 But perhaps that's no less awkward.
 
+### Interfaces vs Functions
+
+A pre-release implementation of this library [constructed functions] rather than structs that implement an interface. The switch to the latter was made for a few reasons:
+* It seems slightly more Go-idiomatic.
+* It allows for adding new methods in the future without breaking the API. (Such as `String()`.)
+* It allows for configuration information to appear in a printf of a strategy struct. This can be useful for logging.
+* The function approach is still easy to use, with the bound `ClientIP` method:
+  ```golang
+  getClientIP := NewRightmostTrustedCountStrategy("Forwarded", 2).ClientIP
+  ```
+
+[constructed functions]: https://github.com/realclientip/realclientip-go/blob/f72b2beafd60ac4a2a07a943f47795760c105688/realclientip.go#L22
+
 ## Other language implementations
 
 If you want to reproduce this implementation in another language, please create an issue and we'll make a repo under this organization for you to use.

_examples/README.md

@@ -0,0 +1 @@
+These are examples of usage that require dependencies that we don't want to become dependencies of our main project.

_examples/tollbooth/main.go

@@ -11,9 +11,9 @@ import (
 
 func main() {
 	// Choose the right strategy for our network configuration
-	clientIPStrategy, err := realclientip.RightmostNonPrivateStrategy("X-Forwarded-For")
+	strat, err := realclientip.NewRightmostNonPrivateStrategy("X-Forwarded-For")
 	if err != nil {
-		log.Fatal("realclientip.RightmostNonPrivateStrategy returned error (bad input)")
+		log.Fatal("realclientip.NewRightmostNonPrivateStrategy returned error (bad input)")
 	}
 
 	lmt := tollbooth.NewLimiter(1, nil)
@@ -23,10 +23,10 @@ func main() {
 	req.Header.Add("X-Forwarded-For", "1.1.1.1, 2.2.2.2, 3.3.3.3, 192.168.1.1")
 	req.RemoteAddr = "192.168.1.2:8888"
 
-	clientIP := clientIPStrategy(req.Header, req.RemoteAddr)
+	clientIP := strat.ClientIP(req.Header, req.RemoteAddr)
 	if clientIP == "" {
 		// This should probably result in the request being denied
-		log.Fatal("clientIPStrategy found no IP")
+		log.Fatal("strat.ClientIP found no IP")
 	}
 
 	// We don't want to include the zone in our limiter key
@@ -37,4 +37,6 @@ func main() {
 	} else {
 		fmt.Println("Request allowed")
 	}
+
+	// Output: Request allowed
 }

_examples/middleware/main.go → example_middleware_test.go

@@ -1,4 +1,4 @@
-package main
+package realclientip_test
 
 import (
 	"context"
@@ -11,15 +11,16 @@ import (
 	"github.com/realclientip/realclientip-go"
 )
 
-func main() {
+func Example_middleware() {
 	// Choose the right strategy for our network configuration
-	clientIPStrategy, err := realclientip.RightmostNonPrivateStrategy("X-Forwarded-For")
+	strat, err := realclientip.NewRightmostNonPrivateStrategy("X-Forwarded-For")
 	if err != nil {
-		log.Fatal("realclientip.RightmostNonPrivateStrategy returned error (bad input)")
+		log.Fatal("realclientip.NewRightmostNonPrivateStrategy returned error (bad input)")
 	}
 
 	// Place our middleware before the handler
-	httpServer := httptest.NewServer(clientIPMiddleware(clientIPStrategy, http.HandlerFunc(handler)))
+	handlerWithMiddleware := clientIPMiddleware(strat, http.HandlerFunc(handler))
+	httpServer := httptest.NewServer(handlerWithMiddleware)
 	defer httpServer.Close()
 
 	req, _ := http.NewRequest("GET", httpServer.URL, nil)
@@ -37,15 +38,19 @@ func main() {
 	}
 
 	fmt.Printf("%s", b)
+	// Output:
+	//  your IP: 3.3.3.3
 }
 
 type clientIPCtxKey struct{}
 
 // Adds the "real" client IP to the request context under the clientIPCtxKey{} key.
 // If the client IP couldn't be obtained, the value will be an empty string.
-func clientIPMiddleware(clientIPStrategy realclientip.Strategy, next http.Handler) http.Handler {
+// We could use the RightmostNonPrivateStrategy concrete type, but instead we'll pass
+// around the Strategy interface, in case we decide to change our strategy in the future.
+func clientIPMiddleware(strat realclientip.Strategy, next http.Handler) http.Handler {
 	fn := func(w http.ResponseWriter, r *http.Request) {
-		clientIP := clientIPStrategy(r.Header, r.RemoteAddr)
+		clientIP := strat.ClientIP(r.Header, r.RemoteAddr)
 		if clientIP == "" {
 			// Write error log. Consider aborting the request depending on use.
 			log.Fatal("Failed to find client IP")
@@ -59,5 +64,6 @@ func clientIPMiddleware(clientIPStrategy realclientip.Strategy, next http.Handle
 }
 
 func handler(w http.ResponseWriter, r *http.Request) {
-	fmt.Fprintln(w, "your IP:", r.Context().Value(clientIPCtxKey{}))
+	clientIP := r.Context().Value(clientIPCtxKey{})
+	fmt.Fprintln(w, "your IP:", clientIP)
 }