Open wking opened 4 years ago
CC @golang/security.
Have a scenario where I am running with a set of internal CAs that can change (updates, removes, etc). These are not system CAs rather instance-specific CAs. GetClientCertificate
and GetCertificate
are great for updating client/server certs on the fly w/o starting/stopping listeners. However, there is no equivalent for RootCAs
.
GetConfigForClient
is the closest thing but at that point access to the original tls.Config
isn't provided and a whole-cloth tls.Config
needs to be returned. The way the code I am dealing with works, the tls.Config
may have been modified or configured and I can not lose those changes.
Adding a function field that can be set (GetRootCas
) would be very nice.
GetConfigForClient
is the closest thing but at that point access to the originaltls.Config
isn't provided and a whole-clothtls.Config
needs to be returned. The way the code I am dealing with works, thetls.Config
may have been modified or configured and I can not lose those changes.
Remember that you can't modify most of tls.Config after it's in use.
Anyway, you can make GetConfigForClient a closure that has access to the lexical scope in which the tls.Config is defined, so it can access it and Clone it.
Is the performance motivation for not fully reloading the CertPool still there since we redid the certificate parser? It should be significantly faster now.
Anyway, you can make GetConfigForClient a closure that has access to the lexical scope in which the tls.Config is defined, so it can access it and Clone it.
A closure would work for accessing the tls.Config
.
Is the performance motivation for not fully reloading the CertPool still there since we redid the certificate parser? It should be significantly faster now.
Performance isn't motivating me. My motivation is to avoid disconnecting clients that have persistent connections. As far as I knew, the only way to provide a new x509.CertPool
was to cycle the listener.
I faced a similar problem but not about CA but for server certificates. When the LE certbot renews a certificate I need to reload it without restarting. This happens once in 60 days so I just going to make a timer job that will run once per day and check if the cert file was changed. So I don't even need any locks for this. I also don't need for file watchers like inotify.
I can't just update the TlsConfig.Certificates
simply because internally it's cloned on a server start and any changes are ignored. In the ServeTLS()
function in src/net/http/server.go:3113 we have cloneTLSConfig()
call. I didn't get why it's needed.
I just wrote an util function ListenAndServeTLS
that works same as http.ListenAndServeTLS()
but doesn't make internally the cloneTLSConfig()
:
func ListenAndServeTLS(srv *http.Server, certFile, keyFile string) error {
cert, certErr := tls.LoadX509KeyPair(certFile, keyFile)
if certErr != nil {
return certErr
}
tlsCfg := &tls.Config{
NextProtos: []string{"h2", "http/1.1"},
Certificates: []tls.Certificate{cert},
}
addr := srv.Addr
if addr == "" {
addr = ":https"
}
ln, err := net.Listen("tcp", addr)
if err != nil {
return err
}
defer ln.Close()
tlsListener := tls.NewListener(ln, tlsCfg)
servErr := srv.Serve(tlsListener)
return servErr
}
I tested and added new certs on the fly and it worked well. I had to omit some logic from the original ListenAndServeTLS
like ability to disable the h2
protocol but that's not critical.
The current recommended workaround is to use TlsConfig.GetCertificate
hook but I really just will reimplement the same logic that already works inside with all parsing X509 (quite a heavy operation BTW) and matching by all DNSNames
.
So while I have a working and simple workaround it looks like I violated some contract and can't be sure that it will continue to work in future.
So could the cloneTLSConfig()
be removed? And clearly allowed to be modified during execution.
Then we can add Certificate
, RootCAs
and maybe modify other fields.
Otherwise it looks like may be needed many of other hooks.
@andrewpmartinez I am facing the same issue. If possible can you share a github gist on how you resolved your issue?
@andrewpmartinez I am facing the same issue. If possible can you share a github gist on how you resolved your issue?
Here is an example. This isn't meant to run as is, the ReloadServerCert()
function doesn't attempt to load anything. However, you should be able to take it from here.
In my solution, we have a backing library for loading x509 certificates/private keys from file/inline PEM/hardware (HSMs, etc.) that wraps this logic up nicely and doesn't use globals. Instead, we have an instance of a struct that we can call .Reload()
or be configured to watch for file system changes. I stripped this example down to make it clear how this works.
var serverCerts []*tls.Certificate
var certLock sync.RWMutex
func ReloadServerCert() {
certLock.Lock()
defer certLock.Unlock()
var newCerts []*tls.Certificate
//create tls.Cert's here, like loading them from a file system
serverCerts = newCerts
}
func newTlsConfig() *tls.Config {
return &tls.Config{
GetCertificate: GetServerCertificate,
//do not set "Certificates" field so that GetServerCertificate is always called
}
}
// GetServerCertificate allows us to support SNI if we want and to return any reloaded server certificates
func GetServerCertificate(hello *tls.ClientHelloInfo) (*tls.Certificate, error) {
certLock.RLock()
defer certLock.RUnlock()
// this block can simply always return a single tls.Certificate if SNI isn't needed
for _, cert := range serverCerts {
if err := hello.SupportsCertificate(cert); err == nil {
return cert, nil
}
}
return nil, errors.New("no server certificate found")
}
@andrewpmartinez Thanks for replying promptly. Apologies for not clarifying earlier, the code gist I was looking for was for the issue you mentioned here: https://github.com/golang/go/issues/35887#issuecomment-943615578
I have a client where the ca bundle are changing in background and I need to trust the new RootCAs
on client that is already running. The motivation is similar to your which is to avoid disconnecting clients that have persistent connections.
@FiloSottile Sorry if I'm abusing this issue but i think it is also related to https://github.com/golang/go/issues/22836
It looks like Kubernetes client-go's library has hit this limitation as well. Just checking in if something has changed since your last comment that would be in favour of adding a callback function for reloading tls.Config.RootCAs
on the client side? Thanks!
From #24254:
One use case is long-running processes that now have to jump through hoops to update their view of the system cert pool (e.g. openshift/cloud-credential-operator#113). On Unix, the loading logic is expensive, traversing multiple directories. But for processes who know they can load certs from a single file, it would be nice to have a way to reload if the backing file had changed but not otherwise. For example, something like:
Obviously the "don't bother
Stat
ing again" time could be configurable if that seemed important. Or maybe checking the current time is about as expensive as running theStat
, so we should just callStat
on everycontains
. Or maybe there would be no auto-refresh incontains
, butCertPool
would become aninterface
:In which case there could be a from-file
CertPool
implementation with aRefresh
method to trigger theStat
check and possible reload. Or something ;). But having something in the stdlib that could be dropped intotls.Config.RootCAs
to get efficient reloads without the caller having to babysit theCertPool
would be great. Thoughts?