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

OCSP openssl compat fixes #8498

Merged
merged 14 commits into from
Feb 28, 2025
Merged

OCSP openssl compat fixes #8498

merged 14 commits into from
Feb 28, 2025

Conversation

rizlik
Copy link
Contributor

@rizlik rizlik commented Feb 24, 2025

Description

  • Fix NO_SHA builds
  • Fix i2d and d2i CERT ID functions

Check that responderID.ByKey is exactly WC_SHA_DIGEST_SIZE as per RFC
6960. KEYID_SIZE can change across build configuration.
tests blobs contains sha-1 hashes in certificate status
- Added validation for digest type in `wolfSSL_OCSP_cert_to_id` function.
- Defined `OCSP_DIGEST` based on available hash types.
- Set `hashAlgoOID` in `certId` based on `OCSP_DIGEST`.
- Updated `asn.h` to define `OCSP_DIGEST` and `OCSP_DIGEST_SIZE` based on
  available hash types.
@rizlik rizlik force-pushed the ocsp_fixes branch 5 times, most recently from ce26fea to 25891dd Compare February 25, 2025 19:26
It will be reused in d2i_CERT_ID
@rizlik rizlik changed the title Ocsp fixes OCSP openssl compat fixes Feb 25, 2025
@rizlik rizlik marked this pull request as ready for review February 25, 2025 22:34
@douzzer
Copy link
Contributor

douzzer commented Feb 25, 2025

retest this please (org.jenkinsci.plugins.workflow.support.steps.AgentOfflineException)

Copy link
Contributor

@douzzer douzzer left a comment

Choose a reason for hiding this comment

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

This fixes all the PQ tests I ran, but it breaks with --enable-sm2. I don't see the failures either without --enable-sm2, or on master, just on the combination.

FAILURES:
   750: test_wolfSSL_OCSP_id_get0_info
   759: test_wolfSSL_OCSP_REQ_CTX
   1090: test_ocsp_certid_enc_dec

Repro with --enable-all --enable-sm3 after moving SM3 sources into place from the wolfsm repo (needs wolfcrypt/src/sm2.c, wolfcrypt/src/sm2_asm.S, wolfcrypt/src/sp_sm2_x86_64.c, and wolfssl/wolfcrypt/sm2.h).

@dgarske dgarske requested a review from douzzer February 26, 2025 22:37
dgarske
dgarske previously approved these changes Feb 26, 2025
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.

I pushed fixes.

@dgarske dgarske assigned SparkiDev and unassigned SparkiDev Feb 26, 2025
@dgarske
Copy link
Contributor

dgarske commented Feb 26, 2025

Retest this please: "PRB-generic-config-parser" no console.

@SparkiDev SparkiDev assigned rizlik and unassigned SparkiDev Feb 27, 2025
@rizlik rizlik requested a review from SparkiDev February 27, 2025 12:56
@rizlik rizlik removed their assignment Feb 27, 2025
@rizlik
Copy link
Contributor Author

rizlik commented Feb 27, 2025

retest this (org.jenkinsci.plugins.workflow.support.steps.AgentOfflineException)

we don't properly support SM2 and SM3 hash algo id properly yet
@SparkiDev
Copy link
Contributor

retest this please

Copy link
Contributor

@douzzer douzzer left a comment

Choose a reason for hiding this comment

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

This still needs at least one more gate:

diff --cc tests/api.c
index 0ee61ddef,0ee61ddef..12807e4fa
--- a/tests/api.c
+++ b/tests/api.c
@@@ -71657,7 -71657,7 +71657,8 @@@ static int test_wolfSSL_OCSP_parse_url(
  
  #if defined(OPENSSL_ALL) && defined(HAVE_OCSP) && \
      defined(WOLFSSL_SIGNER_DER_CERT) && !defined(NO_FILESYSTEM) && \
--    !defined(NO_ASN_TIME)
++    !defined(NO_ASN_TIME) && \
++    !defined(WOLFSSL_SM2) && !defined(WOLFSSL_SM3)
  static time_t test_wolfSSL_OCSP_REQ_CTX_time_cb(time_t* t)
  {
      if (t != NULL) {
@@@ -71672,7 -71672,7 +71673,8 @@@ static int test_wolfSSL_OCSP_REQ_CTX(vo
  {
      EXPECT_DECLS;
  #if defined(OPENSSL_ALL) && defined(HAVE_OCSP) && \
--    defined(WOLFSSL_SIGNER_DER_CERT) && !defined(NO_FILESYSTEM)
++    defined(WOLFSSL_SIGNER_DER_CERT) && !defined(NO_FILESYSTEM) && \
++    !defined(WOLFSSL_SM2) && !defined(WOLFSSL_SM3)
      /* This buffer was taken from the ocsp-stapling.test test case 1. The ocsp
       * response was captured in wireshark. It contains both the http and binary
       * parts. The time test_wolfSSL_OCSP_REQ_CTX_time_cb is set exactly so that

However, even with that patch, wolfSM is still incompatible with OCSP. For now I've modified the ENABLE_ALL_WOLFSM feature set in multi-test to disable OCSP. This exposed additional problems with wolfSM, unrelated to this PR.

@SparkiDev SparkiDev self-assigned this Feb 28, 2025
@SparkiDev SparkiDev merged commit 4f8a39c into wolfSSL:master Feb 28, 2025
178 checks passed
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