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

Drop support for OpenSSL pre-1.1.1 #1951

Merged

Conversation

eboasson
Copy link
Contributor

OpenSSL versions older than 1.1.1 have all been dead for over 4 years. I don't see why an Cyclone would have to continue supporting the bad practice of not updating EOL'd security sensitive libraries full of known vulnerabilities.

Of course nobody should be using OpenSSL 1.1.1 anymore (it has been EOL'd about half a year ago), but I know there are still plenty of systems in the field that rely on it and even the CI on Azure gets the latest Linux images with it pre-installed.

Fixes #1925 — or so I expect anyway. @zhzhzoo-autra I'd appreciate it if you could give it a try.

Copy link
Contributor

@dpotman dpotman left a comment

Choose a reason for hiding this comment

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

Looks good! In the readme there is a reference to OpenSSL 1.1, that line also needs an update:

* Optionally [OpenSSL](https://www.openssl.org/), preferably version 1.1;

And here:

However, it expects (or at least it's preferred to have) version 1.1 or newer,

OpenSSL versions older than 1.1.1 have all been dead for over 4 years. I don't see why an
Cyclone would have to continue supporting the bad practice of not updating EOL'd security
sensitive libraries full of known vulnerabilities.

Of course nobody should be using OpenSSL 1.1.1 anymore (it has been EOL'd about half a
year ago), but I know there are still plenty of systems in the field that rely on it and
even the CI on Azure gets the latest Linux images with it pre-installed.

Signed-off-by: Erik Boasson <[email protected]>
@eboasson eboasson force-pushed the drop-openssl-pre1p1-support branch from 7ca9343 to bee45b1 Compare March 25, 2024 12:49
@eboasson
Copy link
Contributor Author

Looks good! In the readme there is a reference to OpenSSL 1.1, that line also needs an update:

* Optionally [OpenSSL](https://www.openssl.org/), preferably version 1.1;

And here:

However, it expects (or at least it's preferred to have) version 1.1 or newer,

Done. I also had to solve some conflicts because of merging #1826 and force-pushing it seemed the most sensible solution.

@eboasson eboasson merged commit 6f5ce86 into eclipse-cyclonedds:master Mar 25, 2024
20 of 22 checks passed
@zhzhzoo-autra
Copy link

Thanks for your attention! I tried to compile with boringssl with this patch, the compile errors disappeared, but there's a linker error:

bazel-out/k8-opt/bin/external/cyclonedds/_objs/cyclonedds/ddsi_ssl.pic.o:ddsi_ssl.c:function ddsi_ssl_accept: error: undefined reference to 'BIO_do_accept'
collect2: error: ld returned 1 exit status

I thinks it's because boringssl removed some of openssl's api. I have no understanding of Openssl so I don't know if this if a big or small problem...

@eboasson
Copy link
Contributor Author

Thanks for your attention! I tried to compile with boringssl with this patch, the compile errors disappeared, but there's a linker error:

bazel-out/k8-opt/bin/external/cyclonedds/_objs/cyclonedds/ddsi_ssl.pic.o:ddsi_ssl.c:function ddsi_ssl_accept: error: undefined reference to 'BIO_do_accept'
collect2: error: ld returned 1 exit status

I thinks it's because boringssl removed some of openssl's api. I have no understanding of Openssl so I don't know if this if a big or small problem...

Thanks for trying it!

The easiest way to work around this would be to just not include the TCP / TCP+TLS support code (hardly anyone uses it anyway). There is not a separate build option to do that, by I think there should be, so: #1953. If you can give that a whirl after doing cmake -DENABLE_TCP_TLS=0 ...

The real solution is of course to see what BIO_do_accept maps to in boringssl. I'm sure it is straightforward.

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.

Failure to compile with boringssl
3 participants