Skip to content

Comments

Implement OCSP client and responder with HTTP and SCGI transport#200

Open
julek-wolfssl wants to merge 1 commit intowolfSSL:mainfrom
julek-wolfssl:ocsp-responder
Open

Implement OCSP client and responder with HTTP and SCGI transport#200
julek-wolfssl wants to merge 1 commit intowolfSSL:mainfrom
julek-wolfssl:ocsp-responder

Conversation

@julek-wolfssl
Copy link
Member

Depends on wolfSSL/wolfssl#9761

Core OCSP implementation:

  • Register the new WOLFCLU_OCSP mode enum value
  • The responder main loop accepts connections and handles the request in a transport-agnostic way.
  • Add the OCSP mode to the help text in src/tools/clu_funcs.c.

New HTTP utilities (src/tools/clu_http.c):

  • Move the static kHttpGetMsg from src/client/client.c and the static kHttpServerMsg from src/server/server.c into shared accessor functions
  • Add HTTP builder and server helpers

New SCGI protocol implementation (src/tools/clu_scgi.c):

Certificate and config additions (certs/):

  • Add ocsp-responder-cert.pem which is an authorized responder for ca-cert.pem

Test suites:

  • tests/ocsp/ocsp-test.sh: top-level test runner with four interop combinations (wolfssl↔openssl, wolfssl↔wolfssl, openssl↔wolfssl, openssl↔openssl) sequentially
  • tests/ocsp/ocsp-interop-test.sh: test script taking in $OCSP_CLIENT and $OCSP_RESPONDER. Written to take in the same commands when run with wolfssl or openssl on either side
  • tests/ocsp-scgi/ocsp-scgi-test.sh: SCGI integration test using nginx for HTTP termination

Depends on wolfSSL/wolfssl#9761

Core OCSP implementation:
- Register the new WOLFCLU_OCSP mode enum value
- The responder main loop accepts connections and handles the request in a transport-agnostic way.
- Add the OCSP mode to the help text in src/tools/clu_funcs.c.

New HTTP utilities (src/tools/clu_http.c):
- Move the static `kHttpGetMsg` from src/client/client.c and the static `kHttpServerMsg` from src/server/server.c into shared accessor functions
- Add HTTP builder and server helpers

New SCGI protocol implementation (src/tools/clu_scgi.c):
- Implement the SCGI wire protocol per https://python.ca/scgi/protocol.txt

Certificate and config additions (certs/):
- Add ocsp-responder-cert.pem which is an authorized responder for ca-cert.pem

Test suites:
- tests/ocsp/ocsp-test.sh: top-level test runner with four interop combinations (wolfssl↔openssl, wolfssl↔wolfssl, openssl↔wolfssl, openssl↔openssl) sequentially
- tests/ocsp/ocsp-interop-test.sh: test script taking in $OCSP_CLIENT and $OCSP_RESPONDER. Written to take in the same commands when run with wolfssl or openssl on either side
- tests/ocsp-scgi/ocsp-scgi-test.sh: SCGI integration test using nginx for HTTP termination
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements a comprehensive OCSP (Online Certificate Status Protocol) client and responder for wolfCLU, enabling certificate revocation checking with both HTTP and SCGI transport protocols.

Changes:

  • Added OCSP client and responder implementation with transport-agnostic design
  • Implemented HTTP utilities by refactoring existing code and adding server-side helpers
  • Added SCGI protocol support for nginx reverse proxy integration
  • Included comprehensive test suites for interoperability testing (wolfSSL ↔ OpenSSL)

Reviewed changes

Copilot reviewed 29 out of 30 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
src/ocsp/clu_ocsp.c Core OCSP client and responder implementation with index file parsing
src/tools/clu_http.c HTTP utilities including request/response building and server helpers
src/tools/clu_scgi.c SCGI protocol implementation following spec at python.ca/scgi/protocol.txt
src/tools/clu_pem_der.c Certificate and key loading utilities with PEM to DER conversion
wolfclu/clu_header_main.h Function declarations for OCSP, HTTP, and SCGI APIs
wolfclu/clu_optargs.h Added WOLFCLU_OCSP enum value
wolfclu/client.h Removed unnecessary WOLFSSL_THREAD define
src/clu_main.c Integrated OCSP mode into main command dispatcher
src/tools/clu_funcs.c Added OCSP to help text
src/client/client.c Refactored to use shared HTTP GET message
src/server/server.c Refactored to use shared HTTP response message
tests/ocsp/ocsp-test.sh Top-level test runner for four interop combinations
tests/ocsp/ocsp-interop-test.sh Detailed interop test script with 11 test cases
tests/ocsp-scgi/ocsp-scgi-test.sh SCGI integration test with nginx
tests/ocsp-scgi/scgi_params nginx SCGI parameter configuration
src/include.am Added new source files to build system
Makefile.am Added test directories to make check
wolfCLU.vcxproj Added source files for Windows build
certs/renew.sh Added OCSP responder certificate generation
certs/ocsp.cnf OpenSSL configuration for OCSP signing extension
certs/ocsp-responder-*.pem OCSP responder certificate and key
README.md Added deployment documentation for SCGI mode with nginx
.gitignore Added development artifacts
.github/workflows/*.yml Added nginx and openssl to CI dependencies
autogen.sh Removed Git hooks setup
tests/x509/x509-req-test.sh Added cleanup of tmp.csr file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

fi


set -e
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

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

The removal of Git hooks setup from autogen.sh appears intentional but there's no explanation in the PR description. This removes automatic setup of the pre-commit hook. If this is intentional, consider documenting why this change was made.

Copilot uses AI. Check for mistakes.
Comment on lines +868 to +871

if (config->nrequest > 0 && requestsProcessed >= config->nrequest) {
break;
}
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

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

Duplicate check for request limit. The check at lines 861-864 is redundant with the check at lines 869-871. Remove the duplicate check at lines 869-871.

Suggested change
if (config->nrequest > 0 && requestsProcessed >= config->nrequest) {
break;
}

Copilot uses AI. Check for mistakes.
contentLen = XSTRSTR((char*)httpReq, "content-length:");
}
if (contentLen) {
*bodySz = XATOI(contentLen + 15);
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

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

The Content-Length parsing at line 405 does not validate that the parsed value is non-negative. XATOI can parse negative numbers which could cause issues. Add validation that *bodySz >= 0 after parsing.

Suggested change
*bodySz = XATOI(contentLen + 15);
*bodySz = XATOI(contentLen + 15);
if (*bodySz < 0) {
return -1;
}

Copilot uses AI. Check for mistakes.
*derSz = pDer->length;
}
wc_FreeDer(&pDer);
}
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

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

In the PEM conversion path (lines 76-86), if wc_PemToDer succeeds but the subsequent XMALLOC at line 78 fails, pDer is still freed at line 83 but the error return happens without checking if *der allocation succeeded. This is correct, but if wc_PemToDer returns an error (ret != 0), pDer may still be allocated and needs to be freed. Add wc_FreeDer call for the error case before line 85.

Suggested change
}
}
else if (pDer != NULL) {
/* Ensure pDer is freed even when wc_PemToDer returns an error. */
wc_FreeDer(&pDer);
}

Copilot uses AI. Check for mistakes.
Comment on lines +507 to +509
static byte reqBuffer[16384];
static byte respBuffer[16384];

Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

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

The global buffers reqBuffer and respBuffer at lines 507-508 are not thread-safe. If the responder were to handle multiple requests concurrently (though it's currently single-threaded), these buffers could be corrupted. Consider documenting that the responder is single-threaded or making these buffers thread-local if concurrent request handling is planned.

Suggested change
static byte reqBuffer[16384];
static byte respBuffer[16384];
#ifndef WOLFCLU_THREAD_LOCAL
#if defined(_MSC_VER)
#define WOLFCLU_THREAD_LOCAL __declspec(thread)
#elif defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 201112L)
#define WOLFCLU_THREAD_LOCAL _Thread_local
#elif defined(__GNUC__)
#define WOLFCLU_THREAD_LOCAL __thread
#else
/* Fallback: no thread-local support available; buffers remain global.
* This responder should be used single-threaded in this configuration.
*/
#define WOLFCLU_THREAD_LOCAL
#endif
#endif
WOLFCLU_THREAD_LOCAL static byte reqBuffer[16384];
WOLFCLU_THREAD_LOCAL static byte respBuffer[16384];

Copilot uses AI. Check for mistakes.
scgi_param CONTENT_LENGTH $content_length;

scgi_param SCRIPT_NAME $fastcgi_script_name;
scgi_param REQUEST_URI $request_uri;
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

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

REQUEST_URI is defined twice (lines 2 and 8). This is redundant and could potentially cause issues depending on nginx's behavior. Remove the duplicate definition.

Suggested change
scgi_param REQUEST_URI $request_uri;

Copilot uses AI. Check for mistakes.
Comment on lines +811 to +857
ret = transportReadRequest(clientfd, transportType, &ocspReq, &ocspReqSz);
if (ret != WOLFCLU_SUCCESS) {
break;
}

/* Process OCSP request and generate response */
respSz = sizeof(respBuffer);
ret = wc_OcspResponder_WriteResponse(responder, ocspReq, (word32)ocspReqSz,
respBuffer, &respSz);

if (ret != 0) {
enum Ocsp_Response_Status errStatus;

/* Map error to OCSP response status */
switch (ret) {
case ASN_PARSE_E:
errStatus = OCSP_MALFORMED_REQUEST;
break;
case ASN_SIG_HASH_E:
errStatus = OCSP_INTERNAL_ERROR;
break;
case ASN_NO_SIGNER_E:
errStatus = OCSP_UNAUTHORIZED;
break;
case OCSP_CERT_UNKNOWN:
errStatus = OCSP_UNAUTHORIZED;
break;
default:
errStatus = OCSP_INTERNAL_ERROR;
break;
}

/* Generate OCSP error response */
respSz = sizeof(respBuffer);
ret = wc_OcspResponder_WriteErrorResponse(errStatus, respBuffer, &respSz);

if (ret != 0) {
/* If we can't encode an error response, send HTTP/SCGI error */
transportSendError(clientfd, transportType, 500, "Internal Server Error");
break;
}
}

/* Send response via transport layer */
if (transportSendResponse(clientfd, transportType, respBuffer, (int)respSz) != 0) {
break;
}
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

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

Resource leak: When transportReadRequest fails, transportSendError fails, or transportSendResponse fails, the code breaks from the loop without closing clientfd. This leaves the socket open until cleanup. Close clientfd before breaking from the loop in these error paths.

Copilot uses AI. Check for mistakes.
Comment on lines +792 to +799
setupSignalHandlers();

WOLFCLU_LOG(WOLFCLU_L0, "OCSP responder%s listening on port %d",
(transportType == TRANSPORT_SCGI) ? " (SCGI mode)" : "", config->port);

/* Main loop - exit on shutdown signal */
while (!shutdownRequested &&
(config->nrequest == 0 || requestsProcessed < config->nrequest)) {
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

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

On Windows platforms, setupSignalHandlers() is not defined (it's inside #ifndef _WIN32) but is called unconditionally at line 792. This will cause a compilation error on Windows. Either provide a Windows implementation or wrap the call in a platform check.

Suggested change
setupSignalHandlers();
WOLFCLU_LOG(WOLFCLU_L0, "OCSP responder%s listening on port %d",
(transportType == TRANSPORT_SCGI) ? " (SCGI mode)" : "", config->port);
/* Main loop - exit on shutdown signal */
while (!shutdownRequested &&
(config->nrequest == 0 || requestsProcessed < config->nrequest)) {

Copilot uses AI. Check for mistakes.
}
if (lenBuf[i] == ':') {
lenBuf[i] = '\0';
*length = XATOI(lenBuf);
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

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

The netstring length parsing does not validate that the parsed length is non-negative. XATOI will parse negative numbers from the input, which could lead to undefined behavior when used as buffer sizes. Add validation that *length >= 0 after parsing.

Suggested change
*length = XATOI(lenBuf);
*length = XATOI(lenBuf);
if (*length < 0) {
return -1;
}

Copilot uses AI. Check for mistakes.
Comment on lines +284 to +285
if (cl != NULL)
contentLen = XATOI(cl + 15);
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

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

The Content-Length parsing at line 285 does not validate that the parsed value is non-negative. XATOI can parse negative numbers which could cause issues with the comparison at line 289. Add validation that contentLen >= 0 after parsing.

Suggested change
if (cl != NULL)
contentLen = XATOI(cl + 15);
if (cl != NULL) {
contentLen = XATOI(cl + 15);
if (contentLen < 0)
contentLen = 0;
}

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants