Skip to content

Conversation

@ZZromanZZ
Copy link
Contributor

@ZZromanZZ 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.

private $timeout;

/** @var $context */
private $context;
Copy link
Member

Choose a reason for hiding this comment

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

@var array

@dg
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
Copy link
Contributor Author

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

$errno, $error, $this->timeout, STREAM_CLIENT_CONNECT, $context
);
if (!$this->connection) {
if (!$error) $error = error_get_last()['message'];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Here is missing { ... }

@ZZromanZZ ZZromanZZ changed the title WIP: Allow set stream context Allow set stream context Jun 12, 2016
@JanTvrdik
Copy link
Contributor

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

@ZZromanZZ
Copy link
Contributor Author

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
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
Copy link
Contributor Author

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
Copy link
Member

dg commented Jun 12, 2016

Exactly, otherwise it breaks #19

@ZZromanZZ
Copy link
Contributor Author

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
Copy link
Contributor

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@dg
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
Copy link
Contributor Author

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

@dg
Copy link
Member

dg commented Jun 13, 2016

Great

@dg dg merged commit c15fc20 into nette:master Jun 13, 2016
@ZZromanZZ
Copy link
Contributor Author

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants