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 CROSS #461

Closed
wants to merge 11 commits into from
Closed

Add CROSS #461

wants to merge 11 commits into from

Conversation

rtjk
Copy link
Contributor

@rtjk rtjk commented Aug 5, 2024

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.

oqs-template/generate.yml Outdated Show resolved Hide resolved
oqs-template/generate.yml Outdated Show resolved Hide resolved
@rtjk rtjk marked this pull request as ready for review August 6, 2024 16:25
@baentsch
Copy link
Member

@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 liboqs (and not "main")? Thanks in advance for suggestions/HowTos/pointers!

Signed-off-by: rtjk <[email protected]>
Signed-off-by: rtjk <[email protected]>
@rtjk
Copy link
Contributor Author

rtjk commented Aug 19, 2024

Would you want to re-base and/or investigate the CI failures first? Or would you want me to look into the failure reasons in CI?

I just rebased, we should now be synched with oqs-provider:main.

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 fullbuild.sh (which is used in many tests) as well as in various GitHub and CircleCI jobs.

git clone --depth 1 --branch $LIBOQS_BRANCH https://github.com/open-quantum-safe/liboqs.git

git clone --depth=1 --branch main https://github.com/open-quantum-safe/liboqs.git liboqs

git clone --depth 1 --branch main https://github.com/open-quantum-safe/liboqs.git &&

I just tried switching these and a few more cloning operations from open-quantum-safe/liboqs:main to rtjk/liboqs:add-cross and all GitHub tests passed.

I noticed for MAYO CI was initially skipped, should we do the same here? Or maybe wait for CROSS to be added to liboqs?

@baentsch
Copy link
Member

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.

The liboqs branch referenced exactly can be set to a different value when triggering CI at liboqs for such downstream test. Normally, this requires a specific commit tag -- but I'm not sure how to trigger a re-run of that PR. Hence my question above to @SWilson4 in this regard. @rtjk please note that open-quantum-safe/liboqs#1881 also needs re-basing....

.github/CODEOWNERS Outdated Show resolved Hide resolved
baentsch
baentsch previously approved these changes Aug 20, 2024
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.

Functionally and wrt testing LGTM. See single comments regarding "management & support".

@SWilson4
Copy link
Member

SWilson4 commented Aug 20, 2024

@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 liboqs (and not "main")? Thanks in advance for suggestions/HowTos/pointers!

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 open-quantum-safe branches. :(

@baentsch
Copy link
Member

I'm pretty sure it will only work for open-quantum-safe branches. :(

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 oqs-provider but did not for liboqs: Is it possibly a GH permissions issue again? @ryjones : Should I be able to execute successfully the command git push --set-upstream origin add_cross in liboqs?

See status and command sequence below (& sorry for the German of my system's installation):

baentsch:~/git/oqs/liboqs$ git status
Auf Branch add-cross
Ihr Branch ist auf demselben Stand wie 'rtjk/add-cross'.

nichts zu committen, Arbeitsverzeichnis unverändert
baentsch:~/git/oqs/liboqs$ git push --set-upstream origin add_cross
error: Src-Refspec add_cross entspricht keiner Referenz.
error: Fehler beim Versenden einiger Referenzen nach 'github.com:open-quantum-safe/liboqs.git'

@SWilson4
Copy link
Member

I just added an add-cross branch in liboqs and triggered the release tests.

@SWilson4
Copy link
Member

SWilson4 commented Aug 21, 2024

baentsch:~/git/oqs/liboqs$ git status
Auf Branch add-cross
Ihr Branch ist auf demselben Stand wie 'rtjk/add-cross'.

nichts zu committen, Arbeitsverzeichnis unverändert
baentsch:~/git/oqs/liboqs$ git push --set-upstream origin add_cross
error: Src-Refspec add_cross entspricht keiner Referenz.
error: Fehler beim Versenden einiger Referenzen nach 'github.com:open-quantum-safe/liboqs.git'

I could be wrong, but I think this was failing because the branch name had a hyphen and the push command used an underscore :/

@baentsch baentsch self-requested a review August 21, 2024 19:30
@rtjk
Copy link
Contributor Author

rtjk commented Aug 21, 2024

Looking at the release test I notice it enables all algorithms, so all 18 variants of CROSS are on.

# Activate all algorithms
sed -i "s/enable\: false/enable\: true/g" oqs-template/generate.yml

And it fails to do a TLS handshake with one parameter set only: CROSSrsdp256fast.

SSL_accept() failed returning -1, SSL error 1.
809B9FB4A07F0000:error:0A0C0103:SSL routines:tls_construct_cert_verify:internal error:ssl/statem/statem_lib.c:415:
SSL_connect() failed returning -1, SSL error 1.
809B9FB4A07F0000:error:0A000438:SSL routines:ssl3_read_bytes:tlsv1 alert internal error:ssl/record/rec_layer_s3.c:908:SSL alert number 80
  TLS-SIG handshake test failed: CROSSrsdp256fast, return code: -5

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 CRYPTO_BYTES larger than 64K.

oqs-template/generate.yml Outdated Show resolved Hide resolved
@baentsch baentsch dismissed their stale review August 22, 2024 07:26

One algorithm needs to be disabled. See single comment above.

@baentsch
Copy link
Member

@rtjk FYI, the full "release-level" test got much farther now, but then still failed: See log. You should be able to locally reproduce by running ./scripts/release-test.sh.

@baentsch
Copy link
Member

And FWIW, this time it's only (but consistently) "CROSSrsdp256balanced" failing... When looking at the cert sizes, they're conspicuously large:

-rw-rw-r-- 1 baentsch baentsch  17986 Aug 22 16:04 tmp/CROSSrsdp128balanced_CA.crt
-rw-rw-r-- 1 baentsch baentsch  26436 Aug 22 16:04 tmp/CROSSrsdp128fast_CA.crt
-rw-rw-r-- 1 baentsch baentsch  14151 Aug 22 16:04 tmp/CROSSrsdp128small_CA.crt
-rw-rw-r-- 1 baentsch baentsch  38774 Aug 22 16:04 tmp/CROSSrsdp192balanced_CA.crt
-rw-rw-r-- 1 baentsch baentsch  58355 Aug 22 16:04 tmp/CROSSrsdp192fast_CA.crt
-rw-rw-r-- 1 baentsch baentsch  32571 Aug 22 16:04 tmp/CROSSrsdp192small_CA.crt
-rw-rw-r-- 1 baentsch baentsch  69747 Aug 22 16:04 tmp/CROSSrsdp256balanced_CA.crt
-rw-rw-r-- 1 baentsch baentsch 103933 Aug 21 21:25 tmp/CROSSrsdp256fast_CA.crt
-rw-rw-r-- 1 baentsch baentsch  59639 Aug 22 16:04 tmp/CROSSrsdp256small_CA.crt
-rw-rw-r-- 1 baentsch baentsch  12977 Aug 22 16:04 tmp/CROSSrsdpg128balanced_CA.crt
-rw-rw-r-- 1 baentsch baentsch  17361 Aug 22 16:04 tmp/CROSSrsdpg128fast_CA.crt
-rw-rw-r-- 1 baentsch baentsch  11247 Aug 22 16:04 tmp/CROSSrsdpg128small_CA.crt
-rw-rw-r-- 1 baentsch baentsch  32173 Aug 22 16:04 tmp/CROSSrsdpg192balanced_CA.crt
-rw-rw-r-- 1 baentsch baentsch  37620 Aug 22 16:04 tmp/CROSSrsdpg192fast_CA.crt
-rw-rw-r-- 1 baentsch baentsch  25140 Aug 22 16:04 tmp/CROSSrsdpg192small_CA.crt
-rw-rw-r-- 1 baentsch baentsch  54890 Aug 22 16:04 tmp/CROSSrsdpg256balanced_CA.crt
-rw-rw-r-- 1 baentsch baentsch  66814 Aug 22 16:04 tmp/CROSSrsdpg256fast_CA.crt
-rw-rw-r-- 1 baentsch baentsch  44880 Aug 22 16:04 tmp/CROSSrsdpg256small_CA.crt
-rw-rw-r-- 1 baentsch baentsch  11247 Aug 22 16:03 tmp/gw0_CROSSrsdpg128small_CA.crt
-rw-rw-r-- 1 baentsch baentsch  25140 Aug 22 16:07 tmp/gw0_CROSSrsdpg192small_CA.crt
-rw-rw-r-- 1 baentsch baentsch  54890 Aug 22 16:07 tmp/gw0_CROSSrsdpg256balanced_CA.crt
-rw-rw-r-- 1 baentsch baentsch  66814 Aug 22 16:08 tmp/gw0_CROSSrsdpg256fast_CA.crt
-rw-rw-r-- 1 baentsch baentsch  44880 Aug 22 16:08 tmp/gw0_CROSSrsdpg256small_CA.crt
-rw-rw-r-- 1 baentsch baentsch  11247 Aug 22 16:07 tmp/gw3_CROSSrsdpg128small_CA.crt
-rw-rw-r-- 1 baentsch baentsch  32173 Aug 22 16:08 tmp/gw3_CROSSrsdpg192balanced_CA.crt
-rw-rw-r-- 1 baentsch baentsch  37620 Aug 22 16:08 tmp/gw3_CROSSrsdpg192fast_CA.crt
-rw-rw-r-- 1 baentsch baentsch  25140 Aug 22 16:08 tmp/gw3_CROSSrsdpg192small_CA.crt
-rw-rw-r-- 1 baentsch baentsch  54890 Aug 22 16:03 tmp/gw3_CROSSrsdpg256balanced_CA.crt
-rw-rw-r-- 1 baentsch baentsch  38774 Aug 22 16:07 tmp/gw4_CROSSrsdp192balanced_CA.crt
-rw-rw-r-- 1 baentsch baentsch  58355 Aug 22 16:08 tmp/gw4_CROSSrsdp192fast_CA.crt
-rw-rw-r-- 1 baentsch baentsch  32571 Aug 22 16:08 tmp/gw4_CROSSrsdp192small_CA.crt
-rw-rw-r-- 1 baentsch baentsch  69747 Aug 22 16:13 tmp/gw4_CROSSrsdp256balanced_CA.crt
-rw-rw-r-- 1 baentsch baentsch  59639 Aug 22 16:03 tmp/gw4_CROSSrsdp256small_CA.crt
-rw-rw-r-- 1 baentsch baentsch  59639 Aug 22 16:07 tmp/gw5_CROSSrsdp256small_CA.crt
-rw-rw-r-- 1 baentsch baentsch  12977 Aug 22 16:08 tmp/gw5_CROSSrsdpg128balanced_CA.crt
-rw-rw-r-- 1 baentsch baentsch  17361 Aug 22 16:08 tmp/gw5_CROSSrsdpg128fast_CA.crt
-rw-rw-r-- 1 baentsch baentsch  11247 Aug 22 16:09 tmp/gw5_CROSSrsdpg128small_CA.crt
-rw-rw-r-- 1 baentsch baentsch  17986 Aug 22 16:08 tmp/gw6_CROSSrsdp128balanced_CA.crt
-rw-rw-r-- 1 baentsch baentsch  26436 Aug 22 16:08 tmp/gw6_CROSSrsdp128fast_CA.crt
-rw-rw-r-- 1 baentsch baentsch  14151 Aug 22 16:09 tmp/gw6_CROSSrsdp128small_CA.crt
-rw-rw-r-- 1 baentsch baentsch  38774 Aug 22 16:09 tmp/gw6_CROSSrsdp192balanced_CA.crt
-rw-rw-r-- 1 baentsch baentsch  44880 Aug 22 16:07 tmp/gw7_CROSSrsdpg256small_CA.crt

But other than that observation I'm afraid I can only recommend running the test yourself and debugging into this, @rtjk .

@rtjk
Copy link
Contributor Author

rtjk commented Aug 23, 2024

Thanks for the catch @baentsch, it looks like rsdp-256-balanced is also too big for TLS (SSL_R_EXCESSIVE_MESSAGE_SIZE). I'm taking a deeper look but we might have to remove this variant too.

. > .local/bin/openssl s_client -groups kyber512 -CAfile tmp/gw1_CROSSrsdp256balanced_CA.crt -verify_return_error -status -showcerts -connect localhost:45631
Connecting to ::1
Can't use SSL_get_servername
803B9278717F0000:error:0A000098:SSL routines:read_state_machine:excessive message size:ssl/statem/statem.c:647:
CONNECTED(00000003)
---
no peer certificate available
---
No client certificate CA names sent
---
SSL handshake has read 17313 bytes and written 1064 bytes
Verification: OK
---
New, TLSv1.3, Cipher is TLS_AES_256_GCM_SHA384
Protocol: TLSv1.3
This TLS version forbids renegotiation.
Compression: NONE
Expansion: NONE
No ALPN negotiated
Early data was not sent
Verify return code: 0 (ok)
---

@SWilson4
Copy link
Member

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).

SWilson4 added a commit that referenced this pull request Sep 26, 2024
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]>
@baentsch
Copy link
Member

baentsch commented Oct 2, 2024

@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?

This was referenced Oct 2, 2024
@praveksharma
Copy link
Member

Closing since #530 has landed.

@baentsch
Copy link
Member

baentsch commented Oct 3, 2024

@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 oqsprovider towards release and alleviating you of tedious work (rebasing). @alexrow please see #531 with an action on you.

@praveksharma
Copy link
Member

make progress and move oqsprovider towards release and alleviating you of tedious work (rebasing).

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.

@rtjk
Copy link
Contributor Author

rtjk commented Oct 3, 2024

@baentsch @praveksharma No worries at all, I totally understand the situation. Happy to see CROSS merged in oqs-provider too :)

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.

5 participants