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

[Draft] Refactor Dockerfiles for Best Practices and Optimization #327

Closed
wants to merge 14 commits into from

Conversation

hayyaaf
Copy link
Contributor

@hayyaaf hayyaaf commented Dec 1, 2024

Description

This PR enhances all Dockerfiles across the project, ensuring consistency, readability, and performance improvements. The updates include:

  • Syntax Improvements: Standardized the use of keywords (e.g., AS in multi-stage builds).
  • Directory Management: Replaced manual cd commands with WORKDIR for clarity and better structure.
  • Quoting Enhancements: Added proper quoting for variables and commands to prevent word splitting and globbing issues.
  • Package Installation Optimization: Consolidated installation steps and incorporated caching optimizations for efficient builds.
  • Version Updates: Updated dependencies to the latest stable versions.

This PR is currently in Draft status to allow tracking of feedback and incremental updates.

  • curl
  • h2load
  • haproxy
  • httpd
  • locust
  • mosquitto
  • nginx
  • ngtcp2
  • openssh
  • openssl3
  • openvpn
  • wireshark

TODO

  1. Inspect Alpine Issues for OpenSSH:

    • Investigate the issues raised by Alpine Linux when starting the OpenSSH system for the first time.
    • Confirm whether it is acceptable to update relevant files (e.g., /etc/motd) to display a customized welcome message and resolve similar issues.
  2. Update Documentation.

  3. Create a Guide for Newcomers:

    • Draft a guide to help new contributors understand the project setup and workflow.
    • Clarify whether this guide should be included in CONTRIBUTE.md or placed elsewhere.
  4. Ask: Should we use nproc wherever possible, even for steps that do not utilize parallel execution?

- Replaced `as` with `AS` for Dockerfile convention.
- Used `WORKDIR` instead of `cd` for better readability.
- Quoted variables to prevent word splitting and globbing.
- Combined `apk` commands with `--no-cache` to optimize package installation.
- Update OpenSSL
@hayyaaf hayyaaf changed the title Refactor Dockerfiles for Best Practices and Optimization [Draft] Refactor Dockerfiles for Best Practices and Optimization Dec 1, 2024
- Updated OpenSSL to `3.4.0`.
- Replaced `ADD` with `COPY` for files and folders.
- Used `--no-cache` in `apk`.
- Quoted variables to prevent word splitting.
- Removed unused `DEBIAN_FRONTEND` and inlined `LIBOQS_BUILD_DEFINES`.
- Used `WORKDIR` instead of `cd` for better readability.
@hayyaaf
Copy link
Contributor Author

hayyaaf commented Dec 10, 2024

@baentsch @ajbozarth, I’ve done my best to refactor and improve Dockerfile-QUIC. However, I haven’t been able to test the functionality yet, as I’m unsure about the testing process and setup. I would really appreciate it if you could help me test the changes and let me know the results. If there are any errors, and if you know how to fix them, I would be grateful if you could handle them for me or guide me on how to fix them. Additionally, I won’t proceed with merging this PR until all conflicts are fully resolved and the functionality is validated.

@baentsch
Copy link
Member

done my best to refactor and improve Dockerfile-QUIC. However, I haven’t been able to test the functionality yet, as I’m unsure about the testing process and setup

Tagging @pi-314159 for help on this file.

@pi-314159
Copy link
Member

  1. I don't recommend pinning a BoringSSL release, as it is a rolling release. It's not OpenSSL.
  2. Since BoringSSL is designed to work with the latest Ubuntu release, I also don't recommend pinning a specific Ubuntu version.
  3. Since BoringSSL uses a rolling release, the underlying liboqs should not be pinned, as using an old version of liboqs may cause BoringSSL to fail to build.

@baentsch
Copy link
Member

All very good points, @pi-314159 . Indeed I'd be fine omitting boringssl-based Dockerfiles from any "pinning" (fwiw my preference always was to stay as close as possible to the main/master branches such as to have oqs-demos serve as a canary for downstream problems "developing"). I absolutely agree that BoringSSL is much more dynamic than other packages -- actually also developing much quicker regarding PQC:

That said, I'm even wondering whether it's worth while questioning the value of further maintaining the oqs-boringssl branch given that the upstream code is pretty much including all relevant PQC algs by now: Are you aware of anyone using oqs-boringssl with features that are not in boringssl "mainline", @pi-314159 ? If so, out of curiosity: Which are those features?

@hayyaaf
Copy link
Contributor Author

hayyaaf commented Dec 14, 2024

  1. I don't recommend pinning a BoringSSL release, as it is a rolling release. It's not OpenSSL.

  2. Since BoringSSL is designed to work with the latest Ubuntu release, I also don't recommend pinning a specific Ubuntu version.

I initially pinned the versions in the Dockerfile to standardize the template across all projects and improve maintainability. However, as suggested, I will remove the pinning and add a comment to explain the reasoning behind this adjustment.

@pi-314159
Copy link
Member

pi-314159 commented Dec 14, 2024

@baentsch I think this depends on the goals of liboqs. If it's designed for production, then there's no need to maintain the OQS-BoringSSL fork. BoringSSL has already implemented all standardized post-quantum algorithms, including SLH-DSA (liboqs is still using SPHINCS+ round 3 afaik). However, if the goal of liboqs is to test all post-quantum algorithms, then it's reasonable to continue maintaining the OQS-BoringSSL.

@SWilson4
Copy link
Member

Are you aware of anyone using oqs-boringssl with features that are not in boringssl "mainline", @pi-314159 ? If so, out of curiosity: Which are those features?

I am—myself. :) My own research makes use of the BoringSSL fork in Chromium, particularly its support for a wide variety of signature schemes.

@hayyaaf
Copy link
Contributor Author

hayyaaf commented Dec 15, 2024

@baentsch I think this depends on the goals of liboqs. If it's designed for production, then there's no need to maintain the OQS-BoringSSL fork.

To balance stability and experimentation, I suggest maintaining a production-ready version of liboqs for standardized, stable PQC algorithms. Alongside this, we can provide an experimental version that includes the rest of the algorithms for testing and research. An easy way to switch between them, like using build flags, would ensure flexibility without compromising stability. What are your thoughts?

@baentsch
Copy link
Member

maintaining a production-ready version of liboqs for standardized, stable PQC algorithms

Please see/chime in to open-quantum-safe/tsc#1 then.

This is a nice goal and once was held by myself. However, pursuing that would imo require substantially more work than what the community is actually putting into OQS: Just some proof-points:

  1. The number of "production-related" issues, even labelled "Bugs" in OQS is constantly increasing -- without a parallel increase in PRs resolving them;
  2. Concrete issues discussing the required work, e.g., Reliability oqs-provider#483 are not getting any community attention, let alone contribution;
  3. The "seriousness" doing code that I witness at OpenSSL dwarfs the one I see in OQS substantially.

we can provide an experimental version that includes the rest of the algorithms for testing and research.

That's the goal I personally still try to achieve -- but also that one is getting harder and harder (see the lack of support for the most recent version of SLH-DSA (combined with the possibility to actually use standardized algorithms from OpenSSL in OQS and not keep maintaining those in OQS itself) or the decreased of utility of OQS in interop testing).

In my personal opinion, OQS had been an independent "amalgamation point" where everyone with an interest in PQC was willing to contribute. This changed drastically this year:
a) NIST standardized several algorithms, meaning work on truly productive, mostly commercial, often closed source PQC libraries intensified, taking away "cycles" of those developers otherwise more likely to contribute to improve OQS.
b) The (very US-company-centric and -dominated) LinuxFoundation took control of OQS, alienating those contributors that believe in the true "free" nature of FOSS.

An easy way to switch between them, like using build flags, would ensure flexibility without compromising stability. What are your thoughts?

This is the reason for existence of flags like OQS_USE_OPENSSL and algorithm-specific enabler/disablers. However, no-one so far took on the task of clearly delineating which flags provide which (productive) quality guarantees, let alone test & ascertain them (and/or add more suitable ones). Again, this is not impossible -- but it is not getting done.

All told, my personal priority is for OQS to re-gain its standing as a generally useful testbed for any FOSS-related activities around PQC (e.g., aid people like @SWilson4 as per his comment above). The "productization" activities should be driven by the companies commercially benefiting from our work.

@hayyaaf
Copy link
Contributor Author

hayyaaf commented Dec 15, 2024

@baentsch I’ve been following the project logs, and I’m truly impressed by the effort and dedication you’ve put into OQS. It’s clear how much hard work has gone into keeping this project moving forward, and I understand how challenging it must be, especially with the lack of contributors you mentioned.

I fully agree with your concerns and hope more individuals and companies will step up to support this important work.

Thank you once again for your time, effort, and commitment. Your contributions, along with those of the team, are deeply valued. I truly appreciate everyone’s efforts here.

@hayyaaf
Copy link
Contributor Author

hayyaaf commented Dec 15, 2024

I was wondering if we could explore the possibility of becoming part of OpenSSL, or at least being officially supported by them as a branch or extension. Additionally, for projects where OQS has already been integrated, perhaps we could collaborate with those teams to ensure they adopt and verify the latest versions. This might help draw more attention to the project and encourage additional contributors.

I’m sure these ideas may have crossed your minds already, but I’d still love to hear your thoughts on them.

@hayyaaf
Copy link
Contributor Author

hayyaaf commented Dec 15, 2024

@davidgca I’ve noticed that many Alpine-based Docker images include ENV DEBIAN_FRONTEND=noninteractive. As Alpine uses apk instead of apt, I was wondering if this serves any purpose or if it might have been added mistakenly.

…e it to avoid potential confusion

Signed-off-by: Khalid <[email protected]>
Signed-off-by: Khalid <[email protected]>
@baentsch
Copy link
Member

I was wondering if we could explore the possibility of becoming part of OpenSSL, or at least being officially supported by them as a branch or extension

This has been explored and doesn't fly for various reasons (see e.g., openssl/project#431), incl. code provenance and quality. The OpenSSL team actively works on integrating the standard PQC algs with good, "clean" (non-OQS) code and I personally help that effort where possible (I truly am independent in every meaning of the word :). oqsprovider does work as a stopgap (and testbed for the OpenSSL provider concept as well as any new OpenSSL code/features): https://github.com/openssl/openssl/blob/master/test/README-external.md#oqsprovider-test-suite

Additionally, for projects where OQS has already been integrated, perhaps we could collaborate with those teams to ensure they adopt and verify the latest versions.

This already happens with those distributions that voiced interest to include OQS, see e.g., https://github.com/orgs/open-quantum-safe/discussions/1625#discussioncomment-11405477.

This might help draw more attention to the project and encourage additional contributors.

I'm not sure about that: If something functions, it doesn't get contributions it seems :-/ And OQS does function. What concerns me most wrt the "production" aspect are non-functional properties (quality criteria, etc.). And those qualities differentiate software and services for which commercial entities charge; such income motivations then may run counter to motivations to actively improve FOSS. Also, any commercial entity improving FOSS software quality helps its competitors using that same software too (and/or can't differentiate enough)... the classic FOSS quandary, possibly meaning my hope stated above won't come to fruition:

The "productization" activities should be driven by the companies commercially benefiting from our work.

@hayyaaf
Copy link
Contributor Author

hayyaaf commented Dec 16, 2024

  1. I don't recommend pinning a BoringSSL release, as it is a rolling release. It's not OpenSSL.
  2. Since BoringSSL is designed to work with the latest Ubuntu release, I also don't recommend pinning a specific Ubuntu version.
  3. Since BoringSSL uses a rolling release, the underlying liboqs should not be pinned, as using an old version of liboqs may cause BoringSSL to fail to build.

What about quiche? Should I unpin it too?

Signed-off-by: Khalid <[email protected]>
Signed-off-by: Khalid <[email protected]>
@pi-314159
Copy link
Member

What about quiche? Should I unpin it too?

No, it's recommended by cURL. See https://github.com/curl/curl/blob/master/docs/HTTP3.md#build

If you want to test QUIC workflow after you finish, please check this workflow.

@davidgca
Copy link
Contributor

ENV DEBIAN_FRONTEND=noninteractive

Hi @Hawazyn, sorry for the delay. I added this when I initially built the tool, but it seems unnecessary for Locust. I recompiled without the variable DEBIAN_FRONTEND and it worked fine, so we can remove it (at least for Locust).

@hayyaaf
Copy link
Contributor Author

hayyaaf commented Dec 19, 2024

Hi @Hawazyn, sorry for the delay. I added this when I initially built the tool, but it seems unnecessary for Locust. I recompiled without the variable DEBIAN_FRONTEND and it worked fine, so we can remove it (at least for Locust).

Hi @davidgca, no need to apologize at all. I really appreciate you taking the time to look into this. I reached out because I value your expertise and wanted to ensure I wasn’t overlooking anything important. Thanks again for your answer.

@hayyaaf
Copy link
Contributor Author

hayyaaf commented Dec 23, 2024

If you want to test QUIC workflow after you finish, please check this workflow.

Everything works perfectly. Thank you for your efforts on the demo and to whoever created the workflow.

@hayyaaf hayyaaf closed this Dec 23, 2024
@hayyaaf hayyaaf deleted the Dockerfiles branch December 23, 2024 21:09
@hayyaaf hayyaaf restored the Dockerfiles branch December 23, 2024 21:12
@hayyaaf
Copy link
Contributor Author

hayyaaf commented Dec 24, 2024

I have updated all Dockerfiles, adhering to best practices to improve readability, maintainability, and performance. Later today, I will introduce one commit consolidating all changes in this PR.

Request for Input:

@baentsch Could you kindly provide input on the following?

  1. Alpine Issues for OpenSSH:
    Example warnings during runtime:
docker exec --user oqs -it oqs-openssh-server ssh oqs@localhost

The authenticity of host '[localhost]:2222 ([::1]:2222)' can't be established.
ECDSA_NISTP384_MLDSA65 key fingerprint is SHA256:9wTUXpDmDBx0p8S4E4SDNUg8z7VmchZfER3X+r1weLc.
This key is not known by any other names.
Are you sure you want to continue connecting (yes/no/[fingerprint])? yes
Warning: Permanently added '[localhost]:2222' (ECDSA_NISTP384_MLDSA65) to the list of known hosts.
oqs@localhost's password: 
Welcome to Alpine!

The Alpine Wiki contains a large amount of how-to guides and general
information about administrating Alpine systems.
See <https://wiki.alpinelinux.org/>.

You may change this message by editing /etc/motd.

-sh: /etc/profile.d/README: line 1: This: not found
-sh: /etc/profile.d/README: line 7: syntax error: unterminated quoted string

Should I customize the welcome message and modify the related files to resolve these warnings?

  1. Do you agree with the following next steps, or would you recommend any adjustments?

    • Create a streamlined documentation template outlining the essential information for each project, adjusting sections (adding or removing as required) per project.
    • Include guidance on Dockerfile best practices, OpenSSL configuration with OQS, setup testing, and common cross-platform issues (e.g., line separators). Could you clarify whether this guide should be part of CONTRIBUTE.md or a standalone document?
  2. Should nproc be used universally, even in commands that do not involve parallel execution? I noticed it is missing in some commands in ninja and make.

Looking forward to your feedback.

@hayyaaf
Copy link
Contributor Author

hayyaaf commented Dec 24, 2024

I have changed the branch name, but I did not realize that it would close the PR. Should I close this draft PR and create a new one, or should I revert to the old name, or do you suggest another course of action?

@baentsch
Copy link
Member

Should I customize the welcome message and modify the related files to resolve these warnings?

The warnings should be resolved, indeed. In this case, it seems like adding the new keys to the key store may be warranted. And surely the error message should be removed by fixing the actual problem (string quote mismatch)

Do you agree with the following next steps, or would you recommend any adjustments?

If OK for you, I'd first suggest moving the docker scan to (after) each sub-project's build step and remove the separate task that currently fails on main. The next steps you're proposing also make sense but I guess I'm not sure what this actually entails: Particularly what would entail "OpenSSL config documentation with OQS"? Why would this be relevant (beyond what's already documented in https://github.com/open-quantum-safe/oqs-provider/blob/main/USAGE.md)? If there's general documentation missing, what about you create an issue and we discuss (where to put it) there?

Should nproc be used universally, even in commands that do not involve parallel execution? I noticed it is missing in some commands in ninja and make

This may be a good enhancement, certainly for make based builds. I'm not sure it (should) influence ninja as that adapts more automatically to the number of processes available, no? Again, what about creating an issue and discuss the course of action there?

I have changed the branch name, but I did not realize that it would close the PR. Should I close this draft PR and create a new one, or should I revert to the old name, or do you suggest another course of action?

Creating a clean new PR sounds to me like the best way forward.

@hayyaaf hayyaaf deleted the Dockerfiles branch December 24, 2024 12:40
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.

5 participants