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

proposal: crypto/tls: dynamically reload root certificate authorities #64796

Open
p0lyn0mial opened this issue Dec 19, 2023 · 3 comments
Open
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@p0lyn0mial
Copy link

p0lyn0mial commented Dec 19, 2023

Proposal Details

The current state of the tls.Config.RootCAs presents a challenge when it comes to dynamically reloading the Root Certificate Authorities. The existing approaches involve either disabling automatic certificate and host verification or limiting functionality to non-proxied requests. This proposal seeks to address this limitation by introducing a mechanism for dynamic reloading of Root CAs.

The current workaround for dynamically updating Root CAs involves using the VerifyPeerCertificate, but it requires disabling automatic certificate and host verification by setting the InsecureSkipVerify field to true. This approach might introduce some security risks if the custom implementation is not correct.

Alternatively, a custom TLS Dialer could be used. However, this approach falls short when dealing with proxied requests, limiting its applicability in scenarios where proxies are used.

To enhance the flexibility of TLS configurations, this issue proposes modifying the behaviour of tls.Config.RootCAs to support dynamic reloading by introducing a function similar to GetClientCertificate.

Note that in the past, a similar request was rejected. Since commenting on that issue is disabled, I decided to create a new issue to see if something has changed. Having such a mechanism in the standard library would help the Kubernetes community address kubernetes/kubernetes#119483.

Update:

It largely depends on how the tls.Config.RootCAs is used internally. Another solution would be to accept an interface instead of *x509.CertPool and allow clients to inject a thread-safe implementation, enabling trust reloading.

@gopherbot gopherbot added this to the Proposal milestone Dec 19, 2023
@seankhliao seankhliao changed the title proposal: dynamically reload Root Certificate Authorities in tls.Config proposal: crypto/tls: dynamically reload root certificate authorities Dec 19, 2023
@seankhliao seankhliao added the Proposal-Crypto Proposal related to crypto packages or other security issues label Dec 19, 2023
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Jan 2, 2024
@seankhliao
Copy link
Member

cc @golang/security

@espadolini
Copy link

The discussion at #22836 mentions that it's possible to replicate the normal certificate verification behavior by using specifying VerifyConnection and setting InsecureSkipVerify, but from what I can tell there's no inherent way for VerifyConnection to have access to the ServerName in the tls.Config in use if it's an IP address (tls.ConnectionState.ServerName is set to the value sent by the client as SNI, but the spec prescribes that IP addresses should not be sent as SNI, and the Go TLS handshake follows that).

Obviously a VerifyConnection callback will have access to the ServerName as specified in the original tls.Config, but the general behavior with TLS clients in the ecosystem seems to be that the calling code is intended to pass a tls.Config with no ServerName set, and the callee will clone the config and set the server name right before initiating the handshake, which will obviously not work with a custom VerifyConnection that only has a reference to the original tls.Config.

@shaj13
Copy link

shaj13 commented Dec 21, 2024

maybe to introduce a new callback in tls.Config that allows for dynamically retrieving the latest CertPool, similar to the existing callback functions.

// GetRootCAs returns  the set of root certificate authorities
// that clients use when verifying server certificates.
// 
// If GetRootCAs is nil, then the RootCAs is used. 
GetRootCAs func() (*x509.CertPool)

The client handshake will work with the proposed changes as follows:

func (c *Config) rootCAs() *x509.CertPool{
	if c.GetRootCAs == nil {
		return c.RootCAs
	}
        
        return c.GetRootCAs()
}
func (c *Conn) verifyServerCertificate(certificates [][]byte) error {
	} else if !c.config.InsecureSkipVerify {
		opts := x509.VerifyOptions{
			Roots:         c.config.rootCAs(),
			CurrentTime:   c.config.time(),
			DNSName:       c.config.ServerName,
			Intermediates: x509.NewCertPool(),
		}

		for _, cert := range certs[1:] {
			opts.Intermediates.AddCert(cert)
		}
		chains, err := certs[0].Verify(opts)
		if err != nil {
			c.sendAlert(alertBadCertificate)
			return &CertificateVerificationError{UnverifiedCertificates: certs, Err: err}
		}

		c.verifiedChains, err = fipsAllowedChains(chains)
		if err != nil {
			c.sendAlert(alertBadCertificate)
			return &CertificateVerificationError{UnverifiedCertificates: certs, Err: err}
		}
	}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Status: Incoming
Development

No branches or pull requests

5 participants