Skip to content

Rework TLS to OpenSSL-native style (memory BIOs, explicit status codes)#8002

Open
achamayou with Copilot wants to merge 13 commits into
mainfrom
copilot/rework-tls-to-match-openssl-style
Open

Rework TLS to OpenSSL-native style (memory BIOs, explicit status codes)#8002
achamayou with Copilot wants to merge 13 commits into
mainfrom
copilot/rework-tls-to-match-openssl-style

Conversation

Copilot AI commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Now that MbedTLS is gone from the tree, the TLS layer no longer needs to emulate its style. This reworks ccf::tls::Context and TLSSession to use OpenSSL idioms directly, removing the callback machinery and the "negative return" overloading. Net ~330 fewer lines.

Changes

  • In-memory BIOs replace BIO callbacks (context.h): set_bio() attaches plain BIO_s_mem read/write BIOs to the SSL. TLSSession feeds inbound ciphertext via recv() and drains outbound ciphertext via pending_write()/send() after each SSL operation. No more send_callback/recv_callback indirection.
  • Status separated from byte count (context.h, tls.h): handshake/read/write/close return 0 on success or an OpenSSL SSL_ERROR_* code (from SSL_get_error); bytes transferred are reported through an out-param. Removes the negative-return hack. Only TLS_ERR_X509_VERIFY remains as a CCF-specific sentinel to distinguish a handshake cert-verification failure (reported by OpenSSL as a generic SSL_ERROR_SSL) so callers can treat it as an auth failure.
  • OpenSSL error-queue hygiene (context.h): each SSL operation clears unrelated thread-local errors before calling OpenSSL, as required for reliable SSL_get_error() classification.
  • Callers check status explicitly (tls_session.h): read/flush/do_handshake/close switch on SSL_ERROR_WANT_READ/WANT_WRITE/ZERO_RETURN/etc. instead of inspecting negated/overloaded returns. Outbound ciphertext is drained into pending_out and retried on ring-buffer backpressure. Deletes the obsolete handle_send/handle_recv/*_callback_openssl/write_some paths.
  • PlaintextServer (plaintext_server.h): updated to the new virtual signatures over its own pair of memory BIOs.
  • Unit tests (tls/test/main.cpp): rewritten from a threaded socketpair + callback BIOs to a single-threaded BIO pump; all existing cases preserved. Added coverage for stale errors from unrelated OpenSSL operations, which must be cleared before calls interpreted by SSL_get_error().
  • Docs (tls/README.md, architecture/tls_internals.rst): describe the memory-BIO design and status-code contract; drop the MbedTLS-emulation and "future pure-OpenSSL" sections.

Read path, before/after

// Before: status smuggled through the return value (negated on error)
int n = ctx->read(buf, len);
if (n == TLS_ERR_WANT_READ) { ... }
else if (n < 0) { /* error */ }
else { /* n bytes */ }

// After: status and byte count are distinct
size_t readbytes = 0;
auto rc = ctx->read(buf, len, readbytes);
switch (rc)
{
  case 0: /* readbytes valid */ break;
  case SSL_ERROR_WANT_READ:
  case SSL_ERROR_WANT_WRITE: /* retry later */ break;
  case SSL_ERROR_ZERO_RETURN: /* peer closed */ break;
  default: /* error */ break;
}

Notes for reviewers

  • This is security-critical code; the cert-verification classification (peer_cert_ok() on SSL_ERROR_SSL) is the one place where OpenSSL's status is reinterpreted.
  • The Debug build, tls_test, and programmability_and_jwt pass locally. scripts/ci-checks.sh and all required PR CI jobs are green.

Copilot AI and others added 3 commits June 29, 2026 17:42
Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Copilot AI changed the title [WIP] Refactor TLS implementation to align with OpenSSL style Rework TLS to OpenSSL-native style (memory BIOs, explicit status codes) Jun 29, 2026
Copilot AI requested a review from achamayou June 29, 2026 17:48
@achamayou

Copy link
Copy Markdown
Member

@copilot check previous CI runs, find out what failed and fix it.

Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>

Copilot AI commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

@copilot check previous CI runs, find out what failed and fix it.

Investigated the failing runs (ACI SNP Genoa/Milan + VMSS Virtual C): all hung in programmability_and_jwt / test_jwt_without_key_policy. The node logs show the server generated the expected 400 but never delivered it; the client saw Server disconnected without sending a response. main is green on this test, so it was a regression from this PR.

Root cause: in the new memory-BIO design, TLSSession::read() only drained the write BIO before the read (via flush()). Ciphertext that the TLS layer produces during SSL_read (TLS 1.3 NewSessionTickets / post-handshake messages / key updates) was left stranded in the write BIO. The original BIO send-callback pushed those bytes to the host synchronously, so this is an asymmetry the rework introduced.

Fix (3a8... commit): drain outbound with flush_outbound() immediately after ctx->read(), restoring parity with the original behaviour. Please confirm tls_test and programmability_and_jwt are green in CI.

achamayou and others added 4 commits July 3, 2026 09:42
OpenSSL requires the thread-local error queue to be empty before SSL_get_error is used. Clear stale errors at each TLS operation boundary so unrelated crypto validation failures cannot close healthy sessions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copy link
Copy Markdown
Member

Diagnosed and fixed the remaining programmability_and_jwt failure in bca9375.

The invalid X.509 policy check leaves ASN.1 entries on OpenSSL's thread-local error queue. Because SSL_get_error() requires that queue to be empty before the corresponding SSL operation, the next no-data SSL_read_ex() was misclassified as SSL_ERROR_SSL rather than SSL_ERROR_WANT_READ. The session was then closed before its already-generated 400 response was delivered.

The fix clears stale errors before handshake/read/write/shutdown operations and adds a regression test that reproduces the misclassification. I also merged the latest main. Locally, tls_test and programmability_and_jwt pass through tests.sh, and scripts/ci-checks.sh passes.

@achamayou achamayou added 3.x-todo PRs which should be backported to 3.x bench-ab labels Jul 10, 2026
@achamayou achamayou marked this pull request as ready for review July 10, 2026 22:54
@achamayou achamayou requested a review from a team as a code owner July 10, 2026 22:54
Copilot AI review requested due to automatic review settings July 10, 2026 22:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 modernises CCF’s TLS stack to follow OpenSSL-native patterns now that the MbedTLS compatibility layer is gone. It replaces the callback-based I/O emulation with in-memory BIOs and makes TLS APIs return explicit OpenSSL SSL_ERROR_* status codes (with byte counts via out-params), simplifying control flow in TLSSession and related components.

Changes:

  • Replace BIO callbacks with explicit in-memory BIO pump APIs (Context::recv/pending_write/send) and update TLSSession to drive them.
  • Split status and byte counts for handshake/read/write/close, removing the previous negative/overloaded return scheme (keeping TLS_ERR_X509_VERIFY as a sentinel).
  • Update unit tests and docs to reflect the new memory-BIO design and status-code contract.

Custom instructions used:

  • .github/copilot-instructions.md
  • .github/instructions/reviewing.instructions.md

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
src/tls/context.h Switch Context to memory BIOs and explicit OpenSSL-style status codes.
src/enclave/tls_session.h Update session drive loop to feed/drain BIOs and switch on SSL_ERROR_* results.
src/tls/plaintext_server.h Adapt the non-TLS passthrough server to the new BIO-driven virtual interface.
src/tls/tls.h Remove legacy error macros; keep TLS_ERR_X509_VERIFY sentinel.
src/tls/test/main.cpp Rework TLS tests to a single-threaded “BIO pump” model; add stale-error coverage.
src/tls/README.md Update TLS design documentation to describe the new OpenSSL-native approach.
doc/architecture/tls_internals.rst Update architecture docs to describe BIO-driven I/O and status-code contract.
Comments suppressed due to low confidence (1)

src/tls/tls.h:14

  • tls.h uses INT_MIN for TLS_ERR_X509_VERIFY but doesn’t include . Relying on transitive includes is brittle; include (or switch to std::numeric_limits::min()) so this header is self-contained.
// Specific error to flag a certificate validation failure during the handshake.
// This is not emitted by OpenSSL itself (its handshake failures surface as
// SSL_ERROR_SSL), but is set by Context so the caller can distinguish an
// authentication failure from a generic error. We use a bogus negative value
// that won't match any OpenSSL SSL_ERROR_* code.
#define TLS_ERR_X509_VERIFY INT_MIN

#include "ccf/crypto/openssl/openssl_wrappers.h"

#include <string>

Comment thread src/tls/context.h
Comment thread src/tls/context.h
Comment thread src/tls/plaintext_server.h
Comment thread src/tls/plaintext_server.h
Comment thread src/enclave/tls_session.h
Comment thread src/enclave/tls_session.h
Comment thread src/enclave/tls_session.h
@achamayou achamayou added READY TO MERGE and removed 3.x-todo PRs which should be backported to 3.x labels Jul 11, 2026
Use size_t-safe BIO APIs and include SSL statuses in error diagnostics.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants