Skip to content

Add WOLFSSL_STRICT_CIPHER_LIST for TLS 1.3-only cipher lists on a non-TLS1.3 ctx/ssl#10963

Open
miyazakh wants to merge 1 commit into
wolfSSL:masterfrom
miyazakh:f3228_cipher_list
Open

Add WOLFSSL_STRICT_CIPHER_LIST for TLS 1.3-only cipher lists on a non-TLS1.3 ctx/ssl#10963
miyazakh wants to merge 1 commit into
wolfSSL:masterfrom
miyazakh:f3228_cipher_list

Conversation

@miyazakh

Copy link
Copy Markdown
Contributor

Summary(f3228)

  • wolfSSL_CTX_set_cipher_list() / wolfSSL_set_cipher_list() silently
    return WOLFSSL_SUCCESS without touching the configured suites when
    given a cipher list that names only TLS 1.3 suites, but the ctx/ssl
    cannot negotiate TLS 1.3 (OPENSSL_EXTRA builds only, via
    wolfSSL_parse_cipher_list() in src/ssl.c).
  • This mirrors OpenSSL's SSL_CTX_set_ciphersuites() compatibility shim,
    but a caller that treats the non-failing return as confirmation that
    the restriction took effect will keep negotiating whatever <= TLS 1.2
    suites were already configured (default or otherwise).
  • Add an opt-in WOLFSSL_STRICT_CIPHER_LIST build macro. When defined,
    the function returns WOLFSSL_FAILURE instead of silently ignoring
    the list. Off by default, so existing OpenSSL-compat behavior is
    unchanged unless a user explicitly opts in.

Changes

  • src/ssl.c: gate the return value in wolfSSL_parse_cipher_list()
    behind WOLFSSL_STRICT_CIPHER_LIST.
  • src/internal.c: document the new macro in the build-options header
    comment (Cipher Suite Selection section).
  • .wolfssl_known_macro_extras: register WOLFSSL_STRICT_CIPHER_LIST.
  • tests/api/test_tls13.c, tests/api/test_tls13.h: add
    test_tls13_cipher_list_no_tls13_ctx, covering both the default
    (WOLFSSL_SUCCESS) and strict (WOLFSSL_FAILURE) behavior.

Testing

  • ./configure --enable-opensslextra --enable-tls13 --enable-debug && make — builds clean.
  • ./tests/unit.test (default, WOLFSSL_STRICT_CIPHER_LIST
    undefined): Failed/Skipped/Passed/All: 0/314/1726/2040.
  • ./tests/unit.test (CPPFLAGS="-DWOLFSSL_STRICT_CIPHER_LIST"):
    Failed/Skipped/Passed/All: 0/314/1726/2040.
  • test_tls13_cipher_list_no_tls13_ctx passes in both
    configurations, exercising wolfSSL_CTX_set_cipher_list(ctx, "TLS13-AES128-GCM-SHA256") on a wolfTLSv1_2_client_method()
    context.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@miyazakh miyazakh self-assigned this Jul 21, 2026
Copilot AI review requested due to automatic review settings July 21, 2026 22:36

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

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Adds an opt-in build macro to make wolfSSL_{CTX,}_set_cipher_list() fail (instead of silently succeeding) when a TLS 1.3-only cipher list is applied to a context/SSL that cannot negotiate TLS 1.3, and adds tests/documentation to cover the new behavior.

Changes:

  • Add WOLFSSL_STRICT_CIPHER_LIST to make TLS 1.3-only cipher lists return failure on non-TLS1.3 ctx/ssl.
  • Document and register the new macro in internal build-options docs and known-macro list.
  • Add a TLS 1.3 API test covering both default and strict behaviors.

Reviewed changes

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

Show a summary per file
File Description
src/ssl.c Gates return value to fail under WOLFSSL_STRICT_CIPHER_LIST for TLS 1.3-only lists on non-TLS1.3 ctx/ssl.
src/internal.c Documents the new build macro in the cipher suite selection options list.
.wolfssl_known_macro_extras Registers WOLFSSL_STRICT_CIPHER_LIST as a known macro.
tests/api/test_tls13.c Adds a new test exercising strict vs default behavior.
tests/api/test_tls13.h Declares and registers the new test in the TLS 1.3 test group.
Comments suppressed due to low confidence (1)

src/internal.c:1

  • The text wolfSSL_CTX/SSL_set_cipher_list() is ambiguous (it reads like a single function name). Consider updating to the exact API names (e.g., wolfSSL_CTX_set_cipher_list() / wolfSSL_set_cipher_list()) for clarity and consistency with the rest of the build-options documentation.

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

Comment thread src/ssl.c
Comment thread tests/api/test_tls13.c
@miyazakh

Copy link
Copy Markdown
Contributor Author

retest this please

3 similar comments
@miyazakh

Copy link
Copy Markdown
Contributor Author

retest this please

@miyazakh

Copy link
Copy Markdown
Contributor Author

retest this please

@miyazakh

Copy link
Copy Markdown
Contributor Author

retest this please

@miyazakh miyazakh assigned wolfSSL-Bot and unassigned miyazakh Jul 24, 2026

@Frauschi Frauschi 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.

🐺 Skoll Code Review

Overall recommendation: APPROVE
Findings: 3 total — 3 posted, 0 skipped

Posted findings

  • [Medium] Test only exercises the CTX path, not the SSL pathtests/api/test_tls13.c:2466-2489
  • [Low] Test asserts return value but not that suites are actually left untouchedtests/api/test_tls13.c:2480-2484
  • [Low] WOLFSSL_STRICT_CIPHER_LIST can now fail ctx creation under WOLFSSL_SYS_CRYPTO_POLICYsrc/ssl.c:636-641

Review generated by Skoll via Claude/Codex

Comment thread tests/api/test_tls13.c
return EXPECT_RESULT();
}

int test_tls13_cipher_list_no_tls13_ctx(void)

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.

🟡 [Medium] Test only exercises the CTX path, not the SSL path
💡 SUGGEST test

The new macro gates the return value inside wolfSSL_parse_cipher_list(), which backs BOTH wolfSSL_CTX_set_cipher_list() (via ctx) and wolfSSL_set_cipher_list() (via ssl, src/ssl.c:4041). The PR body states the change and test cover both entry points, but the test only calls wolfSSL_CTX_set_cipher_list(). The ssl->version branch of the condition (ssl != NULL && !IsAtLeastTLSv1_3(ssl->version)) is never exercised, so a regression that broke only the SSL path would pass CI.

Suggestion: Add a parallel case that creates a WOLFSSL* from a TLS 1.2 ctx and calls wolfSSL_set_cipher_list(ssl, "TLS13-AES128-GCM-SHA256"), asserting WOLFSSL_FAILURE (strict) / WOLFSSL_SUCCESS (default), to cover the ssl->version branch.

Comment thread tests/api/test_tls13.c
#ifdef WOLFSSL_STRICT_CIPHER_LIST
ExpectIntEQ(wolfSSL_CTX_set_cipher_list(ctx, "TLS13-AES128-GCM-SHA256"),
WOLFSSL_FAILURE);
#else

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.

🔵 [Low] Test asserts return value but not that suites are actually left untouched
💡 SUGGEST test

The default-path comment claims the call succeeds "but leave ctx->suites untouched," yet the test only checks the WOLFSSL_SUCCESS return code. The distinguishing behavior (that the prior <=TLS1.2 suites remain and the TLS1.3-only list did NOT overwrite them) is not asserted. This is the exact silent-no-op behavior the PR is calling out as a footgun, so it would be valuable to pin it down.

Suggestion: Capture ctx->suites->suiteSz (or the suite bytes) before the call and assert they are unchanged after, confirming the TLS1.3-only list was actually ignored rather than merely returning success.

Comment thread src/ssl.c
@@ -3895,8 +3895,17 @@ static int wolfSSL_parse_cipher_list(WOLFSSL_CTX* ctx, WOLFSSL* ssl,
tls13Only = 1;

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.

🔵 [Low] WOLFSSL_STRICT_CIPHER_LIST can now fail ctx creation under WOLFSSL_SYS_CRYPTO_POLICY
💡 SUGGEST question

Beyond the two public setters named in the PR, wolfSSL_parse_cipher_list() is also called during ctx construction when WOLFSSL_SYS_CRYPTO_POLICY is enabled. With WOLFSSL_STRICT_CIPHER_LIST defined, a system crypto policy that lists only TLS 1.3 suites applied to a <=TLS1.2 method would now make wolfSSL_parse_cipher_list() return WOLFSSL_FAILURE, causing wolfSSL_CTX_new_ex to free the ctx and return NULL where it previously succeeded. This is a plausibly-intended consequence of opting into strict behavior (fail loudly), but it is a broader blast radius than the two setters described in the PR body and is not covered by tests. Worth confirming this interaction is intended and documenting it.

Recommendation: Confirm the WOLFSSL_SYS_CRYPTO_POLICY + WOLFSSL_STRICT_CIPHER_LIST interaction (ctx creation can now fail) is intended, and note it in the macro documentation.

@Frauschi Frauschi assigned miyazakh and unassigned wolfSSL-Bot Jul 24, 2026
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.

4 participants