Update Foxy OpenSSL to 1.1.1g on Windows. #454

nuclearsandwich · 2020-05-20T18:39:42Z

This is an alternative to #421 which installs (currently the same) OpenSSL binary via chocolatey. Since the OpenSSL version needs to be compiled in to our debug wheel of cryptography for sros2 I would prefer to opt into updates rather than get them by surprise via chocolatey.

This OpenSSL binary distribution also has a different default install
location: C:\Program Files\OpenSSL-Win64 I didn't see a cli option in the installer to change the target location but one may exist.

This OpenSSL binary distribution also has a different default install location: C:\Program Files\OpenSSL-Win64

nuclearsandwich · 2020-05-20T18:43:03Z

CI building up to sros2 and test_security and testing sros2 and test_security.

Windows
Windows Debug (will hopefully resolve issues introduced by #453 since the cryptography wheel was built against this OpenSSL binary distribution.

nuclearsandwich · 2020-05-20T20:38:29Z

This PR currently exchanges security failures using rmw_connext_cpp in all Windows configurations for working builds with the new Cryptography wheel in Windows Debug.
I'm currently trying to resolve the rmw_connext_cpp issue. I'm not yet ready advocate for regressing these tests to fix the build but I am considering it.

The environment variable introduced in ros2/system_tests#409 is RTI_OPENSSL_LIBS not RTI_OPENSSL_LIB Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

nuclearsandwich · 2020-05-20T21:14:19Z

When looking at it, the Windows dockerfiles always had support for pointing connext to its own OpenSSL installation but there was a disagreement between the environment variable name in the initial and final iterations of the PR. is running with a corrected environment variable name and hopefully it's 🟢.

jacobperron

LGTM, pending the latest CI run 🤞

nuclearsandwich · 2020-05-21T03:44:32Z

Well the latest CI run didn't change the outcome. For some reason the RTI_OPENSSL_LIBS directory is being put on the PATH rather than the RTI_OPENSSL_BIN directory (which on Windows includes both executables and ddls). I'm out of gas for tonight so we'll lose another debug nightly and I'll be back at it in the morning.

jacobperron · 2020-05-21T23:31:19Z

windows_docker_resources/Dockerfile.foxy

@@ -78,11 +78,11 @@ RUN 7z.exe x C:\TEMP\opencv.zip -aoa -oC:\
 RUN 7z.exe x C:\TEMP\OpenSplice.zip -aoa -oC:\opensplice

 # Environment setup
-ENV OPENSSL_CONF C:\OpenSSL-Win64\bin\openssl.cfg
+ENV OPENSSL_CONF "C:\Program Files\OpenSSL-Win64\bin\openssl.cfg"


Strange, on my native Windows machine, the OpenSSL installer defaults to C:\OpenSSL-Win64 as the install root.

I've seen that as well, at one point it was because I customized the install path and then a subsequent reinstall (part of a script) had a different default location. Do you have an earlier version of OpenSSL installed? It might be looking at the previous installation path?

I re-located the old installation of OpenSSL; but it's possible there was still some user-data floating around.

jacobperron · 2020-05-27T00:07:30Z

I'm stuck on this as well. Locally, the Connext security tests pass for me when setting RTI_OPENSSL_LIBS and RTI_OPENSSL_BIN accordingly. It seems there an issue with our CI configuration.

Based on the errors, it sounds like Connext isn't installed properly:

12:41:51 21: [test_publisher-1] >>> [rcutils|error_handling.c:108] rcutils_set_error_state()
12:41:51 21: [test_publisher-1] This error state is being overwritten:
12:41:51 21: [test_publisher-1] 
12:41:51 21: [test_publisher-1]   'failed to create participant, at C:\ci\ws\src\ros2\rmw_connext\rmw_connext_shared_cpp\src\node.cpp:281, at C:\ci\ws\src\ros2\rcl\rcl\src\rcl\node.c:277'
12:41:51 21: [test_publisher-1] 
12:41:51 21: [test_publisher-1] with this new error message:
12:41:51 21: [test_publisher-1] 
12:41:51 21: [test_publisher-1]   'rcl node's rmw handle is invalid, at C:\ci\ws\src\ros2\rcl\rcl\src\rcl\node.c:429'

But I ran rcl tests with this CI branch and it appears to be working (modulo a test failure): https://ci.ros2.org/job/ci_windows/10788/testReport/

@mikaelarguedas any ideas?

jacobperron · 2020-05-27T01:47:51Z

Locally, the Connext security tests pass for me when setting RTI_OPENSSL_LIBS and RTI_OPENSSL_BIN accordingly

It seems that the Connext tests pass locally for me even if RTI_OPENSSL_LIBS and RTI_OPENSSL_LIBS are unset. 🤔

mikaelarguedas · 2020-05-27T06:33:30Z

It seems that the Connext tests pass locally for me even if RTI_OPENSSL_LIBS and RTI_OPENSSL_LIBS are unset. thinking

Could you still be using Openssl 1.0.2 like the current CI does so you dont need to point to RTI's OpenSSL?

This s a bit unfortunate that this doesnt build on the work of #421 that already has context, a bunch of tickets linked to it and acknowledges the work done on this topic over the last 2 months... several of the questions in this PR are answered on the other PR...

the Windows dockerfiles always had support for pointing connext to its own OpenSSL installation but there was a disagreement between the environment variable name in the initial and final iterations of the PR

I believe the "always had support" was mostly a copy-n-paste from the Linux dockerfiles but have never been tested, variable names were not matching and variable contents were never taken into account ros2/system_tests#409. So this is not known the have ever worked in the past that I know of.
Necessity of updating env var name and impact on the PATH highlighted in #421 (comment)

Well the latest CI run didn't change the outcome. For some reason the RTI_OPENSSL_LIBS directory is being put on the PATH rather than the RTI_OPENSSL_BIN directory (which on Windows includes both executables and ddls).

I don't think that's true. A good way to confirm it is to see that the first element of the PATH of the tests in the CI job linked above is C:/connext/openssl-1.0.2n/x64Win64VS2017/release/bin
However it's never been attempted to use that extracted OpenSSL on Windows and I don't know the content of the folders C:\connext\openssl-1.0.2n\x64Win64VS2017\release\lib and C:\connext\openssl-1.0.2n\x64Win64VS2017\release\bin which is why I couldn't push #421 further.
2 things that come to mind:

these paths have VS2017 in it, could it be that we need a version built with a more recent compiler/standard libraries ?
these paths have "release" hard-coded in them, this looks like it may not work out of the box for the debug case...

alternative idea:

if OpenSSL 1.1.1 and OpenSSL 1.0.2 install in different locations, could we install them side by side and just change the environment variables to point to the 1.0.2 version ?

jacobperron · 2020-05-27T18:43:23Z

Could you still be using Openssl 1.0.2 like the current CI does so you dont need to point to RTI's OpenSSL?

I have OpenSSL 1.1.1g installed. And then extracted RTI's OpenSSL into a separate directory. 🤷‍♂️

these paths have VS2017 in it, could it be that we need a version built with a more recent compiler/standard libraries ?

AFAIK, VS2017 and VS2019 are binary compatible.

these paths have "release" hard-coded in them, this looks like it may not work out of the box for the debug case...

Maybe not, but I guess we should first figure out why tests are not passing in a release build.

if OpenSSL 1.1.1 and OpenSSL 1.0.2 install in different locations, could we install them side by side and just change the environment variables to point to the 1.0.2 version ?

I think you're suggesting making this change in test_security, right? We can give it a try.

mikaelarguedas · 2020-05-27T19:45:05Z

I have OpenSSL 1.1.1g installed. And then extracted RTI's OpenSSL into a separate directory. 🤷‍♂️

That's encouraging ! now just need to play spot the difference with CI

if OpenSSL 1.1.1 and OpenSSL 1.0.2 install in different locations, could we install them side by side and just change the environment variables to point to the 1.0.2 version ?
I think you're suggesting making this change in test_security, right? We can give it a try.

What I meant is to make this dockerfile install both openssl 1.1.1 and openssl 1.0.2 from the website and set the RTI_OPENSSL_* variables to point to the OpenSSL 1.0.2 installation folder (without needing a change in test_security) and see if in that case the Connext security plugins can be loaded successfully

jacobperron · 2020-05-27T20:38:20Z

I've just opened this PR that disables the Connext security tests on Windows: ros2/system_tests#433

I think it would be good to land that and this PR so that we unblock Windows debug builds and get OpenSSL 1.1.1 for the security tests with other RMWs.

mikaelarguedas · 2020-05-29T07:21:38Z

I've just opened this PR that disables the Connext security tests on Windows: ros2/system_tests#433

I think it would be good to land that and this PR so that we unblock Windows debug builds and get OpenSSL 1.1.1 for the security tests with other RMWs.

Sure, that's still a step in the right direction 👍

I gave a shot to the alternative of installing both openssl 1.0.2 and 1.1.1 from the website (using this branch https://github.com/ros2/ci/compare/recent-openssl) and ran jobs with FastRTPS and Connext in release and debug and they came back green:
Release:
Debug:

The Fast-RTPS build step is for sure using openssl 1.1.1g, I'm not certain for the tests but it seems likely

Update Foxy OpenSSL to 1.1.1g on Windows.

4fd8b4e

This OpenSSL binary distribution also has a different default install location: C:\Program Files\OpenSSL-Win64

nuclearsandwich self-assigned this May 20, 2020

Fix RTI_OPENSSL_LIBS envar name.

76850a5

The environment variable introduced in ros2/system_tests#409 is RTI_OPENSSL_LIBS not RTI_OPENSSL_LIB Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

nuclearsandwich requested a review from jacobperron May 20, 2020 22:15

jacobperron approved these changes May 20, 2020

View reviewed changes

jacobperron reviewed May 21, 2020

View reviewed changes

nuclearsandwich assigned nuclearsandwich and jacobperron and unassigned nuclearsandwich May 22, 2020

dirk-thomas mentioned this pull request May 26, 2020

ros2 still using OpenSSL 1.0.2 instead of 1.1.1 on Windows 10 #459

Closed

jacobperron mentioned this pull request May 27, 2020

Disable Connext security tests on Windows ros2/system_tests#433

Merged

ivanpauno approved these changes May 29, 2020

View reviewed changes

jacobperron merged commit c382e11 into master Jun 1, 2020

delete-merged-branch bot deleted the foxy/openssl-1.1.1-on-windows branch June 1, 2020 23:22

mikaelarguedas mentioned this pull request Jun 3, 2020

Foxy support ros2/sros2#180

Closed

This was referenced Jun 24, 2020

non-EOL openssl on windows #421

Closed

Connext security tests failing on Windows #488

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Foxy OpenSSL to 1.1.1g on Windows. #454

Update Foxy OpenSSL to 1.1.1g on Windows. #454

nuclearsandwich commented May 20, 2020

nuclearsandwich commented May 20, 2020

nuclearsandwich commented May 20, 2020

nuclearsandwich commented May 20, 2020

jacobperron left a comment

nuclearsandwich commented May 21, 2020

jacobperron May 21, 2020

nuclearsandwich May 22, 2020 •

edited

Loading

jacobperron May 22, 2020

jacobperron commented May 27, 2020

jacobperron commented May 27, 2020

mikaelarguedas commented May 27, 2020

jacobperron commented May 27, 2020

mikaelarguedas commented May 27, 2020

jacobperron commented May 27, 2020 •

edited

Loading

mikaelarguedas commented May 29, 2020

Update Foxy OpenSSL to 1.1.1g on Windows. #454

Update Foxy OpenSSL to 1.1.1g on Windows. #454

Conversation

nuclearsandwich commented May 20, 2020

nuclearsandwich commented May 20, 2020

nuclearsandwich commented May 20, 2020

nuclearsandwich commented May 20, 2020

jacobperron left a comment

Choose a reason for hiding this comment

nuclearsandwich commented May 21, 2020

jacobperron May 21, 2020

Choose a reason for hiding this comment

nuclearsandwich May 22, 2020 • edited Loading

Choose a reason for hiding this comment

jacobperron May 22, 2020

Choose a reason for hiding this comment

jacobperron commented May 27, 2020

jacobperron commented May 27, 2020

mikaelarguedas commented May 27, 2020

jacobperron commented May 27, 2020

mikaelarguedas commented May 27, 2020

jacobperron commented May 27, 2020 • edited Loading

mikaelarguedas commented May 29, 2020

nuclearsandwich May 22, 2020 •

edited

Loading

jacobperron commented May 27, 2020 •

edited

Loading