TAO support revocation lists #1830
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There's an issuse with a cyclomatic complexity of 130. However, the cyclomatic complexity of this function was high before. Can I ignore this issuse? @jwillemsen |
|
Complexity is not a problem |
jwillemsen
requested changes
May 7, 2022
|
Please don't assign me as reviewer, my free time for github reviews is very limited, when you need a guaranteed response/review from my side, see https://www.remedy.nl for our services, without being payed for my time you have to wait until I have spare time which I can't tell when I have. |
|
Don't keep assigning my as reviewer, you have to wait until I have free time or fund my time! There is a huge list of things to do on ACE/TAO and sponsoring is welcome! |
WalkthroughThe changes add support for loading Certificate Revocation Lists (CRLs) into the SSL context. A new method, Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Command Line
participant Factory as SSLIOP_Factory
participant Context as ACE_SSL_Context
participant Store as X509_Store
CLI->>Factory: Parse command-line args (-SSLCRLFile)
Factory->>Factory: Set crl_path & crl_type variables
Factory->>Context: Call load_crl_file(file_name, type)
Context->>Context: Validate context and file parameter
Context->>Store: Retrieve X509_STORE
Store-->>Context: Return store reference
Context->>Context: Load CRL (PEM/ASN1) and add to store
Context->>Store: Update store with loaded CRL and set CRL flags
Context-->>Factory: Return result (success/error)
Factory->>CLI: Log result of CRL load operation
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
ACE/ace/SSL/SSL_Context.h (1)
257-268: Enhance the method documentation.The documentation could be improved by:
- Clarifying the return value (1 for success, others for error).
- Documenting specific error cases.
- Adding an example usage.
Apply this diff to enhance the documentation:
/** * 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. + * @param[in] file_name CRL file pathname. + * @param[in] type CRL file type. Must be one of: + * - SSL_FILETYPE_PEM: File is in PEM format + * - SSL_FILETYPE_ASN1: File is in ASN1 format + * + * @return 1 for success, 0 if file_name or context is null, or -1 on other errors: + * - Failed to load PEM file + * - Failed to create BIO for ASN1 file + * - Failed to read ASN1 file + * - Failed to add CRL to store + * + * @note For PEM files, uses SSL_CTX_load_verify_locations(). + * For ASN1 files, uses X509_STORE_add_crl(). + * + * Example: + * @code + * ACE_SSL_Context* ssl_ctx = ACE_SSL_Context::instance(); + * if (ssl_ctx->load_crl_file("revoked.pem", SSL_FILETYPE_PEM) != 1) { + * // Handle error + * } + * @endcode */ACE/ace/SSL/SSL_Context.cpp (1)
553-599: Improve error handling and memory management.The implementation has several areas for improvement:
- Error handling could be more granular with specific error codes.
- Memory management could be improved using RAII.
Apply this diff to enhance the implementation:
int ACE_SSL_Context::load_crl_file(const char *file_name, int type) { - if (context_ == nullptr || file_name == nullptr) + if (context_ == nullptr) + { + if (TAO_debug_level > 0) + ORBSVCS_ERROR ((LM_ERROR, + ACE_TEXT ("TAO (%P|%t) - SSL_Context::load_crl_file ") + ACE_TEXT ("null context\n"))); + return 0; + } + + if (file_name == nullptr) { + if (TAO_debug_level > 0) + ORBSVCS_ERROR ((LM_ERROR, + ACE_TEXT ("TAO (%P|%t) - SSL_Context::load_crl_file ") + ACE_TEXT ("null file name\n"))); return 0; } int ret = 0; - BIO *in = nullptr; - X509_CRL *x = nullptr; X509_STORE *st = ::SSL_CTX_get_cert_store(context_); if (st == nullptr) { + if (TAO_debug_level > 0) + ORBSVCS_ERROR ((LM_ERROR, + ACE_TEXT ("TAO (%P|%t) - SSL_Context::load_crl_file ") + ACE_TEXT ("null store\n"))); 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) + // Use RAII for BIO + struct BIO_Guard { + BIO *bio; + BIO_Guard(BIO *b) : bio(b) {} + ~BIO_Guard() { BIO_free(bio); } + }; + BIO_Guard in_guard(BIO_new(BIO_s_file())); + if (in_guard.bio == nullptr) { + if (TAO_debug_level > 0) + ORBSVCS_ERROR ((LM_ERROR, + ACE_TEXT ("TAO (%P|%t) - SSL_Context::load_crl_file ") + ACE_TEXT ("failed to create BIO\n"))); goto err; } - x = d2i_X509_CRL_bio(in, nullptr); - if (x == nullptr) + + if (BIO_read_filename(in_guard.bio, file_name) <= 0) { + if (TAO_debug_level > 0) + ORBSVCS_ERROR ((LM_ERROR, + ACE_TEXT ("TAO (%P|%t) - SSL_Context::load_crl_file ") + ACE_TEXT ("failed to read file\n"))); goto err; } - ret = ::X509_STORE_add_crl(st, x); + + // Use RAII for X509_CRL + struct CRL_Guard { + X509_CRL *crl; + CRL_Guard(X509_CRL *c) : crl(c) {} + ~CRL_Guard() { X509_CRL_free(crl); } + }; + CRL_Guard x_guard(d2i_X509_CRL_bio(in_guard.bio, nullptr)); + if (x_guard.crl == nullptr) + { + if (TAO_debug_level > 0) + ORBSVCS_ERROR ((LM_ERROR, + ACE_TEXT ("TAO (%P|%t) - SSL_Context::load_crl_file ") + ACE_TEXT ("failed to parse CRL\n"))); + goto err; + } + + ret = ::X509_STORE_add_crl(st, x_guard.crl); } if (ret == 1) { (void)X509_STORE_set_flags(st, X509_V_FLAG_CRL_CHECK); } err: - X509_CRL_free(x); - (void)BIO_free(in); + if (ret != 1 && TAO_debug_level > 0) + { + ORBSVCS_ERROR ((LM_ERROR, + ACE_TEXT ("TAO (%P|%t) - SSL_Context::load_crl_file ") + ACE_TEXT ("failed with error: %C\n"), + ERR_reason_error_string(ERR_get_error()))); + } return ret; }TAO/orbsvcs/orbsvcs/SSLIOP/SSLIOP_Factory.cpp (1)
651-670: Improve error handling and logging for CRL loading.The CRL loading code could be improved with better error handling and debug logging.
Apply this diff to enhance the implementation:
if (crl_path.in() != 0) { - if (ssl_ctx->load_crl_file(crl_path.in(), crl_type) != 1) + int result = ssl_ctx->load_crl_file(crl_path.in(), crl_type); + if (result != 1) { + const char* error_type = (result == 0) ? "invalid arguments" : "loading failed"; 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()))); + ACE_TEXT ("TAO (%P|%t) - Unable to load CRL file ") + ACE_TEXT ("<%C> in SSLIOP factory: %C (%C)\n"), + crl_path.in(), + error_type, + ERR_reason_error_string(ERR_get_error()))); + // Don't fail the initialization as CRL is optional } 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())); + ACE_TEXT ("TAO (%P|%t) - SSLIOP loaded CRL file ") + ACE_TEXT ("<%C> of type %C\n"), + crl_path.in(), + (crl_type == SSL_FILETYPE_PEM) ? "PEM" : "ASN1")); } } }TAO/docs/Security/SSLIOP-USAGE.html (1)
170-173: Enhance the documentation for the-SSLCRLFileoption.The documentation for the new option could be more comprehensive.
Apply this diff to improve the documentation:
<tr> <td><code>-SSLCRLFile</code> <em>filename</em></td> - <td>Provide a file containing a certificate revocation list.</td> + <td> + <p>Provide a file containing a Certificate Revocation List (CRL). The file must be in either PEM or ASN1 format, + specified using the format prefix (e.g., "PEM:crl.pem" or "ASN1:crl.der").</p> + <p>A CRL is used to identify certificates that have been revoked by their issuing Certificate Authority (CA) + and should no longer be trusted. When this option is used, the SSL/TLS implementation will check + peer certificates against the CRL during the handshake.</p> + <p>Example usage:</p> + <pre> + -SSLCRLFile PEM:/path/to/crl.pem # Load a PEM-formatted CRL + -SSLCRLFile ASN1:/path/to/crl.der # Load an ASN1-formatted CRL</pre> + <p>Note: This option is typically used in conjunction with <code>-SSLCAfile</code> or <code>-SSLCApath</code> + to provide complete certificate validation.</p> + </td> </tr>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ACE/ace/SSL/SSL_Context.cpp(1 hunks)ACE/ace/SSL/SSL_Context.h(1 hunks)TAO/docs/Security/SSLIOP-USAGE.html(1 hunks)TAO/orbsvcs/orbsvcs/SSLIOP/SSLIOP_Factory.cpp(3 hunks)
Comment on lines
+417
to
+426
| 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 ()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ 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; | |
| } | |
| } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TAO support revocation lists
Summary by CodeRabbit