You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2022/12/03 08:29:36 UTC
[GitHub] [trafficserver] serrislew opened a new pull request, #9230: Fail out when intermediate certificate chain fails to load
serrislew opened a new pull request, #9230:
URL: https://github.com/apache/trafficserver/pull/9230
ATS should immediately fail out when intermediate certificate chains fail to load. Currently only a debug log is included when this fails. It originally failed out (by calling `goto fail`) but was missed during refactoring in #853
- This follows similar behavior when `SSL_CTX_add_extra_chain_cert_file` is called, which fails out when returning an error
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #9230: Fail out when intermediate certificate chain fails to load
Posted by GitBox <gi...@apache.org>.
zwoop commented on PR #9230:
URL: https://github.com/apache/trafficserver/pull/9230#issuecomment-1344776555
Cherry-picked to v9.2.x
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] serrislew merged pull request #9230: Fail out when intermediate certificate chain fails to load
Posted by GitBox <gi...@apache.org>.
serrislew merged PR #9230:
URL: https://github.com/apache/trafficserver/pull/9230
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] serrislew commented on a diff in pull request #9230: Fail out when intermediate certificate chain fails to load
Posted by GitBox <gi...@apache.org>.
serrislew commented on code in PR #9230:
URL: https://github.com/apache/trafficserver/pull/9230#discussion_r1042539848
##########
iocore/net/SSLUtils.cc:
##########
@@ -2445,6 +2445,8 @@ SSLMultiCertConfigLoader::load_certs(SSL_CTX *ctx, const std::vector<std::string
// Load up any additional chain certificates
if (!SSL_CTX_add_extra_chain_cert_bio(ctx, bio.get())) {
Debug("ssl", "couldn't add chain to %p", ctx);
+ SSLError("failed to load intermediate certificate chain from %s", cert_names_list[i].c_str());
+ return false;
Review Comment:
This error handling replicates the behavior of all other errors (i.e. `secret_data.empty()` and when cert fails to load certificate chain). Similar to those, it should exit out of that function immediately
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] serrislew commented on pull request #9230: Fail out when intermediate certificate chain fails to load
Posted by GitBox <gi...@apache.org>.
serrislew commented on PR #9230:
URL: https://github.com/apache/trafficserver/pull/9230#issuecomment-1343033672
@shinrich I agree, I think the error handling for all of those errors should be done in a separate effort/PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #9230: Fail out when intermediate certificate chain fails to load
Posted by GitBox <gi...@apache.org>.
ywkaras commented on code in PR #9230:
URL: https://github.com/apache/trafficserver/pull/9230#discussion_r1041626760
##########
iocore/net/SSLUtils.cc:
##########
@@ -2445,6 +2445,8 @@ SSLMultiCertConfigLoader::load_certs(SSL_CTX *ctx, const std::vector<std::string
// Load up any additional chain certificates
if (!SSL_CTX_add_extra_chain_cert_bio(ctx, bio.get())) {
Debug("ssl", "couldn't add chain to %p", ctx);
+ SSLError("failed to load intermediate certificate chain from %s", cert_names_list[i].c_str());
+ return false;
Review Comment:
Clearly this should be an Error. But, if you look at the call stack this function is at the bottom of, it's not clear whether or not it's better to return false here. I assume none of these errors cause Fatal() calls because of the reload case. TS code tends avoid exiting on a reload error. Without necessarily considering it may be worse to keep running in a corrupt state.
Unfortunately, in industrial source bases, even those relied on by established, successful companies, it's risky to assume that something sensible will be done with an error return from a function. https://www.youtube.com/watch?v=lt-udg9zQSE
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #9230: Fail out when intermediate certificate chain fails to load
Posted by GitBox <gi...@apache.org>.
ywkaras commented on code in PR #9230:
URL: https://github.com/apache/trafficserver/pull/9230#discussion_r1042544524
##########
iocore/net/SSLUtils.cc:
##########
@@ -2445,6 +2445,8 @@ SSLMultiCertConfigLoader::load_certs(SSL_CTX *ctx, const std::vector<std::string
// Load up any additional chain certificates
if (!SSL_CTX_add_extra_chain_cert_bio(ctx, bio.get())) {
Debug("ssl", "couldn't add chain to %p", ctx);
+ SSLError("failed to load intermediate certificate chain from %s", cert_names_list[i].c_str());
+ return false;
Review Comment:
Have you seen this happen in production?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #9230: Fail out when intermediate certificate chain fails to load
Posted by GitBox <gi...@apache.org>.
ywkaras commented on code in PR #9230:
URL: https://github.com/apache/trafficserver/pull/9230#discussion_r1042739995
##########
iocore/net/SSLUtils.cc:
##########
@@ -2445,6 +2445,8 @@ SSLMultiCertConfigLoader::load_certs(SSL_CTX *ctx, const std::vector<std::string
// Load up any additional chain certificates
if (!SSL_CTX_add_extra_chain_cert_bio(ctx, bio.get())) {
Debug("ssl", "couldn't add chain to %p", ctx);
+ SSLError("failed to load intermediate certificate chain from %s", cert_names_list[i].c_str());
+ return false;
Review Comment:
Hopefully someone who knows this code better than I do will take a quick look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #9230: Fail out when intermediate certificate chain fails to load
Posted by GitBox <gi...@apache.org>.
ywkaras commented on code in PR #9230:
URL: https://github.com/apache/trafficserver/pull/9230#discussion_r1041626760
##########
iocore/net/SSLUtils.cc:
##########
@@ -2445,6 +2445,8 @@ SSLMultiCertConfigLoader::load_certs(SSL_CTX *ctx, const std::vector<std::string
// Load up any additional chain certificates
if (!SSL_CTX_add_extra_chain_cert_bio(ctx, bio.get())) {
Debug("ssl", "couldn't add chain to %p", ctx);
+ SSLError("failed to load intermediate certificate chain from %s", cert_names_list[i].c_str());
+ return false;
Review Comment:
Clearly this should be an Error. But, if you look at the call stack this function is at the bottom of, it's not clear whether or not it better to return false here. I assume none of these errors cause Fatal() calls because of the reload case. TS code tends avoid exiting on a reload error. Without necessarily considering it may be worse to keep running in a corrupt state.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] serrislew commented on a diff in pull request #9230: Fail out when intermediate certificate chain fails to load
Posted by GitBox <gi...@apache.org>.
serrislew commented on code in PR #9230:
URL: https://github.com/apache/trafficserver/pull/9230#discussion_r1042582322
##########
iocore/net/SSLUtils.cc:
##########
@@ -2445,6 +2445,8 @@ SSLMultiCertConfigLoader::load_certs(SSL_CTX *ctx, const std::vector<std::string
// Load up any additional chain certificates
if (!SSL_CTX_add_extra_chain_cert_bio(ctx, bio.get())) {
Debug("ssl", "couldn't add chain to %p", ctx);
+ SSLError("failed to load intermediate certificate chain from %s", cert_names_list[i].c_str());
+ return false;
Review Comment:
No, not on this specific error. This was the only error case that didn't add a SSLError and exit with returning false
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org