Skip to content

Conversation

@ejohnstown
Copy link
Contributor

@ejohnstown ejohnstown commented May 16, 2025

  1. By default, soft disable AES-CBC. It isn't offered as a default encrypt algorithm, but may be set at runtime.
  2. Add guard where AES-CBC can be added back as a default.
  3. Add option to example client to run it with a custom encrypt algorithm list.
  4. In the client, add macro to add items to the arg lists while checking the number of items in the list.
    (ZD 19606)

This comment was marked as outdated.

This comment was marked as outdated.

@ejohnstown ejohnstown force-pushed the vvv branch 2 times, most recently from d7ec4bb to 4c7062f Compare May 16, 2025 16:29
@ejohnstown ejohnstown requested a review from Copilot May 16, 2025 16:36
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 soft-disable mechanism for AES-CBC by introducing a new preprocessor flag and updates the test and client example code to reflect configurable encryption settings.

  • Introduces new macros for adding string and integer arguments in kex tests
  • Adds the WOLFSSH_NO_AES_CBC_SOFT_DISABLE flag and integrates it in the advertised cipher list
  • Updates the example client to accept a custom cipher list via command-line arguments

Reviewed Changes

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

File Description
tests/kex.c Refactors argument addition using new ADD_ARG macros; expands client and server argument arrays to support additional options
src/internal.c Adds new soft disable flag for AES-CBC and adjusts advertised cipher list accordingly
examples/client/client.c Adds command-line support to specify a custom encryption algorithm list

This comment was marked as outdated.

1. By default, soft disable AES-CBC. It isn't offered as a default
   encrypt algorithm, but may be set at runtime.
2. Add guard where AES-CBC can be added back as a default.
3. Add option to example client to run it with a custom encrypt
   algorithm list.
4. In the client, add macro to add items to the arg lists while checking
   the number of items in the list.
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 soft disables AES-CBC by default while allowing its re-enablement through a runtime flag, and it adds new macros to safely build command-line argument lists for testing and client usage. Key changes include:

  • Introduction of the ADD_ARG and ADD_ARG_INT macros in tests/kex.c to build argument lists.
  • Addition of the WOLFSSH_NO_AES_CBC_SOFT_DISABLE flag and update to the encryption algorithm list in src/internal.c.
  • Updates to the client usage message and argument parsing in examples/client/client.c.

Reviewed Changes

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

File Description
tests/kex.c Added macros for argument list management and updated argument-building calls for server and client configurations.
src/internal.c Introduced WOLFSSH_NO_AES_CBC_SOFT_DISABLE and modified the encryption algorithm list condition.
examples/client/client.c Updated help text and command-line parsing to support a custom cipher list.

@douzzer douzzer closed this May 16, 2025
@douzzer douzzer reopened this May 16, 2025
@douzzer
Copy link
Contributor

douzzer commented May 16, 2025

This PR appears to be unmergeable due to a bug in githib (see https://github.com/orgs/community/discussions/144455)

A trick that often works is to close and reopen a PR. I've used that to clear a rebase conflict (the instant-dismissed-review syndrome). Doesn't work here.

@douzzer douzzer assigned ejohnstown and douzzer and unassigned wolfSSL-Bot May 16, 2025
@LinuxJedi LinuxJedi merged commit e0a1bdd into wolfSSL:master May 17, 2025
180 checks passed
@LinuxJedi
Copy link
Member

No problem merging for me, once I resolved the open conversation I could see.

@ejohnstown ejohnstown deleted the vvv branch May 19, 2025 15:22
@douzzer
Copy link
Contributor

douzzer commented May 19, 2025

Thanks @LinuxJedi -- not sure if I was missing something or if something changed, but all's well that ends well.

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