-
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 6 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 |
|---|---|---|
|
|
@@ -456,7 +456,7 @@ func (cfg *ClusterConfig) ValidateAndInitSSL() error { | |
| if cfg.SslOpts == nil { | ||
| return nil | ||
| } | ||
| actualTLSConfig, err := setupTLSConfig(cfg.SslOpts) | ||
| actualTLSConfig, err := setupTLSConfig(cfg.SslOpts, cfg.logger()) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to initialize ssl configuration: %s", err.Error()) | ||
| } | ||
|
|
@@ -583,7 +583,7 @@ var ( | |
| ErrHostQueryFailed = errors.New("unable to populate Hosts") | ||
| ) | ||
|
|
||
| func setupTLSConfig(sslOpts *SslOptions) (*tls.Config, error) { | ||
| func setupTLSConfig(sslOpts *SslOptions, logger StdLogger) (*tls.Config, error) { | ||
| // Config.InsecureSkipVerify | EnableHostVerification | Result | ||
| // Config is nil | true | verify host | ||
| // Config is nil | false | do not verify host | ||
|
|
@@ -631,5 +631,85 @@ func setupTLSConfig(sslOpts *SslOptions) (*tls.Config, error) { | |
| tlsConfig.Certificates = append(tlsConfig.Certificates, mycert) | ||
| } | ||
|
|
||
| // Emit deprecation warning if the option is used | ||
| if sslOpts.DisableStrictCertificateValidation { | ||
| if logger != nil { | ||
| logger.Println("gocql: WARNING - DisableStrictCertificateValidation is deprecated and will be removed in a future version. " + | ||
| "Please ensure your certificate chains are properly configured to work with strict validation.") | ||
| } | ||
| } | ||
|
|
||
| // Add strict certificate chain validation unless explicitly disabled | ||
| // This ensures that the entire certificate chain is properly validated, | ||
| // not just that one intermediate certificate is trusted. | ||
| if !tlsConfig.InsecureSkipVerify && !sslOpts.DisableStrictCertificateValidation { | ||
| 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 | ||
| } | ||
| } | ||
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"