Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

20240828-WOLFSSL_DEBUG_TRACE_ERROR_CODES-TLS #7917

Merged

Conversation

douzzer
Copy link
Contributor

@douzzer douzzer commented Aug 29, 2024

src/internal.c: in wolfSSL_ERR_reason_error_string(), add missing error string for SCR_DIFFERENT_CERT_E.

wolfssl/ssl.h, wolfssl/error-ssl.h, wolfssl/wolfcrypt/error-crypt.h, wolfcrypt/src/error.c, and src/internal.c:

  • fix values of WOLFSSL_ERROR_SSL and WOLFSSL_ERROR_WANT_X509_LOOKUP to match OpenSSL values;
  • move legacy CyaSSL compat layer error codes from ssl.h to error-ssl.h and renumber them to conform to existing sequence;
  • move enum IOerrors from ssl.h to error-ssl.h to get picked up by support/gen-debug-trace-error-codes.sh;
  • add to enum wolfSSL_ErrorCodes negative counterparts for several positive error return constants;
  • include error-ssl.h from ssl.h;
  • add label (wolfCrypt_ErrorCodes) to error-crypt.h enum, and in wc_GetErrorString(), use switch ((enum wolfCrypt_ErrorCodes)error) to activate switch warnings for missing enums;
  • in wolfSSL_ERR_reason_error_string(), use switch((enum wolfSSL_ErrorCodes)error) to activate switch warnings for missing enums;
  • in ssl.h, add special-case WOLFSSL_DEBUG_TRACE_ERROR_CODES macros for WOLFSSL_FAILURE;
  • in error-crypt.h, add missing WOLFSSL_API attribute to wc_backtrace_render(); and
  • harmonize gating of error codes, ssl.h / error-ssl.h / internal.c:wolfSSL_ERR_reason_error_string() / api.c:error_test().

tests/api.c:

  • add error_test() adapted from wolfcrypt/test/test.c, checking all error strings for expected presence/absence and length, called from existing test_wolfSSL_ERR_strings().
  • in post_auth_version_client_cb(), add missing !NO_ERROR_STRINGS gating.

add numerous WC_NO_ERR_TRACE()s to operand error code uses, cleaning up error traces in general, and particularly when WOLFSSL_DEBUG_TRACE_ERROR_CODES_ALWAYS.

  • crypto lib (36),
  • crypto test&benchmark (20),
  • TLS lib (179),
  • examples (122),
  • linuxkm (3),
  • tests/api.c (2272).

tested with wolfssl-multi-test.sh ... super-quick-check with all-gcc-c99-backtrace added to super-quick-check, and an automated code search for unescaped operand error code references added to all-gcc-c99-backtrace.

…or string for SCR_DIFFERENT_CERT_E, and de-gate error strings previously gated on HAVE_HTTP_CLIENT.

tests/api.c: add error_test() adapted from wolfcrypt/test/test.c, checking all error strings for expected presence/absence and length, called from existing test_wolfSSL_ERR_strings().

wolfssl/ssl.h, wolfssl/error-ssl.h, and wolfssl/wolfcrypt/error-crypt.h:
* move several negative error return codes from ssl.h to error-ssl.h,
* renumber them to conform to existing sequence, and
* include error-ssl.h from ssl.h;
* add special-case WOLFSSL_DEBUG_TRACE_ERROR_CODES macros for WOLFSSL_FAILURE;
* add missing WOLFSSL_API attribute to wc_backtrace_render().

add numerous WC_NO_ERR_TRACE()s to operand error code uses, cleaning up error traces in general, and particularly when WOLFSSL_DEBUG_TRACE_ERROR_CODES_ALWAYS.
* crypto lib (36),
* crypto test&benchmark (20),
* TLS lib (179),
* examples (122),
* linuxkm (3),
* tests/api.c (2272).
@douzzer douzzer requested a review from SparkiDev August 29, 2024 04:14
@douzzer douzzer self-assigned this Aug 29, 2024
…or string for SCR_DIFFERENT_CERT_E.

wolfssl/ssl.h, wolfssl/error-ssl.h, wolfssl/wolfcrypt/error-crypt.h, wolfcrypt/src/error.c, and src/internal.c:
* fix values of WOLFSSL_ERROR_SSL and WOLFSSL_ERROR_WANT_X509_LOOKUP to match OpenSSL values;
* move legacy CyaSSL compat layer error codes from ssl.h to error-ssl.h and renumber them to conform to existing sequence;
* move enum IOerrors from ssl.h to error-ssl.h to get picked up by support/gen-debug-trace-error-codes.sh;
* add to enum wolfSSL_ErrorCodes negative counterparts for several positive error return constants;
* include error-ssl.h from ssl.h;
* add label (wolfCrypt_ErrorCodes) to error-crypt.h enum, and in wc_GetErrorString(), use switch ((enum wolfCrypt_ErrorCodes)error) to activate switch warnings for missing enums;
* in wolfSSL_ERR_reason_error_string(), use switch((enum wolfSSL_ErrorCodes)error) to activate switch warnings for missing enums;
* in ssl.h, add special-case WOLFSSL_DEBUG_TRACE_ERROR_CODES macros for WOLFSSL_FAILURE;
* in error-crypt.h, add missing WOLFSSL_API attribute to wc_backtrace_render(); and
* harmonize gating of error codes, ssl.h / error-ssl.h / internal.c:wolfSSL_ERR_reason_error_string() / api.c:error_test().

tests/api.c:
* add error_test() adapted from wolfcrypt/test/test.c, checking all error strings for expected presence/absence and length, called from existing test_wolfSSL_ERR_strings().
* in post_auth_version_client_cb(), add missing !NO_ERROR_STRINGS gating.

add numerous WC_NO_ERR_TRACE()s to operand error code uses, cleaning up error traces in general, and particularly when WOLFSSL_DEBUG_TRACE_ERROR_CODES_ALWAYS.
* crypto lib (36),
* crypto test&benchmark (20),
* TLS lib (179),
* examples (122),
* linuxkm (3),
* tests/api.c (2272).
@douzzer douzzer force-pushed the 20240828-WOLFSSL_DEBUG_TRACE_ERROR_CODES-TLS branch from 91cfc04 to 17870d4 Compare August 29, 2024 19:23
@dgarske dgarske self-requested a review August 29, 2024 20:05
@dgarske dgarske self-assigned this Aug 29, 2024
embhorn
embhorn previously approved these changes Aug 29, 2024
Copy link
Member

@embhorn embhorn left a comment

Choose a reason for hiding this comment

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

Wow! The unit test is good and noisy with that option!

dgarske
dgarske previously approved these changes Aug 29, 2024
Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Really nice work!

@douzzer douzzer assigned SparkiDev and wolfSSL-Bot and unassigned douzzer Aug 29, 2024
#if defined(OPENSSL_EXTRA) || defined(OPENSSL_EXTRA_X509_SMALL) || \
defined(HAVE_WEBSERVER) || defined(HAVE_MEMCACHED)

WOLFSSL_X509_V_ERR_CERT_SIGNATURE_FAILURE_E = -7, /* note conflict with
Copy link
Contributor

Choose a reason for hiding this comment

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

These don't belong here.
They are only meant to be in X509 verification error not returned through normal APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The incumbent implementation of wolfSSL_ERR_reason_error_string() returns error strings for these values. That's why the shadow values are now added in enum wolfSSL_ErrorCodes. It's all one namespace (of numbers) from the point of view of wolfSSL_ERR_reason_error_string().

That said, we can move the corresponding cases out of the main switch() in wolfSSL_ERR_reason_error_string(), enabling us to remove the shadow values in enum wolfSSL_ErrorCodes without provoking -Wswitch-enum, but then we lose the useful build time check that the cases are covered and self-consistent.

Either way, it's worth noting I didn't bother setting up error traces for WOLFSSL_X509_V_*. The sign-flipped _E variants added to enum wolfSSL_ErrorCodes aren't actually used by anything directly, though they're implicitly instrumented.

Copy link
Contributor

Choose a reason for hiding this comment

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

wolfSSL_ErrorCodes are returned errors.
WOLFSSL_X509_V_ERR_CERT_SIGNATURE_FAILURE_E is not and should not have a string associated with them when SetErrorString is called.
Don't mix the enums.

Copy link
Contributor

@SparkiDev SparkiDev Aug 30, 2024

Choose a reason for hiding this comment

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

But it is all going through wolfSSL_ERR_reason_error_string() as you said. facepalm
Well at least you'll get an error string out! Just that there is overlap!
These X509_V_ERRs aren't return error codes and shouldn't come out as strings when SetErrorString is called.

wolfSSL_ERR_reason_error_string() is all known error reasons, that is each failure has a unique value unlike wolfSSL.
OpenSSL does not include the X509_V_* errors in this and neither should we.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ha! So you're thinking we should remove that behavior of wolfSSL_ERR_reason_error_string()! That's where I was hoping this was headed.

@SparkiDev SparkiDev assigned douzzer and unassigned SparkiDev Aug 29, 2024
… for -WOLFSSL_X509_V_ERR_*, and make corresponding changes in wolfssl/error-ssl.h and tests/api.c.
@douzzer douzzer dismissed stale reviews from dgarske and embhorn via 255465a August 30, 2024 01:03
@douzzer
Copy link
Contributor Author

douzzer commented Aug 30, 2024

retest this please

…g for -WOLFSSL_X509_V_ERR_*, but separated from handling for the proper wolfSSL_ErrorCodes.
@douzzer douzzer requested a review from SparkiDev August 30, 2024 03:02
@douzzer douzzer assigned SparkiDev and unassigned dgarske and douzzer Aug 30, 2024
@SparkiDev SparkiDev merged commit d475ecc into wolfSSL:master Aug 30, 2024
129 checks passed
@douzzer douzzer mentioned this pull request Aug 31, 2024
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.

5 participants