Add ability statically link OpenSSL #80380

kant2002 · 2023-01-09T18:59:37Z

This setup works if I apply this as local customizations, I do not sure that I use OpenSSL in most secure way, I do not competent. This setup and StaticExecutable=true allow package just EXE file + /etc/ssl/certs/ folder in Docker

ghost · 2023-01-09T19:00:09Z

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

This setup works if I apply this as local customizations, I do not sure that I use OpenSSL in most secure way, I do not competent. This setup and StaticExecutable=true allow package just EXE file + /etc/ssl/certs/ folder in Docker

Author:	kant2002
Assignees:	-
Labels:	`area-NativeAOT-coreclr`
Milestone:	-

jkotas · 2023-01-09T19:18:30Z

src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Unix.targets

@@ -87,6 +95,8 @@ The .NET Foundation licenses this file to you under the MIT license.

    <Exec Command="$(IlcHostPackagePath)/native/src/libs/System.Globalization.Native/local_build.sh $(IlcHostPackagePath)/ $(IntermediateOutputPath)" Condition="'$(StaticICULinking)' == 'true'"/>

+    <Exec Command="$(IlcHostPackagePath)/native/src/libs/System.Security.Cryptography.Native/local_build.sh $(IlcHostPackagePath)/ $(IntermediateOutputPath)" Condition="'$(StaticOpenSslLinking)' == 'true'"/>


local_build.sh file missing in the PR?

Oops. But it's the same as in Globalization work basically.

same as in Globalization

Maybe we should have just one script and pass the library to build as an argument?

vcsjones · 2023-01-09T19:45:40Z

@bartonjs @blowdart a heads up this is proposing a change to allow NativeAOT apps to statically link OpenSSL (libcrypto and libssl).

blowdart · 2023-01-09T20:13:30Z

This change would mean any Microsoft product using aot would have to get a cryptoboard exception. Static linking openssl is arguably rather bad security. So many products out there in routers, IoT devices etc. cannot even manage to update dynamically linked OpenSSL, a static linkage makes its use even more hidden.

From a compliance and security PoV if reject this. Has there been a wider discussion I've missed, or is it just this PR?

Cc @GrabYourPitchforks

kant2002 · 2023-01-09T20:32:09Z

For perspective why I was adding this change - I was thinking to have application as single EXE file running inside docker containers built from scratch image. If update needed, then docker container recreated. I do not consider that this would be used in different contexts.

jkotas · 2023-01-09T20:35:26Z

This change would mean any Microsoft product using aot would have to get a cryptoboard exception.

@blowdart That's not correct. This change is not changing any default. The aot build is going to dynamically link openssl by default both before and after this change.

This change is adding option to allow statically linking. This is expert-only option. It requires acquiring extra dependencies and it mirrors what we have done for other libraries (ICU). I do not expect that we are going to document this option in official documentation.

The community has always been interested having an option to statically linking everything together to build "from scratch" container images, etc. This small change makes this niche scenario a bit easier.

Static linking openssl is arguably rather bad security.

It depends on how you orchestrate your deployment pipeline. If your deployment pipeline fully rebuilds your containers for updates, there is a little difference between dynamic and static linking security-wise. You have to always rebuild your container to pick-up OpenSSL update for both cases. The only difference between these two cases are performance characteristics.

Has there been a wider discussion I've missed, or is it just this PR?

This is follow up on #79498

blowdart · 2023-01-09T20:37:53Z

Oh dear, ok, then any AOT compiled app which opts-in will need a cryptoboard exception, before or after this sigh

Does the documentation call this sort of thing out with a large warning/caveat anywhere?

jkotas · 2023-01-09T20:38:26Z

src/native/libs/build-local.sh

+# The .NET Foundation licenses this file to you under the MIT license.
+#
+
+# This script is used only for building libSystem.Security.Cryptography.Native.a


Outdated comment

jkotas · 2023-01-09T20:42:41Z

Oh dear, ok, then any AOT compiled app which opts-in will need a cryptoboard exception, before or after this sigh

No AOT compiled app shipped by Microsoft should be opting-in into this.

Does the documentation call this sort of thing out with a large warning/caveat anywhere?

This option is not documented at all today. You have to find in the source. We can certainly add documentation for this with a disclaimer, but it is only going to make the option more discoverable, and I expect that more people will try to use it.

am11 · 2023-01-09T20:49:16Z

src/native/libs/build-local.sh

+if [ -d "$SHIM_SOURCE_DIR" ]; then
+    LOCAL_SHIM_DIR="$INTERMEDIATE_OUTPUT_PATH"/libs/$TARGET_LIBRARY/build
+    mkdir -p "$LOCAL_SHIM_DIR" && cd "$LOCAL_SHIM_DIR"
+    if [ $? -ne 0 ]; then echo "build-local.sh::ERROR: Cannot use local build directory"; exit 1; fi


Please use the same inline syntax as before for compat. The only change needed was replacement of hardcoded name with $TARGET_LIBRARY.

Thank you! I copy file from browser instead of file system

jkotas · 2023-01-09T20:50:06Z

@kant2002 Could you please mention this next to https://github.com/dotnet/runtime/blob/main/src/coreclr/nativeaot/docs/compiling.md#using-statically-linked-icu and include a large warning/caveat ?

src/native/libs/build-local.sh

src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Unix.targets

kant2002 · 2023-01-09T21:14:19Z

@jkotas is this wording, okay?

jkotas · 2023-01-09T21:14:35Z

src/coreclr/nativeaot/docs/compiling.md

+## Using statically linked OpenSSL
+This feature can statically link OpenSSL libraries (such as libssl.a and libcrypto.a) into your applications at build time.
+NativeAOT binaries built with this feature can run even when OpenSSL libraries are not installed.
+**WARNING:** *This is scenario for advanced users, please use with extreme caution. Incorrect usage of this feature, can cause security vulnerabilities in your product*


@blowdart Does this look good?

am11 · 2023-01-09T21:43:05Z

src/native/libs/build-local.sh

@@ -4,24 +4,25 @@
 # The .NET Foundation licenses this file to you under the MIT license.


Executable bit was dropped during the move: chmod +x src/native/libs/build-local.sh

src/coreclr/nativeaot/docs/compiling.md

src/native/libs/build-local.sh

am11 · 2023-01-10T04:50:34Z

@kant2002, did you test it with code calling native API in System.Security.Cryptography and receive no usable version of libssl was found error? That error means we need to surround the dlopen calls with #ifndef LOCAL_BUILD in the initialization code.

runtime/src/native/libs/System.Security.Cryptography.Native/opensslshim.c

Line 55 in d320fd9

static void OpenLibraryOnce(void)

(similar to 7646e76#diff-5c4725be11b8ef9741d6dba36be1244424dfcfc951ada9793862f002bcaee808)

kant2002 · 2023-01-10T05:16:52Z

I did test in Docker container while making call to https website. https://github.com/kant2002/NativeAOTDocker/tree/main/OpenSslEmbedding

I did not test built product, but rather patch existing ILC runtime nuget content.

Regarding ignoring opensslshim.c that file included if you build platformagnostic SSL, but I do not specify that define and things starts working.

runtime/src/native/libs/System.Security.Cryptography.Native/CMakeLists.txt

Lines 58 to 61 in d320fd9

    
           if (FEATURE_DISTRO_AGNOSTIC_SSL) 
        
               list(APPEND NATIVECRYPTO_SOURCES 
        
                   opensslshim.c 
        
               )

jkotas · 2023-01-14T07:07:29Z

@kant2002 Could you please resolve the conflict?

This setup works if I apply this as local customizations, I do not sure that I use OpenSSL in most secure way, I do not competent. This setup and StaticExecutable=true allow package just EXE file + /etc/ssl/certs/ folder in Docker

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>

…e.Unix.targets Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>

src/native/libs/build-local.sh

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>

davidfowl · 2023-01-14T08:36:50Z

Is there an issue associated with this?

kant2002 · 2023-01-14T10:02:01Z

@davidfowl Technically no, because it start as experiment to create scratch console app with ICU in #79498 and when I do not receive pushback on the scenario, I decide to make Web site on scratch container, like Go do it. Results is approximately 23MB for dotnet new web template using scratch, vs 33 MB using Chiselled Ubuntu.

If you interested, you can look at results of this experiment here https://github.com/kant2002/NativeAOTDocker

davidfowl · 2023-01-14T10:30:08Z

Can you file one?

jkotas

LGTM. Thank you!

kant2002 requested a review from MichalStrehovsky as a code owner January 9, 2023 18:59

dotnet-issue-labeler bot added the area-NativeAOT-coreclr label Jan 9, 2023

ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 9, 2023

jkotas reviewed Jan 9, 2023

View reviewed changes

am11 reviewed Jan 9, 2023

View reviewed changes

src/native/libs/build-local.sh Outdated Show resolved Hide resolved

am11 reviewed Jan 9, 2023

View reviewed changes

src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Unix.targets Outdated Show resolved Hide resolved

jkotas reviewed Jan 9, 2023

View reviewed changes

am11 reviewed Jan 9, 2023

View reviewed changes

build-analysis bot mentioned this pull request Jan 10, 2023

Tracking issue for CI build timeouts #76454

Closed

am11 reviewed Jan 10, 2023

View reviewed changes

src/coreclr/nativeaot/docs/compiling.md Show resolved Hide resolved

am11 reviewed Jan 10, 2023

View reviewed changes

src/native/libs/build-local.sh Outdated Show resolved Hide resolved

kant2002 added 5 commits January 14, 2023 13:34

Add ability statically link OpenSSL

cd9d7bc

This setup works if I apply this as local customizations, I do not sure that I use OpenSSL in most secure way, I do not competent. This setup and StaticExecutable=true allow package just EXE file + /etc/ssl/certs/ folder in Docker

Add missing file

8d3958d

Share build scripts across Globalization and OpenSSL

d57e14c

Change commennt

451f075

Restore formatting

3973f11

kant2002 and others added 5 commits January 14, 2023 13:34

Update src/native/libs/build-local.sh

387e375

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>

Update src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Nativ…

96c61b9

…e.Unix.targets Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>

Add warning

2766c1c

Add warning

e0c3af5

Restore X bit

973e7a3

kant2002 force-pushed the kant/static-link-openssl branch from 35f26b3 to 973e7a3 Compare January 14, 2023 07:36

am11 reviewed Jan 14, 2023

View reviewed changes

src/native/libs/build-local.sh Outdated Show resolved Hide resolved

Update src/native/libs/build-local.sh

c7dbd20

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>

kant2002 mentioned this pull request Jan 14, 2023

Statically linked cloud native executables - FROM SCRATCH containers #80654

Closed

jkotas approved these changes Jan 15, 2023

View reviewed changes

jkotas merged commit 2eca2d3 into dotnet:main Jan 15, 2023

kant2002 deleted the kant/static-link-openssl branch January 15, 2023 09:15

runfoapp bot mentioned this pull request Jan 16, 2023

Infra improvements for Helix #68176

Closed

chylex mentioned this pull request Jan 19, 2023

No usable version of libssl was found dotnet/core#4749

Closed

dotnet locked as resolved and limited conversation to collaborators Feb 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability statically link OpenSSL #80380

Add ability statically link OpenSSL #80380

kant2002 commented Jan 9, 2023

ghost commented Jan 9, 2023

jkotas Jan 9, 2023

kant2002 Jan 9, 2023

jkotas Jan 9, 2023

vcsjones commented Jan 9, 2023

blowdart commented Jan 9, 2023

kant2002 commented Jan 9, 2023

jkotas commented Jan 9, 2023 •

edited

blowdart commented Jan 9, 2023

jkotas Jan 9, 2023

jkotas commented Jan 9, 2023

am11 Jan 9, 2023 •

edited

kant2002 Jan 9, 2023

jkotas commented Jan 9, 2023

kant2002 commented Jan 9, 2023

jkotas Jan 9, 2023

am11 Jan 9, 2023

kant2002 Jan 10, 2023

am11 commented Jan 10, 2023

kant2002 commented Jan 10, 2023

jkotas commented Jan 14, 2023

davidfowl commented Jan 14, 2023

kant2002 commented Jan 14, 2023

davidfowl commented Jan 14, 2023

jkotas left a comment

		@@ -87,6 +95,8 @@ The .NET Foundation licenses this file to you under the MIT license.

		<Exec Command="$(IlcHostPackagePath)/native/src/libs/System.Globalization.Native/local_build.sh $(IlcHostPackagePath)/ $(IntermediateOutputPath)" Condition="'$(StaticICULinking)' == 'true'"/>

		<Exec Command="$(IlcHostPackagePath)/native/src/libs/System.Security.Cryptography.Native/local_build.sh $(IlcHostPackagePath)/ $(IntermediateOutputPath)" Condition="'$(StaticOpenSslLinking)' == 'true'"/>

		@@ -4,24 +4,25 @@
		# The .NET Foundation licenses this file to you under the MIT license.

Add ability statically link OpenSSL #80380

Add ability statically link OpenSSL #80380

Conversation

kant2002 commented Jan 9, 2023

ghost commented Jan 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcsjones commented Jan 9, 2023

blowdart commented Jan 9, 2023

kant2002 commented Jan 9, 2023

jkotas commented Jan 9, 2023 • edited

blowdart commented Jan 9, 2023

Choose a reason for hiding this comment

jkotas commented Jan 9, 2023

am11 Jan 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Jan 9, 2023

kant2002 commented Jan 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

am11 commented Jan 10, 2023

kant2002 commented Jan 10, 2023

jkotas commented Jan 14, 2023

davidfowl commented Jan 14, 2023

kant2002 commented Jan 14, 2023

davidfowl commented Jan 14, 2023

jkotas left a comment

Choose a reason for hiding this comment

jkotas commented Jan 9, 2023 •

edited

am11 Jan 9, 2023 •

edited