-
Notifications
You must be signed in to change notification settings - Fork 72
Make driver fail certificate validation for untrusted chains (with opt-out for backward compatibility) #665
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
base: master
Are you sure you want to change the base?
Changes from 3 commits
da06abf
897cc7a
1e98d89
33d6855
4a9af3e
e09c4d6
5b4f2be
8029281
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -631,5 +631,77 @@ func setupTLSConfig(sslOpts *SslOptions) (*tls.Config, error) { | |
| tlsConfig.Certificates = append(tlsConfig.Certificates, mycert) | ||
| } | ||
|
|
||
| // Add strict certificate chain validation | ||
| // This ensures that the entire certificate chain is properly validated, | ||
| // not just that one intermediate certificate is trusted. | ||
| if !tlsConfig.InsecureSkipVerify { | ||
| tlsConfig.VerifyPeerCertificate = strictVerifyPeerCertificate(tlsConfig.RootCAs) | ||
| } | ||
|
|
||
| return tlsConfig, nil | ||
| } | ||
|
|
||
| // strictVerifyPeerCertificate returns a VerifyPeerCertificate callback that performs | ||
| // strict certificate chain validation. Unlike Go's default behavior which accepts | ||
| // a chain if any intermediate certificate is trusted, this ensures the entire chain | ||
| // up to a trusted root certificate is valid. | ||
| func strictVerifyPeerCertificate(rootCAs *x509.CertPool) func([][]byte, [][]*x509.Certificate) error { | ||
| return func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error { | ||
| if len(rawCerts) == 0 { | ||
| return errors.New("no certificates provided") | ||
| } | ||
|
|
||
| // Parse the leaf certificate | ||
| cert, err := x509.ParseCertificate(rawCerts[0]) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to parse certificate: %v", err) | ||
| } | ||
|
|
||
| // Build the intermediate certificate pool from the provided chain | ||
| intermediates := x509.NewCertPool() | ||
| for i := 1; i < len(rawCerts); i++ { | ||
| intermediateCert, err := x509.ParseCertificate(rawCerts[i]) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to parse intermediate certificate: %v", err) | ||
| } | ||
| intermediates.AddCert(intermediateCert) | ||
| } | ||
|
|
||
| // Verify the certificate chain | ||
| opts := x509.VerifyOptions{ | ||
| Roots: rootCAs, | ||
| Intermediates: intermediates, | ||
| // We're not verifying the hostname here as that's handled separately | ||
| // by the TLS library if ServerName is set | ||
| KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, | ||
| } | ||
|
|
||
| chains, err := cert.Verify(opts) | ||
| if err != nil { | ||
| return fmt.Errorf("certificate verification failed: %v", err) | ||
| } | ||
|
|
||
| // Ensure that the chain terminates at a trusted root certificate | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Drop this comment
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed in commit 5b4f2be. |
||
| if len(chains) == 0 { | ||
| return errors.New("no valid certificate chains found") | ||
| } | ||
|
|
||
| // Verify that each chain ends with a certificate that's in our RootCAs pool | ||
| // This ensures we don't accept chains that terminate at an intermediate cert | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Drop this comment
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed in commit 5b4f2be. |
||
| for _, chain := range chains { | ||
| if len(chain) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| // The last certificate in the chain should be the root | ||
| rootCert := chain[len(chain)-1] | ||
|
|
||
| // Verify this root is self-signed (characteristic of root CAs) | ||
| if err := rootCert.CheckSignatureFrom(rootCert); err != nil { | ||
| return fmt.Errorf("certificate chain does not terminate at a self-signed root: %v", err) | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does not make any sense, it should check all the certificates in the chain if they signed with root certificate
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Simplified in commit 5b4f2be. Removed the self-signed check since cert.Verify() already validates the entire chain properly. The function now relies on cert.Verify() which validates that all certificates in the chain are properly signed, ultimately by a certificate in the RootCAs pool.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one does not make any sense, we need to to check if every certificate in the chain is trusted by root certificate.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented in commit 8029281. Added explicit verification that every certificate in the chain is signed by its parent, all the way to a self-signed root certificate. This ensures intermediate CAs cannot be trusted as roots. |
||
| } | ||
|
|
||
| return nil | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,288 @@ | ||
| //go:build unit | ||
| // +build unit | ||
|
|
||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package gocql | ||
|
|
||
| import ( | ||
| "crypto/ecdsa" | ||
| "crypto/elliptic" | ||
| "crypto/rand" | ||
| "crypto/tls" | ||
| "crypto/x509" | ||
| "crypto/x509/pkix" | ||
| "encoding/pem" | ||
| "math/big" | ||
| "testing" | ||
| "time" | ||
| ) | ||
|
|
||
| // generateCertificate creates a test certificate | ||
| func generateCertificate(isCA bool, parent *x509.Certificate, parentKey *ecdsa.PrivateKey) (*x509.Certificate, *ecdsa.PrivateKey, error) { | ||
| privateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| serialNumber, err := rand.Int(rand.Reader, new(big.Int).Lsh(big.NewInt(1), 128)) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| template := &x509.Certificate{ | ||
| SerialNumber: serialNumber, | ||
| Subject: pkix.Name{ | ||
| Organization: []string{"Test Org"}, | ||
| CommonName: "Test Certificate", | ||
| }, | ||
| NotBefore: time.Now().Add(-1 * time.Hour), | ||
| NotAfter: time.Now().Add(24 * time.Hour), | ||
| KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature, | ||
| ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, | ||
| BasicConstraintsValid: true, | ||
| IsCA: isCA, | ||
| } | ||
|
|
||
| if isCA { | ||
| template.KeyUsage |= x509.KeyUsageCertSign | ||
| } | ||
|
|
||
| // If parent is nil, this is a self-signed certificate | ||
| signingCert := template | ||
| signingKey := privateKey | ||
| if parent != nil { | ||
| signingCert = parent | ||
| signingKey = parentKey | ||
| } | ||
|
|
||
| certDER, err := x509.CreateCertificate(rand.Reader, template, signingCert, &privateKey.PublicKey, signingKey) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| cert, err := x509.ParseCertificate(certDER) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| return cert, privateKey, nil | ||
| } | ||
|
|
||
| func TestStrictVerifyPeerCertificate(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // Generate a valid certificate chain: Root CA -> Intermediate CA -> Leaf | ||
| rootCA, rootKey, err := generateCertificate(true, nil, nil) | ||
| if err != nil { | ||
| t.Fatalf("failed to generate root CA: %v", err) | ||
| } | ||
|
|
||
| intermediateCA, intermediateKey, err := generateCertificate(true, rootCA, rootKey) | ||
| if err != nil { | ||
| t.Fatalf("failed to generate intermediate CA: %v", err) | ||
| } | ||
|
|
||
| leafCert, _, err := generateCertificate(false, intermediateCA, intermediateKey) | ||
| if err != nil { | ||
| t.Fatalf("failed to generate leaf certificate: %v", err) | ||
| } | ||
|
|
||
| // Create a trusted root pool with the root CA | ||
| rootPool := x509.NewCertPool() | ||
| rootPool.AddCert(rootCA) | ||
|
|
||
| t.Run("valid certificate chain", func(t *testing.T) { | ||
| verifyFunc := strictVerifyPeerCertificate(rootPool) | ||
|
|
||
| // Prepare the certificate chain as it would be presented during TLS handshake | ||
| rawCerts := [][]byte{ | ||
| leafCert.Raw, | ||
| intermediateCA.Raw, | ||
| } | ||
|
|
||
| err := verifyFunc(rawCerts, nil) | ||
| if err != nil { | ||
| t.Errorf("expected valid chain to pass verification, got error: %v", err) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("empty certificate chain", func(t *testing.T) { | ||
| verifyFunc := strictVerifyPeerCertificate(rootPool) | ||
|
|
||
| err := verifyFunc([][]byte{}, nil) | ||
| if err == nil { | ||
| t.Error("expected error for empty certificate chain") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("chain with only intermediate CA in root pool", func(t *testing.T) { | ||
| // Create a pool with only the intermediate CA (not the root) | ||
| intermediatePool := x509.NewCertPool() | ||
| intermediatePool.AddCert(intermediateCA) | ||
|
|
||
| verifyFunc := strictVerifyPeerCertificate(intermediatePool) | ||
|
|
||
| rawCerts := [][]byte{ | ||
| leafCert.Raw, | ||
| intermediateCA.Raw, | ||
| } | ||
|
|
||
| // This should fail because the intermediate CA is not self-signed | ||
| // and thus doesn't qualify as a trusted root | ||
| err := verifyFunc(rawCerts, nil) | ||
| if err == nil { | ||
| t.Error("expected error when intermediate CA is in root pool but chain doesn't terminate at self-signed root") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("self-signed CA certificate", func(t *testing.T) { | ||
| // Generate a self-signed CA certificate | ||
| selfSigned, _, err := generateCertificate(true, nil, nil) | ||
| if err != nil { | ||
| t.Fatalf("failed to generate self-signed certificate: %v", err) | ||
| } | ||
|
|
||
| // Add it to the root pool | ||
| selfSignedPool := x509.NewCertPool() | ||
| selfSignedPool.AddCert(selfSigned) | ||
|
|
||
| verifyFunc := strictVerifyPeerCertificate(selfSignedPool) | ||
|
|
||
| rawCerts := [][]byte{ | ||
| selfSigned.Raw, | ||
| } | ||
|
|
||
| err = verifyFunc(rawCerts, nil) | ||
| if err != nil { | ||
| t.Errorf("expected self-signed CA certificate to pass verification when in root pool, got error: %v", err) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("untrusted root", func(t *testing.T) { | ||
| // Generate a different root CA that's not in our trust pool | ||
| untrustedRoot, untrustedRootKey, err := generateCertificate(true, nil, nil) | ||
| if err != nil { | ||
| t.Fatalf("failed to generate untrusted root CA: %v", err) | ||
| } | ||
|
|
||
| untrustedIntermediate, untrustedIntermediateKey, err := generateCertificate(true, untrustedRoot, untrustedRootKey) | ||
| if err != nil { | ||
| t.Fatalf("failed to generate untrusted intermediate CA: %v", err) | ||
| } | ||
|
|
||
| untrustedLeaf, _, err := generateCertificate(false, untrustedIntermediate, untrustedIntermediateKey) | ||
| if err != nil { | ||
| t.Fatalf("failed to generate untrusted leaf certificate: %v", err) | ||
| } | ||
|
|
||
| verifyFunc := strictVerifyPeerCertificate(rootPool) | ||
|
|
||
| rawCerts := [][]byte{ | ||
| untrustedLeaf.Raw, | ||
| untrustedIntermediate.Raw, | ||
| } | ||
|
|
||
| err = verifyFunc(rawCerts, nil) | ||
| if err == nil { | ||
| t.Error("expected error for certificate chain with untrusted root") | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestSetupTLSConfigStrictValidation(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| t.Run("VerifyPeerCertificate set when verification enabled", func(t *testing.T) { | ||
| opts := &SslOptions{ | ||
| EnableHostVerification: true, | ||
| } | ||
|
|
||
| tlsConfig, err := setupTLSConfig(opts) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
|
|
||
| if tlsConfig.InsecureSkipVerify { | ||
| t.Error("expected InsecureSkipVerify to be false") | ||
| } | ||
|
|
||
| if tlsConfig.VerifyPeerCertificate == nil { | ||
| t.Error("expected VerifyPeerCertificate to be set when verification is enabled") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("VerifyPeerCertificate not set when verification disabled", func(t *testing.T) { | ||
| opts := &SslOptions{ | ||
| EnableHostVerification: false, | ||
| } | ||
|
|
||
| tlsConfig, err := setupTLSConfig(opts) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
|
|
||
| if !tlsConfig.InsecureSkipVerify { | ||
| t.Error("expected InsecureSkipVerify to be true") | ||
| } | ||
|
|
||
| if tlsConfig.VerifyPeerCertificate != nil { | ||
| t.Error("expected VerifyPeerCertificate to not be set when verification is disabled") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("VerifyPeerCertificate set with custom tls.Config", func(t *testing.T) { | ||
| opts := &SslOptions{ | ||
| Config: &tls.Config{ | ||
| InsecureSkipVerify: false, | ||
| }, | ||
| } | ||
|
|
||
| tlsConfig, err := setupTLSConfig(opts) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
|
|
||
| if tlsConfig.InsecureSkipVerify { | ||
| t.Error("expected InsecureSkipVerify to be false") | ||
| } | ||
|
|
||
| if tlsConfig.VerifyPeerCertificate == nil { | ||
| t.Error("expected VerifyPeerCertificate to be set") | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func pemEncodeCert(cert *x509.Certificate) []byte { | ||
| return pem.EncodeToMemory(&pem.Block{ | ||
| Type: "CERTIFICATE", | ||
| Bytes: cert.Raw, | ||
| }) | ||
| } | ||
|
|
||
| func pemEncodeKey(key *ecdsa.PrivateKey) ([]byte, error) { | ||
| keyBytes, err := x509.MarshalECPrivateKey(key) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return pem.EncodeToMemory(&pem.Block{ | ||
| Type: "EC PRIVATE KEY", | ||
| Bytes: keyBytes, | ||
| }), nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot - is there any information we can add here, which certificate failed verification, for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enhanced in commit 8029281. Error messages now include the certificate subject and issuer information, e.g.,
certificate verification failed for subject="CN=example.com,O=Example" issuer="CN=Intermediate CA"