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

Adds TLS support #122

Merged
merged 13 commits into from Sep 5, 2018
Merged

Adds TLS support #122

merged 13 commits into from Sep 5, 2018

Conversation

tomponline
Copy link
Contributor

Hi @repejota

This is my initial commit to implement TLS support as request in #121

Its pretty rough at the moment, e.g. its missing the ability to specify a custom CA cert, and does not verify the server's cert.

However I hope it is a basis for further development and an opportunity to check that you are happy with the approach.

Thanks
Tom

@tomponline tomponline changed the title Initial commit for adding TLS support Adds TLS support Jul 21, 2018
@tomponline
Copy link
Contributor Author

@repejota I've updated this PR to allow a custom stream context to be passed in, allowing any of the TLS options to be configured externally to the library. This has been added to the ConnectionOptions class.

Example usage:

<?php
require_once __DIR__.'/vendor/autoload.php';


$context = stream_context_create();
stream_context_set_option($context, 'ssl', 'verify_peer', true);
stream_context_set_option($context, 'ssl', 'cafile', '/var/lib/puppet/ssl/certs/ca.pem');

$connectionOptions = new \Nats\ConnectionOptions();
$connectionOptions->setHost('localhost')->setPort(4443)->setStreamContext($context);
$client = new \Nats\Connection($connectionOptions);
$client->connect();
// Simple Subscriber.
$client->subscribe(
    'foo',
    function ($message) {
        printf("Data: %s\r\n", $message->getBody());
    }
);

// Wait for 1 message.
$client->wait();
$client->close();

$this->processServerInfo($infoResponse);
if ($this->serverInfo->isTLSRequired()) {
set_error_handler(
function ($errno, $errstr, $errfile, $errline) {
Copy link

Choose a reason for hiding this comment

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

@tomponline: Should we include restore_error_handler here (before we throw)? Otherwise once this is caught, we won't have cleaned up the error handler we set, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@ancarda
Copy link

ancarda commented Aug 1, 2018

So, I've been looking into why this pull request failed. What I suspect might be the cause is a recent commit to NATS (June 21st) adds omitempty to several fields including auth_required. This seems to cause undefined index notices when using NATS 1.2.0. This would normally not be an issue but the PHPUnit XML in this repo converts notices to exceptions.

This code was passing in master, possibly as the Docker image for NATS has been updated since the last commit 68e6d24 (June 9th) which is before the aforementioned commit adding omitempty.

@repejota: Is there a way to -- and would you want to -- restrict which version of the Docker image you are using?

As I can't seem to commit directly here, I've prepared a patch file which @tomponline can apply to fix this.

@tomponline
Copy link
Contributor Author

@repejota this has been updated to work with unit tests on latest gnatsd server now, are you OK to merge?

tomponline and others added 5 commits August 17, 2018 10:45
This commit improves support for NATS 1.2.x by checking if several
fields; `auth_required`, `tls_required`, and `tls_verify` are set in
the initial INFO packet. As of NATS 1.2, these fields aren't always
set, which can cause an E_NOTICE to be raised.

Without this change, the unit tests are failing because the Docker
container has been updated to 1.2.0, and PHPUnit is configured to
treat notices as test failures.

See: nats-io/nats-server@17fecd4#diff-91bbeda7eb98a7adc57b9e47e2cf5c2b
@tomponline
Copy link
Contributor Author

Hi @repejota have you had a chance to look at this since you've come back from holiday? Would be great to get this merged if you're happy with it. Trying to avoid having to fork. Cheers

@repejota repejota self-assigned this Sep 5, 2018
@repejota repejota self-requested a review September 5, 2018 12:19
Copy link
Owner

@repejota repejota left a comment

Choose a reason for hiding this comment

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

Yay!
Really good work! I have nothing to comment, this is just a great pull request :)

@repejota repejota merged commit ca1b211 into repejota:develop Sep 5, 2018
@tomponline
Copy link
Contributor Author

Awesome thanks @repejota

@repejota
Copy link
Owner

repejota commented Sep 5, 2018 via email

@onyxg
Copy link

onyxg commented Sep 5, 2018

@tomponline this is gold... will implement with our Golang stack and advise

@mardachevdv
Copy link

Hi @repejota
Could you release new version with this changes?

@sazo
Copy link

sazo commented Oct 31, 2018

@repejota Really need this to go live. We are a company using the package but we cant go to production before TLS is supported. :)

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

Successfully merging this pull request may close these issues.

None yet

6 participants