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

TrustServerCertificate no longer working in v5.11.1 #1478

Closed
esetnik opened this issue Sep 11, 2023 · 9 comments
Closed

TrustServerCertificate no longer working in v5.11.1 #1478

esetnik opened this issue Sep 11, 2023 · 9 comments

Comments

@esetnik
Copy link

esetnik commented Sep 11, 2023

Please check the FAQ (frequently-asked questions) first. If you have other questions or something to report, please address the following (skipping questions might delay our responses):

PHP version
php:8.2.8-fpm-bullseye@sha256:a90c4f5aef3191ad245f59c3b607a9d7e9bc10ce96bf3e1066a9fd536304a4bf

PHP SQLSRV or PDO_SQLSRV version
v5.11.1

Microsoft ODBC Driver version
8.3.1.1-1

SQL Server version
mcr.microsoft.com/mssql/server:2019-CU20-ubuntu-20.04@sha256:5e67a797c69eba6382b1edd34de711cc03d4347dabefcc5a14fbca71e8214315

Client operating system
docker for mac

Problem description
When using encryption with a self-signed certificate, e.g.

        'Encrypt' => 'Yes',
        'TrustServerCertificate' => 'Yes'

is no longer working as of v5.11.1. Reverting back to v5.11.0 allows self-signed certificates to be used again.

Expected behavior and actual behavior
I get a self-signed certificate error indicating that TrustServerCertificate is being ignored. Downgrading to v5.11.0 causes the self-signed certificate error to go away with an otherwise identical config.

Array ( [0] => Array ( [0] => 08001 [SQLSTATE] => 08001 [1] => -1 [code] => -1 [2] => [Microsoft][ODBC Driver 18 for SQL Server]SSL Provider: [error:1416F086:SSL routines:tls_process_server_certificate:certificate verify failed:self signed certificate] [message] => [Microsoft][ODBC Driver 18 for SQL Server]SSL Provider: [error:1416F086:SSL routines:tls_process_server_certificate:certificate verify failed:self signed certificate] ) [1] => Array ( [0] => 08001 [SQLSTATE] => 08001 [1] => -1 [code] => -1 [2] => [Microsoft][ODBC Driver 18 for SQL Server]Client unable to establish connection. For solutions related to encryption errors, see https://go.microsoft.com/fwlink/?linkid=2226722 [message] => [Microsoft][ODBC Driver 18 for SQL Server]Client unable to establish connection. For solutions related to encryption errors, see https://go.microsoft.com/fwlink/?linkid=2226722 ) )

Repro code or steps to reproduce

if (!isset($conn)) {
    $connectionInfo = [
        "UID" => $dbUser,
        "PWD" => $dbPass,
        "Database" => $dbName,
        "LoginTimeout" => 10,
        "CharacterSet" => 'UTF-8',
        "ConnectRetryCount" => 5,
        'Encrypt' => 'Yes',
        'TrustServerCertificate' => 'Yes'
    ];

    $conn = sqlsrv_connect("$dbHost, $dbPort", $connectionInfo);

    if ($conn === false) {
        $errors = sqlsrv_errors();
        http_response_code(503);
        die(print_r($errors, true));
    }
}
@absci
Copy link
Contributor

absci commented Sep 12, 2023

The new release accepts true or false for connection options. You could change it like this.

        'Encrypt' => 'True',
        'TrustServerCertificate' => 'True'

@esetnik
Copy link
Author

esetnik commented Sep 13, 2023

Was this change documented somewhere? Previously 'Yes' and 'No' were the only values that seemed to work.

@esetnik
Copy link
Author

esetnik commented Sep 13, 2023

The new release accepts true or false for connection options. You could change it like this.

        'Encrypt' => 'True',
        'TrustServerCertificate' => 'True'

I tried changing the values per your recommendation and I now get the following error:

Array ( [0] => Array ( [0] => 08001 [SQLSTATE] => 08001 [1] => 0 [code] => 0 [2] => [Microsoft][ODBC Driver 18 for SQL Server]Invalid value specified for connection string attribute 'Encrypt' [message] => [Microsoft][ODBC Driver 18 for SQL Server]Invalid value specified for connection string attribute 'Encrypt' ) )

It looks as though only the following works:

        'Encrypt' => 'Yes',
        'TrustServerCertificate' => 'True'

Is this the expected behavior? It seems really odd for a point release to introduce this change without any release notes.

@absci
Copy link
Contributor

absci commented Sep 13, 2023

This is not intended, looks like there's some inconsistency for the connection option. I'll look into a fix.

@esetnik
Copy link
Author

esetnik commented Sep 15, 2023

I believe I have found the issue. It looks like Encrypt is using srv_encrypt_set_func and TrustServerCertificate is using bool_conn_str_func. bool_conn_str_func does not handle 'Yes' and 'No'.

https://github.com/microsoft/msphpsql/blob/servicing_v5.11.1/source/sqlsrv/conn.cpp#L456-L462
https://github.com/microsoft/msphpsql/blob/servicing_v5.11.1/source/sqlsrv/conn.cpp#L563C1-L571C7

@JohanNyholm
Copy link

JohanNyholm commented Sep 19, 2023

I may be incorrect but it seems like #1460 changed bool_conn_str_func from defaulting to True to defaulting to False. May it be that in 5.11.0 setting bool-options to "yes" and "no" was interpreted as True and now in 5.11.1 they are not?

This should possibly be reported as a separate issue but MultiSubnetFailover also seems to be affected by this change.

As https://www.php.net/manual/en/function.sqlsrv-connect.php refers to connectionOptions using the following link http://msdn.microsoft.com/en-us/library/ff628167.aspx in which MultiSubnetFailover is documented as a "yes/no" option it is a bit confusing.

@absci
Copy link
Contributor

absci commented Sep 20, 2023

I'm thinking of accepting yes/no/true/false for all options to avoid confusion.

@JohanNyholm
Copy link

I'm thinking of accepting yes/no/true/false for all options to avoid confusion.

Sounds good!
Any idea if 1/0 is also used?
It may be that in 5.11.0 '1' was interpreted as true while the string '0' was interpreted as false. I have not investigated this further though.

@esetnik
Copy link
Author

esetnik commented Sep 20, 2023

I'm thinking of accepting yes/no/true/false for all options to avoid confusion.

I'm good with that as long as it also supports the capital case variants of 'Yes' and 'No' as well. It won't personally affect us, since we will modify our own code to use the true / false variants.

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

No branches or pull requests

4 participants