temurin-build icon indicating copy to clipboard operation
temurin-build copied to clipboard

OpenJ9 linux always configure --with-openssl=fetched

Open AdamBrousseau opened this issue 3 years ago • 30 comments

OpenJ9 get_source.sh will fetch the latest OpenSSL version during the build. We should compile against that version rather than 1.0.2r on the system.

Related AdoptOpenJDK/openjdk-infrastructure#2084

Signed-off-by: Adam Brousseau [email protected]

AdamBrousseau avatar Mar 26 '21 03:03 AdamBrousseau

Looking for reviews from @pshipton and @sxa

AdamBrousseau avatar Mar 26 '21 03:03 AdamBrousseau

If we do this will it still run against systems with 1.0.2 as a default e.g. RHEL7?

Yes. The version used for compile shouldn't affect the version used at runtime.

Is it worth the risk of changing it?

Questionable. In theory it shouldn't matter. The OpenJ9 builds fetch and compile against the latest, and that is my preference as doing this will discover a problem if one is ever introduced, which should never happen.

Is there any reason to download and use openssl when all of our machines are configured with the playbooks which will ensure a suitable version is available on platforms where we are not bundling it? It introduces extra risk if the openssl source download fails.

Note the older versions contain known security vulnerabilities if that matters for the maintenance of the machines.

Doing the check then setting it to the /usr/local directory OR fetched runs the risk of behavior changing based on whether it is present. If, for whatever reason, the installation of openssl into /usr/local failed that failure will be hidden and it'll just use a differnet one, which is always risky for ensuring consistency

In theory the version used for compile doesn't matter.

pshipton avatar Mar 26 '21 12:03 pshipton

I should note that we need to ensure the correct and latest version (1.1.1k atm) is bundled for Windows and Mac.

pshipton avatar Mar 26 '21 15:03 pshipton

As Peter says, it should not matter which version of openssl we use at compile time.

Only one source file (currently) needs to include any openssl header files, the others are redundant. With a little work, I expect we can remove those includes as well in which case no version of openssl would be required at build time.

keithc-ca avatar Mar 26 '21 15:03 keithc-ca

I think Win and Mac do the right thing https://github.com/AdoptOpenJDK/openjdk-build/blob/041c33c6a56ff3474133714c6d0ef6d7d9bce233/build-farm/platform-specific-configurations/mac.sh#L51

Running ./configure with arguments 'bash ./configure --verbose  --with-vendor-name=AdoptOpenJDK --with-vendor-url=https://adoptopenjdk.net/ --with-vendor-bug-url=https://github.com/AdoptOpenJDK/openjdk-support/issues --with-vendor-vm-bug-url=https://github.com/eclipse/openj9/issues --with-vendor-version-string=AdoptOpenJDK --with-version-opt=202103241343 --without-version-pre --with-version-build=7 --with-boot-jdk=/Library/Java/JavaVirtualMachines/adoptopenjdk-10.jdk/Contents/Home --with-macosx-codesign-identity="Developer ID Application: London Jamocha Community CIC" --with-debug-level=release --with-native-debug-symbols=external --enable-ccache --with-freemarker-jar=/Users/jenkins/workspace/build-scripts/jobs/jdk11u/jdk11u-mac-x64-openj9/workspace/./build//freemarker-2.3.31/freemarker.jar --with-jvm-variants=server --with-cacerts-file=/Users/jenkins/workspace/build-scripts/jobs/jdk11u/jdk11u-mac-x64-openj9/sbin/../security/cacerts --disable-ccache  --disable-warnings-as-errors --with-openssl=fetched --enable-openssl-bundling --with-sysroot=/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/ --enable-dtrace=auto --with-cmake --with-freetype=bundled'

https://github.com/AdoptOpenJDK/openjdk-build/blob/041c33c6a56ff3474133714c6d0ef6d7d9bce233/build-farm/platform-specific-configurations/windows.sh#L148

Running ./configure with arguments 'bash ./configure --verbose  --with-vendor-name=AdoptOpenJDK --with-vendor-url=https://adoptopenjdk.net/ --with-vendor-bug-url=https://github.com/AdoptOpenJDK/openjdk-support/issues --with-vendor-vm-bug-url=https://github.com/eclipse/openj9/issues --with-vendor-version-string=AdoptOpenJDK --with-version-opt=202103251815 --without-version-pre --with-version-build=7 --with-boot-jdk=/cygdrive/c/openjdk/jdk-10 --with-debug-level=release --with-native-debug-symbols=external --with-jvm-variants=server --with-cacerts-file=/cygdrive/e/jenkins/tmp/sbin/../security/cacerts  --disable-warnings-as-errors --with-freemarker-jar=/cygdrive/c/openjdk/freemarker.jar --with-openssl=/cygdrive/c/openjdk/OpenSSL-1.1.1j-x86_64-VS2017 --enable-openssl-bundling --enable-cuda --with-cuda=C:/PROGRA~2/NVIDIA~2/CUDA/v9.0 --with-toolchain-version=2017 '

AdamBrousseau avatar Mar 26 '21 15:03 AdamBrousseau

Note the older versions contain known security vulnerabilities if that matters for the maintenance of the machines.

We don't change the default on the machine, so any system apps will always be using the one supplied with the OS, not the one we store in /usr/local/openssl-xxx purely for the purposes of building OpenJ9 against.

sxa avatar Mar 26 '21 15:03 sxa

I should note that we need to ensure the correct and latest version (1.1.1k atm) is bundled for Windows and Mac.

Yep I'm finalising that under https://github.com/AdoptOpenJDK/openjdk-build/pull/2553 which is separate from this PR

sxa avatar Mar 26 '21 15:03 sxa

I didn't consider the JITServer use of openssl in my answers. I'll ask @mpirvu to comment on that.

pshipton avatar Mar 26 '21 17:03 pshipton

Note there is another playbook for openssl 1.1.1b for jitserver. https://github.com/AdoptOpenJDK/openjdk-infrastructure/blob/5ffd313d178b3ca4bbbf67d98d1f34f3c7c2ab03/ansible/playbooks/AdoptOpenJDK_Unix_Playbook/roles/OpenSSL/tasks/main.yml

AdamBrousseau avatar Mar 26 '21 17:03 AdamBrousseau

Same answer for JITServer, it should not matter which version of OpenSSL is present at compile time. With respect to different variants of 1.1.x, as long as the API for various routines remains the same, as it should, we should be fine.

mpirvu avatar Mar 26 '21 17:03 mpirvu

Can we have the jitserver use the version that we pull in get_source.sh? That way we could also remove the second playbook.

AdamBrousseau avatar Mar 26 '21 17:03 AdamBrousseau

Can we have the jitserver use the version that we pull in get_source.sh? That way we could also remove the second playbook.

yes, that should work

mpirvu avatar Mar 26 '21 17:03 mpirvu

As Peter says, it should not matter which version of openssl we use at compile time.

Only one source file (currently) needs to include any openssl header files, the others are redundant. With a little work, I expect we can remove those includes as well in which case no version of openssl would be required at build time.

When I wrote the above, I forgot about jdk/crypto/jniprovider/NativeCrypto.c in the extension repos, but, again, compiles against any version should be fine.

keithc-ca avatar Mar 26 '21 18:03 keithc-ca

Any concerns about reproducing older builds with this change?

karianna avatar Mar 29 '21 11:03 karianna

Any concerns about reproducing older builds with this change?

From my perspective, it shouldn't make any difference.

pshipton avatar Mar 29 '21 13:03 pshipton

run tests

karianna avatar Mar 31 '21 09:03 karianna

Failure on GH Action

Compiling 224 properties into resource bundles for jdk.localedata
engines/e_afalg.c:31:4: warning: #warning "AFALG ENGINE requires Kernel Headers >= 4.1.0" [-Wcpp]
 #  warning "AFALG ENGINE requires Kernel Headers >= 4.1.0"
    ^~~~~~~
engines/e_afalg.c:32:4: warning: #warning "Skipping Compilation of AFALG engine" [-Wcpp]
 #  warning "Skipping Compilation of AFALG engine"
    ^~~~~~~
Compiling 3180 files for java.base
test/rsa_test.c: In function 'test_rsa_oaep':
test/rsa_test.c:349:5: warning: 'key' may be used uninitialized in this function [-Wmaybe-uninitialized]
     RSA_free(key);
     ^~~~~~~~~~~~~
ar: creating apps/libapps.a
ar: creating libssl.a
ar: creating test/libtestutil.a
/bin/sh: CCACHE_COMPRESS=1: command not found
Makefile:6863: recipe for target 'fuzz/asn1parse-test' failed
gmake[5]: *** [fuzz/asn1parse-test] Error 127
gmake[4]: *** [all] Error 2
Makefile:174: recipe for target 'all' failed

karianna avatar Mar 31 '21 10:03 karianna

I still don't see a compelling reason for changing this. The comments above seem to suggest there's no technical reason to use a later version on platforms where it is not bundled and it only has the potential to introduce instability by downloading things from third party websites during the build process.

Questions:

  • What is the disadvantage of keeping it as-is?
  • @keithc-ca suggests it might be fairly easy to remove this requirement anyway - is that something that there is a desire for?
  • For @mpirvu: Does JITServer still require 1.1.1 to build against?
  • The playbook role that @AdamBrousseau referred to earlier is not run on any of the AdoptOpenJDK buils systems (and given some of the things in there relating to adjusting default environments I am disinclined to do so) yet we have jitserver enabled on some platforms already e.g. pLinux JDK11 has -enable-jitserver and --with-openssl=system

sxa avatar Mar 31 '21 13:03 sxa

Does JITServer still require 1.1.1 to build against?

JITServer does not need SSL library at build time. At runtime, given the options to force message encryption, JITServer looks for a library named:

   // Library names for OpenSSL 1.1.1, 1.1.0, 1.0.2 and symbolic links
   static const char * const libNames[] =
      {
      "libssl.so.1.1",   // 1.1.x library name
      "libssl.so.1.0.0", // 1.0.x library name
      "libssl.so.10",    // 1.0.x library name on RHEL
      "libssl.so"        // general symlink library name
      };

It should work with any 1.1.x library version.

mpirvu avatar Mar 31 '21 14:03 mpirvu

Does JITServer still require 1.1.1 to build against?

JITServer does not need SSL library at build time.

Correction: it does need the header files. The question is whether the 1.0.2 header files will suffice.

keithc-ca avatar Mar 31 '21 14:03 keithc-ca

has the potential to introduce instability by downloading things from third party websites during the build process

Note that we have been cloning the latest openssl code every build as long as this req has been around.

AdamBrousseau avatar Mar 31 '21 14:03 AdamBrousseau

The question is whether the 1.0.2 header files will suffice.

Yes, 1.0.2 headers will suffice. When we implemented SSL support for JITServer we tested the following scenarios:

building against 1.1.1, loaded with 1.0.2g
building against 1.0.2g, loaded with 1.1.1
building against 1.1.1, loaded with 1.1.1

mpirvu avatar Mar 31 '21 15:03 mpirvu

What is the disadvantage of keeping it as-is?

Is there is ever an API compatibility problem with a new version of openssl it won't be noticed in the Adopt builds. However it will be in the OpenJ9 builds since they build against the latest.

has the potential to introduce instability by downloading things from third party websites during the build process

Atm it comes from github, as does the source for the other repos. If it doesn't work, probably nothing works so you can't build anyway.

pshipton avatar Mar 31 '21 16:03 pshipton

GH Action needs updating.

Compiling 224 properties into resource bundles for jdk.localedata
engines/e_afalg.c:31:4: warning: #warning "AFALG ENGINE requires Kernel Headers >= 4.1.0" [-Wcpp]
 #  warning "AFALG ENGINE requires Kernel Headers >= 4.1.0"
    ^~~~~~~
engines/e_afalg.c:32:4: warning: #warning "Skipping Compilation of AFALG engine" [-Wcpp]
 #  warning "Skipping Compilation of AFALG engine"
    ^~~~~~~
Compiling 3180 files for java.base
test/rsa_test.c: In function 'test_rsa_oaep':
test/rsa_test.c:349:5: warning: 'key' may be used uninitialized in this function [-Wmaybe-uninitialized]
     RSA_free(key);
     ^~~~~~~~~~~~~
ar: creating apps/libapps.a
ar: creating libssl.a
ar: creating test/libtestutil.a
/bin/sh: CCACHE_COMPRESS=1: command not found
Makefile:6863: recipe for target 'fuzz/asn1parse-test' failed

karianna avatar Mar 31 '21 16:03 karianna

Is there is ever an API compatibility problem with a new version of openssl it won't be noticed in the Adopt builds. However it will be in the OpenJ9 builds since they build against the latest.

We frequently test build infrastructure changes on multiple OSs to validate the playbooks including ones which have 1.1.1 as a default so it likely wouldn't be completely missed, although that will be subject to whichever update policy the distribution provides). Also since we are pulling latest 1.1.1 on Windows, changes are we'd notice if the incompatibility such as you describe were to show up.

Other than for developers pulling (potentially unecessarily?) the latest, I'm still unclear on what circumstances where it beneficial though? Since it would only affect people building it, I would argue that's less of a concern than potential incompatibility with older versions. If we were to build against the latest headers, would we be confident that it still work out of the box on distributions like RHEL7 which have OpenSSL 1.0.2? Surely there is a risk that if there is a breaking change in a later 1.1.1 then using the latest one would mask the fact it won't execute on supported platforms if OpenJ9 were to use of new functionality without additional work since we're relying on the system SSL libraries on most platforms and that would be a customer visible break. Unless you plan to prereq 1.1.1 then it seems a risky strategy not to build against the earlier supported version.

Atm it comes from github, as does the source for the other repos. If it doesn't work, probably nothing works so you can't build anyway.

Fair point - I had assumed it was using the tarball from the website :-)

sxa avatar Mar 31 '21 17:03 sxa

GH Action needs updating.

@karianna Can you advise - is that something this PR author needs to be responsible for?

sxa avatar Mar 31 '21 17:03 sxa

GH Action needs updating.

@karianna Can you advise - is that something this PR author needs to be responsible for?

They're the best placed in all likeliness, they can ask on Slack for extra help.

karianna avatar Mar 31 '21 17:03 karianna

If we were to build against the latest headers, would we be confident that it still work out of the box on distributions like RHEL7 which have OpenSSL 1.0.2?

Yes, it's designed to work this way.

Surely there is a risk that if there is a breaking change in a later 1.1.1 then using the latest one would mask the fact it won't execute on supported platforms if OpenJ9 were to use of new functionality without additional work since we're relying on the system SSL libraries on most platforms and that would be a customer visible break.

I don't believe so. Perhaps if there was new work done and it wasn't properly implemented or tested then Adopt building against 1.0.2 may catch that fact, but perhaps not. While building against 1.0.2 is supported today for no particular reason, it's not a benefit or something we need to keep working. If there is related new work done and any reason to do so, I can see support for building against 1.0.2 being dropped. It's more important that 1.0.2 is supported at runtime, which is confirmed by runtime testing, not by what happens at build time.

Unless you plan to prereq 1.1.1 then it seems a risky strategy not to build against the earlier supported version.

1.1.1 is actually a prereq for some features, and 1.1.1 can be used at runtime even if 1.0.2 is used to compile (and vice versa).

In summary, basically it doesn't matter what you compile against. You can take this change, or not. The benefits are very nebulous.

pshipton avatar Mar 31 '21 18:03 pshipton

Is this not redundant now that #2917 is merged?

keithc-ca avatar May 12 '22 18:05 keithc-ca

I wanted to take another look at this because we (Semeru) still use "system" on p/z linux.

AdamBrousseau avatar May 13 '22 03:05 AdamBrousseau