search

open-quantum-safe / oqs-demos

PARTIALLY SUPPORTED Instructions for enabling the use of quantum-safe cryptography in assorted software using the OQS suite. CONTRIBUTORS WANTED.
https://openquantumsafe.org/
132 stars 74 forks source link

Pin libraries to current releases #298

Open ajbozarth opened 2 months ago

ajbozarth commented 2 months ago

Sets the default tag for openssl, liboqs, oqs-provider to the current latest relese instead of main/master. Also updates curl to the latest release.

Inlcudes some fixes to support multi-platform builds, now supporting both linux/amd64 and linux/arm64, where only linux/amd64 worked before.

Related Issues: #230 #213 #182

Fixes #230 Fixes #182 Fixes #273 Fixes #226 Fixes #171

Demos updated (ordered by interest):

ajbozarth commented 2 months ago

I was originally going to open an issue about the fact that the curl demo couldn't build/run on macOS without the --platform linux/amd64 flag, but as I worked on this changes I made solved that issue. Also #213 seems to partially cover that issue

baentsch commented 1 month ago

@ajbozarth Thanks for the fix to the lib64 installpath. What's missing in my view is testing (CI) for this: I'd suggest two runs (via GH CI): One building the "main/master" image as "canary" for OQS; the second one the "version pegged" image for user consumption. Of course only the latter should be pushed to docker hub during merge. And also, it should be a cross-platform image, thus truly testing x64 and arm64 "suitability". Thanks.

planetf1 commented 1 month ago

The curl/README.md should be updated with any new arguments, and the change in defaults (Rather than stating explicit version it could just say 'updated to the most recent versions that have been tested' or similar).

planetf1 commented 1 month ago

Noticed that text in README.md refers to how to build the image, and provides a few examples. But the tag varies.

Once we get to USAGE.md it states to rundocker run -it openquantumsafe/curl perftest.sh- so by default this will run the version from dockerhub.

It could be helpful to at least have the default build example using the correct tag perhaps docker build . -t open-quantum-safe/curl

ajbozarth commented 1 month ago

What's missing in my view is testing (CI) for this

@baentsch After our discussion during this week's sync up I took another look and realized I had only investigated the GitHub Actions and not circleci, leading me to think I would have to write tests from scratch. Which I evaluated would be better done in a follow up PR, but upon finding the circleci test configs I will make updates there. Though as #294 says, we should move our circleci automation to gh actions in a future PR.

But the tag varies

@planetf1 I will take a look through the docs, and make sure the tag it uses (docker hub or local build) matches the context of the section it's in. If it doesn't or is unclear I will made the edit or add clarification to the surrounding text

ajbozarth commented 1 month ago

I just pushed an update addressing @planetf1 inline comments, and adding a couple lines to the circleCI to build and test the curl image using main/master libs as well as the pinned ones.

@planetf1 I looked into the docs for inconsistencies in the build image name and found that is very consistent, using the local image name in the README and using the docker hub image in USAGE. It also includes a line at the top of USAGE clarifying that it uses the public image name throughout that doc.

The curl/README.md should be updated with any new arguments

I updated the README to include the new ARGs and removed direct reference to versions so as any future updates to the pinned version would not require updates to the docs

Lastly, on the CI update, what I pushed was a simple addition since we should add more through tests when we move testing to GH Actions, which currently only does builds, and only for one demo. (covered by #284 IIUC)

ajbozarth commented 1 month ago

As promised in https://github.com/open-quantum-safe/oqs-demos/issues/284#issuecomment-2377475642 I have just pushed my update for the nginx demo and added a task list to the top description for tracking my progress in updating demos. I will also be updating the circleCI for the 3 jobs currently being run (openssl3, nginx, and httpd).

As I will be pushing updated here periodically as I finish them, I will ping again for review when I have finished. Feel free to either wait until then or review as I go at your leisure.

ajbozarth commented 3 weeks ago

Status Update

Fully updated demos

Updated demo (with issues)

Still to be worked on

Personal notes

I believe the work here is getting close to done. I plan to bring up some of my notes above at next Tuesday's meeting to discuss what "done" is here. After the discussion with the team I plan to start looking at #294 as a follow up to this work.

pi-314159 commented 3 weeks ago

ngtcp2 should be able to build with boringssl

ajbozarth commented 3 weeks ago

ngtcp2 should be able to build with boringssl

Supporting building with boringssl is a great idea, but the current demo doesn't support it. So I would consider that a new feature that should be handled in a separate PR

baentsch commented 3 weeks ago

Thanks for the summary, @ajbozarth . Regarding haproxy, may I suggest @pi-314159 and @davidkel sync? IMO all that's needed there is a link to the haproxy documentation as per https://github.com/open-quantum-safe/oqs-demos/issues/273#issuecomment-2284599611. Unless of course you want to keep supporting a docker image despite the upstream apparently already supporting oqs (?). Sounds a bit like wasted effort, no?

ajbozarth commented 3 weeks ago

Unless of course you want to keep supporting a docker image despite the upstream apparently already supporting oqs (?). Sounds a bit like wasted effort, no?

I get what you mean, I'll discuss with @davidkel and see if he still wants to update if it's just "putting the doc steps into a Dockerfile" I'll also raise the topic of just dropping it in Tuesday's meeting (I already planned on discussing dropping at least unbound since it's out of date and there was no interest in it).

ajbozarth commented 2 weeks ago

Sharing plans based on todays discussion on the OQS call:

Demo specific updates:

Follow up work:

planetf1 commented 2 weeks ago

FYI X11 is still working for me on mac sequoia (I have XQuartz installed) and Fedora core 41 .. will try demo

baentsch commented 2 weeks ago

Withe this there will now be three tiers of demos:

Supported: demos that have a support person
Unsupported: demos without a support person (these will still be "supported" by CI though)
Deprecated: demos that are out of date, these will be moved into a deprecated directory and not included in CI

"Unsupported" but CI-supported -- sounds weird. Can I suggest the terms Maintained - Unmaintained - Deprecated instead? For the latter I'd also presume the existing docker images (and links to them) will be removed when these terms get defined/documented as to what they actually entail? For example, I'd imagine Maintained integrations shall also be tested against the respective master while Unmaintained ones will only run CI against the known-good ("pinned") version(?)

ajbozarth commented 2 weeks ago

Maintained - Unmaintained - Deprecated instead

This is much better, thank you

For the latter I'd also presume the existing docker images (and links to them) will be removed when these terms get defined/documented as to what they actually entail?

We did not discuss what to do with already published docker images for deprecated demos, In my opinion as long as those published docker images still run as expected there no reason to delete them.

I'd imagine Maintained integrations shall also be tested against the respective master while Unmaintained ones will only run CI against the known-good ("pinned") version(?)

I had intended to run all of the against main/master, but when I get to the CI work we can discuss if we only want to test the maintained demos and not unmaintained against main/master

planetf1 commented 2 weeks ago

I tried epiphany on fedora workstation 41 (where I can run X11 apps)

I noticed the launch invocation seems incorrect in that it does not pass $DISPLAY. When I do so I get:

➜  epiphany git:(curl) docker run --net=host --env DISPLAY=${DISPLAY} --volume="$HOME/.Xauthority:/home/oqs/.Xauthority:rw" epiphany               

(epiphany:7): dbind-WARNING **: 17:33:17.498: Couldn't connect to accessibility bus: Failed to connect to socket /run/user/1000/at-spi/bus: No such file or directory
Gtk-Message: 17:33:17.521: Failed to load module "canberra-gtk-module"
Gtk-Message: 17:33:17.521: Failed to load module "pk-gtk-module"
Gtk-Message: 17:33:17.522: Failed to load module "canberra-gtk-module"
Gtk-Message: 17:33:17.523: Failed to load module "pk-gtk-module"
bwrap: No permissions to create new namespace, likely because the kernel does not allow non-privileged user namespaces. See <https://deb.li/bubblewrap> or <file:///usr/share/doc/bubblewrap/README.Debian.gz>.

** (epiphany:7): ERROR **: 17:33:17.921: Failed to fully launch dbus-proxy: Child process exited with code 1
/home/oqs/startepiphany.sh: line 9:     7 Trace/breakpoint trap   (core dumped) epiphany

which is some issue relating to the accessibility bus. The main app window starts to draw, then the app exits. However I don't think this is necessarily worth pursuing

planetf1 commented 2 weeks ago

I would suggest we update the textual description for each image published on dockerhub. In particular I agree we don't need to remove old demos, but we should at least clearly add a note to the description that they are maintained or deprecated, as well as ensure there's a link back to the relevant directory in this source repository.

ajbozarth commented 2 weeks ago

Per my above comments I have pushed the following:

This leaves two open action items:

Point of discussion - Wireshark

@davidkel brought to my attention that Wireshark already added a bunch of PQ support a while back (see here), given the overhead for updating the wireshark demo and the existing support I decided to mark it deprecated. If there is push back on this I suggest we open a follow up issue to "un-deprecate it" and some other brave soul can pick it up.

ajbozarth commented 2 weeks ago

Was looking through the issue backlog while waiting on a openlitespeed build and the expanded work in the PR might mean closing a handful of open issue with the PR:

If there's no issues with my above assertions I will add Fixes #230 #182 #273 #226 #266 #171 (note #255 not included) to the top description to be auto closed when this PR is merged.

ajbozarth commented 2 weeks ago

After some effort trying to update the openlitespeed demo, I'm hit too many build issues and don't have the expertise on openlitespeed to sort them out. As there was no interest in the openlitespeed demo to begin with I am marking it as deprecated instead. If there's interest I can still push the updates I made that aren't building if someone else wants to take a shot. They are currently sitting in a separate branch on my fork, where I'll leave it for future reference https://github.com/ajbozarth/oqs-demos/commit/24f81fbb05989249ad27d459e336cc90dac7a2c9

ajbozarth commented 2 weeks ago

As mentioned above I've added a Fixes tag to close the related Issues in the description, if anyone feels one of those Issues should not be closed by this PR according to my previous reasons, I can remove it. I also self assigned those Issues under the assumption that this PR addresses them.

I also opened #312 as a follow up issue to handle the creation of a deprecated directory as we discussed.

As mentioned I left a comment on #255 since I did not include it in the Fixes list despite deprecating wireshark.

Lastly @SWilson4 did you have any luck testing epiphany? Because if it does not work for you I plan to do the following:

ajbozarth commented 2 weeks ago

Worth noting here: I just found out that openssl-3.4.0 was release last week. I'd rather not block the merge of this PR, instead I would like to wait until my CI updates and take the opportunity to use the update to openssl-3.4.0 to test them

SWilson4 commented 2 weeks ago

I get a similar error to @planetf1 when I try to run epiphany on Ubuntu 24.

ajbozarth commented 2 weeks ago

I get a similar error to @planetf1 when I try to run epiphany on Ubuntu 24.

In that case I'll take some time today to go through the steps I listed above to deprecate epiphany.

I also noticed CI complaining about my lack of commit sign-offs for my more recent commits, I'll address that at the same time

ajbozarth commented 2 weeks ago

I've taken the steps to deprecate epiphany, and updated the relevant docs and GitHub issues.

With this, the PR is now in a state of final review. I have no more outstanding work items or review items to address prior to merging.

The only caveat is the DCO CI failing due to my forgetting to sign off on my later commits. Remediating this would be messy due to multiple merges with master. I could do a squash and force push if it is an actual blocker, but afaik every PR is merged with a "Squash and merge" so is it necessary? If so I will need to create a new branch to hold the epiphany commit that's referenced in its deprecation message since a squash and force would delete the referenced url. @baentsch you usually handle merges in this repo.

SWilson4 commented 2 weeks ago

As an FYI in case it's useful in the future for @ajbozarth or others, you can automate signing off on commits for a repo by following the (very well hidden) instructions in .git/hooks/prepare-commit-msg.sample.

ajbozarth commented 2 weeks ago

As an FYI in case it's useful in the future for @ajbozarth or others, you can automate signing off on commits for a repo by following the (very well hidden) instructions in .git/hooks/prepare-commit-msg.sample.

Thanks, I knew I could automate it, but this is the first project I've worked on that required sign offs so I hadn't taken the time to figure out how to do so

ajbozarth commented 1 week ago

A few updates per discussion in todays call:

After this PR has final approval I will run a giant git squash with sign-off to fix the DCO failures as per todays discussion (unless @ryjones you have better solution?)

ryjones commented 1 week ago

After this PR has final approval I will run a giant git squash with sign-off to fix the DCO failures as per todays discussion (unless @ryjones you have better solution?)

You could to a rebase, packing down to a minimal number of commits. A squash is also OK. You could do one commit for deprecated demos, one for updated demos, or whatever reads best.

ajbozarth commented 5 days ago

One thing did deteriorate, though, namely the breadth of algorithms implicitly tested by the different integrations: It was intentional to not always use kyber768 but to also use other algs

Could point out the specific cases where this changed? I had not intended to touch the algorithms used unless it was breaking the build, like switching to mldsa in the openssh demo. On a quick reread it does look like @davidkel did touch algorithms in his changes (haproxy and mosquitto).

@baentsch If you want me to try and address that I can otherwise I'll do the squats and force push previously discussed later today

ajbozarth commented 5 days ago

I have pushed the giant squashed commit with an updated commit message. I also copied the previous git history into a new branch on my fork and protected it like I did with the epiphany and openlitespeed updates. This will allow at least some easy reference for future devs looking at the PR for help.

As discussed with @baentsch in todays OQS call I will be making sure that my test cases I add in my CI work include more algorithm diversity that was lost in this update (specifically haproxy and mosquitto)

This PR should be good to merge now, just double check the Issues it will be automatically closing is correct first.

davidkel commented 3 days ago

@ajbozarth haproxy and mosquitto were modeled on the pattern that was used for openvpn which defined a KEM_LIST with those algorithms which was why they changed although haproxy and mosquitto only had 3 kems listed originally so they weren't diverse much anyway. Given google's push for X25519MLKEM768 and also a recent discussion I had with @bhess around the oqs test server maybe the suggested list for all demos could be

x25519:x448:prime256v1:secp384r1:secp521r1:kyber512:x25519_kyber768:p256_kyber512:kyber768:p384_kyber768:kyber1024:p521_kyber1024:mlkem512:mlkem768:mlkem1024:X25519MLKEM768:SecP256r1MLKEM768

which is what Basil has enabled on the oqs test server

baentsch commented 3 days ago

which is what Basil has enabled on the oqs test server

@davidkel can you please explain what you mean by this statement? I have developed the oqs test server many years ago as a testbed for all algorithms as an aid for the PQC FOSS community -- are you/@bhess now aiming to drop this property? Or are you referring only to the default algs the server accepts at port 443? More generally, we are about to be dropping Kyber support, so the list above may need revising (either now or in the very near future).

bhess commented 3 days ago

Hi @baentsch,

I believe @davidkel is specifically referring to the DEFAULT_GROUPS supported on the ports marked with a *. These ports have always supported a limited set of algorithms, as detailed on the test.openquantumsafe.org page. The change here extends the DEFAULT_GROUPS to include the standardized algorithms. Of course, all algorithms remain fully testable through the various individual ports available on the test server.