Add TLS client certificate to SERVER_PARAMS

[philrennie]

related to #324

Attempt to extract a client certificate from the request and add it to server params as SSL_CLIENT_CERT (as per apache).

Allows Middleware to get at the client cert, which I need in my instance for mutual TLS based auth.

Needs verify_peer and capture_peer_cert to be enabled as ssl options e.g.
$socket = new React\Socket\SecureServer( $socket, $loop, [ 'local_cert' => $localCert, 'local_pk' => $localKey, 'allow_self_signed' => true, 'verify_peer' => true, 'capture_peer_cert' => true, ] );

[clue]

@philrennie Thank you for looking into this, the feature makes perfect sense 👍

May I ask you to add some test case to verify this works as expected? Additionally, the openssl_x509_export() function is not available on all platforms, so this should be checked first. Also, please update this to follow existing PSR-2 code style.

[philrennie]

@clue Happy to keep things up to the projects standards.

I've just realised while starting to update the tests that I'm accessing the stream property of the React\Socket\Connection object, which is marked as @internal.

Would its use here be counted as a valid or invalid internal use of the property? If this is an invalid access I'm not sure what changes to suggest to expose the stream in a way that suits the project.

[ghost]

Any updates on this feature?

@philrennie The @internal attributes discourages usages of the entity outside of the package. Usage of the stream property inside react/http is therefore permitted.

[ghost]

@clue

Additionally, the openssl_x509_export() function is not available on all platforms, so this should be checked first.

The function is always available when the OpenSSL extension is loaded. And the OpenSSL extension must be loaded in order to use TLS (stream_socket_enable_crypto calls to ext-openssl). I'd argue that no check is needed.

[philrennie]

Strangely enough the project that required this went dormant just after I asked, and has just come back round again last week.

I'm now trying to add some supporting tests for this, although for starters I'm having trouble getting parts of the existing test suite to run on my machine. Once I've gotten things running and the tests expanded I'll update the PR.

[WyriHaximus]

I'm now trying to add some supporting tests for this, although for starters I'm having trouble getting parts of the existing test suite to run on my machine. Once I've gotten things running and the tests expanded I'll update the PR.

If there is anyway we can help with that let us know here or on Gitter

[michaelphipps]

I've tried this code out, and there is an error with

$sslOptions = stream_context_get_options($conn->stream);

inside the parseRequest function because $conn hasn't been passed in.

What is the correct way of accessing $conn from inside parseRequest?

[michaelphipps]

The proposed solution just adds the peer_certificate to $serverParams

-----BEGIN CERTIFICATE-----
...
-----END CERTIFICATE-----

When Apache adds SSL Options to $_SERVER, rather than the client certificate, it posts all data of the certificate

[SSL_TLS_SNI], [SSL_SERVER_S_DN_CN], [SSL_SERVER_I_DN_C], [SSL_SERVER_I_DN_O], 
[SSL_SERVER_I_DN_CN], [SSL_CLIENT_S_DN_C], [SSL_CLIENT_S_DN_ST], [SSL_CLIENT_S_DN_L], 
[SSL_CLIENT_S_DN_O], [SSL_CLIENT_S_DN_OU], [SSL_CLIENT_S_DN_CN], [SSL_CLIENT_S_DN_Email], 
[SSL_CLIENT_I_DN_C], [SSL_CLIENT_I_DN_ST], [SSL_CLIENT_I_DN_L], [SSL_CLIENT_I_DN_O], 
[SSL_CLIENT_I_DN_OU], [SSL_CLIENT_I_DN_CN], [SSL_CLIENT_I_DN_Email], [SSL_SERVER_SAN_DNS_0], 
[SSL_VERSION_INTERFACE], [SSL_VERSION_LIBRARY], [SSL_PROTOCOL], [SSL_SECURE_RENEG], 
[SSL_COMPRESS_METHOD], [SSL_CIPHER], [SSL_CIPHER_EXPORT], [SSL_CIPHER_USEKEYSIZE], 
[SSL_CIPHER_ALGKEYSIZE], [SSL_CLIENT_VERIFY], [SSL_CLIENT_M_VERSION], [SSL_CLIENT_M_SERIAL], 
[SSL_CLIENT_V_START], [SSL_CLIENT_V_END], [SSL_CLIENT_V_REMAIN], [SSL_CLIENT_S_DN], [SSL_CLIENT_I_DN], 
[SSL_CLIENT_A_KEY], [SSL_CLIENT_A_SIG], [SSL_CLIENT_CERT_RFC4523_CEA], [SSL_SERVER_M_VERSION], 
[SSL_SERVER_M_SERIAL], [SSL_SERVER_V_START], [SSL_SERVER_V_END], [SSL_SERVER_S_DN], 
[SSL_SERVER_I_DN], [SSL_SERVER_A_KEY], [SSL_SERVER_A_SIG], [SSL_SESSION_ID], [SSL_SESSION_RESUMED]

The certificate data can be accessed with $cert_array = (openssl_x509_parse($cert)); and added to $serverParams

So there's the choice of just having the certificate or the certificate's data in an array. What is the better approach?

[clue]

So there's the choice of just having the certificate or the certificate's data in an array. What is the better approach?

The former appears to be a prerequisite for exposing the latter, so I think this does provide some value on its own, but I agree that exposing this in a more structured form makes this much easier to use 👍

I think exposing the certificate (raw contents or parsed data) is definitely valuable, but we still have to work out proper tests to ensure this works across supported platforms and evaluate potential performance impacts. I don't currently have an immediate use case, but I'm happy to help if anybody feels like picking this up 👍

[SimonFrings]

It seems like this pull request is open for quite a while now and didn't receive any updates since. Last comment is over a year old and I want to avoid pull requests laying around for too long without any further communication, so I'd suggest we close this for now. If this topic is still relevant and needs some further investigation, we can always reopen it in the future and take another look 👍

@clue, @WyriHaximus what do you think?

[michaelphipps]

I've recently been using nginx instead of apache for some projects, and the client ssl certificate is passed through a header rather than server variables. For that reason, I don't think a tls client certificates should be handled by this library. It should be handled separately.

eg, in nginx (setup as a proxy) I add the ssl client certifcate to a header:

 proxy_set_header X-SSL-CERT $ssl_client_escaped_cert;

so I can then access the certificate from reactphp:

$data['ssl_cert'] = urldecode($request->getHeaderLine('X-SSL-CERT'));

[clue]

I've recently been using nginx instead of apache for some projects, and the client ssl certificate is passed through a header rather than server variables. For that reason, I don't think a tls client certificates should be handled by this library. It should be handled separately.

eg, in nginx (setup as a proxy) I add the ssl client certifcate to a header:

 proxy_set_header X-SSL-CERT $ssl_client_escaped_cert;

so I can then access the certificate from reactphp:

$data['ssl_cert'] = urldecode($request->getHeaderLine('X-SSL-CERT'));

@michaelphipps Using a reverse proxy in front for TLS termination is definitely a good idea and I agree using HTTP headers to transport this information should cover this for most use cases. 👍

It seems like this pull request is open for quite a while now and didn't receive any updates since. Last comment is over a year old and I want to avoid pull requests laying around for too long without any further communication, so I'd suggest we close this for now. If this topic is still relevant and needs some further investigation, we can always reopen it in the future and take another look +1

@clue, @WyriHaximus what do you think?

@SimonFrings Agreed! I think this is an interesting feature and I'd love to look into this again in the future, but I agree it's unfortunate this hasn't received any input in a while and should be closed for now.

Please come back with more details if this is still relevant and we can always reopen this! 👍