[jira] [Commented] (TS-2967) failed assert if ssl_multicert.config doesn't exist

[Jack Bates (JIRA)]

    [ 
https://issues.apache.org/jira/browse/TS-2967?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14091882#comment-14091882
 ] 

Jack Bates commented on TS-2967:
--------------------------------

bq. The only place I can see that needs a NULL check is 
{{SSLNetVConnection::sslStartHandShake()}}

Hmm what should {{SSLNetVConnection::sslStartHandShake()}} do if 
{{SSLCertificateConfig::scoped_config}} is NULL?

I think it makes sense for it to behave the same if {{ssl_multicert.config}} 
doesn't exist as if it is blank so if {{SSLCertificateConfig::scoped_config}} 
is NULL because {{ssl_multicert.config}} doesn't exist then I think 
{{SSLNetVConnection::sslStartHandShake()}} needs the 
{{lookup->defaultContext()}} that results from the following lines in 
{{SSLParseCertificateConfiguration()}}

{code}
  // 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 == NULL) {
    ssl_user_config sslMultiCertSettings;
    sslMultiCertSettings.addr = ats_strdup("*");
    ssl_store_ssl_context(params, lookup, sslMultiCertSettings);
  }
{code}

What about the following change?

{code}
--- a/iocore/net/SSLUtils.cc
+++ b/iocore/net/SSLUtils.cc
@@ -1476,10 +1476,7 @@ SSLParseCertificateConfiguration(
     file_buf = readIntoBuffer(params->configFilePath, __func__, NULL);
   }
 
-  if (!file_buf) {
-    Error("failed to read SSL certificate configuration from %s", params->confi
-    return false;
-  }
+  if (file_buf) {
 
   // elevate/allow file access to root read only files/certs
   uint32_t elevate_setting = 0;
@@ -1521,6 +1518,9 @@ SSLParseCertificateConfiguration(
 
     line = tokLine(NULL, &tok_state);
   }
+  } else {
+    Error("failed to read SSL certificate configuration from %s", params->confi
+  }
 
   // We *must* have a default context even if it can't possibly work. The defau
   // bootstrap the SSL handshake so that we can subsequently do the SNI lookup 
{code}

This change resolves this issue because {{SSLCertificateConfig::reconfigure()}} 
will now initialize {{configid}} when {{ssl_multicert.config}} doesn't exist 
and it makes {{SSLNetVConnection::sslStartHandShake()}} behave the same if 
{{ssl_multicert.config}} doesn't exist as if it is blank.

> failed assert if ssl_multicert.config doesn't exist
> ---------------------------------------------------
>
>                 Key: TS-2967
>                 URL: https://issues.apache.org/jira/browse/TS-2967
>             Project: Traffic Server
>          Issue Type: Bug
>            Reporter: Jack Bates
>            Assignee: Jack Bates
>             Fix For: 5.1.0
>
>
> If an ssl_multicert.config file doesn't exist then 
> SSLParseCertificateConfiguration() returns false (SSLUtils.cc line 1435)
> and SSLCertificateConfig::reconfigure() doesn't initialize configid 
> (SSLConfig.cc line 333)
> Then when SSLRecRawStatSyncCount() calls SSLCertificateConfig::acquire() 
> (SSLUtils.cc line 502)
> it calls ConfigProcessor::get() with configid zero (SSLConfig.cc line 342)
> and there's an ink_assert(id != 0) (ProxyConfig.cc line 175)
> One way to avoid the failed assert is if SSLCertificateConfig::acquire() and 
> SSLCertificateConfig::release() only call ConfigProcessor::get/release() if 
> (configid !=0)
> Another way might be if SSLCertificateConfig::reconfigure() initialized 
> configid with configProcessor.set(configid, NULL) if 
> SSLParseCertificateConfiguration() returns false?
> Or SSLParseCertificateConfiguration() could treat a nonexistent 
> ssl_multicert.config the same as it treats a blank file? ({{touch 
> ssl_multicert.config}} makes the failed assert go away.)
> Or maybe there's another better way to avoid the failed assert?



--
This message was sent by Atlassian JIRA
(v6.2#6252)