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

Add PKCS#12 test #400

Merged
merged 4 commits into from
Apr 27, 2024
Merged

Add PKCS#12 test #400

merged 4 commits into from
Apr 27, 2024

Conversation

iyanmv
Copy link
Member

@iyanmv iyanmv commented Apr 18, 2024

OpenSSL supports PKCS#12 and can be used to store in a single encrypted file the private key and certificate.

This PR adds a new test that tries to generate .p12 files for all the supported algorithms. Unfortunately, when oqsprovider is not enabled and -provider oqsprovider is used instead, this generation fails. OpenSSL still returns 0, so I don't know how to catch this issue. Ideas are welcome.

When oqsprovider is not enabled in the config file, errors are printed in the terminal, but the .p12 files are still generated and openssl exits with 0.

See also: #398

@baentsch
Copy link
Member

Thanks for this PR, @iyanmv ! This is exactly how I'd hoped to see a new test.

As everything seems to pass, I'm now trying to understand where the error becomes visible, though: When I run the new script, there's no error appearing; .p12 files get generated, even if commenting out the 2 lines after the "pkcs12 test" comment and using the tests with the tweaked oqsprovider.cnf file (commented as "fails"): Suggestions how to trigger the problem very welcome. I've been using openssl3.0.2 in the hope that as the oldest version it should show the problem easiest... Which version did you use?

@iyanmv
Copy link
Member Author

iyanmv commented Apr 21, 2024

I'm using OpenSSL 3.2.1. I just realized that even though errors are printed (and saved to interop.log), the .p12 files are (sometimes) correctly generated. I will double check again the particular example where I was getting the error but also no file at all, because I know I forgot.

@baentsch
Copy link
Member

I will double check again the particular example where I was getting the error but also no file at all, because I know I forgot.

Looking forward to your instructions how to trigger this then.

As and when you have time, please also rebase your PR to the latest main as I had to add a workaround to cygwin testing to get this to pass CI (openssl "master" seems to crash with the provider).

@iyanmv
Copy link
Member Author

iyanmv commented Apr 25, 2024

@baentsch I cannot replicate the original issue anymore, so probably I did something else wrong. What I still observe (and can be seen in the interop.log file) is that an error message is shown when oqsprovider is not enabled in the config file, but the .p12 file is still generated correctly.

I have rebased the PR with the latest main, and modify the test a little bit. Let me know if it's still useful or not.

@baentsch
Copy link
Member

Let me thing if it's still useful or not?

The added test definitely is useful (to confirm P12 generation works OK) -- but the tests should pass....

@iyanmv
Copy link
Member Author

iyanmv commented Apr 25, 2024

Ups, I think it fails now because of how I check the hashes (sha256sum does not exist in macOS). But I can do that with openssl directly.

See: oqs-provider/discussions/398
Signed-off-by: Iyán Méndez Veiga <[email protected]>
@iyanmv
Copy link
Member Author

iyanmv commented Apr 25, 2024

Fixed

Signed-off-by: Iyán Méndez Veiga <[email protected]>
.gitignore Outdated Show resolved Hide resolved
Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

Thanks again for this improved testing! LGTM with one nit (single comment). I'd squash all into one comment upon merge. Also a question whether you'd like to add yourself to the "Contributors" section in the README (?). Surely good would be an additional statement pertaining to PKCS12 support in https://github.com/open-quantum-safe/oqs-provider?tab=readme-ov-file#status.

@baentsch baentsch merged commit 86605d7 into open-quantum-safe:main Apr 27, 2024
21 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.

2 participants