-
Notifications
You must be signed in to change notification settings - Fork 97
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 CROSS #461
Add CROSS #461
Conversation
1571720
to
b988894
Compare
Signed-off-by: rtjk <[email protected]>
Signed-off-by: rtjk <[email protected]>
Signed-off-by: rtjk <[email protected]>
Signed-off-by: rtjk <[email protected]>
@SWilson4 Do you know how one can trigger GH CI on this PR again? Our CCI script clearly has a bug (indicating a pass while failing) but I'm not overly interested in fixing that given #248. Second question: How can one trigger a (new) CI run using the CROSS PR/branch in |
Signed-off-by: rtjk <[email protected]>
b988894
to
6ad2a96
Compare
Signed-off-by: rtjk <[email protected]>
6ad2a96
to
65d1eb2
Compare
Signed-off-by: rtjk <[email protected]>
I just rebased, we should now be synched with It looks to me like CI is failing because at various points liboqs' main branch is being cloned instead of my fork; the build process fails because it can't find CROSS, which is not part of main yet. This happens in oqs-provider/scripts/fullbuild.sh Line 77 in a0f475d
oqs-provider/.github/workflows/linux.yml Line 127 in a0f475d
oqs-provider/.circleci/config.yml Line 45 in a0f475d
I just tried switching these and a few more cloning operations from I noticed for MAYO CI was initially skipped, should we do the same here? Or maybe wait for CROSS to be added to liboqs? |
The |
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.
Functionally and wrt testing LGTM. See single comments regarding "management & support".
For the first, you can go over to one of the jobs (e.g., https://github.com/open-quantum-safe/oqs-provider/actions/runs/10452640069/job/28946958204?pr=461) and hit "Re-run jobs" -> "Re-run all jobs". For the second, I wrote the '-tracker' script trigger code with release testing in mind, so I didn't take forks into account. I'm pretty sure it will only work for |
That should be easy to establish: Just push the "add-cross[-tracker]" branches into the OQS name space and add a [trigger-downstream] commit... Only "little issue": It worked fine for See status and command sequence below (& sorry for the German of my system's installation):
|
I just added an |
I could be wrong, but I think this was failing because the branch name had a hyphen and the push command used an underscore :/ |
Looking at the release test I notice it enables all algorithms, so all 18 variants of CROSS are on. oqs-provider/scripts/release-test-ci.sh Lines 22 to 23 in a0f475d
And it fails to do a TLS handshake with one parameter set only: CROSSrsdp256fast.
My first guess would be that the signature produced by this variant is too large, it looks like it's the only one in liboqs with |
One algorithm needs to be disabled. See single comment above.
Signed-off-by: rtjk <[email protected]>
Signed-off-by: rtjk <[email protected]>
And FWIW, this time it's only (but consistently) "CROSSrsdp256balanced" failing... When looking at the cert sizes, they're conspicuously large:
But other than that observation I'm afraid I can only recommend running the test yourself and debugging into this, @rtjk . |
Thanks for the catch @baentsch, it looks like rsdp-256-balanced is also too big for TLS (
|
Signed-off-by: rtjk <[email protected]>
Signed-off-by: rtjk <[email protected]>
The proposed configuration update in open-quantum-safe/tsc#77 will fix the CODEOWNERS file issue by providing @alexrow with write permissions (which is necessary for the CODEOWNERS file to work as intended). |
commit 7754b08 Author: rtjk <[email protected]> Date: Tue Aug 27 04:36:34 2024 +0200 updated codeowners and contributors Signed-off-by: rtjk <[email protected]> commit d1b29f4 Author: rtjk <[email protected]> Date: Tue Aug 27 04:14:22 2024 +0200 disabled CROSSrsdp256balanced because it (also) is too large for TLS Signed-off-by: rtjk <[email protected]> commit 008df36 Author: rtjk <[email protected]> Date: Thu Aug 22 12:17:22 2024 +0200 format code Signed-off-by: rtjk <[email protected]> commit e988f78 Author: rtjk <[email protected]> Date: Thu Aug 22 11:49:15 2024 +0200 disabled CROSSrsdp256fast because it's too large for TLS Signed-off-by: rtjk <[email protected]> commit 77c6818 Author: rtjk <[email protected]> Date: Mon Aug 19 13:39:39 2024 +0200 format code Signed-off-by: rtjk <[email protected]> commit 65d1eb2 Author: rtjk <[email protected]> Date: Mon Aug 19 13:29:46 2024 +0200 re-run generate.py Signed-off-by: rtjk <[email protected]> commit 45e961e Author: rtjk <[email protected]> Date: Mon Aug 19 11:58:28 2024 +0200 fromat code Signed-off-by: rtjk <[email protected]> commit 3b08dde Author: rtjk <[email protected]> Date: Tue Aug 13 02:11:50 2024 +0200 switched to official OIDs, added @rtjk as code owner for generate.yml Signed-off-by: rtjk <[email protected]> commit c8f447f Author: rtjk <[email protected]> Date: Tue Aug 6 13:34:01 2024 +0200 switched to free OIDs, temporarely enabled only one CROSS variant Signed-off-by: rtjk <[email protected]> commit c032f53 Author: rtjk <[email protected]> Date: Mon Aug 5 11:15:40 2024 +0200 run generate.py Signed-off-by: rtjk <[email protected]> commit 532d2ae Author: rtjk <[email protected]> Date: Mon Aug 5 11:14:07 2024 +0200 added CROSS to generate.yml Signed-off-by: rtjk <[email protected]>
@rtjk Can I ask you to re-base this PR to the latest main and trigger a CI re-run to validate all "turns green" now so we can proceed to merge? |
Closing since #530 has landed. |
@rtjk @alexrow Please accept my sincere apologies for closing and replacing your PR with #530: While I have not been party to this approach nor approve of it as maintainer valuing every contributor's work with an interest to see it come to completion I think it has been borne out of interest by @praveksharma and @SWilson4 to make progress and move |
Like @baentsch says, the decision to go ahead with #530 was primarily motivated by the desire to see an earlier release through with support for CROSS. My intent was not to devalue or undermine anyone's contributions to this project; although, I see how my actions could come across as such and for that I sincerely apologise to @rtjk and @alexrow. I hope I have not discouraged you from contributing to OQS projects and shall strive to work towards more organised releases moving forward. |
@baentsch @praveksharma No worries at all, I totally understand the situation. Happy to see CROSS merged in oqs-provider too :) |
Tracker for liboqs#1881 which adds the CROSS signature scheme.
NOTE: The Object Identifiers in
generate.yml
are placeholders for now, we are in the process of obtaing new OIDs for CROSS.