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 2020/11/06 17:47:56 UTC
[GitHub] [trafficserver] shinrich opened a new pull request #7313: Make reloading client certificate configuration more reliable
shinrich opened a new pull request #7313:
URL: https://github.com/apache/trafficserver/pull/7313
Addresses configuration reload inconsistencies by adding a coordinating update class to ensure that the SSLConfig which owns the client cert storage is always reloaded before the SNIConfig which processes the sni.yaml file.
This patch addressed the issue seen by @thebadpete. This fix is also part of the larger cert reloading PR #6609
This closes #7312
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich merged pull request #7313: Make reloading client certificate configuration more reliable
Posted by GitBox <gi...@apache.org>.
shinrich merged pull request #7313:
URL: https://github.com/apache/trafficserver/pull/7313
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7313: Make reloading client certificate configuration more reliable
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7313:
URL: https://github.com/apache/trafficserver/pull/7313#discussion_r520239591
##########
File path: iocore/net/SSLConfig.cc
##########
@@ -43,6 +43,7 @@
#include "P_Net.h"
#include "P_SSLClientUtils.h"
#include "P_SSLCertLookup.h"
+#include "P_SSLSNI.h"
Review comment:
I assume this is for `SNIConfig::reconfigure();`. In this way, SSLConfig is going to be another SSLUtil. Let's separate out SSLClientCoordinator.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7313: Make reloading client certificate configuration more reliable
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7313:
URL: https://github.com/apache/trafficserver/pull/7313#issuecomment-732379266
pushed new commit to address @maskit's issues.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7313: Make reloading client certificate configuration more reliable
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7313:
URL: https://github.com/apache/trafficserver/pull/7313#discussion_r519510875
##########
File path: iocore/net/SSLConfig.cc
##########
@@ -426,20 +427,31 @@ SSLConfigParams::getClientSSL_CTX() const
return client_ctx;
}
+void
+SSLClientCoordinator::reconfigure()
+{
+ // The SSLConfig must have its configuration loaded before the SNIConfig.
+ // The SSLConfig owns the client cert context storage and the SNIConfig will load
+ // into it.
+ SSLConfig::reconfigure();
+ SNIConfig::reconfigure();
+}
+
void
SSLConfig::startup()
{
- sslConfigUpdate.reset(new ConfigUpdateHandler<SSLConfig>());
- sslConfigUpdate->attach("proxy.config.ssl.client.cert.path");
- sslConfigUpdate->attach("proxy.config.ssl.client.cert.filename");
- sslConfigUpdate->attach("proxy.config.ssl.client.private_key.path");
- sslConfigUpdate->attach("proxy.config.ssl.client.private_key.filename");
+ sslClientUpdate.reset(new ConfigUpdateHandler<SSLClientCoordinator>());
Review comment:
Why don't you have this line in `SSLClientCoordinator::startup()` ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7313: Make reloading client certificate configuration more reliable
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7313:
URL: https://github.com/apache/trafficserver/pull/7313#issuecomment-724405189
There are places that only call SSLConfig::startup(), and those would cause SNIConfig::reconfigure() without its startup(),
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7313: Make reloading client certificate configuration more reliable
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7313:
URL: https://github.com/apache/trafficserver/pull/7313#issuecomment-732427512
I think the last clang-analyzer failed due to a problem with the server, e.g. out of disk or something. Seeing this at the end of the job
```
Error in tempfile() using template /CA/clang-analyzer/github/7313/2020-11-23-123120-12975-1/temp_buf_XXXXXX: Parent directory (/CA/clang-analyzer/github/7313/2020-11-23-123120-12975-1/) does not exist at /opt/llvm/bin/../libexec/ccc-analyzer line 34.
make[2]: *** [healthchecks/healthchecks.lo] Error 1
make[2]: *** Waiting for unfinished jobs....
libtool: compile: /opt/llvm/bin/../libexec/ccc-analyzer -DHAVE_CONFIG_H -I. -I../include -Dlinux -D_LARGEFILE64_SOURCE=1 -D_COMPILE64BIT_SOURCE=1 -D_REENTRANT -D__STDC_LIMIT_MACROS=1 -D__STDC_FORMAT_MACROS=1 -I/var/jenkins/workspace/clang-analyzer-github/src/proxy/api -I/var/jenkins/workspace/clang-analyzer-github/src/proxy/api -I/var/jenkins/workspace/clang-analyzer-github/src/include/cppapi/include -I/var/jenkins/workspace/clang-analyzer-github/src/lib/cppapi/include -I/var/jenkins/workspace/clang-analyzer-github/src/include -I/var/jenkins/workspace/clang-analyzer-github/src/lib -I/var/jenkins/workspace/clang-analyzer-github/src/lib/yamlcpp/include -D_GNU_SOURCE -DOPENSSL_NO_SSL_INTERN -std=gnu99 -g -pipe -Wall -Wno-deprecated-declarations -Qunused-arguments -Wextra -Wno-ignored-qualifiers -Wno-unused-parameter -O3 -fno-strict-aliasing -mcx16 -MT libloader/libloader.lo -MD -MP -MF libloader/.deps/libloader.Tpo -c libloader/libloader.c -fPIC -DPIC -o libloader/.libs/libloader.o
Error in tempfile() using template /CA/clang-analyzer/github/7313/2020-11-23-123120-12975-1/temp_buf_XXXXXX: Parent directory (/CA/clang-analyzer/github/7313/2020-11-23-123120-12975-1/) does not exist at /opt/llvm/bin/../libexec/ccc-analyzer line 34.
make[2]: *** [libloader/libloader.lo] Error 1
mv -f compress/.deps/compress_compress_la-misc.Tpo compress/.deps/compress_compress_la-misc.Plo
mv -f compress/.deps/compress_compress_la-configuration.Tpo compress/.deps/compress_compress_la-configuration.Plo
make[2]: Leaving directory `/var/jenkins/workspace/clang-analyzer-github/src/plugins'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/var/jenkins/workspace/clang-analyzer-github/src/plugins'
make: *** [all-recursive] Error 1
scan-build: No bugs found.
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7313: Make reloading client certificate configuration more reliable
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7313:
URL: https://github.com/apache/trafficserver/pull/7313#discussion_r520238514
##########
File path: iocore/net/SSLConfig.cc
##########
@@ -426,20 +427,31 @@ SSLConfigParams::getClientSSL_CTX() const
return client_ctx;
}
+void
+SSLClientCoordinator::reconfigure()
+{
+ // The SSLConfig must have its configuration loaded before the SNIConfig.
+ // The SSLConfig owns the client cert context storage and the SNIConfig will load
+ // into it.
+ SSLConfig::reconfigure();
+ SNIConfig::reconfigure();
+}
+
void
SSLConfig::startup()
{
- sslConfigUpdate.reset(new ConfigUpdateHandler<SSLConfig>());
- sslConfigUpdate->attach("proxy.config.ssl.client.cert.path");
- sslConfigUpdate->attach("proxy.config.ssl.client.cert.filename");
- sslConfigUpdate->attach("proxy.config.ssl.client.private_key.path");
- sslConfigUpdate->attach("proxy.config.ssl.client.private_key.filename");
+ sslClientUpdate.reset(new ConfigUpdateHandler<SSLClientCoordinator>());
Review comment:
Yeah, but not only for consistency. If `SSLClientCoordinator` is responsible for making sure the two configurations are loaded in the right order, I think it should take all the responsibility include the startup path.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7313: Make reloading client certificate configuration more reliable
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7313:
URL: https://github.com/apache/trafficserver/pull/7313#issuecomment-738241494
Cherry-picked to v9.0.x branch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on a change in pull request #7313: Make reloading client certificate configuration more reliable
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7313:
URL: https://github.com/apache/trafficserver/pull/7313#discussion_r520156011
##########
File path: iocore/net/SSLConfig.cc
##########
@@ -426,20 +427,31 @@ SSLConfigParams::getClientSSL_CTX() const
return client_ctx;
}
+void
+SSLClientCoordinator::reconfigure()
+{
+ // The SSLConfig must have its configuration loaded before the SNIConfig.
+ // The SSLConfig owns the client cert context storage and the SNIConfig will load
+ // into it.
+ SSLConfig::reconfigure();
+ SNIConfig::reconfigure();
+}
+
void
SSLConfig::startup()
{
- sslConfigUpdate.reset(new ConfigUpdateHandler<SSLConfig>());
- sslConfigUpdate->attach("proxy.config.ssl.client.cert.path");
- sslConfigUpdate->attach("proxy.config.ssl.client.cert.filename");
- sslConfigUpdate->attach("proxy.config.ssl.client.private_key.path");
- sslConfigUpdate->attach("proxy.config.ssl.client.private_key.filename");
+ sslClientUpdate.reset(new ConfigUpdateHandler<SSLClientCoordinator>());
Review comment:
I didn't create a SSLClientCoordinator::startup(). Only introduced the class for the reconfigure case. We hadn't seem ordering problems on a process start.
Probably to be consistent, we could insert SSLClientCoordinator in the startup path as well.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7313: Make reloading client certificate configuration more reliable
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7313:
URL: https://github.com/apache/trafficserver/pull/7313#issuecomment-732427628
[approve ci clang-analyzer]
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org