#2815 don't require ssl_key and ssl_cert to use ssl_ca

[gauauu]

See #2815 -- you should be able to use ssl_ca for mysqli connections without specifying ssl_key and ssl_cert.

[Ocramius]

@gauauu any way to test this effectively?

Also, the CI tests disagree with you here...

[gauauu]

Ah, I missed that unit test. That test is testing for those "mandatory" parameters to be present, but they aren't mandatory. I can remove that test.

Really, all I'm suggesting is REMOVING functionality, so I'm not sure there's a good way to test that (other than me removing the code (which I did) and a unit test that tests that code (Which I'll do)).

[nvanheuverzwijn]

I support this PR. ssl_key and ssl_cert are not mandatory.

Only the ssl_ca parameter is needed to establish a secure connection.

[nvanheuverzwijn]

Is there anything missing here for the merge to happen ?

[lcobucci]

@nvanheuverzwijn time, unfortunately all maintainers are quite busy nowadays with paid work. I'll be working this weekend on trying to merge most of stuff.

[lcobucci]

Could you please modify this test to make it work in the new behaviour (with the proper assertions), instead of simply removing it? Without that we don't have anything testing that setSecureConnection() works correctly.

[nvanheuverzwijn]

@lcobucci

There are two path to this block of code. The first is the parameter validation

        if (! isset($params['ssl_key']) &&
            ! isset($params['ssl_cert']) &&
            ! isset($params['ssl_ca']) &&
            ! isset($params['ssl_capath']) &&
            ! isset($params['ssl_cipher'])
        ) {
            return;
        }

Which translate to "don't do anything to the connection if there are missing parameters". Because this modify the connection, I am not sure how this can be tested with the actual state of the code. We would need to test that ssl_set function is not called on _conn variable .

The other block has the same problem (make sure that ssl_set is called). To properly test the code, we either need a mysql server or rewrite this code so that the _conn is somehow mockable.

$this->_conn->ssl_set(
            $params['ssl_key']    ?? null,
            $params['ssl_cert']   ?? null,
            $params['ssl_ca']     ?? null,
            $params['ssl_capath'] ?? null,
            $params['ssl_cipher'] ?? null
        );

While I highly encourage unit-testing, there are some problem that are not easily testable like this one. It highly depends on a third party program (mysql) and the code is not written for easy unit-testing. Also, the setSecureConnection is a private function which renders testing even more harder. We might consider mocking mysqli_init but this seems hackish to me, I don't know.

The actual state of the code makes unit-testing very hard. A rewrite would be required to test everything this class does. Integrated test would be more suitable (test against a real mysql server).

[lcobucci]

@nvanheuverzwijn you're correct, thanks! Let's ship it, then 😄

[nvanheuverzwijn]

Thank you for your time ! Cheers !

[lcobucci]

I support this PR. ssl_key and ssl_cert are not mandatory.

Only the ssl_ca parameter is needed to establish a secure connection.

@gauauu @nvanheuverzwijn is this correct?

According to the new code all the parameters are NOT mandatory, so maybe we have to adjust something?

[nvanheuverzwijn]

@lcobucci I confirm you that only ssl_ca is needed to establish a secure connection to a remote mysql server.

The ssl_key and ssl_cert parameter are needed when the remote mysql server requires the client to provide a ssl certificate for authentication however, this is not mandatory when doing ssl with mysql.

[lcobucci]

@lcobucci I confirm you that only ssl_ca is needed to establish a secure connection to a remote mysql server.

The ssl_key and ssl_cert parameter are needed when the remote mysql server requires the client to provide a ssl certificate for authentication however, this is not mandatory when doing ssl with mysql.

@nvanheuverzwijn thanks, I've managed to test that with functional test but for some weird reason that test was not working fine in travis 😭. Anyway, let's get it merged (for real this time HAHAHA)

[lcobucci]

@gauauu @nvanheuverzwijn merged and backported, thanks for your contribution!