Conversation
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
There was a problem hiding this comment.
Greptile Overview
Summary
This PR adds MySQL protocol support to the PAM (Privileged Access Management) system, enabling proxied MySQL connections with credential injection. The implementation creates a man-in-the-middle MySQL proxy that authenticates clients while using injected credentials to connect to the actual MySQL server.
Key changes:
- Custom MySQL server implementation (adapted from go-mysql library) for handling client handshakes
- Relay handler that forwards MySQL protocol commands between client and server
- Support for
mysql_native_passwordauthentication - Capability flag parsing and manipulation to control protocol features
Critical issues found:
- Missing audit logging: Unlike the PostgreSQL handler, MySQL queries are not tracked or logged (all query tracking TODOs remain unimplemented). This is a fundamental requirement for PAM systems
- Protocol compliance bugs:
COM_PINGandCOM_INIT_DBcommands fall through to returnnil, causing no response to be sent to clients - Incomplete error handling: Several handler methods panic with "Unexpected function call" rather than gracefully handling edge cases
- TLS support missing: The TODO comment indicates TLS connections are not implemented, which may be required for secure credential transmission
Confidence Score: 1/5
- This PR has critical functional gaps that prevent it from meeting PAM audit requirements
- Score reflects missing audit logging (core PAM requirement), protocol compliance bugs that will break client connections, and unimplemented TLS support. The PostgreSQL handler shows comprehensive query tracking that is completely absent here
- Pay close attention to
packages/pam/handlers/mysql/relay_handler.go(missing audit logging),packages/pam/handlers/mysql/proxy.go(TLS unimplemented), andpackages/pam/handlers/mysql/use_db_handler.go(panicking methods)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| packages/pam/handlers/mysql/proxy.go | 2/5 | Main MySQL proxy with critical issues: typo in error message, missing query tracking, unimplemented TLS support, and protocol handling bugs |
| packages/pam/handlers/mysql/relay_handler.go | 1/5 | Relay handler missing critical audit logging for queries/statements and has protocol compliance issues with COM_PING/COM_INIT_DB commands |
| packages/pam/handlers/mysql/server/conn.go | 4/5 | MySQL server connection handling implementation - appears solid with proper handshake and authentication flow |
| packages/pam/handlers/mysql/any_user_credential_provider.go | 3/5 | Stub credential provider that accepts any username/password - implementation appears incomplete |
| packages/pam/handlers/mysql/use_db_handler.go | 3/5 | Database handler with most methods panicking - incomplete implementation that may cause runtime errors |
Sequence Diagram
sequenceDiagram
participant Client as MySQL Client
participant Proxy as MySQL Proxy
participant Server as MySQL Server
Client->>Proxy: TCP Connection
Proxy->>Server: Connect with injected credentials
Server-->>Proxy: Initial Handshake (version, capabilities, salt)
Note over Proxy: Parse server capabilities<br/>Disable SSL flags
Proxy->>Client: Initial Handshake (modified capabilities)
Client->>Proxy: Handshake Response (client auth)
Note over Proxy: Accept any credentials<br/>(AnyUserCredentialProvider)
Proxy-->>Client: Authentication OK
loop Query Loop
Client->>Proxy: COM_QUERY / COM_STMT_*
Note over Proxy: TODO: Missing query logging<br/>for audit trail
Proxy->>Server: Forward request
Server-->>Proxy: Response (OK/ERR/ResultSet)
Proxy-->>Client: Forward response
end
Client->>Proxy: COM_QUIT
Proxy->>Proxy: Close connections
15 files reviewed, 7 comments
15a8f73 to
71aab1f
Compare
|
Hey @greptileai, I forgot to push changes last night, please review this PR again |
There was a problem hiding this comment.
Greptile Overview
Summary
Adds MySQL PAM (Privileged Access Management) proxy support alongside existing PostgreSQL implementation. The changes include:
- New MySQL proxy handlers in
packages/pam/handlers/mysql/that mirror the PostgreSQL proxy architecture - Session logging integration with query capture and encryption for audit trail
- Credential injection mechanism allowing PAM to control authentication while accepting any client username
- TLS support for secure MySQL connections with custom certificate validation
- Refactored session logger to use interface pattern, improving code reusability between PostgreSQL and MySQL implementations
- Resource type routing in
pam-proxy.gowith switch statement for handling different database types
The implementation closely follows the existing PostgreSQL proxy pattern with proper connection lifecycle management, session expiry handling, and cleanup.
Confidence Score: 3/5
- This PR requires fixing a critical bug in command handling before merge
- The MySQL proxy implementation is well-structured and follows good patterns from the PostgreSQL proxy. However,
HandleOtherCommandreturns errors for common MySQL commands (COM_PING,COM_INIT_DB) which will break health checks and cause connection failures in production. This needs to be fixed before deployment. - packages/pam/handlers/mysql/relay_handler.go requires fixing the
HandleOtherCommandimplementation to handleCOM_PINGandCOM_INIT_DB
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| packages/pam/handlers/mysql/proxy.go | 4/5 | Implements MySQL proxy with connection handling and TLS support; follows postgres.go pattern with proper session logging and cleanup |
| packages/pam/handlers/mysql/relay_handler.go | 3/5 | Handles MySQL command relaying and query logging; HandleOtherCommand returns error for common commands like COM_PING |
| packages/pam/pam-proxy.go | 5/5 | Refactored to support both PostgreSQL and MySQL; properly extracts common TLS setup and adds switch statement for resource types |
Sequence Diagram
sequenceDiagram
participant Client as MySQL Client
participant Proxy as MySQL Proxy
participant Handler as RelayHandler
participant Server as MySQL Server
participant Logger as SessionLogger
Client->>Proxy: Connect
Proxy->>Server: Connect with injected credentials
Server-->>Proxy: Connection established
Proxy->>Proxy: Create server instance (v8.0.11)
Proxy->>Handler: NewRelayHandler(conn, logger)
Proxy->>Client: Accept client connection
loop For each client command
Client->>Proxy: MySQL Command
Proxy->>Handler: HandleCommand()
alt Query Command
Handler->>Server: Execute(query)
Server-->>Handler: Result
Handler->>Logger: LogEntry(query, result)
Handler-->>Proxy: Result
else Prepared Statement
Handler->>Server: Prepare(query)
Server-->>Handler: Statement
Handler-->>Proxy: Statement context
else Statement Execute
Handler->>Server: stmt.Execute(args)
Server-->>Handler: Result
Handler->>Logger: LogEntry(query, result)
Handler-->>Proxy: Result
else Use DB
Handler->>Server: UseDB(dbName)
Server-->>Handler: OK
Handler-->>Proxy: OK
else Other Command
Handler-->>Proxy: Error (not supported)
end
Proxy-->>Client: Response
end
Client->>Proxy: Disconnect
Proxy->>Logger: Close()
Proxy->>Server: Close()
Proxy->>Client: Close()
8 files reviewed, 1 comment
71aab1f to
e64a830
Compare
|
The CVE-2024-41434 introduced by the go-mysql detected by snyk is a false positive. Version tag like |
@fangpenlin, if there's no fixed version for the go-mysql package, then you can do something like this in the go.mod file to replace the dependency version: replace github.com/pingcap/tidb/pkg/parser v<old-version> => github.com/pingcap/tidb/pkg/parser v<new-version> |
|
@DanielHougaard I tried that before, but that doesn't work: and the error I am not particular familiar with go lang's mod versioning stuff |
e64a830 to
895f336
Compare
895f336 to
17cca0f
Compare
17cca0f to
dd549e7
Compare
Passing RelayHandler as value making it copy all the time, the change to it I made are all lost between calls 😑
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Adds MySQL support to the PAM (Privileged Access Management) proxy system, mirroring the existing PostgreSQL implementation.
- Introduced MySQL proxy handler with connection relay, credential injection, and session logging
- Refactored session logger into interface (
SessionLogger) for better code reuse between PostgreSQL and MySQL handlers - Added
go-mysql-org/go-mysqllibrary and dependencies for MySQL protocol implementation - Implemented relay handler for MySQL commands (queries, prepared statements, database switching)
- Uses
AnyUserCredentialProviderto accept any client credentials, then injects actual credentials when connecting to target server - Supports TLS connections with configurable certificate validation
Confidence Score: 4/5
- This PR is safe to merge with minor style improvements recommended
- The implementation follows established patterns from the PostgreSQL handler and properly handles connection lifecycle, session logging, and error conditions. Previous critical issues (uninitialized sessionLogger, COM_PING/COM_INIT_DB handling, typos) have been addressed in follow-up commits. Only minor style issues remain (unused config fields, loop patterns).
- No files require special attention - all critical issues have been resolved
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| packages/pam/handlers/mysql/proxy.go | 4/5 | MySQL proxy implementation with connection handling and relay setup. Previous comments noted unused config fields and typo already fixed. |
| packages/pam/handlers/mysql/relay_handler.go | 4/5 | Relay handler for MySQL commands with session logging. Previous comments noted style issues and missing COM_PING/COM_INIT_DB handling. |
| packages/pam/pam-proxy.go | 5/5 | Refactored PAM proxy to support both PostgreSQL and MySQL with shared TLS configuration and session logger initialization. |
| packages/pam/session/logger.go | 5/5 | Introduced SessionLogger interface to enable better abstraction and reusability across different database handlers. |
Sequence Diagram
sequenceDiagram
participant Client as MySQL Client
participant Proxy as MysqlProxy
participant RelayHandler
participant MySQLServer as MySQL Server
participant SessionLogger
Client->>Proxy: Connect via TLS
activate Proxy
Proxy->>MySQLServer: connectToServer() with injected credentials
activate MySQLServer
MySQLServer-->>Proxy: Connection established
Proxy->>Proxy: Create go-mysql server instance
Proxy->>RelayHandler: NewRelayHandler(selfServerConn, sessionLogger)
activate RelayHandler
Proxy->>Client: Accept connection with AnyUserCredentialProvider
loop Command Loop
Client->>Proxy: Send MySQL command
Proxy->>RelayHandler: Handle command (Query/StmtPrepare/StmtExecute/UseDB)
alt Query Command
RelayHandler->>MySQLServer: Execute(query)
MySQLServer-->>RelayHandler: Result
RelayHandler->>SessionLogger: LogEntry(query, result)
RelayHandler-->>Proxy: Return result
else Prepared Statement
RelayHandler->>MySQLServer: Prepare(query)
MySQLServer-->>RelayHandler: Statement handle
RelayHandler->>MySQLServer: Execute(args)
MySQLServer-->>RelayHandler: Result
RelayHandler->>SessionLogger: LogEntry(query, result)
RelayHandler-->>Proxy: Return result
else UseDB
RelayHandler->>MySQLServer: UseDB(dbName)
MySQLServer-->>RelayHandler: Success
RelayHandler-->>Proxy: Return success
end
Proxy-->>Client: Forward response
alt Connection Lost
MySQLServer--xRelayHandler: ErrBadConn
RelayHandler->>RelayHandler: Set closed=true
RelayHandler->>MySQLServer: Close connection
end
end
Client->>Proxy: Disconnect
Proxy->>RelayHandler: Check Closed()
Proxy->>MySQLServer: Close()
deactivate MySQLServer
deactivate RelayHandler
Proxy->>SessionLogger: Close()
deactivate Proxy
8 files reviewed, 1 comment
Description 📣
Add MySQL PAM support
ref: https://linear.app/infisical/issue/ENG-3913/add-support-for-mysql-in-pam-product
Type ✨
Tests 🛠️
Please see PR Infisical/infisical#4655 for details