Skip to content
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

TAO support revocation lists #1830

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
48 changes: 48 additions & 0 deletions ACE/ace/SSL/SSL_Context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,54 @@ ACE_SSL_Context::load_trusted_ca (const char* ca_file,
return 0;
}

int
ACE_SSL_Context::load_crl_file(const char *file_name, int type)
{
if (context_ == nullptr || file_name == nullptr)
{
return 0;
}

int ret = 0;
BIO *in = nullptr;
X509_CRL *x = nullptr;
X509_STORE *st = ::SSL_CTX_get_cert_store(context_);
if (st == nullptr)
{
goto err;
}

if (type == SSL_FILETYPE_PEM)
{
ret = ::SSL_CTX_load_verify_locations(context_, file_name, nullptr);
}
else if (type == SSL_FILETYPE_ASN1)
{
in = BIO_new(BIO_s_file());
if (in == nullptr || BIO_read_filename(in, file_name) <= 0)
{
goto err;
}
x = d2i_X509_CRL_bio(in, nullptr);
if (x == nullptr)
{
goto err;
}
ret = ::X509_STORE_add_crl(st, x);
}

if (ret == 1)
{
(void)X509_STORE_set_flags(st, X509_V_FLAG_CRL_CHECK);
}

err:
X509_CRL_free(x);
(void)BIO_free(in);

return ret;
}

int
ACE_SSL_Context::private_key (const char *file_name,
int type)
Expand Down
13 changes: 13 additions & 0 deletions ACE/ace/SSL/SSL_Context.h
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,19 @@ class ACE_SSL_Export ACE_SSL_Context
const char* ca_dir = 0,
bool use_env_defaults = true);

/**
* Load the location of the CRL.
*
* @param[in] file_name CRL file pathname. Passed to
* @c SSL_CTX_Load_verify_locations() if not
* 0 and @a type is SSL_FILETYPE_PEM. Pass to
* @c X509_STORE_add_crl if not 0 @a type is SSL_FILETYPE_ASN1.
* @param[in] type CRL file type. Support SSL_FILETYPE_PEM and
* SSL_FILETYPE_ASN1.
* @return 1 for success or others on error.
*/
int load_crl_file(const char* file_name, int type);

/**
* Test whether any CA locations have been successfully loaded and
* return the number of successful attempts.
Expand Down
4 changes: 4 additions & 0 deletions TAO/docs/Security/SSLIOP-USAGE.html
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,10 @@ <h4>SSLIOP Options</h4>
<td><code>-SSLCAfile</code> <em>filename</em></td>
<td>Provide a file containing a trusted certificate, overriding the file named by SSL_CERT_FILE environment variable.</td>
</tr>
<tr>
<td><code>-SSLCRLFile</code> <em>filename</em></td>
<td>Provide a file containing a certificate revocation list.</td>
</tr>
<tr>
<td><code>-SSLCApath</code> <em>directory</em></td>
<td>Provide a directory from which all files are read for trusted certificates overriding the directory named by SSL_CERT_DIR environment variable.<</td>
Expand Down
35 changes: 35 additions & 0 deletions TAO/orbsvcs/orbsvcs/SSLIOP/SSLIOP_Factory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,9 @@ TAO::SSLIOP::Protocol_Factory::init (int argc, ACE_TCHAR* argv[])
int private_key_type = -1;
int dhparams_type = -1;

CORBA::String_var crl_path;
int crl_type = -1;

int prevdebug = -1;

// Force the Singleton instance to be initialized/instantiated.
Expand Down Expand Up @@ -411,6 +414,17 @@ TAO::SSLIOP::Protocol_Factory::init (int argc, ACE_TCHAR* argv[])
}
}

else if (ACE_OS::strcasecmp (argv[curarg],
ACE_TEXT("-SSLCRLFile")) == 0)
{
curarg++;
if (curarg < argc)
{
crl_type = parse_x509_file (ACE_TEXT_ALWAYS_CHAR(argv[curarg]),
crl_path.out ());
}
}
Comment on lines +417 to +426
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling for missing argument.

The command-line parsing should handle the case where no argument is provided after -SSLCRLFile.

Apply this diff to improve error handling:

       else if (ACE_OS::strcasecmp (argv[curarg],
                                    ACE_TEXT("-SSLCRLFile")) == 0)
         {
           curarg++;
           if (curarg < argc)
             {
               crl_type = parse_x509_file (ACE_TEXT_ALWAYS_CHAR(argv[curarg]),
                                           crl_path.out ());
+              if (crl_type == -1)
+                {
+                  ORBSVCS_ERROR ((LM_ERROR,
+                              ACE_TEXT ("TAO (%P|%t) - Invalid CRL file format ")
+                              ACE_TEXT ("<%C>, must be PEM or ASN1\n"),
+                              argv[curarg]));
+                  return -1;
+                }
             }
+          else
+            {
+              ORBSVCS_ERROR ((LM_ERROR,
+                          ACE_TEXT ("TAO (%P|%t) - -SSLCRLFile requires a ")
+                          ACE_TEXT ("filename argument\n")));
+              return -1;
+            }
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
else if (ACE_OS::strcasecmp (argv[curarg],
ACE_TEXT("-SSLCRLFile")) == 0)
{
curarg++;
if (curarg < argc)
{
crl_type = parse_x509_file (ACE_TEXT_ALWAYS_CHAR(argv[curarg]),
crl_path.out ());
}
}
else if (ACE_OS::strcasecmp (argv[curarg],
ACE_TEXT("-SSLCRLFile")) == 0)
{
curarg++;
if (curarg < argc)
{
crl_type = parse_x509_file (ACE_TEXT_ALWAYS_CHAR(argv[curarg]),
crl_path.out ());
if (crl_type == -1)
{
ORBSVCS_ERROR ((LM_ERROR,
ACE_TEXT ("TAO (%P|%t) - Invalid CRL file format ")
ACE_TEXT ("<%C>, must be PEM or ASN1\n"),
argv[curarg]));
return -1;
}
}
else
{
ORBSVCS_ERROR ((LM_ERROR,
ACE_TEXT ("TAO (%P|%t) - -SSLCRLFile requires a ")
ACE_TEXT ("filename argument\n")));
return -1;
}
}


else if (ACE_OS::strcasecmp (argv[curarg],
ACE_TEXT("-SSLAuthenticate")) == 0)
{
Expand Down Expand Up @@ -634,6 +648,27 @@ TAO::SSLIOP::Protocol_Factory::init (int argc, ACE_TCHAR* argv[])
}
}

if (crl_path.in() != 0)
{
if (ssl_ctx->load_crl_file(crl_path.in(), crl_type) != 1)
{
ORBSVCS_ERROR ((LM_ERROR,
ACE_TEXT ("TAO (%P|%t) - Unable to load crl file ")
ACE_TEXT ("<%C> in SSLIOP factory, errno = %C.\n"),
crl_path.in(), ERR_reason_error_string(ERR_get_error())));
}
else
{
if (TAO_debug_level > 0)
{
ORBSVCS_DEBUG ((LM_INFO,
ACE_TEXT ("TAO (%P|%t) - SSLIOP loaded crl file ")
ACE_TEXT("<%C>\n"),
crl_path.in()));
}
}
}

// Load in the DH params. If there was a file explicitly specified,
// then we do that here, otherwise we load them in from the cert file.
// Note that we only do this on the server side, I think so we might
Expand Down