Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Http][DI] Replace REMOTE_ADDR in trusted proxies with the current REMOTE_ADDR#33574

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:4.4frommcfedr:trusted-proxies
Sep 27, 2019

Conversation

@mcfedr
Copy link
Contributor

QA
Branch?4.4
Bug fix?no
New feature?yes
Deprecations?no
LicenseMIT
Doc PR

Currently handling trusted ips when deploying behind some CDNs/Load balancers such as ELB is difficult because they dont have a constant IP address, its possible to overcome this as is suggested by the docs -https://symfony.com/doc/current/deployment/proxies.html#but-what-if-the-ip-of-my-reverse-proxy-changes-constantly - by settings trusted proxies to$request->server->get('REMOTE_ADDR') - but this has to be done in code, and so becomes dangerous if you code is deployed in different environments.

This change would allow the developer to stick to providing the envvarTRUSTED_PROXIES, and in the environment behind a ELB set the value to the literal stringREMOTE_ADDR, and have it replaced at run time. This way in environments that are not using ELB his app is kept safe.

I think doing this replacement inRequest:: setTrustedProxies is the best place because it means this feature isn't exposed to other parts of the code that might callRequest::getTrustedProxies.

@nicolas-grekas
Copy link
Member

I don't think you need this: use'0.0.0.0/0' instead and that should provide what you want.
I think we should update the documentation instead. Would you be up to submit a PR there?

@stof
Copy link
Member

@nicolas-grekas this is not equivalent. Proxies can be chained. TrustingREMOTE_ADDR is trusting the first level of proxy. Trusting the whole web allows anyone to spoof their IP address (as they can always send aForwarded header to pretend that their actual IP is a proxy one)

@stof
Copy link
Member

@mcfedr can you check whether your ELB proxies appears as an remote address belonging to the private IP range ? If yes, maybe you could trust the private IP ranges for your use case (though I don't know how isolated your AWS instances are when not using an AWS VPC).

@nicolas-grekas
Copy link
Member

Trusting REMOTE_ADDR is trusting the first level of proxy. Trusting the whole web allows anyone to spoof their IP address

you may be right, but I'm missing the why:

    public function isFromTrustedProxy()    {        return self::$trustedProxies && IpUtils::checkIp($this->server->get('REMOTE_ADDR'), self::$trustedProxies);    }

This only reads the direct REMOTE_ADDR. When does the difference you describe apply?

@stof
Copy link
Member

@nicolas-grekasgetClientIp cares about chained proxies.isFromTrustedProxy indeed only cares about the last hop. But that's not the only usage ofRequest::$trustedProxies.

@nicolas-grekas
Copy link
Member

getClientIp usesisFromTrustedProxy, so looks safe to me. Do you have a specific example of what I'm missing?

@stof
Copy link
Member

stof commentedSep 18, 2019
edited
Loading

@nicolas-grekas italso uses$this->getTrustedValues when you are from a trusted proxy, which is exactly the case I'm talking about.
Trusting0.0.0.0/0 will consider any (IPv4) value as trusted.

@mcfedr
Copy link
ContributorAuthor

mcfedr commentedSep 18, 2019
edited
Loading

The chain is the problem with using0.0.0.0/0 because the CDN/LB makes a request from a public IP address (in the case of cloudfront its from this massive rangehttps://ip-ranges.amazonaws.com/ip-ranges.json that changes quite often) - so I always want to trust the first level, but if the client request to cloudfront already contains ax-forwarded-for header cloudfront just extends it, so if i put0.0.0.0/0 as trusted - i've automatically trusted the proxy before cloudfront as well. (I've firewalled the servers so only CF can make requests to them)

@nicolas-grekas
Copy link
Member

if the client request to cloudfront already contains a x-forwarded-for header cloudfront just extends it

Oh, OK! Now I get the issue, it's related togetTrustedValues callingnormalizeAndFilterClientIps itself usingself::$trustedProxies. Thanks for your explanations and patience :)

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

(with a minor CS suggestion)

@mcfedr
Copy link
ContributorAuthor

All done.

@fabpot
Copy link
Member

Thank you@mcfedr.

mcfedr reacted with thumbs up emoji

fabpot added a commit that referenced this pull requestSep 27, 2019
… the current REMOTE_ADDR (mcfedr)This PR was merged into the 4.4 branch.Discussion----------[Http][DI] Replace REMOTE_ADDR in trusted proxies with the current REMOTE_ADDR| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | no| New feature?  | yes| Deprecations? | no| License       | MIT| Doc PR        |Currently handling trusted ips when deploying behind some CDNs/Load balancers such as ELB is difficult because they dont have a constant IP address, its possible to overcome this as is suggested by the docs -https://symfony.com/doc/current/deployment/proxies.html#but-what-if-the-ip-of-my-reverse-proxy-changes-constantly - by settings trusted proxies to `$request->server->get('REMOTE_ADDR')` - but this has to be done in code, and so becomes dangerous if you code is deployed in different environments.This change would allow the developer to stick to providing the envvar `TRUSTED_PROXIES`, and in the environment behind a ELB set the value to the literal string `REMOTE_ADDR`, and have it replaced at run time. This way in environments that are not using ELB his app is kept safe.I think doing this replacement in `Request:: setTrustedProxies` is the best place because it means this feature isn't exposed to other parts of the code that might call `Request::getTrustedProxies`.Commits-------643c9ff Replace REMOTE_ADDR in trusted proxies with the current REMOTE_ADDR
@fabpotfabpot merged commit643c9ff intosymfony:4.4Sep 27, 2019
@mcfedrmcfedr deleted the trusted-proxies branchSeptember 27, 2019 06:33
@nicolas-grekasnicolas-grekas removed this from thenext milestoneOct 27, 2019
@nicolas-grekasnicolas-grekas added this to the4.4 milestoneOct 27, 2019
This was referencedNov 12, 2019
nicolas-grekas added a commit that referenced this pull requestSep 3, 2024
…or private IP address ranges to `Request::setTrustedProxies()` (nicolas-grekas)This PR was merged into the 7.2 branch.Discussion----------[HttpFoundation] Add `PRIVATE_SUBNETS` as a shortcut for private IP address ranges to `Request::setTrustedProxies()`| Q             | A| ------------- | ---| Branch?       | 7.2| Bug fix?      | no| New feature?  | no| Deprecations? | no| Issues        | -| License       | MITLet's save some memory allocations and callbacks when we can.Tweaks#33574 and#52924Commits-------6bd4b4a [HttpFoundation] Add `PRIVATE_SUBNETS` as a shortcut for private IP address ranges to `Request::setTrustedProxies()`
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

5 participants

@mcfedr@nicolas-grekas@stof@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp