Skip to content
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 48 additions & 2 deletions pkg/metricsservice/utils/tls.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@
}

certMutex := sync.RWMutex{}
// currentPool holds the live CA pool and is replaced (never appended to)
// on every rotation so the pool does not grow unboundedly.
currentPool := certPool
go func() {
log.V(1).Info("starting mTLS certificates monitoring")
for {
Expand All @@ -97,10 +100,18 @@
log.Error(err, "error reading grpc ca certificate")
continue
}
if !certPool.AppendCertsFromPEM(pemClientCA) {
// Rebuild the pool from scratch to prevent monotonic growth.
newPool, _ := x509.SystemCertPool()
if newPool == nil {
newPool = x509.NewCertPool()
}
if !newPool.AppendCertsFromPEM(pemClientCA) {
log.Error(err, "failed to add client CA's certificate")
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 208097f.

continue
}
certMutex.Lock()
currentPool = newPool
certMutex.Unlock()
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 208097f.

log.V(1).Info("grpc ca certificate has been updated")

// Load certificate of the CA who signed client's certificate
Expand Down Expand Up @@ -144,9 +155,44 @@
}
if server {
config.ClientAuth = tls.RequireAndVerifyClientCert
config.ClientCAs = certPool
// GetConfigForClient is called per-connection; injecting currentPool
// here ensures every new handshake picks up the latest CA pool.
config.GetConfigForClient = func(_ *tls.ClientHelloInfo) (*tls.Config, error) {
certMutex.RLock()
defer certMutex.RUnlock()
clone := config.Clone()
clone.ClientCAs = currentPool
// GetConfigForClient overrides GetCertificate; restore it.
clone.GetCertificate = config.GetCertificate
return clone, nil
}
} else {
config.RootCAs = certPool
// For the client side, VerifyPeerCertificate is used to check against
// the latest pool on every connection.
config.InsecureSkipVerify = true //nolint:gosec // verification done in VerifyPeerCertificate

Check failure

Code scanning / CodeQL

Disabled TLS certificate check High

InsecureSkipVerify should not be used in production code.
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
config.VerifyPeerCertificate = func(rawCerts [][]byte, _ [][]*x509.Certificate) error {
certMutex.RLock()
pool := currentPool
certMutex.RUnlock()
certs := make([]*x509.Certificate, 0, len(rawCerts))
for _, rawCert := range rawCerts {
c, err := x509.ParseCertificate(rawCert)
if err != nil {
return err
}
certs = append(certs, c)
}
opts := x509.VerifyOptions{
Roots: pool,
Intermediates: x509.NewCertPool(),
}
for _, c := range certs[1:] {
opts.Intermediates.AddCert(c)
}
_, err := certs[0].Verify(opts)
return err
}
}

return credentials.NewTLS(config), nil
Expand Down
Loading