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

Allow set stream context #30

Merged
merged 1 commit into from Jun 13, 2016
Merged

Allow set stream context #30

merged 1 commit into from Jun 13, 2016

Conversation

@ZZromanZZ
Copy link
Contributor

ZZromanZZ commented Jun 12, 2016

In current implementation, there is no way to modify stream context parameters.

Which means there is no way to connect (since php 5.6) to SSL/TLS server with self-signed, expired, missmatched CN, etc..

See this problem on forum, for example.

If this is acceptable I would like to discuss possible approaches:

  • allow set full stream context
  • allow set only some values (verify_peer, verify_peer_name, etc...)

Also there is problem how to catch warnings from underlying Openssl library and provide it to SmtpException as error message.

There is MR, more like experiment, implementation is vague.

We can find inspiration for example in Ruby.

@@ -38,6 +38,9 @@ class SmtpMailer implements IMailer
/** @var int */
private $timeout;

/** @var $context */
private $context;

This comment has been minimized.

Copy link
@dg

dg Jun 12, 2016

Member

@var array

@dg

This comment has been minimized.

Copy link
Member

dg commented Jun 12, 2016

In current implementation, there is no way to modify stream context parameters.

There is: http://php.net/manual/en/function.stream-context-set-default.php. So there should be something like $context = $this->context ? stream_context_create($this->context) : stream_context_get_default().

Related #8 #19

@ZZromanZZ

This comment has been minimized.

Copy link
Contributor Author

ZZromanZZ commented Jun 12, 2016

👍 Yikes, I overlooked it in that bunch of PHP functions.

@ZZromanZZ ZZromanZZ force-pushed the ZZromanZZ:master branch from 232d252 to 282d762 Jun 12, 2016
);
if (!$this->connection) {
if (!$error) $error = error_get_last()['message'];

This comment has been minimized.

Copy link
@ZZromanZZ

ZZromanZZ Jun 12, 2016

Author Contributor

Function error_get_last() doesn't return OpenSSL warning here, but...

This comment has been minimized.

Copy link
@dg

dg Jun 12, 2016

Member

Here is missing { ... }

if (!stream_socket_enable_crypto($this->connection, TRUE, STREAM_CRYPTO_METHOD_TLS_CLIENT)) {
throw new SmtpException('Unable to connect via TLS.');
if (!@stream_socket_enable_crypto($this->connection, TRUE, STREAM_CRYPTO_METHOD_TLS_CLIENT)) {
$error = error_get_last();

This comment has been minimized.

Copy link
@ZZromanZZ

ZZromanZZ Jun 12, 2016

Author Contributor

.... here it works as expected and warning is catched:

Unable to connect via TLS: stream_socket_enable_crypto(): SSL operation failed with code 1. OpenSSL Error messages: error:14090086:SSL routines:ssl3_get_server_certificate:certificate verify failed

@ZZromanZZ ZZromanZZ force-pushed the ZZromanZZ:master branch from 282d762 to c6d1211 Jun 12, 2016
@ZZromanZZ ZZromanZZ changed the title WIP: Allow set stream context Allow set stream context Jun 12, 2016
@ZZromanZZ ZZromanZZ force-pushed the ZZromanZZ:master branch from c6d1211 to 8a81fd2 Jun 12, 2016
@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Jun 12, 2016

@ZZromanZZ Setting openssl.cafile in php.ini did not work?

@ZZromanZZ

This comment has been minimized.

Copy link
Contributor Author

ZZromanZZ commented Jun 12, 2016

Of course, my primary intention is to allow ignoring verification via verify_peer=false and verifiy_peer_name=false.

However, I wouldn't like see this in README, docs or other tutorials, because this is basically bad practice.

Next, why allowing modify only some context options, what if someone needs fine-tune other options , not related ony to SSL/TLS.

@Majkl578

This comment has been minimized.

Copy link

Majkl578 commented Jun 12, 2016

Why would anyone conserned about security ever want to disable certificate verification? I don't like the idea with stream_context_get_default either, it's configuration by global environment...
Passing context is a good idea, but this use case is bad.

@ZZromanZZ

This comment has been minimized.

Copy link
Contributor Author

ZZromanZZ commented Jun 12, 2016

It provides same behaviour as before, thus no BC break if anyone used stream_context_set_default before calling connect() method. I think it was meant by @dg , didn't it ?

@dg

This comment has been minimized.

Copy link
Member

dg commented Jun 12, 2016

Exactly, otherwise it breaks #19

@ZZromanZZ

This comment has been minimized.

Copy link
Contributor Author

ZZromanZZ commented Jun 12, 2016

I have still problem with error_get_last(), it doesn't catch all warnings properly.
I'm starting to lean towards that let the implementation without error_get_last() and let SmtpException untouched.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Jun 12, 2016

@ZZromanZZ Why do you need to ignore verification? Just create your own ca bundle with the certificate you need. For example this is what I use https://gist.github.com/JanTvrdik/9ad909f81d37697e580d3c06d9365852

@ZZromanZZ

This comment has been minimized.

Copy link
Contributor Author

ZZromanZZ commented Jun 12, 2016

@JanTvrdik I am aware of workarounds. And I do not take lightly skipping security checks at all.

However, there are circumstances when e.g. certificate is expired, has weak signature, etc... and you do not control server side to replace it. And, of course, show must go on (business must go on).

@ZZromanZZ ZZromanZZ force-pushed the ZZromanZZ:master branch from 8a81fd2 to e5110a5 Jun 13, 2016
@ZZromanZZ

This comment has been minimized.

Copy link
Contributor Author

ZZromanZZ commented Jun 13, 2016

So I dropped touching SmtpException completely. Is there anything else what can I do ?

@dg

This comment has been minimized.

Copy link
Member

dg commented Jun 13, 2016

And what if you want to bypass default context and have no context options? (I don't now if it is useful, just thinking).

It can be done with:

/** @var resource */
private $context;

function __constructor()
{
$this->context = isset($options['context']) ? stream_context_create($options['context']) : stream_context_get_default();
@ZZromanZZ ZZromanZZ force-pushed the ZZromanZZ:master branch from e5110a5 to 008710f Jun 13, 2016
@ZZromanZZ

This comment has been minimized.

Copy link
Contributor Author

ZZromanZZ commented Jun 13, 2016

I changed the implementation accordingly, it might be useful in later use cases, when stream context needs to be bypassed.

@dg

This comment has been minimized.

Copy link
Member

dg commented Jun 13, 2016

Great

@dg dg merged commit c15fc20 into nette:master Jun 13, 2016
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
coverage/coveralls Coverage increased (+0.08%) to 72.59%
Details
@ZZromanZZ

This comment has been minimized.

Copy link
Contributor Author

ZZromanZZ commented Jun 17, 2016

I just noticed, should not be 'context' also in $defaults array in MailExtension ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.