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

non-EOL openssl on windows #421

Closed
wants to merge 2 commits into from

Conversation

mikaelarguedas
Copy link
Member

OpenSSL 1.0.2 is now EOL https://www.openssl.org/policies/releasestrat.html

1.1.1 is the new LTS, also used on macOS and Ubuntu Focal.

This will likely fail CI due to eProsima/Fast-DDS#1087.

@brawner
Copy link
Contributor

brawner commented Mar 24, 2020

1.1.1 is also listed as experimental on the hosting page. Maybe use 1.1.0?

https://slproweb.com/download/Win64OpenSSL-1_1_0L.exe

@mikaelarguedas
Copy link
Member Author

1.1.0 is EOL as well

I believe the "experimental" refers to the MSI installer, not the version of openSSL

@brawner
Copy link
Contributor

brawner commented Mar 24, 2020

Oh you're absolutely correct about that.

@mikaelarguedas
Copy link
Member Author

mikaelarguedas commented Apr 5, 2020

@brawner I ran a job with this branch's first commit trying to use openssl 1.1.1e but apparently it was already replaced by 1.1.1f.
The surprising part is that even if the docker build failed, the job continued without complaining and used an older image.
I would have expected the job to fail if the docker build fails.

Is there a reason not to fail on docker build failure? (otherwise I can look into providing a PR making it fail in that case)

@nuclearsandwich
Copy link
Member

One thing that strikes me about this PR is that it not only changes the OpenSSL version but also the package source. Do you have more info about the OpenSSL chocolatey package. Is that going to track the LTS release or the standard release channel?

@mikaelarguedas
Copy link
Member Author

mikaelarguedas commented Apr 10, 2020

Yeah this is an experiment(still in progress), as the URL broke in the middle of testing this PR it made me think it was worth trying to make this less fragile and less cached by installing via choco.
I have not been able to test the switch to choco yet because the docker build was silently failing for other reasons so the jobs ran without this change.

Knowing chocolatey, it will most likely track the latest stable (similarly to homebrew). So when a non-LTS version is released it will likely bump to that.
It may also be that chocolatey doesnt install the Debug libraries and would need to be revert back to hard-coded URLs.

This will have two advantages:

  • catching breakage earlier, (right now in sros2 we're just finding out now that some implementations don't support OpenSSL LTS because it was not tested/reported before). Catching this breakage will also allow to automatically find out which versions to pin in the installation instructions instead of waiting for user breakage report.
  • avoid silent breakage when the installer URL is deleted in favor a more recent version

But this needs further testing in both release and debug mode to make sure it can be used.

Edit: looks like failing on docker builds will happen #428 removing silent brekages

@sloretz
Copy link
Contributor

sloretz commented May 1, 2020

Testing this with 1 minor tweak to the path and with osrf/homebrew-ros2#8 which has been installed onto mini1. Just test_security since those are the tests that have been failing.
edit: Whoops, wrong pr to comment on

@mikaelarguedas
Copy link
Member Author

This branch worked successfully with Fast-RTPS in Release mode: ros2/system_tests#415 (comment) and failed in Debug mode. Potentially due to outdated wheels.

This has not worked for Connext. It appears that fixing up the name of the RTI_OPENSSL_LIBS is necessary for the PATH to be modified at runtime for the security tests but is not sufficient for a successful loading of the security plugins: https://ci.ros2.org/job/ci_windows/10424/.
The fact that the paths passed via environment variable hard-code "release" is also a point of concern for debug mode in the future:

ENV RTI_OPENSSL_BIN C:\connext\openssl-1.0.2n\x64Win64VS2017\release\bin
ENV RTI_OPENSSL_LIBS C:\connext\openssl-1.0.2n\x64Win64VS2017\release\lib
FastRTPS Connext
Release ✔️
Debug

mikaelarguedas and others added 2 commits May 14, 2020 18:26
@jacobperron
Copy link
Member

@mikaelarguedas I think this ticket can be closed since #454 has been merged. Feel free to comment and reopen if I'm mistaken.

@mikaelarguedas
Copy link
Member Author

@jacobperron The tests are still failing / disabled on WIndows. Do you have another place to track it ?
Did you give a shot at #454 (comment) ?

@jacobperron
Copy link
Member

Good call. I've opened #488 to track the failures.

Did you give a shot at #454 (comment) ?

I did not. The referenced branch doesn't seem to exist anymore. Can you provide an updated reference or PR?

@mikaelarguedas
Copy link
Member Author

Can you provide an updated reference or PR?

Took me a bit to get back to it but see #490 and ros2/system_tests#439

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