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

set engine.TrustedProxies For items that don't use gin.RUN #2692

Merged
merged 8 commits into from
May 25, 2021
Merged

set engine.TrustedProxies For items that don't use gin.RUN #2692

merged 8 commits into from
May 25, 2021

Conversation

yiranzai
Copy link
Contributor

@yiranzai yiranzai commented Apr 15, 2021

#2632

#2675

#2675 causes the engine.prepareTrustedCIDRS to never execute if gin is not started via gin.RUN().

this pr set engine.TrustedProxies and execute engine.prepareTrustedCIDRS For items that don't use gin.RUN

Our project uses http.Handler to start the service instead of gin.Run

@yiranzai
Copy link
Contributor Author

The failed is not my reason https://travis-ci.org/github/gin-gonic/gin/jobs/767120791

@codecov
Copy link

codecov bot commented Apr 21, 2021

Codecov Report

Merging #2692 (60dfff7) into master (328d0b8) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2692      +/-   ##
==========================================
+ Coverage   98.68%   98.69%   +0.01%     
==========================================
  Files          41       41              
  Lines        2054     2070      +16     
==========================================
+ Hits         2027     2043      +16     
  Misses         15       15              
  Partials       12       12              
Impacted Files Coverage Δ
gin.go 99.18% <100.00%> (+0.05%) ⬆️

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 328d0b8...60dfff7. Read the comment docs.

@@ -366,6 +366,23 @@ func (engine *Engine) prepareTrustedCIDRs() ([]*net.IPNet, error) {
return cidr, nil
}

// SetTrustedProxies set Engine.TrustedProxies
func (engine *Engine) SetTrustedProxies(trustedProxies []string) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add some unit test, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

assert.Equal(t, "20.20.20.20", c.ClientIP())

// Use hostname that resolves to all the proxies
c.engine.TrustedProxies = []string{"foo"}
resetTrustedCIDRs(c)
_ = c.engine.SetTrustedProxies([]string{"foo"})
assert.Equal(t, "40.40.40.40", c.ClientIP())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I passed the previous unit tests, but I don't understand why the foo bar bar would expect such a result

@@ -326,11 +326,11 @@ func iterate(path, method string, routes RoutesInfo, root *node) RoutesInfo {
func (engine *Engine) Run(addr ...string) (err error) {
defer func() { debugPrintError(err) }()

trustedCIDRs, err := engine.prepareTrustedCIDRs()
err = engine.parseTrustedProxies()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not add this to all Run* methods too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the other RUN* didn't exist before.

My core idea was to be able to dynamically adjust the TurstedProxies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix bugs caused by #2675

The bugs:

#2693

#2697

@xpunch
Copy link

xpunch commented Apr 30, 2021

TrustedProxies is set with []string{"0.0.0.0/0"} by default, you'd better give trustedCIDRs a default value. So we don't need to call SetTrustedProxies when we don't need to change it.

@yiranzai
Copy link
Contributor Author

yiranzai commented Apr 30, 2021

TrustedProxies is set with []string{"0.0.0.0/0"} by default, you'd better give trustedCIDRs a default value. So we don't need to call SetTrustedProxies when we don't need to change it.

@xpunch

I haven't changed any of the previous logic, this is a PR that avoids #2675 bug in a new way. As for what you said, gin.Run did that

@yiranzai yiranzai mentioned this pull request Apr 30, 2021
@xpunch
Copy link

xpunch commented Apr 30, 2021

Your code is used to set custom TrustedProxies when don't use gin.Run.
I think the simplest way is set default value for trustedCIDRs.

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

Default value of trustedCIDRs is missing when gin.New() called, TrustedProxies is set to []string{"0.0.0.0/0"}, which means trustedCIDRs should also set a default value matchs TrustedProxies.

@evanfuller
Copy link

@yiranzai what is the status on this change? I would love to use it.

@yiranzai
Copy link
Contributor Author

yiranzai commented May 7, 2021

@evanfuller I don't know.

@thinkerou @estroz reviewer?

What else do I need to do?

@yiranzai yiranzai requested a review from thinkerou May 7, 2021 12:45
@atcharles
Copy link

The easiest way:
Copy the following to add to the reverse proxy:
proxy_set_header X-Appengine-Remote-Addr $remote_addr;

@FeodorFitsner
Copy link

When this fix is planned for release?

FeodorFitsner added a commit to pglet/pglet that referenced this pull request May 22, 2021
FeodorFitsner added a commit to pglet/pglet that referenced this pull request May 22, 2021
* Support Google App Engine

* Output all request headers

* PGLET_TRUSTED_PROXIES

* Display remoteIP and trusted proxies

* Temporary fix for content.ClientIP()

gin-gonic/gin#2692

* Principal.IP fixed

* Do not delete page if no clients connected

* Support batched get
@thinkerou thinkerou requested review from manucorporat, appleboy, austinheap, javierprovecho and a team and removed request for a team May 23, 2021 02:00
@thinkerou thinkerou added this to the v1.8 milestone May 23, 2021
Copy link
Member

@thinkerou thinkerou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks!

@appleboy appleboy merged commit b5ca989 into gin-gonic:master May 25, 2021
@yiranzai yiranzai deleted the set_trusted_proxy branch May 25, 2021 08:55
@leafduo
Copy link

leafduo commented Jul 16, 2021

This fix has 2 problems:

  1. it's not backward-compatible
  2. is doesn't update related documents

@almas1992
Copy link

Is there any plan to release this change?

@rbeuque74
Copy link
Contributor

Hello,

Is there any plan to release this change?

I couldn't agree more. Our software have been bumped to v1.7 because we follow security advisories, and you published GHSA-h395-qcrw-5vmq. But at this time, without the release of SetTrustedProxies, we can't use the new version in production.
Would you mind to release v1.7.5 to fix that ?

Thanks,
Romain

@thinkerou thinkerou modified the milestones: v1.8, v1.7.5 Nov 21, 2021
thinkerou pushed a commit that referenced this pull request Nov 21, 2021
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
thinkerou pushed a commit that referenced this pull request Nov 23, 2021
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 23, 2021
thinkerou pushed a commit that referenced this pull request Nov 24, 2021
thinkerou pushed a commit that referenced this pull request Nov 24, 2021
lukseven added a commit to eluv-io/common-go that referenced this pull request Jan 31, 2022
gin 1.7.0 added a feature to accept/refuse proxy headers depending on
the remote IP of the TCP connection. A fix of a data race in gin 1.7.1
made that feature inaccessible unless gin is started through gin.Run(),
which we don't do in the fabric. A PR to fix this has not been approved
for over a month (gin-gonic/gin#2692).

Hence I implemented similar logic in our own helper, and consolidated
with an existing helper (that we used in addition to the gin-specific
functionality!). In addition, the client IP is cached in the gin context
in order to improve performance (multiple middleware handlers use the
client IP for logging, usage, geo lookups, redirect, etc.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.