Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

[release/3.1] Always build the OpenSSL shim in portable mode on macOS. #42801

Merged
merged 2 commits into from
Jan 15, 2020

Conversation

bartonjs
Copy link
Member

@bartonjs bartonjs commented Jan 6, 2020

No description provided.

# and our prebuilts should not assume a specific ABI version for the types
# that use OpenSSL at runtime.
if (APPLE)
set(FEATURE_DISTRO_AGNOSTIC_SSL True)
Copy link
Member

Choose a reason for hiding this comment

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

It seems that rather than hardcoding this in the cmake script, we should pass the -portable option to the build-native.sh like we do for Linux and update the build-native.sh to accept that option for OSX too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about that, but my inclination was to change the build from "never portable" to "always portable", and it seems like rather than failing the build when you run build-native.sh without -portable that I'd just set it to true in here.

If you think that we want to enable the option and update the rpath rewrites to also rpath rewrite 1.1, that's valid. But, unlike Linux--where I expect the distros to build non-portable--I don't think that we'd really go back to a tightly-linked version for macOS.

Copy link
Member

Choose a reason for hiding this comment

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

After thinking about it a bit more, I agree with you.

@danmoseley danmoseley added the Servicing-consider Issue for next servicing release review label Jan 7, 2020
@danmoseley
Copy link
Member

Marking servicing-consider just so we can talk about it now.

@danmoseley danmoseley removed the Servicing-consider Issue for next servicing release review label Jan 7, 2020
@danmoseley
Copy link
Member

Shiproom okays change in principle - please set servicing-consider when this is ready to go, template filled out etc just to follow the usual process.

@bartonjs
Copy link
Member Author

Description

On macOS OpenSSL is only used for 2 platform-agnostic types (AesCcm, AesGcm), both added in .NET Core 3.0, as well as with 5 interop types: DSAOpenSsl, ECDiffieHellmanOpenSsl, ECDsaOpenSsl, RSAOpenSsl, SafeEvpPKeyHandle.

Currently, our macOS builds use a single ABI version of OpenSSL which is determined at compile time. Further, the version is the now out-of-support OpenSSL 1.0.2.

This change will make macOS use the same late binding that the portable Linux release does, and dynamically support the better of OpenSSL 1.0.2 or OpenSSL 1.1.

Customer Impact

Customers who use the interop types will be broken if they are P/Invoking against a different version of OpenSSL than we end up binding to and using those pointers with our *OpenSsl classes. If they are following the guidance for interop with these types they will be aware of the state by reading the SafeEvpPKeyHandle.OpenSslVersion property. These customers can force the downgrade by use of the CLR_OPENSSL_VERSION_OVERRIDE environment variable.

Regression?

No, reacting to dependency lifetimes.

Packaging reviewed?

Only impacts the shared runtime.

Risk

Low. This code has been used for multiple releases with Linux, and has been tested with all 4 combinations of OpenSSL 1.0.2 and OpenSSL 1.1 being present on the system.

@bartonjs bartonjs marked this pull request as ready for review January 10, 2020 16:02
@bartonjs bartonjs added this to the 3.1.x milestone Jan 10, 2020
@bartonjs bartonjs added the Servicing-consider Issue for next servicing release review label Jan 10, 2020
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@danmoseley danmoseley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 14, 2020
@danmoseley danmoseley modified the milestones: 3.1.x, 3.1.2 Jan 14, 2020
@danmoseley
Copy link
Member

approved in email for 3.1.2

@Anipik Anipik merged commit 8fc9be2 into dotnet:release/3.1 Jan 15, 2020
@bartonjs bartonjs deleted the 31_osx_portable_openssl branch February 5, 2020 18:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants