Skip to content

Comments

interop: fix for engine removal#29333

Closed
ep69 wants to merge 2 commits intoopenssl:masterfrom
ep69:fix-interop
Closed

interop: fix for engine removal#29333
ep69 wants to merge 2 commits intoopenssl:masterfrom
ep69:fix-interop

Conversation

@ep69
Copy link
Contributor

@ep69 ep69 commented Dec 8, 2025

TLS interoperability tests need to be fixed now that engines are removed.

Checklist
  • documentation is added or updated
  • tests are added or updated

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Dec 8, 2025
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Dec 8, 2025
@ep69 ep69 force-pushed the fix-interop branch 2 times, most recently from 7a8385f to 6ab65f9 Compare December 8, 2025 14:46
@t8m t8m added branch: master Applies to master branch triaged: bug The issue/pr is/fixes a bug tests: exempted The PR is exempt from requirements for testing labels Dec 8, 2025
@ep69
Copy link
Contributor Author

ep69 commented Dec 8, 2025

Please don't merge yet, it is not final and does not seem to work yet..

@ep69
Copy link
Contributor Author

ep69 commented Dec 8, 2025

I am struggling with not being able to generate certificates with certgen.

So far, it seems to me that something happened in openssl between commits c20d470 (good) and 55a0adf (bad). If somebody has clues what it might be, please let me know. I will continue looking into this tomorrow.

Reproducer:

# source /interop/Library/certgen/lib.sh && x509KeyGen ca && x509SelfSign ca
Using configuration from ca/ca.cnf
Check that the request matches the signature
Signature ok
The Subject's Distinguished Name is as follows
organizationName      :ASN.1 12:'Example CA'
Segmentation fault (core dumped)
x509SelfSign: signing the certificate failed

@nhorman
Copy link
Contributor

nhorman commented Dec 8, 2025

@ep69 I see the problem, I'll have a PR for you shortly

nhorman added a commit to nhorman/openssl that referenced this pull request Dec 8, 2025
We have a few cases in which one of the paramters passed to
ASN1_TIME_diff is null (i.e. the caller doesn't care about the psec
differnce and so passes NULL as that pointer parameter).

However, OPENSSL_gmtime_diff assumes both pointers are valid, and so
writes to them unilaterally resulting in a crash as observed here:
openssl#29333 (comment)

Check the pointers before writing to them.
@nhorman
Copy link
Contributor

nhorman commented Dec 8, 2025

@ep69 #29337 should fix you up.

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Dec 9, 2025
@ep69 ep69 changed the title interop: fix and update interop: fix for engine removal Dec 9, 2025
@ep69
Copy link
Contributor Author

ep69 commented Dec 9, 2025

@nhorman thanks for your fix, I tested it and it seems to work (when patched manually). Shall we wait until #29337 is merged and then trigger the tests to make sure they work indeed?

@nhorman
Copy link
Contributor

nhorman commented Dec 9, 2025

@nhorman thanks for your fix, I tested it and it seems to work (when patched manually). Shall we wait until #29337 is merged and then trigger the tests to make sure they work indeed?

I think yes, please. Lets land #29337, and then you can rebase on top of it

@beldmit
Copy link
Member

beldmit commented Dec 9, 2025

@ep69 could you please also file the ICLA? The corporate CLA already covers your contribuitons, but the individual one is still necessary

openssl-machine pushed a commit that referenced this pull request Dec 9, 2025
We have a few cases in which one of the paramters passed to
ASN1_TIME_diff is null (i.e. the caller doesn't care about the psec
differnce and so passes NULL as that pointer parameter).

However, OPENSSL_gmtime_diff assumes both pointers are valid, and so
writes to them unilaterally resulting in a crash as observed here:
#29333 (comment)

Check the pointers before writing to them.

Reviewed-by: Tomas Mraz <[email protected]>
Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Norbert Pocs <[email protected]>
(Merged from #29337)
cxx194832 pushed a commit to cxx194832/openssl that referenced this pull request Dec 12, 2025
We have a few cases in which one of the paramters passed to
ASN1_TIME_diff is null (i.e. the caller doesn't care about the psec
differnce and so passes NULL as that pointer parameter).

However, OPENSSL_gmtime_diff assumes both pointers are valid, and so
writes to them unilaterally resulting in a crash as observed here:
openssl#29333 (comment)

Check the pointers before writing to them.

Reviewed-by: Tomas Mraz <[email protected]>
Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Norbert Pocs <[email protected]>
(Merged from openssl#29337)
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Dec 12, 2025
@ep69 ep69 marked this pull request as ready for review December 12, 2025 15:10
@ep69 ep69 requested a review from quarckster as a code owner December 12, 2025 15:10
@t8m t8m added the approval: review pending This pull request needs review by a committer label Dec 12, 2025
@t8m t8m requested a review from nhorman December 12, 2025 15:47
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

Note for merging - the drop! commit must be dropped.

@nhorman nhorman added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Dec 12, 2025
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Dec 13, 2025
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@nhorman
Copy link
Contributor

nhorman commented Dec 14, 2025

merged to master with 2nd commit dropped. Thank you!

@nhorman nhorman closed this Dec 14, 2025
openssl-machine pushed a commit that referenced this pull request Dec 14, 2025
CLA: trivial

Reviewed-by: Neil Horman <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29333)
@ep69 ep69 deleted the fix-interop branch December 14, 2025 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants