-
Notifications
You must be signed in to change notification settings - Fork 91
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
Composite sigs update #549
Conversation
Signed-off-by: venturf <[email protected]>
Signed-off-by: feventura <[email protected]>
…the right pss is not found Signed-off-by: feventura <[email protected]>
Signed-off-by: feventura <[email protected]>
Signed-off-by: feventura <[email protected]>
Signed-off-by: feventura <[email protected]>
Signed-off-by: feventura <[email protected]>
Signed-off-by: feventura <[email protected]>
Signed-off-by: feventura <[email protected]>
Signed-off-by: feventura <[email protected]>
Signed-off-by: feventura <[email protected]>
Changed the logic on get_composite_idx to one that would not depend on the idx might be an issue if the two lists compared are not in sync, instead the search to get the OID is done by the algorithm name. |
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.
Thanks for the update @feventura and sorry for the delay reviewing: I had hoped other committers take a look but after 2 weeks guess that won't happen (or they don't like the code :).
So, generally I'm OK with the diffs I understand -- but cannot claim I do understand all. But given this code is not for serious use but meant to support research and testing, that's arguably OK (unless someone objects?).
So my question to you (and maybe @praveksharma if you're still following the IETF interop hackathon?): Is this code working OK there (not just for composite but also for plain PQC and hybrid)? That for me would be enough of an indication to approve the PR. Merging this would also be a "functional change" justification to do a new release, agreed, @praveksharma ?
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.
Thanks for the update and explanations, @feventura . LGTM for now.
@praveksharma, would you be OK merging this and cutting an RC for 0.7.2 giving that some real reason for existence? Or would you want me to do that?
Thank you for reviewing this PR @baentsch, I will handle the 0.7.2 release. |
Thanks @praveksharma ! Before doing that, can I repeat the question whether you (or anyone else) tested this functionally, e.g., in the IETF hackathon testbed? |
I believe there is interop testing being done with composite signatures but not all participants have them implemented just yet (see here). As more participants implement composite sigs interop testing may uncover issues that need addressing. At the moment the oqs-provider implementation isn't being tested, I shall enable these tests along with making the switch to the R4 format during the hackathon this weekend. |
@baentsch I missed you question about IETF hackathon. |
Thanks for the pointers, @feventura . Looking at the matrix it seems |
Talking pure PQ, yes it is just passing MLDSA44ipd and some SLH-DSA algs, the MLDSA65ipd, MLDSA87, Falcon512 and Falcon1024 are failing on the bouncycastle interop. (you can see that by looking at the tables below the big one) |
Thanks for the contribution and background information @feventura ! |
This PR include: