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

Support arbitrary version strings in OpenSSL wrapper and add minimum_openssl_version option #2475

Merged
merged 12 commits into from
Jun 21, 2021

Conversation

lexming
Copy link
Contributor

@lexming lexming commented Jun 15, 2021

As discussed in Slack, software needing OpenSSL v1.1.1(eg Qt5) will not work with v1.1.0. Therefore we have to make our wrapper aware of the full version of OpenSSL.

Since the soname of libssl is limited to a major minor version, the system will only provide a single libssl.so.1.1 in any case. So the existing code to find the library file still applies. All that is needed is an extra check on the version string contained in the shared object.

Changelog:

  • the version of the wrapper easyconfig can have any depth and it determines the minimum version of OpenSSL required in the system
  • regex the full version string from OpenSSL library files in the system
  • check that the version of the system library fulfils the version of the wrapper
  • apply the same regex used for the libraries to the opensslv.h header
  • check that the version in the header matches the version of the OpenSSL library
  • stop the search process in the system as soon as any component is missing

update in 6d5124e

  • search the version string in both libssl and libcrypto as some distros ship libssl without any version string

update in 044ef73, 70d0186, 5656949

  • add extra option minimum_openssl_version to explicitly set the minimum version of OpenSSL required in the system
  • minimum_openssl_version can only increase the depth of the wrapper easyconfig version

Existing easyconfigs with just major.minor versions continue to work.

Example of the results obtained with different combinations of OpenSSL versions:

Wrapper Easyconfig Version minimum_openssl_version System OpenSSL Version Result
1.0 none 1.0.2k wrapped
1.0 1.0.2u 1.0.2k built in EB
1.0 1.1 1.0.2k error
1.0.2 none 1.0.2k wrapped
1.0.2u none 1.0.2k built in EB
1.1 none 1.0.2k built in EB
1.1 none 1.1.0k wrapped
1.1 none 1.1.1g wrapped
1.1 1.1.1 1.1.0k built in EB
1.1 1.1.1 1.1.1g wrapped
1.1 1.1.1k 1.1.1g built in EB
1.1 1.0 1.1.1g error
1.1 1.2 1.1.1g error
1.1.1 none 1.1.0k built in EB
1.1.1 none 1.1.1g wrapped
1.1.1k none 1.1.1g built in EB

@boegelbot

This comment has been minimized.

@Micket
Copy link
Contributor

Micket commented Jun 15, 2021

I have some concerns. I suggested in slack

We can

  1. Consider the name OpenSSL-1.1.eb to refer to the shlib name
  2. Introduce a minimum actual specific version required for consideration of wrapping (minimum_wrapped_version = 1.1.1g ) and somehow check for that against openssl version or something. Else, revert to building it from source.

Because I think this approach has some issues

  1. To fix what we have already shipped, we would have to replace the existing OpenSSL-1.1.eb easyconfig since it is insufficient, and replace the name in all the easyconfigs that depend on it so far and having everyone rebuild the modules for them. Or make the default easyconfigs unusable for those affected.
  2. The easyconfig would presumably be named something like OpenSSL-1.1.1g, but, it might actually be actually be OpenSSL 1.1.1k (built or wrapped)? Or maybe I misunderstand the code (what happens if system is 1.1.1k and eb is 1.1.1g?). If so, then we can't specify anything that works on more than a single OS.
  3. Or if you actually do build the version you specify in self.version, you can never do any in-place rebuild-updates without it starting to mismatch

I think having the separate "minimum_wrapped_version" parameter sorts out these issues.
It could default to self.minimum_wrapped_version = self.version, else just verify that self.minimum_wrapped_version.startswith(self.version) and proceed to verify against this more specific version. Then we could enrich the existing easyconfigs with this parameter.

@lexming
Copy link
Contributor Author

lexming commented Jun 15, 2021

@Micket with this PR as it is, it will indeed be complicated to push an existing installation using the v1.1 wrapper to v1.1.1. However, I think that it is not necessary to go that way. Existing installations are fine in my opinion. We do not currently have any easyconfig in the tree that needs OpenSSL v1.1.1. So I would not touch the current v1.1 wrapper used in 2021a.

If there is an agreement to set a minimum version with an extra option such as minimum_wrapped_version in the wrapper, I'll add it of course. With the current code is quite simple to do. But using an internal version for OpenSSL has also its own issues. The main one is that it is very hard to notice when the wrapper gets updated. For instance, a probable scenario is

  • toolchain releases with wrapper version 1.x and internal version 1.x.a
  • months later new software in toolchain needs OpenSSL version 1.x.b (similar to the situation with Qt5 now)
  • that new software gets in the tree and wrapper has the internal version bumped to 1.x.b
  • I, as a user of EB, will only see the easyconfig of the new software landing in the tree (because I blindly pulled the branch or I just don't read changelogs of new releases)
  • I'll try to install the new software and it will fail, because I already installed the wrapper months ago and nothing will trigger its re-installation as it has the same version and only the internal option changed

I would follow the same rules that we have for other software, we set a version per toolchain and we stick to it. In this case, v1.1 for 2021a (which is equivalent to a minimum 1.1.0 version) and we can jump to v1.1.1 for 2021b. The current PR already supports using a letter revision as version of the wrapper, so we could even require minimum revisions of OpenSSL in future toolchains.

@Micket
Copy link
Contributor

Micket commented Jun 16, 2021

Just to mention this in the PR for posterity; as far as I know OpenSSL is pretty far from semver. 1.1.1 is an LTS release, while 1.1.0 wasn't. They are even adding functions to the letter-versions, 1.1.1k has functionality that 1.1.1g lacks.
1.1.0 series is already out of support (last update almost 2 years ago) and shouldn't be used by recommendation of OpenSSL developers (and it rarely is, which is probably why we missed it).
I also suspect that there might never be a 1.2, or even a 1.1.2 version, but next jump would be to 3.0.0 so in practice, this is a really specific case we are covering here; making sure those on an outdated 1.1.0 release instead gets an up-to-date version.

We have actually already messed in the 4.4.0 release here since it does already include an implicit assumption on >=1.1.1 with Qt5 5.15.2, so something needs changing, and I don't think we ever want 1.1.0.

I would also find it a bit strange if versions don't actually match exactly if we go with "1.1.1x/1.1.1" versions compared to the intentionally vague 1.1.
I considered this to be one where rebuilding isn't totally out of the question, if for no other reason than to just keep up with security fixes.
This does happen every now and then, for Java, R, and whenever we discover some issue we can't really resolve any other way.

Perhaps some third/fourth/fifth opinions on all of this. @boegel maybe?

@lexming
Copy link
Contributor Author

lexming commented Jun 16, 2021

I saw that libssl in Fedora does not contain any version string, but libcrypto does. So 6d5124e changes the search to loop over all known libraries in OpenSSL until it finds one with the version string.

We have actually already messed in the 4.4.0 release here since it does already include an implicit assumption on >=1.1.1 with Qt5 5.15.2, so something needs changing, and I don't think we ever want 1.1.0.

Oh right, I missed that merge. Then it's already an issue for 2021a. It's not ideal, but in that case the best solution might indeed be to add that internal minimum version to the 1.1 wrapper. I'll look into it.

This does happen every now and then, for Java, R, and whenever we discover some issue we can't really resolve any other way.

For Java it's different because it is very unlikely that anything breaks if the Java installation is not kept up to date. With OpenSSL, software will start to not build if not updated properly.

@lexming
Copy link
Contributor Author

lexming commented Jun 16, 2021

@Micket added the extra option minimum_openssl_version to the wrapper as requested. It sets the minimum version of OpenSSL required in the system, which the easyblock checks in the libraries and headers of OpenSSL. minimum_openssl_version can only increase the depth of the wrapper easyconfig version.

Updated the PR description and table with example cases. Ready for tests.

@lexming lexming changed the title Support arbitrary version strings in OpenSSL wrapper Support arbitrary version strings in OpenSSL wrapper and add minimum_openssl_version option Jun 17, 2021
@boegel boegel added this to the next release (4.4.1) milestone Jun 17, 2021
@lexming
Copy link
Contributor Author

lexming commented Jun 17, 2021

Test report by @lexming

Overview of tested easyconfigs (in order)

  • SUCCESS OpenSSL-1.0.eb
  • SUCCESS OpenSSL-1.1.eb

Build succeeded for 2 out of 2 (2 easyconfigs in total)
login2.cerberus.os - Linux centos linux 7.9.2009, x86_64, Intel(R) Xeon(R) Gold 6126 CPU @ 2.60GHz, Python 2.7.5
See https://gist.github.com/1cdb5971ac7639ce5d2cf1d586efdff7 for a full test report.

CentOS 7.9 with OpenSSL 1.0.2k as main install and 1.1.1g on the side

  • OpenSSL-1.0.eb: wraps
  • OpenSSL-1.1.eb: wraps

@lexming
Copy link
Contributor Author

lexming commented Jun 17, 2021

Test report by @lexming

Overview of tested easyconfigs (in order)

  • SUCCESS OpenSSL-1.0.eb
  • SUCCESS OpenSSL-1.1.eb

Build succeeded for 2 out of 2 (2 easyconfigs in total)
login3.phoenix.os - Linux centos linux 7.9.2009, x86_64, Intel(R) Xeon(R) Gold 6148 CPU @ 2.40GHz, Python 2.7.5
See https://gist.github.com/cc1639e3cad6430c7192586679a62da0 for a full test report.

CentOS 7.9 with OpenSSL 1.0.2k as main install and 1.1.1g partially installed on the side without headers

  • OpenSSL-1.0.eb: wraps
  • OpenSSL-1.1.eb: builds

@lexming
Copy link
Contributor Author

lexming commented Jun 17, 2021

Test report by @lexming

Overview of tested easyconfigs (in order)

  • SUCCESS OpenSSL-1.0.eb
  • SUCCESS OpenSSL-1.1.eb

Build succeeded for 2 out of 2 (2 easyconfigs in total)
login3.phoenix.os - Linux centos linux 7.9.2009, x86_64, Intel(R) Xeon(R) Gold 6148 CPU @ 2.40GHz, Python 2.7.5
See https://gist.github.com/359626d332ef7f4d975a955f955970ab for a full test report.

CentOS 7.9 with OpenSSL 1.0.2k

  • OpenSSL-1.0.eb: wraps
  • OpenSSL-1.1.eb: builds

@lexming
Copy link
Contributor Author

lexming commented Jun 17, 2021

Test report by @lexming

Overview of tested easyconfigs (in order)

  • SUCCESS OpenSSL-1.0.eb
  • SUCCESS OpenSSL-1.1.eb

Build succeeded for 2 out of 2 (2 easyconfigs in total)
angook - Linux Fedora 33, x86_64, Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz, Python 3.9.5
See https://gist.github.com/ff4f8bb2c7b5240e866ca8ecd9bf9e13 for a full test report.

Fedora 33 with OpenSSL 1.1.1k and compatibility library for 1.0.2o that lacks the openssl executable and headers

  • OpenSSL-1.0.eb: builds
  • OpenSSL-1.1.eb: wraps

Copy link
Contributor

@Micket Micket left a comment

Choose a reason for hiding this comment

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

lgtm

@Micket

This comment has been minimized.

@Micket
Copy link
Contributor

Micket commented Jun 21, 2021

Test report by @Micket

Overview of tested easyconfigs (in order)

  • SUCCESS OpenSSL-1.1.eb

Build succeeded for 1 out of 1 (1 easyconfigs in total)
alvis-c1 - Linux centos linux 7.9.2009, x86_64, Intel Xeon Processor (Skylake), Python 3.6.8
See https://gist.github.com/a36937b2bc19f6cda57b75410fb82f44 for a full test report.

@Micket Micket merged commit a74484d into easybuilders:develop Jun 21, 2021
@lexming lexming deleted the ssl-patch-version branch June 23, 2021 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants