Fix Real IP logic

[cl-bvl]

Hello. This fix for realIP logic. We should check for trusting not real IP, but RemoteIP, who sends the request. For example, we have a client - 1.1.1.1 and LB - 8.8.8.8. LB are trusting, all requests sended by it have X-Real-Ip header with client IP and we should extract it from headers. We should not extract RealIP from requests sended from another hosts (not our LB). Current implementation checking client IP for trusting, but it's incorrect.

[cl-bvl]

Hello. Can you please review this changes

[aldas]

Is cause by https://github.com/labstack/echo/issues/1834

[aldas]

Maybe something like that would be better

In case X-Real-Ip header is present we check if we can trust Request.RemoteAddr and If we can -we will use X-Real-Ip value. Also we parse X-Real-Ip value just to be sure it is valid IP.

// ExtractIPFromRealIPHeader extracts IP address using X-Real-Ip header only when we trust Request.RemoteAddr IP.
// Use this if you put proxy which uses this header.
func ExtractIPFromRealIPHeader(options ...TrustOption) IPExtractor {
	checker := newIPChecker(options)
	return func(req *http.Request) string {
		directIP := extractIP(req)
		realIP := req.Header.Get(HeaderXRealIP)
		if realIP != "" {
			if dIP := net.ParseIP(directIP); dIP != nil && checker.trust(dIP) {
				realIP = strings.TrimPrefix(realIP, "[")
				realIP = strings.TrimSuffix(realIP, "]")
				if rIP := net.ParseIP(realIP); rIP != nil {
					return realIP
				}
			}
		}
		return directIP
	}
}

p.s. Do not forget to make TestExtractIPFromRealIPHeader tests fail with current implementation and then add fixed implementation that should now pass these modified tests. If there are no failing tests then please add at lease one.

[cl-bvl]

Hello, thanks. The tests are fixed. Now it's failing on master code and passes on fixed version.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (584cb85) 92.89% compared to head (92a8221) 92.90%. Report is 4 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2550   +/-   ##
=======================================
  Coverage   92.89%   92.90%           
=======================================
  Files          39       39           
  Lines        4658     4662    +4     
=======================================
+ Hits         4327     4331    +4     
  Misses        240      240           
  Partials       91       91           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[codercms]

@aldas is there any chance to get this changes merged? Current X-Real-IP logic is really frustrating because it checks client IP extracted from X-Real-IP header, but it should check direct IP for trust.

[aldas]

alright, done. I look this issue couple weeks ago but did not want to merge because I did not remember how this IP worked. Fortunately we have fairly good explanations at the beggining of ip.go. I do not want to merge stuff that I do not remember how it should work.