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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(engine): add trustedproxies and remoteIP #2632

Merged
merged 24 commits into from Apr 6, 2021
Merged

feat(engine): add trustedproxies and remoteIP #2632

merged 24 commits into from Apr 6, 2021

Conversation

@manucorporat
Copy link
Contributor

@manucorporat manucorporat commented Feb 7, 2021

TODO:

  • add new tests
  • fix tests
@codecov
Copy link

@codecov codecov bot commented Feb 7, 2021

Codecov Report

Merging #2632 (9018d58) into master (f3de813) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2632      +/-   ##
==========================================
+ Coverage   98.64%   98.67%   +0.03%     
==========================================
  Files          41       41              
  Lines        1990     2038      +48     
==========================================
+ Hits         1963     2011      +48     
  Misses         15       15              
  Partials       12       12              
Impacted Files Coverage Δ
context.go 97.64% <100.00%> (+0.13%) ⬆️
gin.go 99.12% <100.00%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3de813...9018d58. Read the comment docs.

gin.go Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
return ip
}
}
}
Copy link
Member

@thinkerou thinkerou Feb 8, 2021

			if !cidr.Contains(remoteIP) {
                             continue
                         }
			for _, headerName := range c.engine.RemoteIPHeaders {
				ip, valid := validateHeader(c.requestHeader(headerName))
				if valid {
					return ip
				}
			}

suggestion, thanks!

Copy link
Contributor Author

@manucorporat manucorporat Feb 8, 2021

refactored it a little bit, review again... now thee logic is better separated between validating the trusted proxy and parsing the header, also a nw AP

ip, trusted = c.RemoteIP()

allows to even implement your own logic, or trust othe headers that might not be even related with IP!

Copy link
Contributor

@agmt agmt Aug 24, 2021

Do you expect that HTTP proxy running on c.RemoteIP() resets X-Forwarded-For? Because if it appends, then we can't inherit trustiness of c.RemoteIP() to all other proxies.

context.go Outdated Show resolved Hide resolved
@RainbowMango
Copy link

@RainbowMango RainbowMango commented Feb 8, 2021

Hi Forks, would you like to cut a new release after this PR?

@javierprovecho
Copy link
Member

@javierprovecho javierprovecho commented Feb 8, 2021

@RainbowMango we’ll work towards it, as this PR brings some new properties to gin.engine, the release should be 1.7.0, so I may need some sync with @appleboy and @thinkerou on that milestone.

@RainbowMango
Copy link

@RainbowMango RainbowMango commented Feb 8, 2021

Thanks for your quick response. I'm looking forward to the new release.

@manucorporat
Copy link
Contributor Author

@manucorporat manucorporat commented Feb 8, 2021

Alright team.

Added a new public API called:

ip, trusted := c.RemoteIP()

this function parsed the remote IP, and checks if it's a trusted proxy OR not.

Then clientIP function logic focuses in using this information and parse the appropriated headers.

Should help to keep logic separated and easier to test as well as a interesting new API, that can be used by developers to use their own logic and for example "trust" other headers.

Thoughts?

@thinkerou
Copy link
Member

@thinkerou thinkerou commented Apr 6, 2021

@appleboy we merge the pr and publish v1.7 now? thanks!

@appleboy
Copy link
Member

@appleboy appleboy commented Apr 6, 2021

@thinkerou Yes, waiting for the final Travis report.

@appleboy appleboy merged commit bfc8ca2 into master Apr 6, 2021
4 checks passed
@appleboy appleboy deleted the fix-client-ip-cve branch Apr 6, 2021
@sorenisanerd
Copy link
Contributor

@sorenisanerd sorenisanerd commented Apr 7, 2021

Why even have contribution guidelines if you don't follow them yourselves and ignore other people's contributions?

@sorenisanerd
Copy link
Contributor

@sorenisanerd sorenisanerd commented Apr 7, 2021

I mean.. Yeah, sure, that's one way to address a failing test:

https://github.com/sorenisanerd/gin/blob/d0bf406364d992443be33739745d61386fec4044/context_test.go#L1444-L1446

vs

gin/context_test.go

Lines 1431 to 1433 in bfc8ca2

// Only trust RemoteAddr
c.engine.TrustedProxies = []string{"40.40.40.40"}
assert.Equal(t, "20.20.20.20", c.ClientIP())

Also, your 230 days of optimization probably haven't yielded the returns you were hoping for:

gin/context.go

Line 770 in bfc8ca2

trustedCIDRs, _ := c.engine.prepareTrustedCIDRs()

@appleboy
Copy link
Member

@appleboy appleboy commented Apr 8, 2021

data race issue: #2674

@JasonCeng
Copy link

@JasonCeng JasonCeng commented Apr 20, 2021

Does CVE-2020-28483 have been fixed in gin V1.7.0 and later?

@sorenisanerd
Copy link
Contributor

@sorenisanerd sorenisanerd commented Apr 27, 2021

Does CVE-2020-28483 have been fixed in gin V1.7.0 and later?

Nope.

@xpunch
Copy link

@xpunch xpunch commented Apr 30, 2021

Hi guys, thanks for your work. But I meet an issue that ctx.ClientIP() not work cause engine.Run() not called.
My application uses gin as web handler, so it don't need to startup by calling engine.Run(), which makes trustedCIDRs uninitialized. I think I'm not the only one who use gin this way, could you consider move trustedCIDRs initialization into somewhere like a public method, so I can call it when I need if I don't want to call Run().

@xpunch
Copy link

@xpunch xpunch commented Apr 30, 2021

func New() *Engine {
	...
        engine.trustedCIDRs = []*net.IPNet{{IP: net.IP{0x0, 0x0, 0x0, 0x0}, Mask: net.IPMask{0x0, 0x0, 0x0, 0x0}}}
        ...
}

Should also set default value for trustedCIDRs.

@tsmethurst
Copy link

@tsmethurst tsmethurst commented Jul 24, 2021

Hi guys, thanks for your work. But I meet an issue that ctx.ClientIP() not work cause engine.Run() not called.
My application uses gin as web handler, so it don't need to startup by calling engine.Run(), which makes trustedCIDRs uninitialized. I think I'm not the only one who use gin this way, could you consider move trustedCIDRs initialization into somewhere like a public method, so I can call it when I need if I don't want to call Run().

I need this as well for a use case where I want to shut down the http server nicely; i also don't call Run() and wondered why it wasn't parsing trusted proxies properly... now i see!

@tsmethurst
Copy link

@tsmethurst tsmethurst commented Jul 24, 2021

Ahh i'm sorry, seems you already added it in here: superseriousbusiness@b5ca989

nevermind! :)

@montanaflynn
Copy link

@montanaflynn montanaflynn commented Aug 4, 2021

This appears to have changed the old behavior, is that correct? I was under the assumption this would be an opt-in change.

How can I just trust all proxies? I'm behind cloudfront and trust their proxies but don't have all their ip addresses.


// Only trust RemoteAddr
c.engine.TrustedProxies = []string{"40.40.40.40"}
assert.Equal(t, "20.20.20.20", c.ClientIP())
Copy link
Contributor

@agmt agmt Aug 24, 2021

If we trust proxy 40.40.40.40, but not trust 30.30.30.30 (proxy it is or not), then ClientIP should be 30.30.30.30 as 20.20.20.20 was set by somebody untrusted.
Please do not forget that X-Forwarded-For is appended, so it should be processed right-to-left:

right-most IP address is the IP address of the most recent proxy and the left-most IP address is the IP address of the originating client.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment