fix: fix sha256_password to work correctly over a TLS connection

[viladimiru]

After reading the MySQL documentation, I understood that the sha256_password plugin works partially correctly, but it did not account for the fact that manual encryption is not required when connecting via TLS.

documentation

Follow up for #2888

[codecov]

Codecov Report

❌ Patch coverage is 11.11111% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.78%. Comparing base (28d1562) to head (5654ffc).
⚠️ Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
lib/auth_plugins/sha256_password.js	11.11%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3809      +/-   ##
==========================================
- Coverage   89.83%   89.78%   -0.06%     
==========================================
  Files          86       86              
  Lines       13596    13605       +9     
  Branches     1606     1606              
==========================================
+ Hits        12214    12215       +1     
- Misses       1382     1390       +8     

Flag	Coverage Δ
compression-0	88.89% <11.11%> (-0.06%)	⬇️
compression-1	89.76% <11.11%> (-0.06%)	⬇️
static-parser-0	87.35% <11.11%> (-0.06%)	⬇️
static-parser-1	88.12% <11.11%> (-0.06%)	⬇️
tls-0	89.20% <11.11%> (-0.06%)	⬇️
tls-1	89.55% <11.11%> (-0.06%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[wellwelwel]

Thanks, @viladimiru! I didn't get to test it in practice, but just bringing the quote in question here (the same applies to MySQL 8 and 9):

For clients that use the sha256_password plugin, passwords are never exposed as cleartext when connecting to the server. How password transmission occurs depends on whether a secure connection or RSA encryption is used:

• If the connection is secure, an RSA key pair is unnecessary and is not used. This applies to connections encrypted using TLS. The password is sent as cleartext but cannot be snooped because the connection is secure.

• If the connection is not secure, and an RSA key pair is available, the connection remains unencrypted. This applies to connections not encrypted using TLS. RSA is used only for password exchange between client and server, to prevent password snooping. When the server receives the encrypted password, it decrypts it. A scramble is used in the encryption to prevent repeat attacks.

• If a secure connection is not used and RSA encryption is not available, the connection attempt fails because the password cannot be sent without being exposed as cleartext.

Also, regarding the missing tests, it is not actually related to this PR: this part of the logic was never covered.

[wellwelwel]

I believe that instead of relying on the configuration provided by the user (if (connection.config.ssl) ...), we can ensure it through the connection itself, for example:

const tls = require('tls');

// ...

if (
  connection.stream instanceof tls.TLSSocket &&
  connection.stream.encrypted === true
) {
// ...

• The tests that passed in the bellow screenshot are based on this approach 🙋🏻‍♂️

Note

Testing locally by forcing sha256_password and SSL, I was able to see both the error mentioned in #2888 and the solution working:

[viladimiru]

I believe that instead of relying on the configuration provided by the user (if (connection.config.ssl) ...), we can ensure it through the connection itself, for example:

const tls = require('tls');

// ...

if (
  connection.stream instanceof tls.TLSSocket &&
  connection.stream.encrypted === true
) {
// ...

Hi @wellwelwel! Thank you for the suggestion!

Unfortunately, I'm not very knowledgeable about this particular issue, so if you think it's better this way, I will adjust the condition.

[wellwelwel]

Unfortunately, I'm not very knowledgeable about this particular issue [...]

When using rejectUnauthorized, the connection.config.ssl will be truthy because it 'll be an object ({}), while the connection could be unencrypted. It is not an issue for MySQL2 or MySQL, but I consider it worth covering this possibility.

By checking the connection itself, we can confirm that it is actually secure before sending data as plain text.

[wellwelwel]

Thanks again, @viladimiru! It's already available in mysql2@canary and soon in the latest version too 🤝