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

Improve TLS documentation for older servers #1657

Closed
wants to merge 1 commit into from

Conversation

evanelias
Copy link
Contributor

@evanelias evanelias commented Jan 3, 2025

Description

Older/EOL database server versions tend to be built with ancient OpenSSL or yaSSL, which lack support for modern cipher suites and/or lack TLS 1.2+. Meanwhile, recent Golang versions have updated the default client tls.Config in ways that are incompatible with these old server versions. This pull request improves TLS documentation to mention this incompatibility, provide sample code for solving it, and explain how "preferred" plaintext fallback mode is not triggered in cases of TLS version mismatch or cipher suite mismatch.

Closes #1635 by providing example code for solving the handshake failure.

Additional information which may be helpful for reviewers/maintainers

TLS version

  • Go 1.18+ changes the default client tls.Config.MinVersion to be TLS 1.2
  • MySQL 5.5 and 5.6 supports TLS 1.0
  • MySQL 5.7.0-5.7.27 supports TLS 1.1
  • MySQL 5.7.28+ supports TLS 1.2
  • MariaDB 10.1+ supports TLS 1.2
  • I did not examine MySQL 5.1 or MariaDB 10.0 or anything more ancient

Cipher suites

  • Go 1.22+ changes the default client tls.Config to remove cipher suites which use RSA key exchange
  • MySQL 8.0+ and MariaDB 10.2+ fully support ECDHE cipher suites and are compatible with Go's current default cipher suite list.
  • MySQL 5.x typically needs RSA key exchange cipher suites, due to https://bugs.mysql.com/bug.php?id=82935. Likewise for MariaDB 10.1.
  • There are some exceptions, for example Percona Server 5.7 is built with a newer OpenSSL, https://docs.percona.com/percona-server/5.7/security/ssl-improvement.html. It is also possible to custom compile MySQL 5.7 with a newer OpenSSL version to solve the cipher suite issue, but this is not common.

Checklist

  • Code compiles correctly
  • Created tests which fail without the change (if possible) (does not apply, change only affects docs/comments)
  • All tests passing
  • Extended the README / documentation, if necessary
  • Added myself / the copyright holder to the AUTHORS file (already present from a previous pull request)

Summary by CodeRabbit

  • Documentation
    • Updated README with detailed explanations of allowFallbackToPlaintext and tls DSN configuration parameters
    • Clarified TLS connection settings and fallback behaviors
    • Added guidance for connecting to older server versions with specific TLS configurations

Older/EOL database server versions tend to be built with ancient OpenSSL or
yaSSL, which lack support for modern cipher suites and/or lack TLS 1.2+. At
the same time, recent Golang versions have updated the default client
tls.Config in ways that are incompatible with these old server versions. This
commit improves TLS documentation to mention this incompatibility, provide
sample code for solving it, and explain how "preferred" plaintext fallback
mode is not triggered in cases of TLS incompatibilities.

Closes #1635 by providing example code for solving the handshake failure.

Additional information which may be helpful for reviewers/maintainers:

TLS version
* Go 1.18+ changes the default client TLS MinVersion to be TLS 1.2
* MySQL 5.5 and 5.6 supports TLS 1.0
* MySQL 5.7.0-5.7.27 supports TLS 1.1
* MySQL 5.7.28+ supports TLS 1.2
* MariaDB 10.1+ supports TLS 1.2
* I did not examine MySQL 5.1 or MariaDB 10.0 or anything more ancient

Cipher suites
* Go 1.22+ changes the default client TLS config to remove cipher suites which
  use RSA key exchange
* MySQL 8.0+ and MariaDB 10.2+ fully support ECDHE cipher suites and are
  compatible with Go's current default cipher suite list.
* MySQL 5.x typically needs RSA key exchange cipher suites, due to
  https://bugs.mysql.com/bug.php?id=82935. Likewise for MariaDB 10.1.
* There are some exceptions, for example Percona Server 5.7 is built with a
  newer OpenSSL, https://docs.percona.com/percona-server/5.7/security/ssl-improvement.html
* It is also possible to custom compile MySQL 5.7 with a newer OpenSSL
  version to solve the cipher suite issue, but this is not common.
Copy link

coderabbitai bot commented Jan 3, 2025

Walkthrough

The pull request focuses on improving documentation and configuration support for TLS connections in the MySQL driver. The changes primarily update the README.md to provide more detailed explanations about TLS connection parameters, specifically allowFallbackToPlaintext and tls. In utils.go, a commented section was added to highlight potential cipher suite configurations for compatibility with older MySQL servers, particularly version 5.7.

Changes

File Change Summary
README.md - Enhanced documentation for allowFallbackToPlaintext parameter
- Updated description of tls parameter with more detailed configuration guidance
utils.go - Added commented section about TLS cipher suites for older server compatibility

Sequence Diagram

sequenceDiagram
    participant Client
    participant MySQL Server
    Client->>MySQL Server: Attempt TLS Connection
    alt TLS Configuration Compatible
        MySQL Server-->>Client: Successful Handshake
    else TLS Configuration Incompatible
        MySQL Server-->>Client: Handshake Failure
    end
Loading

Assessment against linked issues

Objective Addressed Explanation
Resolve TLS handshake failure with MySQL 5.7 [#1635] Commented cipher suite configuration suggests potential compatibility improvements, but no direct code changes implemented
Support SSL client certificates for older MySQL versions Documentation updates hint at configuration needs, but no explicit code modifications

Poem

🐰 A rabbit's tale of TLS delight,
Connecting servers with cipher might,
README speaks of fallback's grace,
Older servers find their embrace,
Encryption's dance, a gentle flight! 🔒


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
README.md (2)

184-188: Add security warning about plaintext connections

The documentation clearly explains the behavior of allowFallbackToPlaintext, but it should include a security warning about the risks of allowing plaintext connections, especially in production environments.

Add a warning note like this:

 `allowFallbackToPlaintext=true` permits fallback to an unencrypted connection if the server is not configured to use TLS. This behavior is enabled automatically when using `tls=preferred`, but it is also available as a separate parameter to allow plaintext fallback when using a custom TLS configuration.

+> [!WARNING]
+> Allowing plaintext fallback can expose sensitive data. Use this option with caution, especially in production environments where security is critical.
+
 Plaintext fallback only occurs if the server is not configured to use TLS at all. MySQL 5.7+ and MariaDB 11.4+ ship with TLS support pre-enabled by default using a self-signed server cert, so plaintext fallback typically won't occur with these server versions.
🧰 Tools
🪛 LanguageTool

[style] ~188-~188: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ... allows RSA key exchange cipher suites. Very old server versions also require reducing t...

(EN_WEAK_ADJECTIVE)


438-440: Add example TLS configuration for older servers

While the documentation explains the need for custom TLS configuration with older servers, providing a concrete example would make it more actionable for users.

Add an example configuration like this:

 In recent Golang versions, establishing an encrypted connection to older server versions (MySQL 5.x or MariaDB 10.1) often requires a custom TLS configuration, in order to allow RSA key exchange cipher suites and a lower minimum TLS version. See documentation for [`mysql.RegisterTLSConfig`](https://godoc.org/github.com/go-sql-driver/mysql#RegisterTLSConfig) for example code.

+Example configuration for older servers:
+```go
+cfg := &tls.Config{
+    MinVersion: tls.VersionTLS10,
+    CipherSuites: []uint16{
+        tls.TLS_RSA_WITH_AES_128_CBC_SHA,
+        tls.TLS_RSA_WITH_AES_256_CBC_SHA,
+        // Add other cipher suites as needed
+    },
+}
+mysql.RegisterTLSConfig("oldserver", cfg)
+```
🧰 Tools
🪛 LanguageTool

[uncategorized] ~438-~438: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... false ``` tls=true enables TLS / SSL encrypted connection to the server. Use `skip-ver...

(EN_COMPOUND_ADJECTIVE_INTERNAL)


[style] ~440-~440: Consider a shorter alternative to avoid wordiness.
Context: ...en requires a custom TLS configuration, in order to allow RSA key exchange cipher suites an...

(IN_ORDER_TO_PREMIUM)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7403860 and 0c6cf40.

📒 Files selected for processing (2)
  • README.md (2 hunks)
  • utils.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • utils.go
🧰 Additional context used
🪛 LanguageTool
README.md

[style] ~188-~188: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ... allows RSA key exchange cipher suites. Very old server versions also require reducing t...

(EN_WEAK_ADJECTIVE)


[uncategorized] ~438-~438: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... false ``` tls=true enables TLS / SSL encrypted connection to the server. Use `skip-ver...

(EN_COMPOUND_ADJECTIVE_INTERNAL)


[style] ~440-~440: Consider a shorter alternative to avoid wordiness.
Context: ...en requires a custom TLS configuration, in order to allow RSA key exchange cipher suites an...

(IN_ORDER_TO_PREMIUM)

@coveralls
Copy link

Coverage Status

coverage: 83.141%. remained the same
when pulling 0c6cf40 on skeema:tls-mysql5-docs
into 7403860 on go-sql-driver:master.

@methane
Copy link
Member

methane commented Jan 5, 2025

I don't like this because it decrease S/N ratio for regular users who don't use EOL versions.

Please add new section for connecting old MySQL servers.

@evanelias
Copy link
Contributor Author

Is that regarding the README changes, the utils.go doc comment change, or both?

@methane
Copy link
Member

methane commented Jan 5, 2025

both. Do not pollute RegisterTLSConfig at all. Add only README section.

@evanelias
Copy link
Contributor Author

OK, I'll close this for now, apologies for the distraction. If I find some time to rewrite it as a separate README section in the future, I'll open a new PR then.

@evanelias evanelias closed this Jan 5, 2025
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.

Error tls: handshake failure on try to connect to mysql 5.7 with SSL client certs
3 participants