Skip PKCS7 with indefinite length test in AWS-LC #871
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rhenium
reviewed
Mar 21, 2025
test/openssl/test_pkcs7.rb
Outdated
| @@ -376,14 +378,9 @@ def test_split_content | |||
| END | |||
| pki_msg = OpenSSL::PKCS7.new(pki_message_pem) | |||
| store = OpenSSL::X509::Store.new | |||
| pki_msg.verify(nil, store, nil, OpenSSL::PKCS7::NOVERIFY) | |||
| assert_equal(pki_msg.verify(nil, store, nil, OpenSSL::PKCS7::NOVERIFY), true) | |||
There was a problem hiding this comment.
Suggested change
| assert_equal(pki_msg.verify(nil, store, nil, OpenSSL::PKCS7::NOVERIFY), true) | |
| assert_equal(true, pki_msg.verify(nil, store, nil, OpenSSL::PKCS7::NOVERIFY)) |
test-unit's assert_equal takes the expected value as the first argument.
AWS-LC had been decoding the indefinite BER to an unusable output. We should skip the test until indefinite BER decoding in AWS-LC is properly fixed. Changes: 1. AWS-LC will be looking to fix the parsing for indefinite BER constructed strings in AWS-LC soon, so I've marked the test as `pend` for now and removed the AWS-LC specific logic at the end. 2. I've added an assertion to verify that `OpenSSL::PKCS7.verify` behaves correctly before doing content comparisons. I noticed this was failing initially in AWS-LC, but that will be fixed soon as well. This shouldn't effect OpenSSL/LibreSSL builds and should improve the test.
samuel40791765
force-pushed
the
update-awslc-pkcs7
branch
from
d90c352 to
69fd7f8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extension from this original thread regarding the PKCS7 test we skipped in AWS-LC: #855 (comment)
We've been doing more investigations against the PKCS7 issue like we promised. It turns out @rhenium was right in that this was an issue with "decoding" indefinite BER rather than "encoding".
ASN1_TFLG_NDEFis in charge of encoding indefinite BER for PKCS7 in OpenSSL, but that's only used in the "PKCS7 streaming APIs" and is not used by default. The PKCS7 test file has indefinite BER within it, which AWS-LC was not properly decoding.AWS-LC had been decoding the indefinite BER to an unusable output. Instead of allowing an invalid state to be parsed, we've decided to revert aws/aws-lc@2a72226 (with aws/aws-lc@1a1eb18) until we've actually fixed our parsing for indefinite BER. The Ruby PKCS7 test will bail earlier with the new changes, so we thought it'd be best for us to help you update to minimize churn.
Changes:
pendfor now and removed the AWS-LC specific logic at the end.OpenSSL::PKCS7.verifybehaves correctly before doing content comparisons. I noticed this was failing in AWS-LC and will be fixed with Add support for verifying PKCS7 signed attributes aws/aws-lc#2264. This shouldn't effect OpenSSL/LibreSSL builds and should improve the test.