radsecproxy, TLS : Make SIGHUP reload client certs and keys#76
radsecproxy, TLS : Make SIGHUP reload client certs and keys#76imtiyaz-arista wants to merge 2 commits intoradsecproxy:masterfrom
Conversation
Today, SIGHUP causes the CA and CRLs to be reloaded. This change makes client certificates and keys to be reloaded as well. This is needed for the case when the certificates have expired.
fmauchle
left a comment
fmauchle
left a comment
There was a problem hiding this comment.
There is lots of duplicate code already present in tlscreatectx. This should be avoided.
Did you check if calling these SSL_CTX functions is threadsave?
tlscommon.c
Outdated
| if (conf->certkeypwd) { | ||
| debug(DBG_INFO, "tls: set default passwd for certkeypwd"); | ||
| SSL_CTX_set_default_passwd_cb_userdata(conf->tlsctx, conf->certkeypwd); | ||
| SSL_CTX_set_default_passwd_cb(conf->tlsctx, pem_passwd_cb); | ||
| } |
There was a problem hiding this comment.
Not much sense in redoing the password callback. It can't have changed.
There was a problem hiding this comment.
Thanks for your comments Fabian. Agree with you, this block is not needed.
| while ((error = ERR_get_error())) | ||
| debug(DBG_ERR, "tls SSL: %s", ERR_error_string(error, NULL)); | ||
| debug(DBG_ERR, "tls: Error initialising SSL/TLS in TLS context %s", conf->name); | ||
| SSL_CTX_free(conf->tlsctx); |
There was a problem hiding this comment.
This is extremely dangerous. tlsctx is still refered to, and the next time a connection is opened, this will get accessed; likely resulting in a segfault.
There was a problem hiding this comment.
My bad. You are right. But this is a tricky situation there. I don't know the internal details of the three APIs, SSL_CTX_use_certificate_chain_file, SSL_CTX_use_certificate_chain_file and SSL_CTX_check_private_key. Now, in the above expression, if SSL_CTX_use_certificate_chain_file() succeeds, it might change something about conf->tlsctx now assuming SSL_CTX_use_PrivateKey_file() fails for some reason, it will be too late to just bail out with a partially modified conf->tlsctx. We need some way to ensure we update all the three APIs or None.
I tried to find about the SSL_CTX being threadsafe or not and could not find anything concrete to suggest either way. Some literature did suggest that there might be race issues between the SSL_CTX_* APIs and SSL_new() and looking at the code closely, We are using server->conf->tlsconf->lock to protect SSL_new(). Perhaps, we can protect the SSL_CTXs with this lock and that will take care of the race. Since we already are updating CA and CRLs, it seemed like a straightforward thing to reload other certs and keys. There is another way of doing this but that is more effort (from some discussion on internet):- One way to It is possible to call SSL_CTX_free() on the old context even while worker thread: update thread: |
|
If SSL_CTX is not explicitly declared threadsafe, I would consider it not to be and external locking is required. This is already done in the tls and dtls code using the As for the idea of creating a new SSL_CTX, I think this would be quite a good solution. It would also allow error handling and aborting the certificate reload if anything goes wrong (and continue to use the old one instead of breaking the proxy or even crashing it). |
|
radsecproxy now completely reloads the TLS context on SIGHUP, which also re-reads clielnt/server certificates and keys. |
2 participants
Today, SIGHUP causes the CA and CRLs to be reloaded.
This change makes client certificates and keys to be reloaded as well.
This is needed for the case when the client certificates have expired.
PS : client implies, radsecproxy and server being the radius server itself.