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

🐛 N°8115 - Unattended Install: unable to connect to MySQL with TLS #694

Merged
merged 8 commits into from
Jan 31, 2025

Conversation

tomrss
Copy link
Contributor

@tomrss tomrss commented Dec 19, 2024

Base information

Question Answer
Related to a SourceForge thead / Another PR / Combodo ticket? No
Type of change? Bug fix

Symptom (bug) / Objective (enhancement)

Unattended install process does not support TLS connections to MySQL.

Reproduction procedure (bug)

  1. On iTop 3.2.0-2 (not related to version)
  2. With PHP 8.3.12 (not related to version)
  3. MySQL instance configured with require_secure_transport=ON
  4. Setup MySQL connections with TLS (config db_tls.enabled and db_tls.ca)
  5. Then launch the setup/unattended-install/unattended-install.php script
  6. Finally, see that the TLS connection to MySQL cannot be established:
Fatal error: Uncaught mysqli_sql_exception: Connections using insecure transport are prohibited while --require_secure_transport=ON. in /var/www/html/setup/unattended-install/unattended-install.php:313

Cause (bug)

TLS connection not implemented in the unattended install script:

$oMysqli = new mysqli($sDBServer, $sDBUser, $sDBPwd);
if ($oMysqli->connect_errno)

and

$oMysqli = new mysqli($sDBServer, $sDBUser, $sDBPwd);
if (!$oMysqli->connect_errno)

Proposed solution (bug and enhancement)

Add support for TLS connection based on this code in the docs and existing MySQL connection code in the app:

$oMysqli = new mysqli();
if ($bTlsEnabled)
{
$iFlags = (empty($sTlsCa))
? MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT
: MYSQLI_CLIENT_SSL;
$sTlsCert = null; // not implemented
$sTlsCaPath = null; // not implemented
$sTlsCipher = null; // not implemented
$oMysqli->ssl_set($bTlsEnabled, $sTlsCert, $sTlsCa, $sTlsCaPath, $sTlsCipher);
}
$oMysqli->real_connect($sServer, $sUser, $sPwd, '', $iPort, ini_get("mysqli.default_socket"), $iFlags);

Disclaimer: PHP is not my language, sorry if something is broken or strange! Just let me know and I'll fix it.

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have tested all changes I made on an iTop instance
  • I have added a unit test, otherwise I have explained why I couldn't
  • Is the PR clear and detailed enough so anyone can understand digging in the code?

Copy link
Contributor

@Hipska Hipska left a comment

Choose a reason for hiding this comment

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

Looks good, may as well make use of the return value of the connect method..

setup/unattended-install/unattended-install.php Outdated Show resolved Hide resolved
setup/unattended-install/unattended-install.php Outdated Show resolved Hide resolved
@jf-cbd jf-cbd changed the base branch from develop to support/3.2 January 15, 2025 08:19
@jf-cbd jf-cbd changed the base branch from support/3.2 to develop January 15, 2025 08:24
@jf-cbd
Copy link
Contributor

jf-cbd commented Jan 16, 2025

Accepted in functional review, it should be added in iTop 3.2.1.

@jf-cbd jf-cbd changed the title 🐛 Unattended Install: unable to connect to MySQL with TLS 🐛 N°8115 - Unattended Install: unable to connect to MySQL with TLS Jan 16, 2025
@rquetiez rquetiez self-requested a review January 22, 2025 09:53
Copy link
Contributor

@rquetiez rquetiez left a comment

Choose a reason for hiding this comment

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

Though the fix is relevant, it would be even more relevant to invoke CMDBSource::GetMysqliInstance to do the job:

  1. This refactor is feasible because GetMysqliInstance is purely static. Note that it throws Exceptions in case of failure.
  2. There is already another difference between the proposed implementation and the implementation of GetMysqliInstance that is relevant to all use cases. See the call to real_connect on line 186 that takes into account the PHP setting mysqli.default_socket
  3. We will automatically benefit from future improvements of GetMysqliInstance

@tomrss
Copy link
Contributor Author

tomrss commented Jan 22, 2025

Thanks for the feedback @rquetiez , i copied the snippet exactly from that class, but I didn't know if coupling with core part was desirable.

I'll implement the requested changes within a couple of days.

@tomrss tomrss requested a review from rquetiez January 24, 2025 15:07
@jf-cbd jf-cbd merged commit ab92957 into Combodo:develop Jan 31, 2025
@jf-cbd
Copy link
Contributor

jf-cbd commented Jan 31, 2025

Thank you for this contribution @tomrss 😊

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.

5 participants