Skip to content
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

client TLS should not set InsecureSkipVerify if need to verify Common Name #53357

Closed
lance6716 opened this issue May 17, 2024 · 0 comments · Fixed by #53358
Closed

client TLS should not set InsecureSkipVerify if need to verify Common Name #53357

lance6716 opened this issue May 17, 2024 · 0 comments · Fixed by #53358
Assignees
Labels
affects-5.4 This bug affects the 5.4.x(LTS) versions. affects-6.1 This bug affects the 6.1.x(LTS) versions. affects-6.5 This bug affects the 6.5.x(LTS) versions. affects-7.1 This bug affects the 7.1.x(LTS) versions. affects-7.5 This bug affects the 7.5.x(LTS) versions. affects-8.1 This bug affects the 8.1.x(LTS) versions. severity/moderate sig/sql-infra SIG: SQL Infra type/bug The issue is confirmed as a bug.

Comments

@lance6716
Copy link
Contributor

lance6716 commented May 17, 2024

in https://github.com/pingcap/tidb/pull/37479/files#r962687063 I always set InsecureSkipVerify and think as the comment of InsecureSkipVerify said, our VerifyPeerCertificate is enough.

// InsecureSkipVerify controls whether a client verifies the server's
// certificate chain and host name. If InsecureSkipVerify is true, crypto/tls
// accepts any certificate presented by the server and any host name in that
// certificate. In this mode, TLS is susceptible to machine-in-the-middle
// attacks unless custom verification is used. This should be used only for
// testing or in combination with VerifyConnection or VerifyPeerCertificate.
InsecureSkipVerify bool

However, the comment of VerifyPeerCertificate said

// VerifyPeerCertificate, if not nil, is called after normal
// certificate verification by either a TLS client or server. It
// receives the raw ASN.1 certificates provided by the peer and also
// any verified chains that normal processing found. If it returns a
// non-nil error, the handshake is aborted and that error results.
//
// If normal verification fails then the handshake will abort before
// considering this callback. If normal verification is disabled (on the
// client when InsecureSkipVerify is set, or on a server when ClientAuth is
// RequestClientCert or RequireAnyClientCert), then this callback will be
// considered but the verifiedChains argument will always be nil. When
// ClientAuth is NoClientCert, this callback is not called on the server.
// rawCerts may be empty on the server if ClientAuth is RequestClientCert or
// VerifyClientCertIfGiven.
//
// This callback is not invoked on resumed connections, as certificates are
// not re-verified on resumption.
//
// verifiedChains and its contents should not be modified.
VerifyPeerCertificate func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error

For the client, if our verify functions need to read verifiedChains, InsecureSkipVerify should be false. We only have WithVerifyCommonName needs to read verifiedChains, and tidb repo does not use this function, so the issue is not found before. And I found this issue when porting WithVerifyCommonName to tiflow repo.

@lance6716 lance6716 added type/bug The issue is confirmed as a bug. severity/moderate affects-5.4 This bug affects the 5.4.x(LTS) versions. affects-6.1 This bug affects the 6.1.x(LTS) versions. affects-6.5 This bug affects the 6.5.x(LTS) versions. affects-7.1 This bug affects the 7.1.x(LTS) versions. affects-7.5 This bug affects the 7.5.x(LTS) versions. affects-8.1 This bug affects the 8.1.x(LTS) versions. labels May 17, 2024
@lance6716 lance6716 changed the title TLS should not set InsecureSkipVerify if need to verify Common Name client TLS should not set InsecureSkipVerify if need to verify Common Name May 17, 2024
@jebter jebter added the sig/sql-infra SIG: SQL Infra label May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-5.4 This bug affects the 5.4.x(LTS) versions. affects-6.1 This bug affects the 6.1.x(LTS) versions. affects-6.5 This bug affects the 6.5.x(LTS) versions. affects-7.1 This bug affects the 7.1.x(LTS) versions. affects-7.5 This bug affects the 7.5.x(LTS) versions. affects-8.1 This bug affects the 8.1.x(LTS) versions. severity/moderate sig/sql-infra SIG: SQL Infra type/bug The issue is confirmed as a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants