Support MySQL VERIFY_CA SSL Mode #1742
Conversation
WalkthroughAdds a tls-verify configuration and DSN parameter to choose certificate verification mode ("identity" or "ca"); implements CA-only verification helpers; applies tls-verify in TLS config assembly (including for custom TLS configs); updates README and AUTHORS; and adds comprehensive tests covering system and custom CA verification paths. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DSN as DSN Parser
participant Config as Config Builder
participant TLS as TLS Assembler
participant Verify as CA Verify Helper
Client->>DSN: Parse DSN (may include tls, tls-verify)
DSN->>Config: Set Config.TLSVerify ("identity" | "ca")
Note over Config,TLS: Select or build tls.Config (system, named, or custom)
Config->>TLS: Request tls.Config with TLSVerify applied
alt TLSVerify == "ca"
TLS->>Verify: createVerifyCAConfig(baseConfig, rootCAs)
Verify-->>TLS: tls.Config (InsecureSkipVerify=true + VerifyPeerCertificate)
else TLSVerify == "identity"
TLS-->>Client: tls.Config with normal hostname verification
end
TLS-->>Client: Return configured tls.Config for handshake
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
utils.go (1)
91-151: CA-only verification logic is sound; consider tightening VerifyOptionsThe VERIFY_CA implementation here looks correct overall: you disable the default verification with
InsecureSkipVerify: trueand then rebuild the chain yourself against either the providedrootCAsor the system pool, using intermediates from the rest of the chain and surfacing clear errors.Two small follow-ups to consider:
Restrict extended key usages:
x509.Certificate.Verifydefaults to “any EKU”. For TLS you typically wantExtKeyUsageServerAuth. You can preserve current behavior but slightly tighten checks with something like:opts := x509.VerifyOptions{ Roots: rootCAs, Intermediates: intermediates, KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, }This keeps VERIFY_CA semantics (no hostname check) but aligns better with what crypto/tls does for normal server auth.
TLS version policy: the new
tls.Configyou construct inherits Go’s default minimum TLS version. That matches existing code paths (e.g.tls=truewith&tls.Config{}), so I wouldn’t change it just for VERIFY_CA in this PR, but if the project decides to require TLS 1.3+ it would be better to applyMinVersionconsistently across all TLS configs in a separate change rather than only here.utils_test.go (1)
523-589: Negative-path coverage is good; consider adding a positive verify caseThese tests do a nice job exercising the error paths (no certs, empty list, invalid DER) and basic shape of the CA-only config (InsecureSkipVerify + VerifyPeerCertificate set, custom pool wiring).
If you want to harden this further, you could add an optional “happy path” test that builds a minimal self-signed CA + leaf chain in-memory and asserts that:
verifyCACallbacksucceeds when given that chain and the correspondingCertPool, andcreateVerifyCAConfigwith that pool allows a handshake simulation to pass the callback.That would catch regressions in how intermediates and root pools are wired without depending on external files.
README.md (1)
437-470: TLS/tls-verify documentation matches behavior; only minor style nitsThe new explanation of how
tls(CA source) andtls-verify(verification mode) interact is clear and correctly reflects the code paths indsn.goandutils.go, and the examples are helpful.If you care about polishing details, a few optional tweaks:
- Hyphenate compound adjectives: e.g. “self-signed certificate” and “server-side certificate”.
- You could shorten “works in conjunction with the
tlsparameter” to “works with thetlsparameter” for brevity.- Add a language identifier (e.g. ```text) to the small fenced block under
##### tls-verifyto satisfy markdown linters.All of these are purely cosmetic; the technical content looks solid.
dsn_test.go (6)
432-471: Consider adding explicit checks for identity mode behavior.The test correctly validates CA mode behavior but only implicitly tests identity mode in the else block. For completeness and clarity, consider explicitly checking that
VerifyPeerCertificateisnilin identity mode to ensure the callback is only set when needed.Apply this diff to add explicit identity mode checks:
} else { + // identity (default) should not set VerifyPeerCertificate + if cfg.TLS.VerifyPeerCertificate != nil { + t.Error("identity mode should not have VerifyPeerCertificate callback set") + } // identity (default) should set ServerName if cfg.TLS.ServerName != "example.com" { t.Errorf("identity mode should set ServerName to 'example.com', got %q", cfg.TLS.ServerName) } }
473-520: Consider adding explicit checks for identity mode behavior.Similar to
TestTLSVerifySystemCA, consider explicitly verifying thatVerifyPeerCertificateisnilin identity mode for better test coverage.Apply this diff:
} else { + // identity (default) should not set VerifyPeerCertificate + if cfg.TLS.VerifyPeerCertificate != nil { + t.Error("identity mode should not have VerifyPeerCertificate callback set") + } // identity (default) should preserve custom config's ServerName if cfg.TLS.ServerName != "customServer" { t.Errorf("identity mode should preserve custom ServerName 'customServer', got %q", cfg.TLS.ServerName) } }
566-572: LGTM! Consider optionally checking error message.The test correctly validates that invalid
tls-verifyvalues return an error. Optionally, you could verify the error message contains relevant information to help users understand what went wrong, but checking for error presence is sufficient for this level.
574-584: Consider testing case variations of reserved keys.The test validates that reserved keys cannot be registered, but only tests lowercase versions. Since the implementation in
utils.gousesreadBool()(which likely handles case-insensitive comparison) andstrings.ToLower()for "skip-verify" and "preferred", case variations like "True", "FALSE", "Skip-Verify", and "PREFERRED" should also be rejected.Consider adding a test for case variations:
func TestRegisterTLSConfigReservedKey(t *testing.T) { reservedKeys := []string{"true", "false", "skip-verify", "preferred"} for _, key := range reservedKeys { err := RegisterTLSConfig(key, &tls.Config{}) if err == nil { t.Errorf("RegisterTLSConfig should reject reserved key %q", key) } DeregisterTLSConfig(key) // Clean up in case it was registered } + + // Test case variations + caseVariations := []string{"True", "FALSE", "Skip-Verify", "PREFERRED"} + for _, key := range caseVariations { + err := RegisterTLSConfig(key, &tls.Config{}) + if err == nil { + t.Errorf("RegisterTLSConfig should reject reserved key %q (case variation)", key) + } + DeregisterTLSConfig(key) + } }
475-477: Optional: Address static analysis warnings about missing TLS MinVersion.Static analysis flags the test TLS configs for not specifying
MinVersion. While these configs are only used in tests and never establish actual connections, addingMinVersion: tls.VersionTLS12(or higher) would satisfy the linter and demonstrate security best practices even in test code.For line 475-477:
customConfig := &tls.Config{ ServerName: "customServer", + MinVersion: tls.VersionTLS12, RootCAs: nil, // Use system CA pool for this test }For line 578:
- err := RegisterTLSConfig(key, &tls.Config{}) + err := RegisterTLSConfig(key, &tls.Config{MinVersion: tls.VersionTLS12})Also applies to: 578-578
20-84: Consider adding tls-verify examples to testDSNs for round-trip testing.The
testDSNsarray is used byTestDSNReformatto verify that DSNs can be parsed and reformatted correctly. Adding entries with the newtls-verifyparameter would ensure that this parameter is properly preserved during the parse-format-parse cycle.Consider adding test cases like:
}, { "foo:bar@tcp(192.168.1.50:3307)/baz?timeout=10s&connectionAttributes=program_name:MySQLGoDriver%2FTest,program_version:1.2.3", &Config{User: "foo", Passwd: "bar", Net: "tcp", Addr: "192.168.1.50:3307", DBName: "baz", Loc: time.UTC, Timeout: 10 * time.Second, MaxAllowedPacket: defaultMaxAllowedPacket, Logger: defaultLogger, AllowNativePasswords: true, CheckConnLiveness: true, ConnectionAttributes: "program_name:MySQLGoDriver/Test,program_version:1.2.3"}, +}, { + "user:password@tcp(localhost:5555)/dbname?tls=true&tls-verify=ca", + &Config{User: "user", Passwd: "password", Net: "tcp", Addr: "localhost:5555", DBName: "dbname", Loc: time.UTC, MaxAllowedPacket: defaultMaxAllowedPacket, Logger: defaultLogger, AllowNativePasswords: true, CheckConnLiveness: true, TLSConfig: "true", TLSVerify: "ca"}, +}, { + "user:password@tcp(localhost:5555)/dbname?tls=true&tls-verify=identity", + &Config{User: "user", Passwd: "password", Net: "tcp", Addr: "localhost:5555", DBName: "dbname", Loc: time.UTC, MaxAllowedPacket: defaultMaxAllowedPacket, Logger: defaultLogger, AllowNativePasswords: true, CheckConnLiveness: true, TLSConfig: "true", TLSVerify: "identity"}, }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
AUTHORS(1 hunks)README.md(1 hunks)dsn.go(4 hunks)dsn_test.go(1 hunks)utils.go(2 hunks)utils_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: shogo82148
Repo: go-sql-driver/mysql PR: 1576
File: CHANGELOG.md:1-6
Timestamp: 2024-07-26T20:55:56.049Z
Learning: PRs #1562 and #1570 in the go-sql-driver/mysql repository collectively address the same underlying issue related to fixing a race condition when a context is canceled, as documented in the CHANGELOG.md for version 1.8.1.
🧬 Code graph analysis (2)
utils.go (1)
dsn.go (1)
Config(37-85)
dsn_test.go (2)
dsn.go (2)
ParseDSN(420-498)Config(37-85)utils.go (2)
RegisterTLSConfig(58-71)DeregisterTLSConfig(74-80)
🪛 ast-grep (0.40.0)
utils.go
[warning] 99-104: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
VerifyPeerCertificate: func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error {
return verifyCACallback(rawCerts, verifiedChains, rootCAs)
},
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
dsn_test.go
[warning] 474-477: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
ServerName: "customServer",
RootCAs: nil, // Use system CA pool for this test
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 577-577: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
dsn.go
[warning] 211-211: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 214-214: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 216-216: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 LanguageTool
README.md
[uncategorized] ~437-~437: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... false ``` tls=true enables TLS / SSL encrypted connection to the server with full cert...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~437-~437: Use a hyphen to join words.
Context: ...lf-signed or invalid certificate (server side) or use preferred to use TLS only...
(QB_NEW_EN_HYPHEN)
[style] ~465-~465: ‘in conjunction with’ might be wordy. Consider a shorter alternative.
Context: ...erification level. This parameter works in conjunction with the tls parameter: - identity: Full...
(EN_WORDINESS_PREMIUM_IN_CONJUNCTION_WITH)
🪛 markdownlint-cli2 (0.18.1)
README.md
459-459: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (2)
AUTHORS (1)
46-46: Author entry looks goodNew contributor is added in the correct alphabetical position; no issues.
dsn_test.go (1)
522-564: LGTM! Excellent backwards compatibility coverage.This test effectively validates that existing DSNs continue to work as expected with the new
tls-verifyparameter defaulting to "identity". The test cases cover all key scenarios includingtls=true,tls=false,tls=skip-verify, andtls=preferred.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (1)
437-471: TLS /tls-verifydocumentation matches implementation; consider minor markdown tweakThe new “TLS Verification Modes” and
tls-verifysections accurately describe howtls,tls=true,tls=<name>, andtls-verify={identity,ca}are wired indsn.go/utils.go(system vs custom CA, VERIFY_IDENTITY vs VERIFY_CA, and no effect forskip-verify/preferred). The examples for?tls=true[&tls-verify=…]and?tls=custom[&tls-verify=…]are consistent with the current behavior.If you care about markdownlint (MD040), you may want to give the
tls-verifyfenced block a language (e.g., ```text) for lines 461–465; other new code fences already do this.dsn_test.go (1)
593-601: Consider usingstrings.Containsfrom the standard library.The
containsStringhelper reimplements functionality already available instrings.Contains. The standard library function is idiomatic and has no meaningful overhead.Apply this diff if you'd like to use the standard library:
+import ( + "strings" + // ... other imports +) -// containsString checks if s contains substr (simple implementation to avoid importing strings) -func containsString(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false -}And update line 588:
- if err != nil && !containsString(err.Error(), expectedMsg) { + if err != nil && !strings.Contains(err.Error(), expectedMsg) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)driver_test.go(1 hunks)dsn.go(4 hunks)dsn_test.go(3 hunks)utils.go(2 hunks)utils_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: shogo82148
Repo: go-sql-driver/mysql PR: 1576
File: CHANGELOG.md:1-6
Timestamp: 2024-07-26T20:55:56.049Z
Learning: PRs #1562 and #1570 in the go-sql-driver/mysql repository collectively address the same underlying issue related to fixing a race condition when a context is canceled, as documented in the CHANGELOG.md for version 1.8.1.
🧬 Code graph analysis (3)
driver_test.go (2)
utils.go (1)
RegisterTLSConfig(58-71)dsn.go (1)
Config(37-85)
dsn_test.go (2)
dsn.go (2)
Config(37-85)ParseDSN(420-498)utils.go (2)
RegisterTLSConfig(58-71)DeregisterTLSConfig(74-80)
utils.go (1)
dsn.go (1)
Config(37-85)
🪛 ast-grep (0.40.0)
dsn.go
[warning] 211-211: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 214-214: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 216-216: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
driver_test.go
[warning] 1528-1530: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
dsn_test.go
[warning] 663-663: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
utils.go
[warning] 104-104: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 LanguageTool
README.md
[uncategorized] ~437-~437: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... false ``` tls=true enables TLS / SSL encrypted connection to the server with full cert...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 markdownlint-cli2 (0.18.1)
README.md
461-461: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (19)
utils.go (3)
13-25: Import ofcrypto/x509is appropriate for CA-only verification logicThe added
crypto/x509import is used solely by the new CA-verification helpers below and is scoped cleanly to TLS verification concerns, with no impact on unrelated utilities.
91-115:createVerifyCAConfigcorrectly layers VERIFY_CA behavior over a basetls.ConfigThis helper follows the Go-recommended pattern for custom verification: it preserves an existing
tls.ConfigviaClone()when provided, or allocates a fresh one, and then overrides only the verification-related fields by settingInsecureSkipVerify = trueand wiringVerifyPeerCertificateto the CA-only callback. That keeps all other TLS tuning (client certs, protocol versions, ciphers, ALPN,ServerName, etc.) intact for custom configs.
117-161:verifyCACallbackimplements CA-only verification correctlyThe callback:
- Rejects empty
rawCertswith a clear error.- Parses the full chain, builds an intermediates pool from non-leaf certs, and verifies the leaf with
x509.Certificate.Verifyusing the provided roots (or system pool whenrootCAsis nil) andExtKeyUsageServerAuth.- Intentionally omits
DNSNamefromVerifyOptions, matching VERIFY_CA semantics (no hostname check) while still enforcing chain, CA trust, and validity period.The error paths and messages are well-structured and are exercised in
utils_test.go.driver_test.go (1)
1518-1533: New TLS tests cover VERIFY_CA and custom-config integrationThe added
runTestsinvocations for:
tls=true&tls-verify=catls=true&tls-verify=identitytls=custom-ca-verify&tls-verify=caafter registering a custom TLS configexercise the new DSN
tls-verifyhandling for both system and custom configs through a real handshake, reusingtlsTestReqto assert TLS is active. This gives good end-to-end coverage on top of the lower-leveldsn/utilstests.dsn.go (4)
52-55:Config.TLSVerifyfield is well-scoped and documentedIntroducing
TLSVerifyas a separate string field (distinct fromTLSConfig/TLS) cleanly models verification mode (identityvsca) without overloading the TLS config name. The comment accurately describes its intended values.
198-233: TLS normalization correctly wirestls-verifyinto system and custom TLS configsWithin
normalize():
- Defaulting
TLSVerifyto"identity"when TLS is being configured and no value is set maintains backward-compatible VERIFY_IDENTITY semantics for existing DSNs.- For
TLSConfig=="true":
TLSVerify=="identity"uses&tls.Config{}(letting Go perform standard CA+hostname verification).TLSVerify=="ca"routes throughcreateVerifyCAConfig(nil, nil), implementing VERIFY_CA with system roots.- For custom
TLSConfignames:
getTLSConfigClonepreserves the registered config, and the"ca"branch wraps it viacreateVerifyCAConfig(cfg.TLS, cfg.TLS.RootCAs), so client certs and other TLS settings are retained while only verification behavior changes.The skip-verify and preferred branches remain unaffected and ignore
TLSVerifyas documented. Overall, this matches the README’s description oftlsvstls-verifyresponsibilities.
392-395:FormatDSNemission oftls-verifyis sensible and backward compatibleOnly emitting
tls-verifywhenTLSVerifyis non-empty and not"identity"avoids bloating DSNs while still persisting explicitcasettings. This maintains stable DSN strings for existing callers who don’t opt into VERIFY_CA.
684-691:tls-verifyDSN parsing is strict and normalizedThe
parseDSNParamscase fortls-verify:
- Lowercases the value and only accepts
"identity"or"ca", returning a clear error for anything else.- Stores the normalized mode in
cfg.TLSVerify, enabling predictable behavior and consistentFormatDSNoutput.This is a good balance between flexibility (case-insensitivity) and safety (rejecting unknown modes rather than silently ignoring them).
utils_test.go (3)
11-24: Additional crypto imports are justified by TLS verification testsThe new crypto/x509, tls, rsa, pkix, rand, and math/big imports are all exercised in the CA/leaf certificate generation and config-preservation tests below, and they’re appropriately confined to the test file.
528-616:TestVerifyCACallbackexercises both error and success paths thoroughlyThis test:
- Confirms that nil and empty
rawCertsare rejected.- Ensures parse failures on malformed cert bytes are surfaced.
- Builds a minimal CA+leaf certificate chain and verifies that
verifyCACallbackaccepts it when the CA is in the provided pool.That gives good coverage of the callback’s core behavior and error reporting.
618-709:TestCreateVerifyCAConfigvalidates VERIFY_CA config construction and base-config preservationThe subtests for
createVerifyCAConfig:
- Check that a config is always returned, with
InsecureSkipVerify=trueandVerifyPeerCertificateset for both system and custom CA pools.- Verify that the callback is wired correctly (returns an error when given nil certs).
- Confirm that wrapping a base
tls.Configpreserves important fields like Min/MaxVersion, cipher suites,ServerName,NextProtos, and client certificates.Together, they reduce the risk of regressions in VERIFY_CA configuration behavior.
dsn_test.go (8)
13-13: LGTM!The
crypto/x509import is appropriately added to support TLS certificate pool operations in the new testTestTLSVerifyPreservesCustomConfig.
84-90: LGTM!The new test DSN entries comprehensively cover both
tls-verifymodes (caandidentity) with system CA, ensuring the DSN parsing and formatting work correctly.
439-482: LGTM!The test comprehensively validates the TLS verification behavior with system CA across three scenarios (ca mode, explicit identity, default identity). The assertions correctly verify:
- CA mode uses
InsecureSkipVerify=truewith aVerifyPeerCertificatecallback and no automaticServerName- Identity mode sets
ServerNameand uses standard verification without the callback
484-535: LGTM!The test properly validates that
tls-verifymodes work with custom TLS configurations and preserve the customServerNamefor SNI in both modes, which is essential for proper TLS handshake.
537-579: LGTM!This test is critical for validating backwards compatibility. It correctly verifies that existing DSN configurations continue to work as expected with
identityas the default verification mode.
581-591: LGTM!The test appropriately validates that invalid
tls-verifyvalues are rejected with a clear error message.
603-653: LGTM!Excellent comprehensive test that ensures custom TLS configuration settings are fully preserved when applying
tls-verify=camode. This validates that the implementation correctly overlays the verification mode without overwriting user-specified TLS options.
655-670: LGTM!The test appropriately validates that reserved TLS configuration keys (case-insensitive) cannot be registered, preventing conflicts with built-in TLS modes. The static analysis warning about missing
MinVersionon line 664 is a false positive, as the config content is irrelevant for testing the key rejection logic.
Implements VERIFY_CA SSL mode to verify certificate authority without hostname verification, matching MySQL client's --ssl-mode=VERIFY_CA. This addresses a long-standing limitation where users needed TLS with CA verification but couldn't use hostname verification due to: - Connecting via IP addresses instead of hostnames - Dynamic IPs or load-balanced MySQL instances - Certificates with SANs that don't match connection strings - Multiple hostnames for the same MySQL instance Adds new DSN parameter with two values: - identity: Full verification (CA + hostname) - default - ca: CA verification only (no hostname check) Works with both system CA pool and custom registered TLS configs: - ?tls=true&tls-verify=ca (system CA) - ?tls=custom&tls-verify=ca (custom CA) This is particularly important for users migrating to MySQL 8.0's caching_sha2_password authentication, which requires encrypted connections by default, making TLS support more critical. Implementation follows Go team's recommended pattern from golang/go issues #21971, #31791, #31792, #35467: using InsecureSkipVerify with custom VerifyPeerCertificate callback that performs CA validation via x509.Certificate.Verify() without hostname checking. Related: go-sql-driver#899 See also: golang/go#31792, golang/go#24151, golang/go#21971, golang/go#28754, golang/go#31791, golang/go#35467
pourtorabehsan
force-pushed
the
feat/support_verify_ca
branch
from
c93c813 to
e612245
Compare
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
|
Why FWIW, MariaDB team said:
|
This is a strange concern/demand for what is clearly a desired feature. There are clearly many people asking for feature parity with the official MySQL client's own behavior (the PR author mentions a few in the PR description). If maintaining feature parity with the official MySQL client and documentation isn't reason enough, it doesn't take a security expert to see that verifying the signing CA is still movement toward better security. From the above MySQL documentation:
There are circumstances (such as large, complex, dynamic environments) where connectivity happens via IPs that aren't necessarily fixed, so being able to at least verify that the cert being presented was issued by a CA that the client trusts is better than no verifications at all. Is it ideal? No. Is it better than no verification at all? Undeniably. IMO, the library should include this PR to add feature parity, though it should also be clear that |
|
@methane Thanks for your comment, I don't argue that Also, regardless of how secure these options are, I don't think this is the driver's responsibility to decide it. The driver's responsibility is to provide the feature parity with the official client. The This is also supported in other languages: Java, Ruby, Rust We like it or not, many people are using this feature already because it's officially supported, having this in the driver just makes it easier. |
3 participants
Description
Adds support for MySQL's VERIFY_CA SSL mode, which verifies the certificate authority without requiring hostname verification. This matches the behavior of MySQL client's
--ssl-mode=VERIFY_CAoption.Motivation
This feature has been requested multiple times across several years in either mysql driver or generally for tls verification:
Why This Is Important
MySQL 8.0 Default Authentication: The
caching_sha2_passwordauthentication plugin (default since MySQL 8.0) requires encrypted connections for initial authentication. This makes TLS support more critical than ever.Real-World Use Cases:
MySQL Client Compatibility: MySQL's official client supports
--ssl-mode=VERIFY_CA, so Go applications should have equivalent functionality.Implementation
Follows the Go team's recommended pattern as explained here:
golang/go#31792
https://go-review.googlesource.com/c/go/+/193620/8/src/crypto/tls/example_test.go
New
tls-verifyDSN ParameterAdded a new
tls-verifyparameter that works orthogonally with the existingtlsparameter:identity- Full verification: CA + hostname (default, backwards compatible)ca- CA verification only, no hostname check (new)The parameter name
tls-verifywas chosen over alternatives liketls-modeto avoid confusion, sincetls=already mimics MySQL's--ssl-mode. This makes it clear that:tls=selects the CA source (system, custom config, or behavior like skip-verify)tls-verify=selects the verification level (CA-only vs full)Usage Examples
System CA with VERIFY_CA:
Custom CA with VERIFY_CA:
Full verification (default):
Checklist