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

replacing openssl111 with openssl3 #94

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gobbledy-gook
Copy link

@gobbledy-gook gobbledy-gook commented Oct 27, 2024

This is PR has "rough edges" such as some dependencies could be extra or I might have retained extra stuff while building the smaller image in stage 2 or multi-stage build in the dockerfile.

The main goal of this PR is to replace openssl111 with openssl3 which includes the usage of ops-provider. Earlier version of the docker file used python3.8 which did not have support for openssl3 so I had to install the latest python version 3.12 for this purpose. Secondly, the minitest.py files is adjusted to give output for port:6138 with sig:dilithium2 specifically which requires changing (an easy fix, can be done in a commit).

Garvit Shah added 3 commits October 27, 2024 22:04
Copy link
Member

@SWilson4 SWilson4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the effort to bring this feature up to date, @gobbledy-gook! Is this Docker setup tested anywhere in CI?

ARG MAKE_DEFINES

LABEL version="2"
LABEL version="4"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be version 3.

@gobbledy-gook
Copy link
Author

Is this Docker setup tested anywhere in CI?

I'm sorry but I don't know what means, haven't done before.

@SWilson4
Copy link
Member

Is this Docker setup tested anywhere in CI?

I'm sorry but I don't know what means, haven't done before.

No worries! It would be nice to build the Docker image and maybe run a test in it as part of our GitHub Actions Continuous Integration tests. The configuration for these tests is in https://github.com/open-quantum-safe/liboqs-python/blob/main/.github/workflows. I don't believe any of the Docker-related functionality is tested there.

I triggered the automated tests to run on this PR, and it looks like there are a couple of build failures to sort out.

@baentsch
Copy link
Member

I don't believe any of the Docker-related functionality is tested there.

That is a correct observation @SWilson4 and an oversight on my part (well, honestly I wasn't convinced anyone else but me uses docker images for python :), sorry. Unless @gobbledy-gook wants to learn how to do this and add that as part of this PR(?) I'll add a test docker build in a separate PR.

it looks like there are a couple of build failures to sort out.

Indeed, the PR failures look like general CI setup problems independent of the PR, so nothing for @gobbledy-gook to worry about. Tagging @vsoftco for advice.

@gobbledy-gook
Copy link
Author

Unless @gobbledy-gook wants to learn how to do this and add that as part of this PR(?) I'll add a test docker build in a separate PR.

Indeed. I am open to this. Always eager to expand my scope.

Indeed, the PR failures look like general CI setup problems independent of the PR, so nothing for @gobbledy-gook to worry about.

Yes, I observed it being suited for openssl111 and uses python3.10 whereas I have used python3.12 which is the only lowest version that supports openssl3 (afaik).

@baentsch
Copy link
Member

Always eager to expand my scope.

Great! As a quick way to get started, you may want to take a look at adding a step to https://github.com/open-quantum-safe/liboqs-python/blob/main/.github/workflows/python_detailed.yml following the docker (test) build in https://github.com/open-quantum-safe/oqs-demos/blob/main/.github/workflows/linux.yml (just without docker hub things like login and push).

Yes, I observed it being suited for openssl111 and uses python3.10 whereas I have used python3.12 which is the only lowest version that supports openssl3 (afaik).

Thanks for that information. Should we create a separate issue to track and fix @vsoftco ?

home_dir = os.path.expanduser("~")
oqs_install_dir = os.path.abspath(home_dir + os.path.sep + "_oqs") # $HOME/_oqs
home_dir = os.path.expanduser("/opt")
oqs_install_dir = os.path.abspath(home_dir + os.path.sep + "oqssa") # $HOME/_oqs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this change is causing the CI failures—is it necessary?

Copy link
Author

@gobbledy-gook gobbledy-gook Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this is utterly necessary or not, but it is where the shared object liboqs.so is found by oqs.py.

As I understand (please correct if I am wrong), previously, liboqs.so was being installed during runtime (while oqs.py tries to load the shared object but does not find it and as a result installs at ~/_oqs), however in this case the shared object is required to build ops-provider. Thus now it is installed in ${INSTALLDIR}=/opt/oqssa with all other binaries.

@vsoftco
Copy link
Member

vsoftco commented Oct 31, 2024

Always eager to expand my scope.

Great! As a quick way to get started, you may want to take a look at adding a step to https://github.com/open-quantum-safe/liboqs-python/blob/main/.github/workflows/python_detailed.yml following the docker (test) build in https://github.com/open-quantum-safe/oqs-demos/blob/main/.github/workflows/linux.yml (just without docker hub things like login and push).

Yes, I observed it being suited for openssl111 and uses python3.10 whereas I have used python3.12 which is the only lowest version that supports openssl3 (afaik).

Thanks for that information. Should we create a separate issue to track and fix @vsoftco ?

Yes, please, let's have a separate issue for this.

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.

4 participants