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.
|
|
|
Maybe, its time to use internal package. |
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.
|
rebase on current master, and corrected the point above |
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, ...)
|
rebase on current master, and added fde44b6 to ensure public key casting |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
auth_sha256.go (2)
71-73: Good error handling for empty packets.The code correctly checks for empty packets before accessing packet contents, preventing potential panic, as suggested in a previous review comment.
92-95: Proper type assertion with error checking.The code correctly checks if the type assertion to
*rsa.PublicKeysucceeded, preventing potential panic, as suggested in a previous review comment.
🧹 Nitpick comments (2)
README.md (1)
537-547: Maintain consistency in list formatting.The list items in this section use dash (
-) characters, while the rest of the document uses asterisks (*) for unordered lists.-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_caching_sha2.go (1)
55-143: Comprehensive handling of multi-stage authentication.The implementation thoroughly handles all authentication scenarios, including fast auth success, full authentication with TLS/Unix sockets, and RSA encryption for non-TLS connections. Error messages are descriptive and specific to each failure case.
However, the code has a complex nesting structure which might benefit from some refactoring for readability.
Consider refactoring the nested switch statements to improve readability:
func (p *CachingSha2PasswordPlugin) continuationAuth(packet []byte, authData []byte, mc *mysqlConn) ([]byte, error) { 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: - switch len(packet) { - case 1: - return mc.readPacket() // Auth successful - - case 2: - switch packet[1] { - case 3: - // the password was found in the server's cache - return mc.readPacket() - - case 4: - // indicates full authentication is needed + if len(packet) == 1 { + return mc.readPacket() // Auth successful + } + + if len(packet) == 2 { + if packet[1] == 3 { + // the password was found in the server's cache + return mc.readPacket() + } + + if packet[1] == 4 { + // indicates full authentication is needed // For TLS connections or Unix socket, send cleartext password
📜 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)
- const.go
- packets.go
✅ Files skipped from review due to trivial changes (1)
- auth_cleartext.go
🚧 Files skipped from review as they are similar to previous changes (3)
- auth_mysql_native.go
- driver.go
- auth_plugin.go
🧰 Additional context used
🧬 Code Graph Analysis (6)
auth.go (2)
auth_plugin.go (1)
AuthPlugin(12-26)errors.go (1)
ErrMalformPkt(21-21)
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_ed25519.go (2)
auth_plugin.go (2)
SimpleAuth(28-30)RegisterAuthPlugin(61-63)dsn.go (1)
Config(37-84)
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_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)
🪛 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 (17)
- GitHub Check: test (ubuntu-latest, 1.23, 9.0)
- GitHub Check: test (windows-latest, 1.24, mariadb-10.11)
- GitHub Check: test (macos-latest, 1.24, mariadb-11.4)
- GitHub Check: test (windows-latest, 1.24, mariadb-10.5)
- GitHub Check: test (windows-latest, 1.24, mariadb-11.4)
- GitHub Check: test (windows-latest, 1.24, mariadb-11.1)
- GitHub Check: test (windows-latest, 1.24, mariadb-11.2)
- GitHub Check: test (windows-latest, 1.24, 5.7)
- GitHub Check: test (windows-latest, 1.24, 8.0)
- GitHub Check: test (macos-latest, 1.24, 8.4)
- GitHub Check: test (macos-latest, 1.24, mariadb-11.2)
- GitHub Check: test (windows-latest, 1.24, 8.4)
- GitHub Check: test (macos-latest, 1.24, 8.0)
- GitHub Check: test (ubuntu-latest, 1.24, 5.7)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-11.1)
- GitHub Check: test (macos-latest, 1.24, 5.7)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-11.2)
🔇 Additional comments (27)
connector.go (4)
21-23: LGTM! Good addition of a recursion limit.The new constant
authMaximumSwitchprovides a secure limit to prevent infinite authentication plugin switches.
147-150: LGTM! Better error handling for unsupported plugins.The code now fails early with a meaningful error message if an authentication plugin isn't supported, rather than potentially failing later with a less clear error.
153-157: Improved error handling and flow.The code now consistently handles authentication errors by cleaning up the connection before returning the error, making the error handling more robust.
164-164: LGTM! Consistent interface-based approach.The code now correctly passes the plugin instance and remaining switch count to the auth handler, consistent with the new plugin-based architecture.
auth.go (5)
80-86: LGTM! Effective recursion protection.The function now enforces a maximum number of authentication switches to prevent infinite recursion. This is a security improvement for multi-factor authentication scenarios.
92-94: LGTM! Improved error handling for malformed authentication responses.The code now checks for empty authentication response packets and returns a clear error with a reference to the underlying issue.
96-99: LGTM! Delegation to the plugin-specific implementation.The code now delegates authentication continuation to the plugin implementation, supporting the new modular architecture.
101-128: LGTM! Well-structured packet type handling.The switch statement is a clear improvement over the previous approach, with specific handling for OK, ERROR, and authentication switch packets.
131-153: LGTM! Well-implemented auth switch data parsing.This helper function properly extracts authentication plugin name and data, with appropriate handling for special cases like the old password protocol and null-terminated strings.
auth_test.go (3)
54-56: LGTM! Proper refactoring to use the plugin struct.Test has been updated to use the
OldPasswordPlugintype directly instead of calling standalone functions, aligning with the new plugin architecture.
1335-1369: LGTM! Good test coverage for simple auth switching.This new test ensures the authentication flow correctly handles a switch from SHA256 to cleartext password authentication, verifying both requests are handled properly.
1372-1413: LGTM! Comprehensive test for multi-step authentication.This test validates a more complex scenario with public key exchange followed by authentication method switching, providing thorough coverage of the new plugin-based authentication flow.
auth_ed25519.go (3)
17-19: Good use of embedding for plugin architecture.The plugin properly embeds
SimpleAuth, aligning with the plugin architecture pattern established in the codebase, which allows for consistent implementation of the required interfaces.
21-23: Proper self-registration in init.The plugin correctly registers itself with the global plugin registry during initialization, following the established pattern for plugin registration.
29-64:✅ Verification successful
Well-implemented cryptographic algorithm with clear references.
The Ed25519 implementation follows the correct cryptographic protocol with proper references to the original MariaDB implementation. The code correctly handles key derivation, signing, and error conditions. The comments provide valuable context about the algorithm's origin.
However, verify that this dependency is stable:
🏁 Script executed:
#!/bin/bash # Check if the edwards25519 package is a stable dependency go list -m filippo.io/edwards25519Length of output: 67
Dependency Stability Confirmed: filippo.io/edwards25519 v1.1.0
The Ed25519 implementation is well-implemented with clear references to the MariaDB and Go standard library examples. Thefilippo.io/edwards25519module is at semantic version 1.1.0, indicating a stable major release—no further action required.auth_old_password.go (5)
11-12: Good use of compact struct definition.The plugin embeds
SimpleAuthusing a concise struct definition, following the plugin architecture pattern established in the codebase.
22-33: Well-handled configuration check for legacy authentication.The method correctly checks if old passwords are allowed through configuration and returns an appropriate error if not. The comment about edge cases with a reference to issue #184 provides valuable context about known limitations.
35-55: Accurate implementation of the legacy password scrambling.The implementation faithfully reproduces the legacy MySQL pre-4.1 password scrambling algorithm. The code is well-structured with clear separation of concerns between scrambling, random number generation, and hashing.
57-71: Precise implementation of the custom PRNG.The
myRndstruct andNextBytemethod accurately implement the pseudo-random number generator from MariaDB. The comments linking to test cases that verify equivalence to the original floating-point implementation add valuable validation.
83-108: Thorough implementation of the legacy password hashing.The
pwHashfunction correctly implements the pre-4.1 password hashing algorithm, including proper handling of spaces and tabs, and application of the mask to remove the sign bit.auth_caching_sha2.go (4)
19-28: Good plugin implementation with clear documentation.The plugin implementation and registration follow the established pattern in the codebase. The documentation explaining that this plugin provides secure password-based authentication with server-side caching is helpful.
34-42: Clear documentation of the SHA-256 scrambling process.The method is well-documented with a clear explanation of the three-step SHA-256 hashing process. The implementation correctly delegates to the
scrambleSHA256Passwordfunction.
44-54: Well-structured method for handling authentication flow.The documentation clearly explains the different authentication paths, and the initial error handling for empty packets is appropriate.
146-181: Well-implemented SHA-256 scrambling with clear documentation.The
scrambleSHA256Passwordfunction correctly implements the MySQL 8+ password scrambling algorithm with SHA-256. The comments explaining each step make the code easy to understand and maintain.auth_sha256.go (3)
20-30: Good plugin implementation with proper registration.The plugin is implemented correctly with proper embedding of the
AuthPlugininterface and registration with the global plugin registry.
32-62: Well-structured authentication initialization with clear branching logic.The method handles different authentication scenarios appropriately:
- Empty passwords
- TLS connections
- Available server public key
- Public key request
The comments explaining the difference from
caching_sha2_passwordregarding Unix transport are very helpful.
115-138: Secure implementation of password encryption.The
encryptPasswordfunction properly:
- Checks for nil public key
- XORs password with seed to prevent replay attacks
- Uses RSA-OAEP with SHA1 for encryption, which is more secure than PKCS#1 v1.5
The implementation follows best practices for secure password handling in this context.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
auth_caching_sha2.go (3)
28-30: Embedding the AuthPlugin interface is unusualThe struct embeds the
AuthPlugininterface, which is an uncommon pattern. Typically, a struct would just implement the interface methods without embedding the interface itself. Embedding an interface doesn't provide the implementations of its methods - it only adds method requirements, which seems unnecessary here.Consider removing the embedded interface:
type CachingSha2PasswordPlugin struct { - AuthPlugin }
90-127: Consider caching the public key after retrievalWhen the public key is retrieved from the server, consider updating
mc.cfg.pubKeyto cache it for future connections. This would prevent unnecessary key requests in subsequent authentications.pubKey, ok = pkix.(*rsa.PublicKey) if !ok { return nil, fmt.Errorf("server sent an invalid public key type: %T", pkix) } +// Cache the public key for future use +mc.cfg.pubKey = pubKey
106-108: Enhance error messaging with packet detailsFor better debuggability, consider including the actual packet content (or at least part of it) in the error message when unexpected packet types are encountered.
-return nil, fmt.Errorf("unexpected packet type %d when requesting public key", packet[0]) +return nil, fmt.Errorf("unexpected packet type %d when requesting public key, packet: %x", packet[0], packet[:min(len(packet), 20)])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)auth_caching_sha2.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🧬 Code Graph Analysis (1)
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)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: test (windows-latest, 1.24, mariadb-11.1)
- GitHub Check: test (windows-latest, 1.24, mariadb-11.2)
- GitHub Check: test (windows-latest, 1.24, 9.0)
- GitHub Check: test (windows-latest, 1.24, mariadb-11.4)
- GitHub Check: test (windows-latest, 1.24, 8.0)
- GitHub Check: test (ubuntu-latest, 1.23, 9.0)
- GitHub Check: test (windows-latest, 1.24, mariadb-10.6)
- GitHub Check: test (macos-latest, 1.24, mariadb-11.1)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-10.11)
- GitHub Check: test (macos-latest, 1.24, mariadb-11.2)
- GitHub Check: test (windows-latest, 1.24, 5.7)
- GitHub Check: test (macos-latest, 1.24, mariadb-10.11)
- GitHub Check: test (windows-latest, 1.24, mariadb-10.11)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-10.6)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-10.5)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-11.2)
- GitHub Check: test (macos-latest, 1.24, 5.7)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-11.4)
- GitHub Check: test (macos-latest, 1.24, 8.0)
- GitHub Check: test (ubuntu-latest, 1.24, 8.0)
🔇 Additional comments (4)
auth_caching_sha2.go (4)
46-48: Implementation matches MySQL 8+ scrambling algorithmThe password scrambling follows the correct algorithm for MySQL 8+. The implementation delegates to the
scrambleSHA256Passwordfunction which properly implements the three-step SHA256 hash process.
160-187: The scrambleSHA256Password function is correctly implementedThe implementation correctly follows the MySQL 8+ password scrambling algorithm as documented in the comments. It properly handles empty passwords by returning an empty byte slice.
1-188: Overall implementation aligns with the PR objectivesThe implementation successfully provides modular authentication for the caching_sha2_password plugin, aligning with the PR's goal of enabling multiple authentication methods. The plugin correctly implements the defined interface and handles the various authentication flows securely.
122-126:✅ Verification successful
Verify that the type assertion is secure
The code performs a type assertion to check if the public key is an RSA key. While this is correct, ensure that the server is expected to always send an RSA key, as this code will reject other key types.
🏁 Script executed:
#!/bin/bash # Find where pubKey is being set elsewhere in the codebase # and verify if there's any code handling different key types # Search for pubKey field assignment in the codebase rg "pubKey\s*=|cfg\.pubKey" --type goLength of output: 874
🏁 Script executed:
#!/bin/bash # Locate and display the getServerPubKey implementation to verify which key types it returns rg -n -A 20 "func getServerPubKey" --type goLength of output: 967
Type assertion is safe—no changes needed
Confirmed that
getServerPubKeyand all assignments tocfg.pubKeyuse only * rsa.PublicKey. The type assertion inauth_caching_sha2.gowill always succeed under the current codepaths.
| case iAuthMoreData: | ||
| switch len(packet) { | ||
| case 1: | ||
| return mc.readPacket() // Auth successful |
There was a problem hiding this comment.
i've keeped this part because was existing, but this is not in protocol https://dev.mysql.com/doc/dev/mysql-server/latest/page_caching_sha2_authentication_exchanges.html, server is expected to either send result fast_auth_success(3) or perform_full_authentication(4) only
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