-
Notifications
You must be signed in to change notification settings - Fork 865
Refresh default TLS context on secret update #13342
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -134,8 +134,8 @@ struct SSLCertLookup : public ConfigInfo { | |
| std::unique_ptr<SSLContextStorage> ssl_storage; | ||
| std::unique_ptr<SSLContextStorage> ec_storage; | ||
|
|
||
| shared_SSL_CTX ssl_default; | ||
| bool is_valid = true; | ||
| mutable shared_SSL_CTX ssl_default; | ||
| bool is_valid = true; | ||
|
|
||
| int insert(const char *name, SSLCertContext const &cc); | ||
| int insert(const IpEndpoint &address, SSLCertContext const &cc); | ||
|
|
@@ -154,10 +154,18 @@ struct SSLCertLookup : public ConfigInfo { | |
| SSLCertContext *find(const std::string &name, SSLCertContextType ctxType = SSLCertContextType::GENERIC) const; | ||
|
|
||
| // Return the last-resort default TLS context if there is no name or address match. | ||
| SSL_CTX * | ||
| shared_SSL_CTX | ||
| defaultContext() const | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One optional suggestion: the default ctx is now stored twice (the |
||
| { | ||
| return ssl_default.get(); | ||
| std::lock_guard<std::mutex> lock(default_ctx_mutex); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs |
||
| return ssl_default; | ||
| } | ||
|
|
||
| void | ||
| setDefaultContext(shared_SSL_CTX ctx) const | ||
| { | ||
| std::lock_guard<std::mutex> lock(default_ctx_mutex); | ||
| ssl_default = std::move(ctx); | ||
| } | ||
|
|
||
| unsigned count(SSLCertContextType ctxType = SSLCertContextType::GENERIC) const; | ||
|
|
@@ -170,6 +178,8 @@ struct SSLCertLookup : public ConfigInfo { | |
| ~SSLCertLookup() override; | ||
|
|
||
| private: | ||
| mutable std::mutex default_ctx_mutex; | ||
|
|
||
| // Map cert_secret name to lookup keys | ||
| std::unordered_map<std::string, std::vector<std::string>> cert_secret_registry; | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -281,7 +281,8 @@ QUICPacketHandlerIn::_recv_packet(int /* event ATS_UNUSED */, UDPPacket *udp_pac | |
| QUICConnectionId new_cid; | ||
|
|
||
| QUICCertConfig::scoped_config server_cert; | ||
| SSL *ssl = SSL_new(server_cert->defaultContext()); | ||
| auto default_ctx = server_cert->defaultContext(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not for this PR, but worth a note in the description or a follow-up issue: |
||
| SSL *ssl = SSL_new(default_ctx.get()); | ||
|
|
||
| quiche_conn *quiche_con = quiche_conn_new_with_tls( | ||
| new_cid, new_cid.length(), retry_token.original_dcid(), retry_token.original_dcid().length(), &udp_packet->to.sa, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1733,6 +1733,18 @@ SSLMultiCertConfigLoader::update_ssl_ctx(const std::string &secret_name) | |
| if (!ctx) { | ||
| retval = false; | ||
| } else { | ||
| // Address and default-context lookups are owned by the same policy, but | ||
| // are not part of the certificate CN/SAN name sets below. | ||
| for (unsigned i = 0; i < lookup->count(loadingctx.ctx_type); ++i) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This predicate matches every entry the policy owns, including the unique-SAN entries — everything from one |
||
| SSLCertContext *cc = lookup->get(i, loadingctx.ctx_type); | ||
| if (cc && cc->ctx_type == loadingctx.ctx_type && cc->userconfig.get() == policy_iter->get()) { | ||
| cc->setCtx(ctx); | ||
| } | ||
| } | ||
| if ((*policy_iter)->addr && strcmp((*policy_iter)->addr, "*") == 0) { | ||
| lookup->setDefaultContext(ctx); | ||
| this->_set_handshake_callbacks(ctx.get()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unlike the load path, this lookup is live, so the new ctx is reachable by net threads ( |
||
| } | ||
| for (auto const &name : common_names) { | ||
| SSLCertContext *cc = lookup->find(name, loadingctx.ctx_type); | ||
| if (cc && cc->userconfig.get() == policy_iter->get()) { | ||
|
|
@@ -1787,8 +1799,8 @@ SSLMultiCertConfigLoader::_store_single_ssl_ctx(SSLCertLookup *lookup, const sha | |
| if (strcmp(sslMultCertSettings->addr, "*") == 0) { | ||
| Dbg(dbg_ctl_ssl_load, "Addr is '*'; setting %p to default", ctx.get()); | ||
| if (lookup->insert(sslMultCertSettings->addr, SSLCertContext(ctx, ctx_type, sslMultCertSettings, keyblock)) >= 0) { | ||
| inserted = true; | ||
| lookup->ssl_default = ctx; | ||
| inserted = true; | ||
| lookup->setDefaultContext(ctx); | ||
| this->_set_handshake_callbacks(ctx.get()); | ||
| } | ||
| } else { | ||
|
|
@@ -1890,7 +1902,7 @@ SSLMultiCertConfigLoader::load(SSLCertLookup *lookup, bool firstLoad) | |
| // We *must* have a default context even if it can't possibly work. The default context is used to | ||
| // bootstrap the SSL handshake so that we can subsequently do the SNI lookup to switch to the real | ||
| // context. | ||
| if (lookup->ssl_default == nullptr) { | ||
| if (lookup->defaultContext() == nullptr) { | ||
| shared_SSLMultiCertConfigParams sslMultiCertSettings(new SSLMultiCertConfigParams); | ||
| sslMultiCertSettings->addr = ats_strdup("*"); | ||
| if (!this->_store_ssl_ctx(lookup, sslMultiCertSettings)) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,151 @@ | ||
| ''' | ||
| Test default server certificate updates through TSSslSecretSet/TSSslSecretUpdate. | ||
| ''' | ||
| # 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. | ||
|
|
||
| import os | ||
|
|
||
| Test.Summary = ''' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional: tls_check_cert_select_plugin.test.py already has the same plugin/wildcard-policy/CopyAs+touch scaffolding — a no-SNI run added there might cover this without a second copy of the update dance. |
||
| Verify that secret updates refresh the default server SSL_CTX used without SNI. | ||
| ''' | ||
|
|
||
|
|
||
| class TestDefaultSecretUpdate: | ||
| '''Verify default server certificate updates through the secret API.''' | ||
|
|
||
| initial_cert = 'signed-bar.pem' | ||
| updated_cert = 'signed2-bar.pem' | ||
| key_file = 'signed-bar.key' | ||
| plugin = 'ssl_secret_load_test.so' | ||
|
|
||
| def __init__(self): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Type annotations on these methods, please — matching the other class-style tests in this directory (e.g. |
||
| '''Configure the ATS process, origin server, and test runs.''' | ||
| self._configure_server() | ||
| self._configure_traffic_server() | ||
| self._add_initial_certificate_run() | ||
| self._add_mtime_delay_run() | ||
| self._add_certificate_update_run() | ||
| self._add_secret_update_wait_run() | ||
| self._add_updated_certificate_run() | ||
|
|
||
| def _configure_server(self): | ||
| '''Configure the origin server.''' | ||
| server = Test.MakeOriginServer('server') | ||
| self._server = server | ||
|
|
||
| request_header = {'headers': 'GET / HTTP/1.1\r\nHost: doesnotmatter\r\n\r\n', 'timestamp': '1469733493.993', 'body': ''} | ||
| response_header = {'headers': 'HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n', 'timestamp': '1469733493.993', 'body': ''} | ||
| server.addResponse('sessionlog.json', request_header, response_header) | ||
| return server | ||
|
|
||
| def _configure_traffic_server(self): | ||
| '''Configure the Traffic Server process.''' | ||
| ts = Test.MakeATSProcess('ts', enable_tls=True) | ||
| self._ts = ts | ||
|
|
||
| ts.addSSLfile(f'ssl/{self.initial_cert}') | ||
| ts.addSSLfile(f'ssl/{self.updated_cert}') | ||
| ts.addSSLfile(f'ssl/{self.key_file}') | ||
| Test.PrepareTestPlugin(os.path.join(Test.Variables.AtsTestPluginsDir, self.plugin), ts) | ||
|
|
||
| ts.Disk.records_config.update( | ||
| { | ||
| 'proxy.config.diags.debug.enabled': 1, | ||
| 'proxy.config.diags.debug.tags': 'ssl_secret_load_test|ssl|ssl_config_updateCTX', | ||
| 'proxy.config.ssl.server.cert.path': f'{ts.Variables.SSLDir}/../', | ||
| 'proxy.config.ssl.server.private_key.path': f'{ts.Variables.SSLDir}/../', | ||
|
Comment on lines
+69
to
+70
|
||
| 'proxy.config.exec_thread.autoconfig.scale': 1.0, | ||
| 'proxy.config.url_remap.pristine_host_hdr': 1, | ||
| }) | ||
|
|
||
| ts.Disk.ssl_multicert_yaml.AddLines( | ||
| [ | ||
| 'ssl_multicert:', | ||
| ' - dest_ip: "*"', | ||
| f' ssl_cert_name: {self.initial_cert}', | ||
| f' ssl_key_name: {self.key_file}', | ||
| ]) | ||
| ts.Disk.remap_config.AddLine(f'map / http://127.0.0.1:{self._server.Variables.Port}') | ||
| return ts | ||
|
|
||
| def _add_initial_certificate_run(self): | ||
| '''Verify the original default certificate before the secret update.''' | ||
| tr = self._add_curl_run( | ||
| 'Initial default certificate', | ||
| expected_cn='signer.yahoo.com', | ||
| unexpected_cn='signer2.yahoo.com', | ||
| expected_description='Initial cert uses signer.', | ||
| unexpected_description='Initial cert is not updated yet.') | ||
| tr.Processes.Default.StartBefore(self._server) | ||
| tr.Processes.Default.StartBefore(self._ts) | ||
| return tr | ||
|
|
||
| def _add_mtime_delay_run(self): | ||
| '''Make the replacement certificate mtime differ from the original.''' | ||
| tr = Test.AddTestRun('Make the cert mtime differ') | ||
| tr.Processes.Default.Command = 'sleep 2' | ||
| tr.Processes.Default.ReturnCode = 0 | ||
| self._keep_processes_running(tr) | ||
| return tr | ||
|
|
||
| def _add_certificate_update_run(self): | ||
| '''Replace the plugin backing certificate file.''' | ||
| tr = Test.AddTestRun('Update the plugin backing certificate') | ||
| tr.Setup.CopyAs(f'ssl/{self.updated_cert}', '.', f'{self._ts.Variables.SSLDir}/{self.initial_cert}') | ||
| tr.Processes.Default.Command = f'touch {self._ts.Variables.SSLDir}/{self.initial_cert}' | ||
| tr.Processes.Default.ReturnCode = 0 | ||
| self._keep_processes_running(tr) | ||
| return tr | ||
|
|
||
| def _add_secret_update_wait_run(self): | ||
| '''Wait for the test plugin to update the certificate secret.''' | ||
| tr = Test.AddTestRun('Wait for the secret update') | ||
| tr.Processes.Default.Command = 'echo awaiting secret update' | ||
| tr.Processes.Default.ReturnCode = 0 | ||
| await_update = tr.Processes.Process('await_update', 'sleep 30') | ||
| await_update.Ready = When.FileContains(self._ts.Disk.traffic_out.Name, 'update cert for secret') | ||
| tr.Processes.Default.StartBefore(await_update) | ||
| self._keep_processes_running(tr) | ||
| return tr | ||
|
|
||
| def _add_updated_certificate_run(self): | ||
| '''Verify the updated default certificate after the secret update.''' | ||
| return self._add_curl_run( | ||
| 'Updated default certificate', | ||
| expected_cn='signer2.yahoo.com', | ||
| unexpected_cn='signer.yahoo.com', | ||
| expected_description='Updated cert uses signer2.', | ||
| unexpected_description='Updated cert no longer uses signer.') | ||
|
|
||
| def _add_curl_run(self, name, expected_cn, unexpected_cn, expected_description, unexpected_description): | ||
| '''Add a curl run against ATS without SNI.''' | ||
| tr = Test.AddTestRun(name) | ||
| self._keep_processes_running(tr) | ||
| tr.MakeCurlCommand( | ||
| f"-k -v --http1.1 -H 'host: doesnotmatter' https://127.0.0.1:{self._ts.Variables.ssl_port}/", ts=self._ts) | ||
| tr.Processes.Default.ReturnCode = 0 | ||
| tr.Processes.Default.Streams.All = Testers.ContainsExpression(f'issuer:.*CN={expected_cn}', expected_description) | ||
| tr.Processes.Default.Streams.All += Testers.ExcludesExpression(f'issuer:.*CN={unexpected_cn}', unexpected_description) | ||
| return tr | ||
|
|
||
| def _keep_processes_running(self, tr): | ||
| '''Keep ATS and the origin server running after a TestRun.''' | ||
| tr.StillRunningAfter = self._ts | ||
| tr.StillRunningAfter = self._server | ||
|
|
||
|
|
||
| TestDefaultSecretUpdate() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this move to the private section next to
default_ctx_mutex? Nothing accesses it directly anymore, and keeping it public leaves the door open for the kind of unlocked read this PR fixed in SSLStats.cc.