Authentication #1696
Authentication #1696
Conversation
WalkthroughThis set of changes introduces a modular authentication plugin system to the Go MySQL driver. The authentication logic is refactored to use a generic plugin interface, allowing multiple authentication methods to be implemented as separate plugins. Each plugin encapsulates its own protocol, including native password, caching SHA2, cleartext, old password, SHA256, and Ed25519 authentication. The driver now maintains a global plugin registry, and authentication flows are managed through this registry and the plugin interface. Extensive cryptographic and protocol-specific logic is moved from the core authentication code into the respective plugins. Tests are updated to use the new plugin-based interface. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Connector
participant mysqlConn
participant PluginRegistry
participant AuthPlugin
participant Server
App->>Connector: Connect()
Connector->>mysqlConn: initiate connection
mysqlConn->>PluginRegistry: GetPlugin(plugin_name)
PluginRegistry-->>mysqlConn: AuthPlugin instance
mysqlConn->>AuthPlugin: InitAuth(authData, config)
AuthPlugin-->>mysqlConn: auth response
mysqlConn->>Server: send auth response
loop (up to max switches)
Server-->>mysqlConn: auth result / switch request
alt OK or ERR
mysqlConn-->>Connector: authentication success/failure
else Auth Switch
mysqlConn->>PluginRegistry: GetPlugin(new_plugin_name)
PluginRegistry-->>mysqlConn: AuthPlugin instance
mysqlConn->>AuthPlugin: continuationAuth(packet, authData, conn)
AuthPlugin-->>mysqlConn: next auth response
mysqlConn->>Server: send next auth response
end
end
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
driver.go (1)
42-44: Global plugin registry introduction
This new variable and initialization appear well-structured for the upcoming plugin-based authentication system. Ensure theglobalPluginRegistryregistration and usage is thread-safe, especially if plugin registrations can occur at runtime.README.md (1)
540-546: Use consistent list style per markdownlint
Static analysis flags these lines for inconsistent unordered list style. Switch from dashes to asterisks to comply with the configured Markdown style guidelines.Apply the following diff to update the list style:
- - `mysql_native_password` - The default MySQL authentication method - - `caching_sha2_password` - Default authentication method in MySQL 8.0+ - - `mysql_clear_password` - Cleartext authentication (requires `allowCleartextPasswords=true`) - - `mysql_old_password` - Old MySQL authentication (requires `allowOldPasswords=true`) - - `sha256_password` - SHA256 authentication - - `client_ed25519` - MariaDB Ed25519 authentication + * `mysql_native_password` - The default MySQL authentication method + * `caching_sha2_password` - Default authentication method in MySQL 8.0+ + * `mysql_clear_password` - Cleartext authentication (requires `allowCleartextPasswords=true`) + * `mysql_old_password` - Old MySQL authentication (requires `allowOldPasswords=true`) + * `sha256_password` - SHA256 authentication + * `client_ed25519` - MariaDB Ed25519 authentication🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
541-541: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
542-542: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
543-543: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
544-544: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
545-545: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
546-546: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
auth_ed25519.go (1)
29-63: Ed25519 authentication logic appears correct.
The scalar clamping, base multiplication, uniform hashing, and final combined signature steps reflect standard Ed25519 signing internals. Error handling for scalar creation is handled properly. One optional consideration is whether to sanitize (zero) sensitive intermediate data in memory once done, if there's concern about side-channel leaks in ephemeral memory. Overall, the plugin method is well-structured.auth_caching_sha2.go (1)
67-74: Use descriptive constants for magic numbers.
The sub-switch values3and4can be replaced by clearly named constants to enhance maintainability and code clarity.auth_sha256.go (1)
107-131: RSA-OAEP with SHA1.
While this aligns with MySQL's required method, note that SHA1 is considered weaker cryptographically. Keep it for compatibility, or consider offering a stronger alternative if not constrained by server support.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
AUTHORS(1 hunks)README.md(1 hunks)auth.go(2 hunks)auth_caching_sha2.go(1 hunks)auth_cleartext.go(1 hunks)auth_ed25519.go(1 hunks)auth_mysql_native.go(1 hunks)auth_old_password.go(1 hunks)auth_plugin.go(1 hunks)auth_sha256.go(1 hunks)auth_test.go(53 hunks)connector.go(2 hunks)const.go(0 hunks)driver.go(1 hunks)driver_test.go(4 hunks)packets.go(0 hunks)
💤 Files with no reviewable changes (2)
- const.go
- packets.go
🧰 Additional context used
🧬 Code Graph Analysis (9)
driver.go (1)
auth_plugin.go (1)
NewPluginRegistry(42-47)
auth_cleartext.go (3)
auth_plugin.go (2)
SimpleAuth(28-30)RegisterAuthPlugin(61-63)dsn.go (1)
Config(37-84)errors.go (1)
ErrCleartextPassword(23-23)
auth_plugin.go (1)
dsn.go (1)
Config(37-84)
auth_old_password.go (3)
auth_plugin.go (2)
SimpleAuth(28-30)RegisterAuthPlugin(61-63)dsn.go (1)
Config(37-84)errors.go (1)
ErrOldPassword(25-25)
auth_mysql_native.go (3)
auth_plugin.go (2)
SimpleAuth(28-30)RegisterAuthPlugin(61-63)dsn.go (1)
Config(37-84)errors.go (1)
ErrNativePassword(24-24)
auth_sha256.go (3)
auth_plugin.go (2)
AuthPlugin(12-26)RegisterAuthPlugin(61-63)dsn.go (1)
Config(37-84)errors.go (1)
ErrMalformPkt(21-21)
auth_test.go (2)
auth_old_password.go (1)
OldPasswordPlugin(12-12)auth_mysql_native.go (1)
NativePasswordPlugin(14-16)
auth_caching_sha2.go (3)
auth_plugin.go (2)
AuthPlugin(12-26)RegisterAuthPlugin(61-63)dsn.go (1)
Config(37-84)errors.go (1)
ErrMalformPkt(21-21)
auth.go (2)
auth_plugin.go (1)
AuthPlugin(12-26)errors.go (1)
ErrMalformPkt(21-21)
🪛 markdownlint-cli2 (0.17.2)
README.md
541-541: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
542-542: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
543-543: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
544-544: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
545-545: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
546-546: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
🔇 Additional comments (96)
AUTHORS (1)
40-40: No concerns found with this addition
Adding the new author entry in alphabetical order is correct and maintains consistency with the file’s structure.README.md (1)
537-539: Good addition of the new authentication section
Introducing a dedicated subsection clarifies how the new pluggable authentication system works. The explanation gives a clear overview for end users.connector.go (4)
147-150: Appropriate plugin existence check
Returning an error if the plugin is missing is a good defensive approach. Make sure any dynamic plugin loading is handled before this point to avoid unexpected failures for known custom plugins.
153-156: Plugin-driven auth initialization
DelegatingInitAuthto the plugin fosters modularity. The cleanup after error is correct.
164-165: Bounded authentication flow
PassingauthMaximumSwitchand reusing the same plugin instance ensures a controlled multi-step authentication without fallback confusion.
21-23:✅ Verification successful
Reasonable limit on plugin switch attempts
Using a constant for the maximum number of authentication plugin switches keeps the logic maintainable. Confirm that the limit of 5 attempts aligns with all supported multi-factor authentication scenarios.
🏁 Script executed:
#!/bin/bash # Description: Check references to authMaximumSwitch across the codebase to ensure it is not exceeded or bypassed during multi-factor auth flows. rg -A 5 "authMaximumSwitch"Length of output: 876
Verification Confirmed: Consistent and Intentional Limit
The constant
authMaximumSwitchis set to 5 inconnector.goand is referenced both in the authentication flow (mc.handleAuthResult) and in generating the error message ("maximum of %d authentication switch reached") inauth.go. The grep output confirms that the limit is applied consistently across the multi-factor authentication logic.
- The constant is declared and used in a way that prevents exceeding the intended number of switch attempts.
- There’s no evidence from the codebase that the limit is bypassed or that additional scenarios require a higher count.
If later multi-factor authentication scenarios demand a different limit, please update the constant accordingly.
driver_test.go (3)
1633-1658: Improved collation test to handle server overrides - good approach.The added code correctly handles cases where the server overrides the default collation based on
character_set_collations. This makes the test more robust against different server configurations by:
- Fetching the server's character set collations mapping
- Parsing it into a usable dictionary structure
- Checking if the expected collation's charset has a server-mandated override
- Using the overridden value for comparison when necessary
1721-1721: Updated timezone test locations for broader coverage.Changed test timezones from "US/Central", "US/Pacific" to "America/New_York", "Asia/Hong_Kong" which provides more global timezone coverage and uses IANA standard timezone names.
3577-3585: Good addition of performance_schema availability check.This change adds an important check to verify the performance_schema is enabled before attempting to query connection attributes. This prevents test failures when running against MySQL servers with performance_schema disabled.
auth_cleartext.go (4)
11-23: Good implementation of the cleartext password plugin.The ClearPasswordPlugin is well-structured and properly embeds SimpleAuth. The comments clearly document the security implications of cleartext authentication and when it should be used (TLS/SSL, Unix sockets, PAM).
25-27: Appropriate plugin registration in init.The plugin correctly registers itself with the global plugin registry during package initialization, allowing the driver to discover it without manual registration.
29-31: Plugin identification method.The GetPluginName method correctly returns "mysql_clear_password" which matches MySQL's internal plugin name.
33-46: Well-implemented authentication initialization.The InitAuth method has good security practices:
- It checks if cleartext passwords are allowed in the configuration
- Returns an appropriate error if not allowed
- Correctly appends a null terminator to the password as required by the protocol
The null termination handling is particularly important for external authentication systems that need access to the original password.
auth_mysql_native.go (5)
13-16: Proper implementation of native password plugin.The plugin structure is clean and correctly embeds SimpleAuth for shared functionality.
18-20: Self-registration pattern is consistent.The plugin registers itself in init, matching the pattern used in other authentication plugins.
22-24: Plugin name matches MySQL's internal name.The GetPluginName method correctly returns "mysql_native_password" in accordance with MySQL's naming.
26-34: Security check in initialization method.InitAuth properly checks configuration settings before proceeding:
- Verifies AllowNativePasswords configuration flag
- Handles empty passwords correctly
- Calls scramblePassword with the correct portion of authData (first 20 bytes)
36-64: Accurate implementation of MySQL 4.1+ password hashing.The scramblePassword function correctly implements the MySQL 4.1+ native password hashing algorithm:
- Properly handles empty passwords
- Calculates the SHA1 hash of the password (stage1)
- Creates a SHA1 hash of the stage1 hash
- Creates a SHA1 hash of the scramble + stage1 hash
- XORs the scramble hash with stage1 to produce the authentication token
The implementation follows the MySQL protocol specification precisely.
auth_plugin.go (4)
11-26: Well-designed plugin interface for authentication.The AuthPlugin interface is cleanly defined with three essential methods:
- GetPluginName - Identifies the plugin
- InitAuth - Handles initial authentication
- ContinuationAuth - Manages subsequent authentication steps
The interface provides enough flexibility for different authentication mechanisms while maintaining a consistent API. The documentation for each method clearly explains parameters and purpose.
28-34: Good base implementation with SimpleAuth.The SimpleAuth struct provides a sensible default implementation for ContinuationAuth, making it easier to implement simple plugins by embedding this struct. This avoids code duplication for plugins that don't need multi-step authentication.
36-58: Clean implementation of plugin registry.The PluginRegistry implementation is straightforward and effective:
- Uses a map to store plugins by name
- Provides methods to register and retrieve plugins
- Has a proper constructor (NewPluginRegistry)
- Returns a boolean status with GetPlugin to safely handle missing plugins
This design allows for efficient plugin lookup while avoiding panics for unavailable plugins.
60-63: Global registry access with RegisterAuthPlugin.This helper function simplifies plugin registration by accessing the global registry, following a common pattern in Go libraries. This makes it easy for both built-in and external plugins to register themselves.
auth_ed25519.go (4)
1-8: License and package header look good.
No issues found in the licensing block or package declaration.
16-19: Struct definition is straightforward.
EmbeddingSimpleAuthaligns with the plugin-based design. No immediate concerns.
21-23: Plugin registration is correct.
RegisterAuthPlugininvocation follows the new authentication system’s pattern.
25-27: Plugin name retrieval is fine.
The function name and return value match plugin naming conventions.auth_test.go (41)
53-55: Initiating OldPasswordPlugin in test.
Constructing the plugin directly is acceptable for targeted testing. No issues.
122-122: Invocation of handleAuthResult.
Looks consistent with new multi-step auth approach.
137-138: Retrieving plugin from registry and calling InitAuth.
This is correct and aligns with the plugin-based design.
179-186: Setting up plugin and verifying handshake response flow.
Combining registry lookup withInitAuthis correct. The subsequentwriteHandshakeResponsePacketcall properly handles the returned bytes.
236-237: Caching SHA2 plugin retrieval and initialization.
Implementation follows the same pattern as other plugins. No issues.
289-290: Secure plugin initialization.
Reading from registry and callingInitAuthis consistent.
346-347: Cleartext password plugin retrieval and InitAuth check.
This test ensures errors are returned when not allowed. Logic is fine.
364-365: Acquiring plugin instance and retrieving auth response.
Matches the plugin design. Good coverage for empty password.
391-391: handleAuthResult usage.
Consistent approach to finalizing authentication.
407-408: Same pattern for plugin acquisition and initialization.
Design is uniform—straightforward test validation.
511-511: InitAuth with native password plugin in test.
Implementation is correct, verifying empty password logic.
553-553: sha256_password plugin test.
Empty password scenario is covered. Appropriately tested.
602-602: SHA256 password plugin retrieval & initialization.
No issues—standard plugin usage.
631-631: handleAuthResult for RSA scenario.
Ensures RSA logic is properly tested.
652-652: SHA256 plugin with custom pubKey.
Test coverage is thorough, ensuring plugin handles configured RSA key.
685-685: sha256_password plugin with secure connection.
Test ensures fallback to sending decrypted password over TLS. Implementation is correct.
714-714: handleAuthResult final check.
Standard multi-step auth logic, no issues.
741-741: Auth switch with &NativePasswordPlugin{}.
Matches plugin-based approach. No concerns.
773-773: handleAuthResult with empty password.
Trivial test scenario is validated.
807-807: Multi-step RSA-based handshake switch.
Test ensures partial hush vs. final hush approach. No issues.
849-849: Authorization switch with provided pubKey.
Flow is consistent with the plugin’s logic.
889-889: handleAuthResult for secure SHA256.
Ensures it sends cleartext password if TLS is verified. Good coverage.
913-913: Invalid plugin switch for cleartext password.
Expects error. Aligned with new plugin design.
935-935: handleAuthResult with explicit plugin object.
Demonstrates multi-auth switch for cleartext password.
959-959: Empty password old auth test.
Ensures correct fallback or error.
979-979: Disallow old password scenario.
Tests the config-based restriction. Good coverage.
1001-1001: Handling native password switch with non-empty password.
Ensures the scramble is created properly.
1029-1029: Empty password with native password switch.
Covered scenario; no issues.
1047-1047: handleAuthResult for old password not allowed.
Correctly returnsErrOldPassword. Test logic is fine.
1062-1062: handleAuthResult for old password with OldAuthSwitch request.
Same approach, verifying it fails.
1084-1084: Allowed old passwords.
Ensures scramble logic is tested.
1109-1109: Handling OldAuthSwitch with allowed old passwords.
Test confirms fallback is correct.
1132-1132: Handling old password with empty credentials.
Test ensures short-circuit for empty password.
1155-1155: Confirm empty old password switch.
No issues. Good coverage for an edge case.
1180-1180: Auth switch for sha256 with empty plugin data.
Verifies no crash or unexpected errors.
1211-1211: RSA-based pub key response.
Ensures correctness of encryption step.
1243-1243: RSA-based key scenario with custom pubKey.
Ensures no break with user-provided key.
1275-1275: Secure connection usage.
Checks fallback to sending password in cleartext over TLS.
1315-1319: Verifying Ed25519 client output.
Checks 64-byte response correctness, ensuring the derived signature matches expectations. Logic is correct.
1335-1370: TestMultiAuthSimpleSwitch.
Demonstrates multi-step switch fromcaching_sha2_passwordtomysql_clear_password. Asserts the correct handshake sequence is written.
1372-1413: TestMultiAuthSwitch.
Similar multi-step example, now adding RSA pub key steps before switching to cleartext. Thorough test coverage of multi-factor flow.auth_old_password.go (6)
1-8: License and package declaration.
No concerns with the added header.
11-16: Plugin struct registration.
EmbeddingSimpleAuthand registering undermysql_old_passwordis consistent with the new system.
18-20: Plugin name retrieval.
Matches the typical plugin naming pattern. No issues.
22-33: InitAuth method for old password.
Properly checksAllowOldPasswordsconfig flag. ThescrambleOldPasswordcall is restricted to the first 8 bytes of the server scramble, matching MySQL’s old approach. This is correct for pre-4.1 style.
35-55: scrambleOldPassword implementation.
The code matches the historical MySQL algorithm, including skipping tabs/spaces and performing an insecure hash-based scramble. The approach is intentionally insecure but historically compatible. No immediate issues beyond inherent weakness of the scheme.
57-109: myRnd and pwHash logic.
Implements the legacy MySQL random generator and two-component hashing precisely. This is correct for old-style password scrambling.auth_caching_sha2.go (12)
19-24: Clean separation of the plugin struct.
EmbeddingAuthPlugininCachingSha2PasswordPluginaligns well with the new plugin-based approach, making the design modular and maintainable.
26-28: Automatic plugin registration.
Registering the plugin in theinit()function ensures it is discovered without additional user code, improving developer experience.
30-32: Correct and consistent plugin name.
Returning"caching_sha2_password"matches MySQL’s official plugin name and fulfills theAuthPlugininterface requirement.
34-42: Clear initial scramble implementation.
Delegating the scrambling logic toscrambleSHA256Passwordkeeps the implementation focused and maintainable.
55-57: Empty packet handling.
It's good to error on an empty authentication response packet. Confirm there's no valid scenario in which the server sends an empty packet mid-auth.
59-62: iERR case verification.
Returning(packet, nil)foriERRrelies on higher-level logic to recognize an error packet. Double-check that the caller properly treats it as an error and does not inadvertently proceed.
63-66: Fast auth success scenario.
This path indicates an immediate indication of success and proceeds tomc.readPacket(). Ensure this matches MySQL's spec for cached credentials with zero extended data.
75-127: Validate cleartext sending logic.
When TLS or a Unix socket is used, the password is sent in cleartext. Verify if you need to respectAllowCleartextPasswordsor similar flags to avoid unexpected exposures.
129-131: Clear error for unknown state.
Throwing an error on unrecognized state ensures robust error handling and quicker debugging.
133-135: Unexpected packet length fallback.
Raising an error for mismatched lengths prevents subtle protocol mishandling.
136-139: Failsafe for invalid packet types.
Rejecting unknown packet types maintains stability, ensuring the driver doesn’t silently ignore server anomalies.
141-176: MySQL 8+ scramble algorithm.
The three-step SHA256-based hashing followed by XOR is consistent with caching_sha2_password specification.auth_sha256.go (5)
20-22: Struct embedding for sha256_password plugin.
Adhering to the same plugin pattern keeps authentication logic uniform across plugins.
24-26: Init-based plugin registration.
Similar to the other plugins, this ensures the plugin is discovered and available at runtime.
28-30: Accurate plugin naming.
"sha256_password" properly identifies the authentication method for MySQL and matches server expectations.
32-62: Comprehensive InitAuth logic.
Handles empty passwords, TLS-based cleartext, RSA encryption fallback, and public key requests. Confirm any relevantAllowCleartextPasswordsor usage constraints are enforced as needed.
64-105: ContinuationAuth multi-step support.
The switch statement properly handles OK, error, or public-key retrieval. Double-check iERR handling to ensure the caller interprets the error scenario correctly.auth.go (6)
12-12: Introducing thebytesimport.
Used to parse and locate the plugin name from null-terminated data, which is appropriate within packet parsing logic.
59-65: Guard against infinite re-auth loops.
LimitingremainingSwitchavoids potential recursion issues in multi-factor or repeated authentication switch scenarios.
67-79: Delegation toContinuationAuth.
Continuing the authentication flow via the plugin maintains modular design. Ensure the returned packet is consistently valid for follow-up logic.
80-85: Direct handling of OK/ERR.
Returning the result or error packet as-is is concise. Validate that a server error viaiERRis recognized upstream.
86-104: Auth switch recursion with new plugin.
Efficiently fetches next plugin from the registry and re-invokes auth logic, decrementingremainingSwitch. This structure elegantly supports multi-step authentication.
110-132: Plugin name extraction.
Automatically falls back tomysql_old_passwordif only a single byte remains, preserving legacy compatibility. Good approach for older server versions.
|
|
driver.go
Outdated
| dials map[string]DialContextFunc | ||
| dialsLock sync.RWMutex | ||
| dials map[string]DialContextFunc | ||
| globalPluginRegistry = NewPluginRegistry() |
There was a problem hiding this comment.
| globalPluginRegistry = NewPluginRegistry() | |
| pluginRegistry = NewPluginRegistry() |
There was a problem hiding this comment.
cannot do since that's the name of the struct
|
Maybe, its time to use internal package. |
Current implementation was requiring authentication plugin that are have multiple step to begin with iAuthMoreData, but that's only required for "caching_sha2_password" and "sha256_password" plugins. Additionally, now permits multi-authentication (in mariadb: https://mariadb.com/kb/en/create-user/#identified-viawith-authentication_plugin / mysql: https://dev.mysql.com/doc/refman/8.0/en/multifactor-authentication.html) goal is to add MariaDB main authentication plugins (like parsec, PAM, GSSAPI, ...)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
README.md (1)
537-547: Documentation looks great, consider using consistent list styleThe new Authentication Plugin System section provides clear documentation of the supported authentication methods. However, the unordered list uses dash (-) markers instead of asterisks (*) which is inconsistent with the rest of the document.
-The driver implements a pluggable authentication system that supports various authentication methods used by MySQL and MariaDB servers. The built-in authentication plugins include: - -- `mysql_native_password` - The default MySQL authentication method -- `caching_sha2_password` - Default authentication method in MySQL 8.0+ -- `mysql_clear_password` - Cleartext authentication (requires `allowCleartextPasswords=true`) -- `mysql_old_password` - Old MySQL authentication (requires `allowOldPasswords=true`) -- `sha256_password` - SHA256 authentication -- `client_ed25519` - MariaDB Ed25519 authentication +The driver implements a pluggable authentication system that supports various authentication methods used by MySQL and MariaDB servers. The built-in authentication plugins include: + +* `mysql_native_password` - The default MySQL authentication method +* `caching_sha2_password` - Default authentication method in MySQL 8.0+ +* `mysql_clear_password` - Cleartext authentication (requires `allowCleartextPasswords=true`) +* `mysql_old_password` - Old MySQL authentication (requires `allowOldPasswords=true`) +* `sha256_password` - SHA256 authentication +* `client_ed25519` - MariaDB Ed25519 authentication🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
541-541: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
542-542: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
543-543: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
544-544: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
545-545: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
546-546: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
auth_old_password.go (2)
11-12: Add missing documentation for SimpleAuth embedding.The
OldPasswordPluginembedsSimpleAuth, but there's no documentation explaining whatSimpleAuthprovides. Consider adding a comment to clarify the inheritance relationship and available functionality.
83-108: Consider adding security warning for production use.While the implementation correctly follows the legacy protocol, it would be helpful to add a comment warning that this hashing algorithm is cryptographically weak by modern standards and should only be used for backward compatibility.
// Generate binary hash from byte string using insecure pre 4.1 method +// WARNING: This algorithm is cryptographically weak by modern standards +// and should only be used for backward compatibility with legacy systems. func pwHash(password []byte) (result [2]uint32) {auth_caching_sha2.go (1)
149-176: Consider using crypto/subtle for constant-time operations.Since this is a cryptographic function, consider using
crypto/subtlepackage's constant-time functions for operations involving sensitive data to avoid potential timing side-channel attacks.// XOR the first hash with the third hash for i := range message1 { - message1[i] ^= message2[i] + message1[i] = message1[i] ^ message2[i] // Use XOR operator explicitly for clarity }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
README.md(1 hunks)auth.go(2 hunks)auth_caching_sha2.go(1 hunks)auth_cleartext.go(1 hunks)auth_ed25519.go(1 hunks)auth_mysql_native.go(1 hunks)auth_old_password.go(1 hunks)auth_plugin.go(1 hunks)auth_sha256.go(1 hunks)auth_test.go(53 hunks)connector.go(2 hunks)const.go(0 hunks)driver.go(1 hunks)packets.go(0 hunks)
💤 Files with no reviewable changes (2)
- packets.go
- const.go
✅ Files skipped from review due to trivial changes (1)
- auth_mysql_native.go
🚧 Files skipped from review as they are similar to previous changes (4)
- driver.go
- connector.go
- auth_cleartext.go
- auth_plugin.go
🧰 Additional context used
🧬 Code Graph Analysis (5)
auth_caching_sha2.go (3)
auth_plugin.go (2)
AuthPlugin(12-26)RegisterAuthPlugin(61-63)dsn.go (1)
Config(37-84)errors.go (1)
ErrMalformPkt(21-21)
auth_ed25519.go (2)
auth_plugin.go (2)
SimpleAuth(28-30)RegisterAuthPlugin(61-63)dsn.go (1)
Config(37-84)
auth_test.go (2)
auth_old_password.go (1)
OldPasswordPlugin(12-12)auth_mysql_native.go (1)
NativePasswordPlugin(14-16)
auth.go (2)
auth_plugin.go (1)
AuthPlugin(12-26)errors.go (1)
ErrMalformPkt(21-21)
auth_sha256.go (3)
auth_plugin.go (2)
AuthPlugin(12-26)RegisterAuthPlugin(61-63)dsn.go (1)
Config(37-84)errors.go (1)
ErrMalformPkt(21-21)
🪛 markdownlint-cli2 (0.17.2)
README.md
541-541: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
542-542: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
543-543: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
544-544: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
545-545: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
546-546: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: test (windows-latest, 1.24, 8.4)
- GitHub Check: test (windows-latest, 1.24, mariadb-11.1)
- GitHub Check: test (windows-latest, 1.24, mariadb-11.4)
- GitHub Check: test (windows-latest, 1.24, 5.7)
- GitHub Check: test (windows-latest, 1.24, 8.0)
- GitHub Check: test (windows-latest, 1.24, mariadb-10.5)
- GitHub Check: test (windows-latest, 1.24, 9.0)
- GitHub Check: test (windows-latest, 1.24, mariadb-10.11)
- GitHub Check: test (windows-latest, 1.24, mariadb-10.6)
- GitHub Check: test (windows-latest, 1.24, mariadb-11.2)
- GitHub Check: test (macos-latest, 1.24, 8.0)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-11.2)
- GitHub Check: test (macos-latest, 1.24, 9.0)
- GitHub Check: test (macos-latest, 1.24, mariadb-11.2)
- GitHub Check: test (macos-latest, 1.24, mariadb-10.6)
- GitHub Check: test (macos-latest, 1.24, mariadb-10.5)
🔇 Additional comments (15)
auth_test.go (4)
1335-1370: Well-designed test for simple authentication switch flowThis test effectively verifies the multi-authentication support by testing a scenario where authentication switches from SHA256 password to cleartext password. The test checks both the beginning and end of the authentication data, confirming that the protocol flows correctly through multiple authentication methods.
1372-1413: Comprehensive test for complex multi-authentication flowThis test covers a more complex scenario with multiple authentication switches including public key retrieval, demonstrating the robustness of the new plugin-based authentication system. The verification checks both the prefix and suffix of the response data, ensuring the correct sequence of authentication packets.
53-56: Good refactoring of old password testing to use plugin architectureThe test was correctly refactored to use the new plugin-based approach, instantiating the
OldPasswordPluginand using its method instead of a global function.
122-124: Updated interface for handleAuthResult with proper recursion limitThe signature change for
handleAuthResultnow includes the remaining switch count (5) to prevent infinite loops during authentication. This is a good security practice.auth_ed25519.go (2)
29-64: Strong cryptographic implementation of Ed25519 authenticationThe Ed25519 authentication implementation is secure and follows cryptographic best practices:
- It derives keys using SHA-512 hashing
- Properly implements scalar clamping
- Uses the
edwards25519library for cryptographic operations- Has appropriate error handling at each step
The implementation is attributed to the MariaDB server source and follows the Go style guidelines.
16-23: Clean plugin architecture with proper registrationThe plugin follows the established plugin architecture pattern, embedding
SimpleAuthand registering itself during initialization.auth.go (2)
80-129: Improved authentication flow with plugin-based approachThe new
handleAuthResultmethod implements a more modular and extensible authentication flow:
- It sets a limit on authentication switches to prevent infinite loops
- Properly delegates to the authentication plugin for continuation
- Handles standard MySQL protocol responses (OK, ERR, EOF)
- Manages authentication switches by retrieving the appropriate plugin
- Uses recursion with a decremented counter for subsequent authentication steps
This approach improves maintainability and makes it easier to add new authentication methods.
131-153: Well-designed helper for authentication switch parsingThe new
parseAuthSwitchDatamethod correctly extracts plugin name and authentication data from packets, with special handling for the old authentication protocol. The method makes a copy of the authentication data to prevent unintended modifications.auth_old_password.go (2)
22-33: Good implementation with proper error handling.The method handles configuration checks and empty password cases correctly. The reference to a known issue with a GitHub link provides helpful context for future maintainers.
35-55: Implementation follows the legacy protocol correctly.The scramble implementation properly follows the pre-4.1 MySQL protocol. The method correctly limits the scramble to 8 bytes and uses the legacy hashing and XOR operations.
auth_caching_sha2.go (4)
54-57: Error handling improvement for empty packet.Good error handling with context using the wrapped error pattern for better diagnostic information.
59-138: Well-structured authentication flow with proper error handling.The implementation correctly handles all the authentication paths with proper error messages and comprehensive packet validation. The code is readable and well-organized with clear comments explaining each step of the process.
76-81:✅ Verification successful
Verify error handling when sending cleartext password.
Ensure the error handling correctly propagates errors when sending cleartext passwords. Currently, you're wrapping the error with additional context, which is good, but verify this interaction with the actual connection implementation.
🏁 Script executed:
#!/bin/bash # Check how writeAuthSwitchPacket handles errors in other places grep -rn "writeAuthSwitchPacket" --include="*.go" .Length of output: 479
Cleartext password error handling is correct
The
writeAuthSwitchPacketmethod returns any underlying write errors directly, and yourfmt.Errorf("failed to send cleartext password: %w", err)wrapper cleanly adds context while propagating the error. No further changes are needed here.
22-24:❌ Incorrect review comment
Fix inconsistent struct definition.
The
CachingSha2PasswordPluginembedsAuthPluginwhich appears to be an interface, not a struct. Compare with other plugins that embedSimpleAuthor properly implement the interface. This might lead to unexpected behavior ifAuthPlugindoesn't have default implementations.
🏁 Script executed:
#!/bin/bash # Verify the definition of AuthPlugin interface and SimpleAuth grep -rn "type AuthPlugin" --include="*.go" . grep -rn "type SimpleAuth" --include="*.go" .Length of output: 188
🏁 Script executed:
#!/bin/bash # List other AuthPlugin implementations to compare struct embedding patterns grep -R "type .*Plugin struct" -n --include="*.go" .Length of output: 430
🏁 Script executed:
#!/bin/bash # Inspect constructors for password plugins to see how they initialize embedded fields grep -R "func New.*PasswordPlugin" -n --include="*.go" -A5 -B5 .Length of output: 66
Ignore inconsistent struct definition warning
Embedding the
AuthPlugininterface inCachingSha2PasswordPluginis intentional and matches the pattern used by other password plugins:
- auth_cleartext.go:21
type ClearPasswordPlugin struct{ AuthPlugin }- auth_ed25519.go:17
type ClientEd25519Plugin struct{ AuthPlugin }- auth_mysql_native.go:14
type NativePasswordPlugin struct{ AuthPlugin }- auth_sha256.go:22
type Sha256PasswordPlugin struct{ AuthPlugin }No changes are needed here.
Likely an incorrect or invalid review comment.
auth_sha256.go (1)
107-130: Strong implementation with good security practices.The password encryption implementation properly XORs the password with the auth seed to prevent replay attacks and uses RSA-OAEP with SHA1, which is more secure than PKCS#1 v1.5 padding. Consider using a more modern hash function like SHA-256 instead of SHA-1 in the future, though this might be constrained by protocol requirements.
| // Send encrypted password | ||
| enc, err := encryptPassword(mc.cfg.Passwd, authData, pub.(*rsa.PublicKey)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to encrypt password with server key: %w", err) | ||
| } |
There was a problem hiding this comment.
Add type assertion error checking.
The type assertion pub.(*rsa.PublicKey) on line 90 doesn't check if the assertion succeeded. If the server returns an unexpected key type, this could panic. Add error checking for the type assertion.
// Send encrypted password
-enc, err := encryptPassword(mc.cfg.Passwd, authData, pub.(*rsa.PublicKey))
+pubKey, ok := pub.(*rsa.PublicKey)
+if !ok {
+ return nil, fmt.Errorf("server sent an invalid public key type: %T", pub)
+}
+enc, err := encryptPassword(mc.cfg.Passwd, authData, pubKey)
if err != nil {
return nil, fmt.Errorf("failed to encrypt password with server key: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Send encrypted password | |
| enc, err := encryptPassword(mc.cfg.Passwd, authData, pub.(*rsa.PublicKey)) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to encrypt password with server key: %w", err) | |
| } | |
| // Send encrypted password | |
| pubKey, ok := pub.(*rsa.PublicKey) | |
| if !ok { | |
| return nil, fmt.Errorf("server sent an invalid public key type: %T", pub) | |
| } | |
| enc, err := encryptPassword(mc.cfg.Passwd, authData, pubKey) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to encrypt password with server key: %w", err) | |
| } |
| func (p *Sha256PasswordPlugin) continuationAuth(packet []byte, authData []byte, mc *mysqlConn) ([]byte, error) { | ||
|
|
||
| switch packet[0] { | ||
| case iOK, iERR, iEOF: | ||
| return packet, nil | ||
|
|
||
| case iAuthMoreData: | ||
| // Parse public key from PEM format | ||
| block, rest := pem.Decode(packet[1:]) | ||
| if block == nil { | ||
| return nil, fmt.Errorf("invalid PEM data in auth response: %q", rest) | ||
| } | ||
|
|
||
| // Parse the public key | ||
| pub, err := x509.ParsePKIXPublicKey(block.Bytes) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse public key: %w", err) | ||
| } | ||
|
|
||
| // Send encrypted password | ||
| enc, err := encryptPassword(mc.cfg.Passwd, authData, pub.(*rsa.PublicKey)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to encrypt password with server key: %w", err) | ||
| } | ||
|
|
||
| // Send the encrypted password | ||
| if err = mc.writeAuthSwitchPacket(enc); err != nil { | ||
| return nil, fmt.Errorf("failed to send encrypted password: %w", err) | ||
| } | ||
|
|
||
| return mc.readPacket() | ||
|
|
||
| default: | ||
| return nil, fmt.Errorf("%w: unexpected packet type %d", ErrMalformPkt, packet[0]) | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing check for packet length before accessing.
There's no check for packet length before accessing packet[0] on line 72. If the server returns an empty packet, this will cause a panic. Add a length check similar to what you implemented in auth_caching_sha2.go.
+if len(packet) == 0 {
+ return nil, fmt.Errorf("%w: empty auth response packet", ErrMalformPkt)
+}
switch packet[0] {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (p *Sha256PasswordPlugin) continuationAuth(packet []byte, authData []byte, mc *mysqlConn) ([]byte, error) { | |
| switch packet[0] { | |
| case iOK, iERR, iEOF: | |
| return packet, nil | |
| case iAuthMoreData: | |
| // Parse public key from PEM format | |
| block, rest := pem.Decode(packet[1:]) | |
| if block == nil { | |
| return nil, fmt.Errorf("invalid PEM data in auth response: %q", rest) | |
| } | |
| // Parse the public key | |
| pub, err := x509.ParsePKIXPublicKey(block.Bytes) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to parse public key: %w", err) | |
| } | |
| // Send encrypted password | |
| enc, err := encryptPassword(mc.cfg.Passwd, authData, pub.(*rsa.PublicKey)) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to encrypt password with server key: %w", err) | |
| } | |
| // Send the encrypted password | |
| if err = mc.writeAuthSwitchPacket(enc); err != nil { | |
| return nil, fmt.Errorf("failed to send encrypted password: %w", err) | |
| } | |
| return mc.readPacket() | |
| default: | |
| return nil, fmt.Errorf("%w: unexpected packet type %d", ErrMalformPkt, packet[0]) | |
| } | |
| } | |
| func (p *Sha256PasswordPlugin) continuationAuth(packet []byte, authData []byte, mc *mysqlConn) ([]byte, error) { | |
| // Ensure we have at least one byte before inspecting packet[0] | |
| if len(packet) == 0 { | |
| return nil, fmt.Errorf("%w: empty auth response packet", ErrMalformPkt) | |
| } | |
| switch packet[0] { | |
| case iOK, iERR, iEOF: | |
| return packet, nil | |
| case iAuthMoreData: | |
| // Parse public key from PEM format | |
| block, rest := pem.Decode(packet[1:]) | |
| if block == nil { | |
| return nil, fmt.Errorf("invalid PEM data in auth response: %q", rest) | |
| } | |
| // Parse the public key | |
| pub, err := x509.ParsePKIXPublicKey(block.Bytes) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to parse public key: %w", err) | |
| } | |
| // Send encrypted password | |
| enc, err := encryptPassword(mc.cfg.Passwd, authData, pub.(*rsa.PublicKey)) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to encrypt password with server key: %w", err) | |
| } | |
| // Send the encrypted password | |
| if err = mc.writeAuthSwitchPacket(enc); err != nil { | |
| return nil, fmt.Errorf("failed to send encrypted password: %w", err) | |
| } | |
| return mc.readPacket() | |
| default: | |
| return nil, fmt.Errorf("%w: unexpected packet type %d", ErrMalformPkt, packet[0]) | |
| } | |
| } |
|
rebase on current master, and corrected the point above |
Description
authentication has been revamp. this is a simplification of #1694, without new authentication plugin addition, since it's already a big change.
Current implementation was requiring authentication switch plugins data to begin with iAuthMoreData, but that's only required for "caching_sha2_password" and "sha256_password" plugins.
Additionally, now permits multi-authentication (in mariadb: https://mariadb.com/kb/en/create-user/#identified-viawith-authentication_plugin / mysql: https://dev.mysql.com/doc/refman/8.0/en/multifactor-authentication.html)
goal is to add MariaDB main authentication plugins (like parsec, PAM, GSSAPI, ...) based on plugin like this
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests