Add option novalidatecert to connect(); closes #63 by trasher · Pull Request #94 · laminas/laminas-mail

trasher · 2020-07-09T12:57:55Z

Q	A
Documentation	no
Bugfix	no
BC Break	no
New Feature	yes
RFC	no
QA	no

Description

Allow connection with self signed SSL certificates; see #63; port of existing zendframework/zend-mail#247.

Signed-off-by: Johan Cwiklinski <jcwiklinski@teclib.com>

trasher · 2020-07-09T13:05:11Z

Thanks @cedric-anne :))

weierophinney

First off, thanks for this patch! I'm sure a number of people will appreciate having it in place, and it will make testing locally, where you might be forced to use self-signed certs, far easier!

I think you can improve this substantially by moving the various functionalities around the $novalidatecert property and setter into the ProtocolTrait, as well as the functionality for creating the socket options. I've provided guidance in the comments below.

Additionally, it would be great if you could figure out a way to unit test this, as the new functionality is not covered at all. While I'm reasonably certain it will be fine, a unit test ensures we don't break it in the future.

weierophinney · 2020-07-09T15:14:38Z

                }
        }

+        $socket_options = [];


Please use camelCase for variable names for consistency (this is part of our coding standard).

Changed. I've also factorized this piece of code, since this was entirely duplicated in both Imap and Pop3 classes

weierophinney · 2020-07-09T15:16:25Z

-    public function __construct($host = '', $port = null, $ssl = false)
+    public function __construct($host = '', $port = null, $ssl = false, $novalidatecert = false)
    {
+        $this->novalidatecert = $novalidatecert;


Please call $this->setNoValidateCert($novalidatecert) to ensure you get proper validation of the value (see my notes on that method as well).

weierophinney · 2020-07-09T15:16:37Z

-    public function __construct($host = '', $port = null, $ssl = false)
+    public function __construct($host = '', $port = null, $ssl = false, $novalidatecert = false)
    {
+        $this->novalidatecert = $novalidatecert;


Please call $this->setNoValidateCert($novalidatecert) to ensure you get proper validation of the value (see my notes on that method as well).

Done aswell

weierophinney · 2020-07-09T15:17:05Z

+    public function setNoValidateCert($novalidatecert)
+    {
+
+        if (is_bool($novalidatecert)) {
+            $this->novalidatecert = $novalidatecert;
+        }
+    }
+


Please see my notes on the same method in the Imap class.

weierophinney · 2020-07-09T15:23:48Z

+        $socket_options = [];
+
+        if ($this->novalidatecert) {
+            $socket_options = [
+                'ssl' => [
+                    'verify_peer_name' => false,
+                    'verify_peer'      => false,
+                ]
+            ];
+        }
+
+        $socket_context = stream_context_create($socket_options);
+
        ErrorHandler::start();
-        $this->socket = fsockopen($host, $port, $errno, $errstr, self::TIMEOUT_CONNECTION);
+        $this->socket = stream_socket_client(
+            $host . ":" . $port,
+            $errno,
+            $errstr,
+            self::TIMEOUT_CONNECTION,
+            STREAM_CLIENT_CONNECT,
+            $socket_context
+        );
+


Please see my notes on this in the Imap class.

Since this code is reproduced in both classes, I'd argue that you should:

Move the $novalidatecert property into ProtocolTrait

Move the setNoValidateCert() method into ProtocolTrait

Create a new method in ProtocolTrait:

/** * @return array */ private function prepareSocketOptions() { return $this->novalidatecert ? [ 'ssl' => [ 'verify_peer_name' => false, 'verify_peer' => false, ], ] : []; }

Then, in the connect() method of each, you can do:

$this->socket = stream_socket_client( $host . ':' . $port, $errno, $errstr, self::TIMEOUT_CONNECTION, STREAM_CLIENT_CONNECT, stream_context_create($this->prepareSocketOptions()) );

(This also means the comments about camelCase names become irrelevant, as the variables are never created.)

I've moved then entire socket preparation, and splitted options preparation to another method

trasher · 2020-07-10T05:47:34Z

Hi @weierophinney thank you for the feedbacks :)

I will fix according to your review comments.
I do not know if this is possible to add tests on that; I guess some case should be added on the existing "Storage" ones; I can try that and we'll see the result.

Signed-off-by: Johan Cwiklinski <jcwiklinski@teclib.com>

trasher · 2020-07-15T12:00:11Z

Tests are failing: Laminas\Mail\Protocol\AbstractProtocol and Laminas\Mail\Protocol\ProtocolTrait define the same property ($socket); maybe the socket creation method should be moved in AbstractProtocol.

Signed-off-by: Johan Cwiklinski <jcwiklinski@teclib.com>

trasher · 2020-07-16T06:34:39Z

I've added a very basic test; but I do not know if it is possible to do more; and I do not really have time to investigate on that.

glensc · 2020-07-16T08:11:48Z

@trasher let's hope @weierophinney is happy with the changes. 3️⃣ pairs of 👀 reviewed the code now.

Signed-off-by: Johan Cwiklinski <jcwiklinski@teclib.com>

weierophinney

Looks good! I've made some minor changes locally (pushing momentarily) to cover some nitpicks I had but didn't want to bother you with.

🚢

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>

weierophinney · 2020-07-28T21:33:49Z

I rebased locally and made edits... but don't have rights to push back to the origin repo. Rest assured, though - your commits are in!

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>

trasher mentioned this pull request Jul 9, 2020

Mailgate fail after upgrade to 9.5 glpi-project/glpi#7619

Closed

cedric-anne approved these changes Jul 9, 2020

View reviewed changes

Comment thread docs/book/read.md Outdated

Comment thread src/Protocol/Imap.php Outdated

Add option novalidatecert to connect(); closes #63

6c721a5

Signed-off-by: Johan Cwiklinski <jcwiklinski@teclib.com>

weierophinney requested changes Jul 9, 2020

View reviewed changes

trasher added 3 commits July 10, 2020 14:18

Move common code to existing trait, add getter

911fbef

Signed-off-by: Johan Cwiklinski <jcwiklinski@teclib.com>

Factorize socket creation code, use camelCase for variables

cd9192b

Signed-off-by: Johan Cwiklinski <jcwiklinski@teclib.com>

Prepare options in distinct method

6b2d0c3

Signed-off-by: Johan Cwiklinski <jcwiklinski@teclib.com>

michalbundyra added the Enhancement label Jul 15, 2020

michalbundyra linked an issue Jul 15, 2020 that may be closed by this pull request

[SSL/TLS] Support for pass the stream context #63

Closed

trasher added 2 commits July 15, 2020 14:17

Remove socket variable duplicate declaration in trait

cda35c8

Signed-off-by: Johan Cwiklinski <jcwiklinski@teclib.com>

Fix CS

b18f997

Signed-off-by: Johan Cwiklinski <jcwiklinski@teclib.com>

michalbundyra added the Unit Test Needed label Jul 15, 2020

glensc reviewed Jul 15, 2020

View reviewed changes

Comment thread src/Protocol/ProtocolTrait.php Outdated

glensc reviewed Jul 15, 2020

View reviewed changes

Comment thread src/Protocol/ProtocolTrait.php

glensc reviewed Jul 15, 2020

View reviewed changes

Comment thread src/Storage/Imap.php

Unconsistent param usage between imap and pop, and cast it

e66d072

Signed-off-by: Johan Cwiklinski <jcwiklinski@teclib.com>

trasher mentioned this pull request Jul 16, 2020

Mail collector cannot use self signed certificates glpi-project/glpi#7693

Closed

trasher added 2 commits July 16, 2020 08:02

Change visibility

514fb7e

Signed-off-by: Johan Cwiklinski <jcwiklinski@teclib.com>

Add basic test

35a9b5c

Signed-off-by: Johan Cwiklinski <jcwiklinski@teclib.com>

cedric-anne reviewed Jul 16, 2020

View reviewed changes

Comment thread src/Protocol/ProtocolTrait.php Outdated

Comment thread src/Protocol/ProtocolTrait.php Outdated

Change method name, clean dead code

8047d02

Signed-off-by: Johan Cwiklinski <jcwiklinski@teclib.com>

cedric-anne approved these changes Jul 16, 2020

View reviewed changes

weierophinney approved these changes Jul 28, 2020

View reviewed changes

weierophinney added a commit that referenced this pull request Jul 28, 2020

docs: adds CHANGELOG entry for #94

28df66a

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>

weierophinney closed this Jul 28, 2020

weierophinney mentioned this pull request Jul 29, 2020

Fix E_NOTICE on undefined $socket property #99

Merged

artemii-karkusha pushed a commit to artemii-karkusha/laminas-mail that referenced this pull request May 24, 2023

docs: adds CHANGELOG entry for laminas#94

bad96e8

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>

Uh oh!

Conversation

trasher commented Jul 9, 2020

Description

Uh oh!

Uh oh!

Uh oh!

trasher commented Jul 9, 2020

Uh oh!

weierophinney left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

trasher commented Jul 10, 2020

Uh oh!

trasher commented Jul 15, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

trasher commented Jul 16, 2020

Uh oh!

glensc commented Jul 16, 2020

Uh oh!

Uh oh!

Uh oh!

weierophinney left a comment

Choose a reason for hiding this comment

Uh oh!

weierophinney commented Jul 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants