Update pipeline for OpenSSL #22
Conversation
- For users, which use a name which has uppercase name.
- To use my forked repository.
- To use the lower case name, for the docker container.
- Fixed CVE.
- Check an ML-KEM, ML-DSA and SLH-DSA certificate chain, if the OpenSSL version is >= 3.5
- To check whether the pipeline works or not.
- Updated unit tests. - Added new check for alg is SLH-DSA or ML-DSA. - Add debugging print_extensions.
- Move cannot_be_validated_with_openssl. - Update check for ML-KEM. - Add error message.
- Fix elif. - Add unit tests. - Add TODO for maybe use oqsprovider. - Renamed.
# Conflicts: # .github/workflows/check_quality.yml # REUSE.toml # client_tests/README.md # client_tests/cmp_client.py # client_tests/cmp_tests_jinja.robot # requirements.txt
- Verifies if the rf-test are run able or something went wrong.
…PipelineWithOpenSSL # Conflicts: # Makefile # data/dockerfiles/Dockerfile.base # requirements-dev.txt
- Changed OpenSSL version to 3.6. - Fix correct repo. name.
| if s1 != s2: | ||
| print("✗ secret mismatch") | ||
| return False | ||
| if len(s1) not in (32,): # ML-KEM typically yields 32-byte secrets |
There was a problem hiding this comment.
Why not not len(s1) == 32?
ralienpp
left a comment
ralienpp
left a comment
There was a problem hiding this comment.
This PR changes too many things, not just the logic related to what it takes to verify a chain with OpenSSL (e.g., RF runability check, changes in formatting here and there). It is difficult to review the delta, because it is cluttered by unnecessary changes.
- Which version of OpenSSL is actually needed for the feature you need?
- Debian's latest version has OpenSSL 3.5, would that work?
- The PR must be broken down into smaller, independent chunks.
|
Thanks for the feedback. You’re right about the PR does too much. I noticed the pipeline was failing because of leftover issues from earlier changes, and I thought it would be simpler to fix everything in one go. I didn’t consider how much harder that would make reviewing the actual logic changes. I’ll avoid that in the future and keep PRs strictly focused on a single feature or fix, with clear notes about anything that’s intentionally left out or deferred to a follow-up PR. Regarding OpenSSL: my intention was to use a custom version so we could adopt newer features sooner. For example, OpenSSL 3.7 might eventually include support for verifying HSS signatures. That said, for this PR specifically, you’re correct that OpenSSL 3.5 should be sufficient. I’ll double-check what the GitHub pipeline is using and switch to the appropriate Debian version. So should I than add this custom build in a new PR or is this not relevant? I’ll fix this PR into by cherry-pick only the relevant commits so the review is easier. |
At the moment the CI pipeline uses Ubuntu, maybe keeping it at that (instead of switching to Debian) would be easier; recent versions of Ubuntu come with OpenSSL 3.5 |
|
Yes I did it at #31. But should I add the custom build in a new (or this) PR or drop it? |
2 participants
Add the latest version of OpenSSL to the
Dockerfileto enable PQC certificate chain validation with OpenSSL.Note
Description
Dockerfile.baseto include the required new dependencies.cryptographylibrary.Motivation and Context
How Has This Been Tested?