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

Network resolver instead of operate with REMOTE ADDR and more #119

Open
kamarton opened this issue Oct 2, 2019 · 16 comments

Comments

@kamarton
Copy link
Contributor

@kamarton kamarton commented Oct 2, 2019

I see the middleware already contains IP filtering so I would raise the following issues and suggestions.

The following examples are based on the use of yii2.

What steps will reproduce the problem?

Yii2's basic idea of ​​secure networks, headers, and IP definition is too simple.

  • On the one hand, there is an unnecessary function in the Request class trustedHosts and secureHeaders when not in use.
  • On the other hand, it is difficult or impossible to handle some of the networks schema.

The yii2 solution unnecessarily write conditions in the code that are difficult to redefine.

Here are some examples of network resolution:

  • check IP by ISP or reverse DNS
  • check route by network endpoints sequence (example from X-Forwarded-For header)
  • check and skip optional route's IP, if needed
  • check and skip route loops
  • get IP by tagged endpoint (The multiple routes require the IP addresses of the network endpoints, which may check and get the first, the last, all and exists.)
  • resolve user IP from accepted sequenced route

The former need not be integrated into the framework, but should be defined as an interface with a separate layer.

Suggests

interface NetworkResolverInterface {
  public function getRemoteIp(): string;
  public function getUserIp(): string;
  public function getIsSecureConnection(): bool;
}
class Request ... {
   private $networkResolver;

   __construct(..) {
     $this->networkResolver = SimpleNetworkResolver(); // use REMOTE_ADDR, SERVER_PORT etc.
   }

   public function withNetworkResolver(NetworkResolver $resolver) {..}
   public function getNetworkResolver(): NetworkResolver {...}
}

example TrustedHostsResolver class is the equivalent of yii2 trusted hosts.

Q A
Version 1.0.0-under development
PHP version -
Operating system -
@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Oct 2, 2019

  1. I don't think getIsSecureConnection() belongs here. It is part of PSR-7 i.e. you can do $request->getUri()->getScheme() === 'https' and it should be reliable.
  2. NetworkResolverInterface becomes IpInformationInterface or IpResolverInterface then.
@kamarton

This comment has been minimized.

Copy link
Contributor Author

@kamarton kamarton commented Oct 3, 2019

I don't think getIsSecureConnection() belongs here. It is part of PSR-7 i.e. you can do $request->getUri()->getScheme() === 'https' and it should be reliable.

PSR-7 OK.

interface NetworkResolverInterface {
  public function getRemoteIp(): string;
  public function getUserIp(): string;
  public function getScheme(): string;
}
class Request {
  public function getScheme() {
    return $this->getNetworkResolver()->getScheme();
  }
}

NetworkResolverInterface becomes IpInformationInterface or IpResolverInterface then.

The main problem is that network routes may require the all at once use of an IP path (remote address + X-forwarder-for), which already includes handling of headers. Also, example the HTTP schema definition may only come from the x-forwarded-proto header.

In addition, somehow the data from the headers must be passed through the request-> resolver-> request object path. So, the interface is actually still small.

In addition, I can imagine that matchIP() would be aggregated here, as it is possible to use some IP paths for parts of the network path. The problem, of course, is when to define in which layer it is appropriate to using it.

I noticed this use:

if ($request->getServerParams()['REMOTE_ADDR'] !== $this->allowedIp) {

I do not consider it a good approach, what if a local software transmits it? (always 127.0.0.1):

@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Oct 3, 2019

getScheme() is part of PSR-7. Why do we need it separately?

@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Oct 3, 2019

Is it possible that we have http:// in URL but https in your variant of getScheme()?

@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Oct 3, 2019

Seems I was tired. Sorry for misleading comments. getIsSecureConnection() or getScheme() is valid for the interface.

@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Oct 3, 2019

X-Forwarded-Host is likely to be added as well.

@samdark samdark added the type:feature label Oct 4, 2019
@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Oct 4, 2019

  1. I agree that it should be a different component considering that wrapping PSR ServerRequest isn't that good idea (would potentially lock-in middleware created by community to be Yii-specific).
  2. Behavior should be ported from Yii 2 after yiisoft/yii2#16122, yiisoft/yii2#17521 are solved.
  3. I agree that IP filter should be modified to use it.
@kamarton

This comment has been minimized.

Copy link
Contributor Author

@kamarton kamarton commented Oct 4, 2019

I agree that it should be a different component considering that wrapping PSR ServerRequest isn't that good idea (would potentially lock-in middleware created by community to be Yii-specific).

Yes, it adds a lot of functionality to the Request class of Yii2 as well, which unduly increases its complexity.

Seems I was tired. Sorry for misleading comments. getIsSecureConnection() or getScheme() is valid for the interface.

The result of getIsSecureConnection() can be decided based on getScheme(), so getScheme() is sufficient.

@kamarton

This comment has been minimized.

Copy link
Contributor Author

@kamarton kamarton commented Oct 4, 2019

Another problem with the Request class used with Yii2 is that it hides headers (eg X-Forward-For), in fact just to make trustedHosts functionality is correct.

@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Oct 4, 2019

The result of getIsSecureConnection() can be decided based on getScheme(), so getScheme() is sufficient.

Yes, but in the following setup (external network uses HTTPS but internal uses HTTP) there is a protocol mismatch:

Client --HTTPS--> Trusted Proxy --HTTP--> Server

So it would be determined incorrectly in ServerRequestFactory since we're currently using $server['REQUEST_METHOD'].

So this network resolver could consist of two parts:

  1. Resolver itself.
  2. A middleware that modifies ServerRequest setting schema / host obtained from a trusted proxy.
@kamarton

This comment has been minimized.

Copy link
Contributor Author

@kamarton kamarton commented Oct 5, 2019

Yes, but in the following setup (external network uses HTTPS but internal uses HTTP) there is a protocol mismatch:

                       |
   client ---https---- proxy1 ---https---- proxy2 ---http--- server ... (---fastcgi--- PHP)
                       |
   so far relevant --->|<---- from here it is not interesting for normal use
interface NetworkResolverInterface {
  public function getRemoteIp(): string;
// user connection details
  public function getUserIp(): string;
  public function getUserScheme(): string;
// For internal processing of headers and additional data
  public function withWebRequest(WebRequest $request);
}

So it would be determined incorrectly in ServerRequestFactory since we're currently using $server['REQUEST_METHOD'].

Yes, that's why I thought about this NetworkResolver because it's not part of the Request, data manipulation.

I do not really see the function or purpose of the middleware currently being implemented

@samdark samdark changed the title Network resolver instaed of operate with REMOTE ADDR and more Network resolver instead of operate with REMOTE ADDR and more Oct 5, 2019
@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Oct 5, 2019

so far relevant --->|<---- from here it is not interesting for normal use

Correct but PSR-7 request instance is using a factory either it's ServerRequestFactory that uses $_SERVER or something like RoadRunner request factory we have no control of. Therefore, if we want user to have correct data in server request instance, we have to have a middleware that modifies it based on NetworkResolver.

kamarton pushed a commit to kamarton/yii-web that referenced this issue Oct 9, 2019
@kamarton kamarton referenced this issue Oct 9, 2019
4 of 4 tasks complete
kamarton pushed a commit to kamarton/yii-web that referenced this issue Oct 9, 2019
@kamarton

This comment has been minimized.

Copy link
Contributor Author

@kamarton kamarton commented Oct 9, 2019

@samdark Implementation in progress, experiences:

  • ServerRequest is not injectable (final class).
  • I don't really see if there is a better solution:
$nr = (new BasicNetworkResolver())->withServerRequest($request);
$userIp = $nr->getUserIp();
// vs. yii2
$userIp = Yii::$app->request->userIp;
@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Oct 9, 2019

Left some comment on the pull request.

kamarton pushed a commit to kamarton/yii-web that referenced this issue Oct 9, 2019
kamarton pushed a commit to kamarton/yii-web that referenced this issue Oct 12, 2019
kamarton pushed a commit to kamarton/yii-web that referenced this issue Oct 13, 2019
kamarton pushed a commit to kamarton/yii-web that referenced this issue Oct 15, 2019
kamarton pushed a commit to kamarton/yii-web that referenced this issue Oct 16, 2019
kamarton pushed a commit to kamarton/yii-web that referenced this issue Oct 17, 2019
kamarton pushed a commit to kamarton/yii-web that referenced this issue Oct 18, 2019
kamarton pushed a commit to kamarton/yii-web that referenced this issue Oct 20, 2019
kamarton pushed a commit to kamarton/yii-web that referenced this issue Oct 20, 2019
kamarton pushed a commit to kamarton/yii-web that referenced this issue Oct 22, 2019
kamarton pushed a commit to kamarton/yii-web that referenced this issue Oct 22, 2019
kamarton pushed a commit to kamarton/yii-web that referenced this issue Oct 23, 2019
kamarton pushed a commit to kamarton/yii-web that referenced this issue Oct 23, 2019
samdark added a commit that referenced this issue Oct 23, 2019
@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Oct 23, 2019

Basic resolver done by @kamarton.

@kamarton

This comment has been minimized.

Copy link
Contributor Author

@kamarton kamarton commented Nov 13, 2019

kamarton pushed a commit to kamarton/yii-web that referenced this issue Nov 19, 2019
kamarton pushed a commit to kamarton/yii-web that referenced this issue Nov 20, 2019
kamarton pushed a commit to kamarton/yii-web that referenced this issue Nov 20, 2019
@kamarton kamarton referenced this issue Nov 20, 2019
0 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.