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

Re-enable Fast-RTPS security tests #415

Merged
merged 1 commit into from
May 15, 2020

Conversation

nuclearsandwich
Copy link
Member

Reverts #413. To be tested and merged once Fast-RTPS has support for the necessary OpenSSL versions (eProsima/Fast-DDS#1087)

@nuclearsandwich
Copy link
Member Author

Current ros2 master (Fast-RTPS 1.10.x)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

ros2/ros2#906 (Fast-RTPS 2.0.x)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@mikaelarguedas
Copy link
Member

MacOS: IDK why the tests are failing, it looks like they timeout so my guess is that they dont succeed to connect and exchange messages

Windows: These jobs still use openssl 1.0.2, so we don't know if the new fastrtps fixes the tests on Windows for openssl 1.1.1*
ros2/ci#421 can be used for testing more recent openssl versions

@nuclearsandwich
Copy link
Member Author

Windows: These jobs still use openssl 1.0.2, so we don't know if the new fastrtps fixes the tests on Windows for openssl 1.1.1*

It's a good thing to check but IMO it's most important that Fast-RTPS security works with the currently supported OpenSSL version on Windows, which is still 1.0.2 for the moment although potentially something we can resolve for Foxy. One concern with bumping OpenSSL on Windows is that I don't know if the OpenSSL binaries we are installing are co-installable. Which means that we need to take further measures to use the appropriate binaries for Dashing and Eloquent CI builds.

@mikaelarguedas
Copy link
Member

Which means that we need to take further measures to use the appropriate binaries for Dashing and Eloquent CI builds.

This is true, although it looks like this is not the only dependency that will require a different environment setup between Foxy and previous distros e.g. ros2/ci#432 (comment).

currently supported OpenSSL version on Windows, which is still 1.0.2 for the moment although potentially something we can resolve for Foxy.

Yeah it would be really great to not force users into installing and using an EOL, non-security patched version of OpenSSL (of all libraries).
Below is some CI to inform us on how this behaves with OpenSSL 1.1.1g on windows (using ros2/ci#421):

  • None: Build Status
  • Release: Build Status
  • Debug: Build Status

Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

Seems like these should be re-enabled based on this CI eProsima/Fast-DDS#1087 (comment)

@nuclearsandwich
Copy link
Member Author

Seems like these should be re-enabled based on this CI eProsima/Fast-RTPS#1087 (comment)

Except that the more recent CI build above fails in Debug. So it seems that merging this now would break nightlies if we updated the Windows OpenSSL version.

@mikaelarguedas
Copy link
Member

My main concern here is that all the tests are failing on MacOS https://ci.ros2.org/job/ci_osx/8584/. Do we have a better idea why ?

the more recent CI build above fails in Debug

This may be due to the install of OpenSSL from chocolatey. The following jobs are using 1.1.1g with the hardcoded URL from slproweb so maybe it'll work better ros2/ci@5d89f38

  • Release: Build Status
  • Debug: Build Status

@mikaelarguedas
Copy link
Member

Crazy idea: @mjcarroll is it possible that the cryptography debug wheel is too old ?
The Release build installs 2.9.2 from pip, but the debug job installs 2.7.
(I don't know how much work in involved) would it be possible to test that same job with a wheel at the same version as the one currently pulled from pip ?

@jacobperron
Copy link
Member

is it possible that the cryptography debug wheel is too old ?

That sounds plausible. Here are the instructions that were used to create the current cyptography debug wheel: https://github.com/ros2/ros2/tree/cryptography-archives

@mikaelarguedas
Copy link
Member

That sounds plausible. Here are the instructions that were used to create the current cyptography debug wheel: https://github.com/ros2/ros2/tree/cryptography-archives

Thanks for the pointer, that line in the instructions seems to confirm it might need to be rebuilt to target recent OpenSSL:

 # This last line is needed if using OpenSSL version < 1.1.0
 set CRYPTOGRAPHY_WINDOWS_LINK_LEGACY_OPENSSL=True

@nuclearsandwich
Copy link
Member Author

I have added rebuilding the wheel to my Foxy tasks for next week.

@nuclearsandwich
Copy link
Member Author

I've built a new cryptography wheel and am trying it out here: Build Status

@mikaelarguedas
Copy link
Member

I've built a new cryptography wheel and am trying it out here

thanks for doing that! That job seems to say it's still using the cryptography 2.7 wheel. Did it not pick up the new 2.9.2 wheel for some reason ?

@nuclearsandwich
Copy link
Member Author

That job seems to say it's still using the cryptography 2.7 wheel. Did it not pick up the new 2.9.2 wheel for some reason ?

I built the same wheel version as our previous debug wheel, just against openssl 1.1.1 instead. I'm pretty sure that the wheel I built was used just didn't work. :(

@mikaelarguedas
Copy link
Member

I built the same wheel version as our previous debug wheel, just against openssl 1.1.1 instead. I'm pretty sure that the wheel I built was used just didn't work. :(

That is unfortunate. I hope that cryptography 2.7 does support OpenSSL 1.1.1g. It looks like it became the default target only as of cryptography 2.9.1.

This is a Windows Debug only issue so I don't think it should be holding this specific PR.


However the failing MacOS jobs seem to be an issue. Do we have an idea of what if failiing there ? maybe @sloretz from your recent MacOS investigations?

@nuclearsandwich
Copy link
Member Author

nuclearsandwich commented May 14, 2020

This is a Windows Debug only issue so I don't think it should be holding this specific PR.

Windows debug runs nightly and I'm not going to be the one to knowingly break a nightly build.

@mikaelarguedas
Copy link
Member

Windows debug runs nightly and I'm not going to be the one to knowingly break a nightly build.

My point was that merging this PR will keep the Windows jobs green, and that the investigation of "why will it be failing on Windows debug once we bump openssl version to 1.1.1*" could be tied to the PR changing the version of openssl on windows ros2/ci#421 rather than holding this one

@nuclearsandwich
Copy link
Member Author

My point was that merging this PR will keep the Windows jobs green

Right. Somehow I lost track of the fact that we were running CI against 1.1.1 on Windows. With that in mind, I'll do another CI run with defaults and if it's clean I say let's do this!

I hope that cryptography 2.7 does support OpenSSL 1.1.1g. It looks like it became the default target only as of cryptography 2.9.1.

I think it's reasonable to look at bumping the versions on all of our debug wheels to match the current versions being used in Release. I am a bit worried about ros2/ci#432 (comment) with Python 3.8 on Windows affecting packages we use but don't maintain.

@nuclearsandwich
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
  • Windows Debug Build Status

Let's do this 🤞

@kyrofa
Copy link
Member

kyrofa commented May 15, 2020

Thanks @nuclearsandwich!

@mikaelarguedas
Copy link
Member

🎉

@nuclearsandwich nuclearsandwich merged commit b9bebaf into master May 15, 2020
@nuclearsandwich nuclearsandwich deleted the revert-413-disable-fastrtps-security-tests branch May 15, 2020 20:40
@nuclearsandwich
Copy link
Member Author

Thanks very much to @mikaelarguedas and @kyrofa for keeping this one moving forward as I got allocated elsewhere. As a preview, I am waiting on helping with ros2/ci#421 until we settle the python 3.8 work as I'll have to rebuild a lot of stuff on Windows for that and cryptography will just be one of them.

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