Skip to content

radsecproxy, TLS : Make SIGHUP reload client certs and keys #76

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.

Sign up for GitHub

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

Closed
imtiyaz-arista wants to merge 2 commits into from
Closed

radsecproxy, TLS : Make SIGHUP reload client certs and keys #76

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3a94baa
radsecproxy, TLS : Make SIGHUP reload client certs and keys
imtiyaz-arista Jan 7, 2021
1c33989
TLS : recache certificates on SIGHUP. Address review comments
imtiyaz-arista Mar 9, 2021
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
67 changes: 57 additions & 10 deletions tlscommon.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,18 +534,65 @@ void tlsreloadcrls() {
for (entry = hash_first(tlsconfs); entry; entry = hash_next(entry)) {
conf = (struct tls *)entry->data;
#ifdef RADPROT_TLS
if (conf->tlsctx) {
if (conf->tlsexpiry)
conf->tlsexpiry = now.tv_sec + conf->cacheexpiry;
tlsaddcacrl(conf->tlsctx, conf);
}
if (conf->tlsctx) {
/*
* Little tricky situation now. If we try the SSL_CTX_* APIs with the
* original tlsctx and it fails, we might end up with a partially updated
* tlsctx and that does good to know one. As a quick workaround,
* we will create a dummy tlsctx and try out the APIs on it. If it goes fine,
* we will replay the same with the live tlsctx. Ofcourse, we would need to
* obtain a lock before handling the same.
*/
testtlsctx = tlscreatectx(RAD_TLS, conf);
if (!testtlsctx) {
return;
}
SSL_CTX_free(testtlsctx);

pthread_mutex_lock(&conf->lock);
if (!SSL_CTX_use_certificate_chain_file(conf->tlsctx, conf->certfile) ||
!SSL_CTX_use_PrivateKey_file(conf->tlsctx, conf->certkeyfile, SSL_FILETYPE_PEM) ||
!SSL_CTX_check_private_key(conf->tlsctx)) {
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);

@fmauchle fmauchle Jan 8, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@imtiyaz-arista imtiyaz-arista Jan 8, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

conf->tlsctx = NULL;
return;
}

debug(DBG_INFO, "tls: private certificate and keys updated");
if (conf->tlsexpiry)
conf->tlsexpiry = now.tv_sec + conf->cacheexpiry;
tlsaddcacrl(conf->tlsctx, conf);
pthread_mutex_unlock(&conf->lock);
}
#endif
#ifdef RADPROT_DTLS
if (conf->dtlsctx) {
if (conf->dtlsexpiry)
conf->dtlsexpiry = now.tv_sec + conf->cacheexpiry;
tlsaddcacrl(conf->dtlsctx, conf);
}
if (conf->dtlsctx) {
testtlsctx = tlscreatectx(RAD_DTLS, conf);
if (!testtlsctx) {
return;
}
SSL_CTX_free(testtlsctx);

pthread_mutex_lock(&conf->lock);
if (!SSL_CTX_use_certificate_chain_file(conf->dtlsctx, conf->certfile) ||
!SSL_CTX_use_PrivateKey_file(conf->dtlsctx, conf->certkeyfile, SSL_FILETYPE_PEM) ||
!SSL_CTX_check_private_key(conf->dtlsctx)) {
while ((error = ERR_get_error()))
debug(DBG_ERR, "dtls SSL: %s", ERR_error_string(error, NULL));
debug(DBG_ERR, "dtls: Error initialising SSL/TLS in TLS context %s", conf->name);
SSL_CTX_free(conf->dtlsctx);
return;
}
debug(DBG_INFO, "dtls: private certificate and keys updated");

if (conf->dtlsexpiry)
conf->dtlsexpiry = now.tv_sec + conf->cacheexpiry;
tlsaddcacrl(conf->dtlsctx, conf);
pthread_mutex_unlock(&conf->lock);
}
#endif
}
}
Expand Down