Implementation of Composite Sig

[feventura]

This is an implementation of the version 10 Composite Signatures For Use In Internet PKI.
Current status of this work:

• I'm working on fixing the issues pointed by the Git Actions workflow tests.
• There will be alterations to update this for version 11 when it is released.
• To be precise with the IETF draft, we need to wait for ML-DSA-ipd to be implemented.

[baentsch]

Hi Felipe, very glad to see this contribution to the project. It's now coming at a suboptimal moment though: We're going through an external code security audit and can't accept large commits to "main" until that is complete. Would you consider contributing to a separate branch (say "entrust-composite") for now? That'd also give us opportunity to tune and complete together, e.g. pertaining to testing and integrating the pending addition of ml-dsa?

[feventura]

Hello Michael, that is unfortunate but the branch suggestion sounds good.
A bit update, I already made the changes for v11, and I'm working on the git actions issues.

[baentsch]

Code audit is through, so we're ready to move forward with this, Felipe. Please mark as Ready for Review as and when you feel this PR is in that stage.

[baentsch]

Thanks very much @feventura for all this work! I'm particularly impressed with how well you used the templating mechanism and that all testing is unchanged -- but does its job, looking at CI...

See single comments to get this over the finishing line. I glanced over the code without reviewing every single line, particularly wrt to composite handling (also as I don't have a grip on that spec). Please let me know if you consider this necessary. Otherwise I assume this code has been used successfully in the hackathon testing such as to ascertain interoperability with other implementations: OK assumption?

[baentsch]

PS: I'm working right now on a workaround to the Kyber/ML-KEM OID mess as well as the lack of ML-KEM/-DSA support in the distros. Should have a solution tomorrow to rebase on and get CI to "green". That said, the Windows CI failure may warrant your attention, @feventura .

[baentsch]

One more thought, @feventura : This is such a significant addition to oqsprovider that I'd like to ask you to please consider adding yourself to the contributors list while addressing the issues (and re-basing to main as CI now should "turn green" also on the PR).

[baentsch]

@feventura @SWilson4 Would you mind sharing your thoughts (at least with each other) as to which of your two PRs should be merged first?

My suggestion would be to merge Falcon-padded first such as to allow the Composite's to also support those. Any other opinion good for me, as long as the two of you agree with each other in order to minimize your respective effort re-basing.

[SWilson4]

@feventura @SWilson4 Would you mind sharing your thoughts (at least with each other) as to which of your two PRs should be merged first?

My suggestion would be to merge Falcon-padded first such as to allow the Composite's to also support those. Any other opinion good for me, as long as the two of you agree with each other in order to minimize your respective effort re-basing.

Makes sense to me. I imagine (?) we also want Falcon-padded to land ASAP so that oqs-provider can be released along liboqs.

[feventura]

@feventura @SWilson4 Would you mind sharing your thoughts (at least with each other) as to which of your two PRs should be merged first?

My suggestion would be to merge Falcon-padded first such as to allow the Composite's to also support those. Any other opinion good for me, as long as the two of you agree with each other in order to minimize your respective effort re-basing.

Go ahead, the composite draft dropped Falcon in EntrustCorporation/draft-ounsworth-composite-sigs#138 so it should not impact the composite implementation.

[baentsch]

@feventura : We're now closing in on the release: Is this ready from your perspective? Should I give it a final review?

[feventura]

@baentsch Yes, I fixed the problems with the windows CI

[baentsch]

@feventura Thanks again for all this work. One nit regarding documentation that you may want to fix while re-basing. Would it be possible to also put this all into one commit such as to not clutter the commit history?

Last item addressing @thb-sb : Would you have time to give this PR a second pair of "review" eyes prior to merge?

[ghost]

@feventura Thanks again for all this work. One nit regarding documentation that you may want to fix while re-basing. Would it be possible to also put this all into one commit such as to not clutter the commit history?

Last item addressing @thb-sb : Would you have time to give this PR a second pair of "review" eyes prior to merge?

Let me do that now.

[feventura]

@feventura Thanks again for all this work. One nit regarding documentation that you may want to fix while re-basing. Would it be possible to also put this all into one commit such as to not clutter the commit history?

Last item addressing @thb-sb : Would you have time to give this PR a second pair of "review" eyes prior to merge?

Thanks! (:
I'm doing the rebase today and solving the conflicts.
After @thb-sb give the feedback from the "review" I'll squash my commits into a single one.
I'll wait for this review in case I'll need to make further changes.

[ghost]

I think this PR deserves testing too. I don't feel it's right to add such a massive and exciting new feature without a single line of test code...

[baentsch]

Err, IMO this is tested--at least the new algs show up in CI... Testament to the quality of the test framework :-)

[baentsch]

Just re-started CI to re-confirm...

[baentsch]

@thb-sb , fyi, CI re-run confirms both command line and API-level test of (also) composite sigs to have occurred and passed.

[baentsch]

So, glancing over all comments I think we've only got things left that are not "Composite-specific", agree, @thb-sb ? So would it be good to ask @feventura for a final squash to then commit this to "main"? Or did I overlook something? I'd like to merge this to then move towards a release soon...

[feventura]

@baentsch @thb-sb I implemented the changed requested by Thomas and I solved the DCO check by rebasing to add signature to the old commits.
You should be able to squash all the commits before accepting the merge.
Is there some other changes you would like me to do?

[baentsch]

Thanks for the "good-to-go" message, @feventura . I think @thb-sb has some concerns still, but may look which ones are essential before landing and which ones should be covered as issues for future resolution. Regarding the merge, may I ask you to squash all into one commit such as for you to decide which commit message(s) to retain for "posteriority" -- I suppose not all 164 commit messages are relevant, right? If I were squashing this, I'd delete all messages -- but this is possibly not what you want (?).

[baentsch]

Thanks for the review, @thb-sb ! @feventura it looks like asan is unhappy about the code now (2 tests crashing): Could you please take a look?

[ghost]

Thanks for the review, @thb-sb ! @feventura it looks like asan is unhappy about the code now (2 tests crashing): Could you please take a look?

Regarding these ASan tests, I've noticed something quite unusual while working on #374, which makes me think that it's unrelated to @feventura changes: I also had totally random failure on this job. Since everything is compiled and linked against ASan, and since we have --output-on-failure, in case of a memory leak or a memory corruption, we should see an ASan report.

But in that case (and same for my PR), we don't have any ASan report being displayed before the segfault. This is why I've added --repeat until-pass:5 here: https://github.com/thb-sb/oqs-provider/blob/3b69bbe728819ad9b325580b79d9c9a672ae366e/.github/workflows/linux.yml#L149-L152

I think GitHub has changed their runner configuration recently, and something has been broken here. I've run these tests a million times locally using act (to ensure that they were running under the very same environment as GitHub action's). I couldn't managed to reproduce a single crash.

My guess is that since ASan is virtually allocating the whole address space to catch any memory corruption ops, someone may see this as a process requiring too much memory, although this is purely virtual.

[feventura]

Thanks for the review, @thb-sb ! @feventura it looks like asan is unhappy about the code now (2 tests crashing): Could you please take a look?

Regarding these ASan tests, I've noticed something quite unusual while working on #374, which makes me think that it's unrelated to @feventura changes: I also had totally random failure on this job. Since everything is compiled and linked against ASan, and since we have --output-on-failure, in case of a memory leak or a memory corruption, we should see an ASan report.

But in that case (and same for my PR), we don't have any ASan report being displayed before the segfault. This is why I've added --repeat until-pass:5 here: https://github.com/thb-sb/oqs-provider/blob/3b69bbe728819ad9b325580b79d9c9a672ae366e/.github/workflows/linux.yml#L149-L152

I think GitHub has changed their runner configuration recently, and something has been broken here. I've run these tests a million times locally using act (to ensure that they were running under the very same environment as GitHub action's). I couldn't managed to reproduce a single crash.

My guess is that since ASan is virtually allocating the whole address space to catch any memory corruption ops, someone may see this as a process requiring too much memory, although this is purely virtual.

Yes, I found weird that ASan reported as SegFault but I have not extra information about it. I also tested locally several times locally using act and it didn`t crashed once.

What I noticed is that the GitHub actions in this PR failed due to oqs_signatures and oqs_endecode. On my fork it passed successfully when I pushed Saturday, so I repeated on my fork today and it failed due to oqs_tlssig and oqs_endecode, which also point to the totally random failure on this job.

Regardless, I will run act locally several times to see today if I can reproduce it.

[ghost]

Let me open an issue on GitHub' support regarding this issue.
In the meantime, I think it's safe to use the workaround I've found with ctest (the --repeat until-pass:5)

edit: just opened a ticket (it seems that we can't share tickets on GitHub…)

[baentsch]

added --repeat until-pass:5

worthwhile trying also in this PR, @feventura? Clearly just a workaround, but better than wasting more effort on something we may not be able to help

[baentsch]

@feventura @thb-sb Thanks again for your diligence & support to work through this PR. If I don't hear objections, I will merge later today, squashing all commits into one just saying "Adding CompositeSig support".

[ghost]

@feventura @thb-sb Thanks again for your diligence & support to work through this PR. If I don't hear objections, I will merge later today, squashing all commits into one just saying "Adding CompositeSig support".

Sounds good to me!

[baentsch]

Merged. Thanks very much for the contribution, @feventura !

[bhess]

Yes, I found weird that ASan reported as SegFault but I have not extra information about it.

I had the same issue with another project starting last week. Seems to be an issue with the github runner image. There's a fix that appears to be deployed soon.

[ghost]

Yes, I found weird that ASan reported as SegFault but I have not extra information about it.

I had the same issue with another project starting last week. Seems to be an issue with the github runner image. There's a fix that appears to be deployed soon.

Wow, I missed that!
Thank you Basil :)