-
Notifications
You must be signed in to change notification settings - Fork 717
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
Fix: update default cert chain for unit tests #4582
Conversation
Can you try testing this with the patch that RHEL9 is using? I think it shouldn't be too hard. |
I appreciate that your work on a fix for the Issue I've reported. If there is anything I can do to help, please let me know. |
@wombelix since it is difficult for us to replicate RHEL9 environment, could you apply this patch to see if it fixes the issue? We have updated our test certificate chain so it now uses SHA256 instead of SHA1.
|
Of course, my pleasure to help :)
|
Thank you for testing @wombelix Hmm this one looks like another issue outside of SHA1 deprecation, but I'm not exactly sure why it fails. The certs used in this test are using SHA256 with RSAE. It might be helpful if you could troubleshoot this to see why this specific test is not working with RHEL9. @maddeleine @goatgoose any guesses? |
I was wrong. The OCSP response is using SHA1 and that could be the problem.
In order to fix this the OCSP response file needs to be updated to not use SHA1 |
It seems that we currently only use sha1 for cert_id and therefore the test fails even with an OCSP response generated with sha256: s2n-tls/tls/s2n_x509_validator.c Lines 877 to 879 in ff03b94
To move forward, we should still update the default cert chain with sha256, but we do not plan to support sha256 for cert_id digest for now without sufficient investigations on why/why not. @wombelix you might just need to modify or turn this test off for now. |
tests/unit/s2n_x509_validator_test.c
Outdated
EXPECT_SUCCESS(s2n_connection_set_cipher_preferences(connection, "default")); | ||
|
||
/* alter a random byte in the certificate to make it invalid */ | ||
chain_data[500] = (uint8_t) (chain_data[500] << 2); | ||
chain_data[200] = (uint8_t) (chain_data[200] << 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use chain_len - 1
or something here to make sure we're not writing past chain_data
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is trying to alter the "end-entity certificate" to make it invalid. I believe that is stored earlier in the chain_data[] array so using chain_len - 1
does not work. If I use smaller values like 5
, it then throws error decoding certificate
presumably because it messes up the header information and now doesn't know how to decode the cert. I'm not sure if there is a good way to skip this header bytes accurately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it okay. In that case, could we at least assert that 200 doesn't index past chain_data
?
chain_data[200] = (uint8_t) (chain_data[200] << 2); | |
size_t corrupt_index = 200; | |
EXPECT_TRUE(chain_len > corrupt_index); | |
chain_data[corrupt_index] = (uint8_t) (chain_data[corrupt_index] << 2); |
Actually, sha1 used on cert_id may not matter. What matters is sha1 used for signature algorithm. I updated the oscp response file to use sha256 with rsa encryption. @wombelix can you apply this patch on top of the previous patch to see if it resolves the error?
|
I was able to replicate RHEL9 build issues thanks to @maddeleine's help. Steps to recreate the issue:
The updated cert and ocsp response file seem to be passing the unit test, so I can conform this change will fix the RHEL9 build issue. |
tests/unit/s2n_x509_validator_test.c
Outdated
EXPECT_SUCCESS(s2n_connection_set_cipher_preferences(connection, "default")); | ||
|
||
/* alter a random byte in the certificate to make it invalid */ | ||
chain_data[500] = (uint8_t) (chain_data[500] << 2); | ||
chain_data[200] = (uint8_t) (chain_data[200] << 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it okay. In that case, could we at least assert that 200 doesn't index past chain_data
?
chain_data[200] = (uint8_t) (chain_data[200] << 2); | |
size_t corrupt_index = 200; | |
EXPECT_TRUE(chain_len > corrupt_index); | |
chain_data[corrupt_index] = (uint8_t) (chain_data[corrupt_index] << 2); |
Resolved issues:
Resolves #4541 where unit tests fail due to SHA1 deprecation when built with RHEL9 environment
Description of changes:
currently,
S2N_DEFAULT_CERT_CHAIN
is set torsa_2048_pkcs1_cert.pem
which is signed with SHA1, and we want a similar cert chain that uses signature algorithm that is not SHA1to do this:
S2N_DEFAULT_TEST_CERT_CHAIN
is now defined withrsae_pkcs_2048_sha256/server-chain.pem
S2N_DEFAULT_TEST_PRIVATE_KEY
is now defined withrsae_pkcs_2048_sha256/server-key.pem
Some tests are modified to adopt cert changes
s2n-tls/tests/pems/ocsp/ocsp_response_revoked.der
is now signed withsha256WithRSAEncryption
instead ofsha1WithRSAEncryption
Call-outs:
Testing:
Confirmed all unit tests pass. We will reach out the user in order to confirm this change fixes their issue with RHEL9 build
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.