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

Expose underlying connection in ServerRequest #324

Open
dwgebler opened this issue Sep 1, 2018 · 4 comments
Open

Expose underlying connection in ServerRequest #324

dwgebler opened this issue Sep 1, 2018 · 4 comments

Comments

@dwgebler
Copy link

dwgebler commented Sep 1, 2018

The request object passed to the request handler should be able to access the underlying socket connection. At the moment, there is no way for the Connection object to be passed between a connection event handler and a request handler - an example of why this is an issue is data from a client certificate when using mutual authentication cannot be passed to the request handler (e.g. certificate Common Name).

@dwgebler
Copy link
Author

dwgebler commented Sep 1, 2018

Doesn't seem like it should be too difficult to expose the connection to a request handler. In StreamingServer, we should be able to do

$response = $callback($request, $conn);

Then surely just a matter of getting the MiddlewareRunner to make use of the connection being passed to it:

public function __invoke(ServerRequestInterface $request, ConnectionInterface $conn)
    {

       ...
        return $this->call($request, 0, $conn);
    }

    /** @internal */
    public function call(ServerRequestInterface $request, $position, ConnectionInterface $conn=null)
    {
        // final request handler will be invoked without a next handler
        if (!isset($this->middleware[$position + 1])) {
            $handler = $this->middleware[$position];
            return $handler($request, $conn);
        }

        $that = $this;
        $next = function (ServerRequestInterface $request) use ($that, $position, $conn) {
            return $that->call($request, $position + 1, $conn);
        };

        // invoke middleware request handler with next handler
        $handler = $this->middleware[$position];
        return $handler($request, $next, $conn);
    }

Due to the nature of __invoke, the extra $conn parameter should just be ignored on existing middleware handlers.

Is this kind of approach workable? I'm new to ReactPHP, so just trying to figure this out as I go.

@clue
Copy link
Member

clue commented Sep 2, 2018

@dwgebler Thanks you for bringing this up, I can see how this would be useful! I see there was discussion about this on IRC and it seems to resolve mostly around TLS (formerly SSL) parameters?

In vanilla PHP & Apache, you would expect to be able to retrieve the peer certificate from $_SERVER; obviously with React, depending how you design your server you may not be capturing peer certificates but what if the request serverParams could include the peer certificate if captured? that would line up the behaviour with ordinary PHP

This sounds like a nice feature to me! I agree that we should provide support for accessing TLS parameters (if available) and agree that "server params" would probably be the best place for this. This mimics how nginx (http://nginx.org/en/docs/http/ngx_http_ssl_module.html#variables) and Apache (http://httpd.apache.org/docs/current/mod/mod_ssl.html#envvars) work.

That being said, it's my understanding that this should only be exposed as part of "server params" without exposing the underlying connection. The concept of "connection" is a lower-level detail that we should probably not leak to the HTTP layer.

The TLS parameters can be accessed through stream_get_meta_data() as of PHP 7+ (http://php.net/manual/en/migration70.deprecated.php#migration70.deprecated.capture-session-meta). In PHP 5.6+ this required an additional context option (http://php.net/manual/en/migration56.openssl.php#migration56.openssl.metadata) that appears to be undocumented otherwise (http://php.net/manual/en/context.ssl.php). For the reference: We've used similar logic in the past (https://github.com/reactphp/http/pull/167/files#diff-c611f5024b5b13f69f779505d1c6735eR408) which became obsolete with #244. The gist of this is that we should probably provide a way to expose these options in the underlying socket component first (somewhat similar to reactphp/socket#150 possibly).

Before looking into this further, I would love to hear some more thoughts from more people 👍

I've you're feeling adventurous, here's a somewhat hacky gist to access these TLS parameters without having any dedicated APIs for this:

$server = new Server(function (ServerRequestInterface $request) {
    $connection = null;
    foreach (debug_backtrace() as $trace) {
        foreach ($trace['args'] as $arg) {
            if ($arg instanceof ConnectionInterface) {
                $connection = $arg;
                break 2;
            }
        }
    }

    ob_start();
    var_dump(
        $connection,
        isset($connection->stream) ? stream_get_meta_data($connection->stream) : null,
        isset($connection->stream) ? stream_context_get_options($connection->stream) : null
    );
    $response = ob_get_clean();

    return new Response(
        200,
        array(
            'Content-Type' => 'text/plain'
        ),
        $response
    );
});

@dwgebler
Copy link
Author

dwgebler commented Sep 2, 2018

Thanks @clue yes my interest in exposing the connection is to retrieve a client certificate at the HTTP layer, my particular use case being user level access control in a RESTful API. Obviously I've been looking for a quick solution (appreciate the hacky approach, which is great to carry on building a proof of concept of my API, but not a solution I can rely on in production) but yes from a design perspective, the HTTP layer should have the client certificate as part of the server environment rather than needing to know about a socket connection. Note we would need both meta data and context options to build the same server environment as Apache mod_ssl injects and would need to parse the peer certificate but AFAIK parsing is only possible with the openssl_ functions.

@phpbg
Copy link

phpbg commented Jan 24, 2019

+1 for:

  • not exposing directly the connection object (I see no reason to do it)
  • exposing client certificate somewhere

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

No branches or pull requests

3 participants