Skip to content

Conversation

@shane-digi
Copy link

@shane-digi shane-digi commented Jun 20, 2025

Add SQIsign from upstream to liboqs.

Fixes #1946

  • Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
  • Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider will also need to be ready for review and merge by the time this is merged.)

@shane-digi
Copy link
Author

There are quite a few things that will need to be changed here but I thought I'd get the ball rolling on this. Besides all of the formatting issues that will have to be addressed there is probably an issue with the included mini-gmp. It's lgpl and I don't think that's what is wanted so we'll need to figure out what to do about GMP. @bhess knows about that and is working to help with this and the changes to upstream.

@praveksharma
Copy link
Member

Thank you for getting this started @shane-digi! I've approved CI workflows and they're running now. Please have a look here to see how to resolve the failing DCO test.

@shane-digi
Copy link
Author

The broadwell builds need -maes as a compiler flag. How would you like that added?

@dstebila
Copy link
Member

The broadwell builds need -maes as a compiler flag. How would you like that added?

Is this because the code brings its own implementation of AES? Or because it happens to use one of the instructions available from -maes for some other purpose?

@shane-digi
Copy link
Author

The broadwell builds need -maes as a compiler flag. How would you like that added?

Is this because the code brings its own implementation of AES? Or because it happens to use one of the instructions available from -maes for some other purpose?

It has it's own AES. I'm going to work on using the OQS one instead. That should fix the issues.

@dstebila
Copy link
Member

The broadwell builds need -maes as a compiler flag. How would you like that added?

Is this because the code brings its own implementation of AES? Or because it happens to use one of the instructions available from -maes for some other purpose?

It has it's own AES. I'm going to work on using the OQS one instead. That should fix the issues.

That would be our preference, since we have optimized AES backends for a variety of targets. Unless it is particularly problematic to disentangle.

@shane-digi
Copy link
Author

Any ideas/preferences on how to handle the GMP dependency (I'm not sure when @bhess is back from vacation but he has some ideas I believe)?

@bhess
Copy link
Member

bhess commented Jul 7, 2025

Any ideas/preferences on how to handle the GMP dependency (I'm not sure when @bhess is back from vacation but he has some ideas I believe)?

I'm just back today :) Thanks a lot, @shane-digi, for kicking off the SQIsign integration. I’ll take a closer look at the PR this week.
Regarding the GMP dependency: one approach could be to avoid importing the mini-gmp sources from upstream, since they’re not actually needed. Instead, we could dynamically link against a system-installed GMP library (e.g., via standard Linux package managers). This is, for example, handled in the upstream CMake configuration, and using add_library(GMP SHARED IMPORTED).

For liboqs users who don’t have GMP installed, or prefer not to depend on it, we could consider adding a configuration option to disable the GMP dependency and algorithms that depend on it entirely.

@shane-digi
Copy link
Author

Add dependency on GMP.
Don't build sqisign if GMP is not available.
Is this what you would like me to do?

Add gmp to nix [full tests]

Signed-off-by: Basil Hess <[email protected]>
@bhess
Copy link
Member

bhess commented Jul 16, 2025

I've added a commit on top of this PR to link against GMP: see the branch here.
I'll also check the CI containers to ensure GMP is properly installed.
Please feel free to incorporate this commit into the PR if you find it helpful.

That said, I believe there's still a blocker before we can proceed with merging: sqisign currently includes dpe.h, which is LGPL-licensed. Since liboqs does not distribute LGPL-licensed code, we'll need to wait for an upstream update that removes this dependency.

In the meantime, it might be best to convert this PR to a draft until the dependency issue is resolved.

@praveksharma
Copy link
Member

In the meantime, it might be best to convert this PR to a draft until the dependency issue is resolved.

Converting to draft now, apologies if this issue is already resolved.

@praveksharma praveksharma marked this pull request as draft August 13, 2025 18:59
@baentsch
Copy link
Member

As this looks a bit "dormant" (not to say abandoned), question to @open-quantum-safe/oqs-committers : How shall this move towards merge-ability now that Pravek is not active in the project any more? Anyone willing to take the review lead? @shane-digi are you still interested in merging this? If so, could you please re-base to get rid of the conflicts?

@dstebila
Copy link
Member

As this looks a bit "dormant" (not to say abandoned), question to @open-quantum-safe/oqs-committers : How shall this move towards merge-ability now that Pravek is not active in the project any more? Anyone willing to take the review lead? @shane-digi are you still interested in merging this? If so, could you please re-base to get rid of the conflicts?

This has been blocked by licensing concerns which @hartm spoke about in the TSC meeting earlier this week, which he is continuing to work on resolving.

@bhess
Copy link
Member

bhess commented Sep 18, 2025

I've been working in the meantime to resolve some issues and get the CI passing. You can see the progress on this branch. This should allow us to move forward once the license blocker is addressed.
@shane-digi, would you be okay if I open a PR based on that branch, superseding this one? Your commit history will of course be preserved.

@shane-digi
Copy link
Author

I've been working in the meantime to resolve some issues and get the CI passing. You can see the progress on this branch. This should allow us to move forward once the license blocker is addressed. @shane-digi, would you be okay if I open a PR based on that branch, superseding this one? Your commit history will of course be preserved.

I'm fine with that. Whatever is easiest.

@bhess bhess mentioned this pull request Sep 18, 2025
2 tasks
@bhess
Copy link
Member

bhess commented Sep 18, 2025

I've been working in the meantime to resolve some issues and get the CI passing. You can see the progress on this branch. This should allow us to move forward once the license blocker is addressed. @shane-digi, would you be okay if I open a PR based on that branch, superseding this one? Your commit history will of course be preserved.

I'm fine with that. Whatever is easiest.

Ok, closing this PR, see #2277.

@bhess bhess closed this Sep 18, 2025
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.

[Feature request] SQIsign

5 participants