Make reloading client certificate configuration more reliable

iocore/net/Makefile.am

@@ -120,6 +120,7 @@ libinknet_a_SOURCES = \
 	P_SSLNextProtocolSet.h \
 	P_SSLSNI.h \
 	P_SSLUtils.h \
+        P_SSLClientCoordinator.h \
 	P_SSLClientUtils.h \
 	P_OCSPStapling.h \
 	P_UDPConnection.h \
@@ -137,6 +138,7 @@ libinknet_a_SOURCES = \
 	ProxyProtocol.cc \
 	Socks.cc \
 	SSLCertLookup.cc \
+        SSLClientCoordinator.cc \
 	SSLClientUtils.cc \
 	SSLConfig.cc \
 	SSLDiags.cc \

iocore/net/P_SSLClientCoordinator.h

@@ -0,0 +1,34 @@
+/** @file
+
+  P_SSLClientCoordinator.h - coordinate the loading of SSL related configs
+
+  @section license License
+
+  Licensed to the Apache Software Foundation (ASF) under one
+  or more contributor license agreements.  See the NOTICE file
+  distributed with this work for additional information
+  regarding copyright ownership.  The ASF licenses this file
+  to you under the Apache License, Version 2.0 (the
+  "License"); you may not use this file except in compliance
+  with the License.  You may obtain a copy of the License at
+
+      http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+ */
+
+#include "ProxyConfig.h"
+#include <memory>
+
+// A class to pass the ConfigUpdateHandler, so both SSLConfig and SNIConfig get updated
+// when the relevant files/configs get updated.
+class SSLClientCoordinator
+{
+public:
+  static void startup();
+  static void reconfigure();
+};

iocore/net/SSLClientCoordinator.cc

@@ -0,0 +1,54 @@
+/** @file
+
+  SSLClientCoordinator.cc - Coordinate the loading of SSL related configs
+
+  @section license License
+
+  Licensed to the Apache Software Foundation (ASF) under one
+  or more contributor license agreements.  See the NOTICE file
+  distributed with this work for additional information
+  regarding copyright ownership.  The ASF licenses this file
+  to you under the Apache License, Version 2.0 (the
+  "License"); you may not use this file except in compliance
+  with the License.  You may obtain a copy of the License at
+
+      http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+ */
+
+#include "P_SSLClientCoordinator.h"
+#include "P_SSLConfig.h"
+#include "P_SSLSNI.h"
+
+std::unique_ptr<ConfigUpdateHandler<SSLClientCoordinator>> sslClientUpdate;
+
+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
+SSLClientCoordinator::startup()
+{
+  // 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.
+  sslClientUpdate.reset(new ConfigUpdateHandler<SSLClientCoordinator>());
+  sslClientUpdate->attach("proxy.config.ssl.client.cert.path");
+  sslClientUpdate->attach("proxy.config.ssl.client.cert.filename");
+  sslClientUpdate->attach("proxy.config.ssl.client.private_key.path");
+  sslClientUpdate->attach("proxy.config.ssl.client.private_key.filename");
+  SSLConfig::startup();
+  sslClientUpdate->attach("proxy.config.ssl.servername.filename");
+  SNIConfig::startup();
+}

iocore/net/SSLConfig.cc

@@ -43,6 +43,7 @@
 #include "P_Net.h"
 #include "P_SSLClientUtils.h"
 #include "P_SSLCertLookup.h"
+#include "P_SSLSNI.h"
 #include "SSLDiags.h"
 #include "SSLSessionCache.h"
 #include "SSLSessionTicket.h"
@@ -75,7 +76,6 @@ int SSLConfigParams::async_handshake_enabled = 0;
 char *SSLConfigParams::engine_conf_file      = nullptr;
 
 static std::unique_ptr<ConfigUpdateHandler<SSLCertificateConfig>> sslCertUpdate;
-static std::unique_ptr<ConfigUpdateHandler<SSLConfig>> sslConfigUpdate;
 static std::unique_ptr<ConfigUpdateHandler<SSLTicketKeyConfig>> sslTicketKey;
 
 SSLConfigParams::SSLConfigParams()
@@ -429,17 +429,13 @@ SSLConfigParams::getClientSSL_CTX() const
 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");
   reconfigure();
 }
 
 void
 SSLConfig::reconfigure()
 {
+  Debug("ssl", "Reload SSLConfig");
   SSLConfigParams *params;
   params = new SSLConfigParams;
   params->initialize(); // re-read configuration
@@ -658,6 +654,8 @@ SSLConfigParams::getCTX(const char *client_cert, const char *key_file, const cha
   ts::bwprint(top_level_key, "{}:{}", ca_bundle_file, ca_bundle_path);
   ts::bwprint(ctx_key, "{}:{}", client_cert, key_file);
 
+  Debug("ssl", "Load client cert %s %s", client_cert, key_file);
+
   auto ctx_map_iter = top_level_ctx_map.find(top_level_key);
   if (ctx_map_iter != top_level_ctx_map.end()) {
     auto ctx_iter = ctx_map_iter->second.find(ctx_key);

iocore/net/SSLNetProcessor.cc

@@ -26,8 +26,8 @@
 #include "records/I_RecHttp.h"
 #include "P_SSLUtils.h"
 #include "P_OCSPStapling.h"
-#include "P_SSLSNI.h"
 #include "SSLStats.h"
+#include "P_SSLClientCoordinator.h"
 
 //
 // Global Data
@@ -60,9 +60,8 @@ SSLNetProcessor::start(int, size_t stacksize)
 {
   // This initialization order matters ...
   SSLInitializeLibrary();
-  SSLConfig::startup();
+  SSLClientCoordinator::startup();
   SSLPostConfigInitialize();
-  SNIConfig::startup();
 
   if (!SSLCertificateConfig::startup()) {
     return -1;

iocore/net/SSLSNIConfig.cc

@@ -32,14 +32,12 @@
 #include "P_SSLSNI.h"
 #include "tscore/Diags.h"
 #include "tscore/SimpleTokenizer.h"
-#include "P_SSLConfig.h"
 #include "tscore/ink_memory.h"
 #include "tscpp/util/TextView.h"
 #include "tscore/I_Layout.h"
 #include <sstream>
 #include <pcre.h>
 
-static ConfigUpdateHandler<SNIConfig> *sniConfigUpdate;
 static constexpr int OVECSIZE{30};
 
 const NextHopProperty *
@@ -191,14 +189,13 @@ SNIConfigParams::~SNIConfigParams()
 void
 SNIConfig::startup()
 {
-  sniConfigUpdate = new ConfigUpdateHandler<SNIConfig>();
-  sniConfigUpdate->attach("proxy.config.ssl.servername.filename");
   reconfigure();
 }
 
 void
 SNIConfig::reconfigure()
 {
+  Debug("ssl", "Reload SNI file");
   SNIConfigParams *params = new SNIConfigParams;
 
   params->Initialize();

tests/gold_tests/tls/gold/proxycert-accesslog.gold

@@ -6,3 +6,7 @@
 502 https://127.0.0.1:``/ 2 0
 404 https://127.0.0.1:``/ 2 0
 502 https://127.0.0.1:``/ 2 0
+404 https://127.0.0.1:``/ 2 0
+502 https://127.0.0.1:``/ 2 0
+200 https://127.0.0.1:``/ 2 0
+502 https://127.0.0.1:``/ 2 0

tests/gold_tests/tls/tls_client_cert.test.py

@@ -301,7 +301,7 @@
 tr4fail.Processes.Default.Streams.stdout = Testers.ContainsExpression("Could Not Connect", "Check response")
 
 tr = Test.AddTestRun("Wait for the access log to write out")
-tr.Processes.Default.StartBefore(server4, ready=When.FileExists(ts.Disk.squid_log))
+tr.Processes.Default.StartBefore(server4, ready=When.FileContains(ts.Disk.squid_log.Name, 'https', 12))
 tr.StillRunningAfter = ts
 tr.Processes.Default.Command = 'echo "log file exists"'
 tr.Processes.Default.ReturnCode = 0