Skip to content

client: make CLIENT_QUERY_ATTRIBUTES opt-out to restore proxy compatibility #1041

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

masahide
Copy link
Contributor

Background — why proxies built with this SDK break

masahide/mysql8-audit-proxy#8

A common architecture for a MySQL audit / filter / router built on go-mysql looks like this:

[application] -- go-mysql/server  -->  [proxy]  -- go-mysql/client -->  [mysqld]
  • Downstream side (go-mysql/server)
    Implements the old 5.7/8.0 protocol and does not recognise
    CLIENT_QUERY_ATTRIBUTES
    .
    When an application connects, the handshake therefore has the bit cleared.

  • Upstream side (go-mysql/client ≥ v1.12.0)
    In writeAuthHandshake() the client automatically copies
    CLIENT_QUERY_ATTRIBUTES from the server greeting
    , claiming the proxy
    will send query attributes.

  • Mismatch
    The proxy simply forwards the raw COM_QUERY it received from the
    application.
    Because the downstream never added the attributes section, the packet sent
    upstream is missing mandatory fields → MySQL returns
    ERROR 1835 (HY000) Malformed communication packet.

What the proxy author needs

A way to say “don’t advertise CLIENT_QUERY_ATTRIBUTES on the upstream
connection even if the server supports it, because my downstream side can’t
handle that mode.”

Currently that is impossible: UnsetCapability() removes the flag from
ccaps, but writeAuthHandshake() puts it back from the server’s flag set.

Patch overview

  1. Add dcaps (“deny caps”) to client.Conn to track flags explicitly
    disabled by the caller.

  2. Modify SetCapability / UnsetCapability:

    SetCapability(x): ccaps |= x ; dcaps &^= x
    UnsetCapability(x): ccaps &^= x ; dcaps |= x
  3. In writeAuthHandshake():

    inherit := c.capability &^ c.dcaps          // server flags minus explicit denies
    // ...
    // Advertise QUERY_ATTRIBUTES only if:
    //   a) caller opted-in  OR
    //   b) server supports it AND caller did NOT opt-out
  4. Store the final negotiated set back into c.capability for later use.

Resulting behaviour

Scenario Downstream (server pkg) Proxy call Upstream handshake
Default no support none bit set (auto)
Proxy wants legacy mode no support UnsetCapability(CLIENT_QUERY_ATTRIBUTES) bit cleared
Force new mode anyway anything SetCapability(CLIENT_QUERY_ATTRIBUTES) bit set

Example — disabling the flag in a proxy

backend, _ := client.Connect(
    upstreamAddr, user, pass, db,
    func(c *client.Conn) error {
        c.UnsetCapability(mysql.CLIENT_QUERY_ATTRIBUTES)
        return nil
    },
)

With this one-liner the proxy once again works with both modern MySQL servers
and the existing go-mysql/server front end.

Safety & compatibility

  • No API break – existing code compiles; only behaviour changes when the
    caller explicitly opts out.
  • Maintains the convenient “auto-enable” path for direct clients.
  • Restores plug-and-play compatibility for every proxy built on
    go-mysql/server.

Adjust client capability flags based on server support,
and account for explicitly disabled capabilities.

- client/auth.go: Update client capability flags
- client/conn.go: Add field to manage disabled capabilities
@lance6716 lance6716 requested a review from Copilot June 1, 2025 07:49
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request implements an opt-out mechanism for the CLIENT_QUERY_ATTRIBUTES capability to restore proxy compatibility with legacy MySQL servers. The key changes include:

  • Adding a new field (dcaps) to track explicitly disabled capabilities.
  • Updating the SetCapability and UnsetCapability functions to modify both ccaps and dcaps.
  • Revising the capability negotiation logic in writeAuthHandshake to conditionally advertise CLIENT_QUERY_ATTRIBUTES.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
client/conn.go Introduced dcaps and updated SetCapability/UnsetCapability functions.
client/auth.go Adjusted capability inheritance logic in writeAuthHandshake.
Comments suppressed due to low confidence (2)

client/conn.go:48

  • [nitpick] Consider renaming 'dcaps' to 'disabledCaps' for improved clarity.
dcaps uint32 // disabled capabilities

client/auth.go:215

  • [nitpick] Optionally, consider renaming 'inherit' to 'inheritedCaps' to more clearly convey its purpose.
inherit := c.capability & ^c.dcaps             // Server-side capabilities minus explicit denies

Copy link
Collaborator

@lance6716 lance6716 left a comment

Choose a reason for hiding this comment

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

lgtm at the first glance. If you are not urgent, I want to check if there are other potential problems before merging this PR.

Copy link
Collaborator

@dveeden dveeden left a comment

Choose a reason for hiding this comment

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

Seems fine to me on first glance

capability |= c.capability & mysql.CLIENT_LONG_FLAG
capability |= c.capability & mysql.CLIENT_QUERY_ATTRIBUTES
// ---- Inherit capabilities that the server has and the user has NOT explicitly denied ----
inherit := c.capability & ^c.dcaps // Server-side capabilities minus explicit denies
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe name this inheritedCaps or inherited?

@@ -45,6 +45,7 @@ type Conn struct {
capability uint32
// client-set capabilities only
ccaps uint32
dcaps uint32 // disabled capabilities
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see you followed the naming style of ccaps. Just a weak comment: capability, ccaps and dcaps are hard to understand at their usage functions. Maybe rename them to serverCaps, clientExplicitOnCaps, clientExplicitOffCaps or something.

@@ -211,8 +211,15 @@ func (c *Conn) writeAuthHandshake() error {
capability := mysql.CLIENT_PROTOCOL_41 | mysql.CLIENT_SECURE_CONNECTION |
mysql.CLIENT_LONG_PASSWORD | mysql.CLIENT_TRANSACTIONS | mysql.CLIENT_PLUGIN_AUTH
// Adjust client capability flags based on server support
capability |= c.capability & mysql.CLIENT_LONG_FLAG
capability |= c.capability & mysql.CLIENT_QUERY_ATTRIBUTES
Copy link
Collaborator

@lance6716 lance6716 Jun 3, 2025

Choose a reason for hiding this comment

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

I think we don't need to specially process CLIENT_QUERY_ATTRIBUTES. There are 3 states of capability:

  1. developer calls SetCapability, which means the ccaps bit is 1 and the dcaps bit is 0
  2. developer calls UnsetCapability, which means ccaps bit is 0 and the dcaps bit is 1
  3. No calls, which means both bits are 0

So here we turn on the bits by ccaps at around line 219, and have another round to turn off bits by dcaps. I suggest

capability |= c.ccaps&mysql.CLIENT_FOUND_ROWS | ...
capability &= c.dcaps... // ⬅️ here we add CLIENT_QUERY_ATTRIBUTES and other flags from above line to turn off

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants