Shim gss api on Linux to delay loading libgssapi_krb5.so #55037

VSadov · 2021-07-01T20:19:46Z

Introduced a shim for GSS APIs similar to ssl shim. There is no longer a static dependency on libgssapi_krb5.so and loader does require it as a prerequisite to run singlefile apps.

On the first use of anything from libgssapi_krb5.so, we load the library dynamically and set up pointers to all the functions of interest and call the functions via these indirections.

This case is simpler than ssl though - there are much less functions to shim and no gnarly issues with versioning, functions that may optional be present and so on.

Fixes: #45720 , #54347

ghost · 2021-07-01T20:36:42Z

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

In progress.

Author:	VSadov
Assignees:	-
Labels:	`* NO MERGE *`, `NO REVIEW`, `area-System.Net.Security`
Milestone:	-

VSadov · 2021-07-05T20:04:58Z

Here is the list of needed shared libraries for the singlefilehost after this change:

readelf -d singlefilehost | grep NEEDED
 0x0000000000000001 (NEEDED)             Shared library: [libpthread.so.0]
 0x0000000000000001 (NEEDED)             Shared library: [libdl.so.2]
 0x0000000000000001 (NEEDED)             Shared library: [libz.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [librt.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libgcc_s.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libstdc++.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [libm.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [ld-linux-x86-64.so.2]

libgssapi_krb5.so is no longer in the list.
The rest seem reasonable for a minimally required set.

ghost · 2021-07-05T20:07:35Z

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

Issue Details

Introduced a shim similar to ssl. There is no longer a static dependency on libgssapi_krb5.so and loader does not reach for it on app start.
On the first use of anything from libgssapi_krb5.so, we loads the library and set up pointers to the functions of interest and call the functions via these indirections.

This case is simpler than ssl though - there is a lot less functions to shim and no issues with versioning, optional and so on.

Fixes: #45720 , #54347

Author:	VSadov
Assignees:	-
Labels:	`area-Single-File`, `area-System.Net.Security`
Milestone:	-

…PI probing.

janvorli · 2021-07-08T12:33:24Z

src/libraries/Native/Unix/System.Net.Security.Native/extra_libs.cmake

+  if(CLR_CMAKE_TARGET_LINUX)
+    # On Linux libgssapi_krb5.so is loaded on demand to tolerate its absense in singlefile apps that do not use it
+    list(APPEND ${NativeLibsExtra} dl)
+    add_definitions(-DGSS_DYNAMIC_LIB="libgssapi_krb5.so")


I would prefer defining the name in the actual shim. We do it that way for openssl and I don't see a benefit of having it here.

janvorli · 2021-07-08T12:38:59Z

src/libraries/Native/Unix/System.Net.Security.Native/pal_gssapi.c

+//       data dependency ensures that method pointers are loaded after reading and null-checking the shim reference.
+static gss_shim_t* volatile s_gss_shim_ptr = NULL;
+
+static void init_gss_shim()


We could place this into a shared library constructor and then we would not have to do anything atomic or check for initialization when accessing the functions. The constructor is any void function with void argument list marked by __attribute__((constructor)). It can be static and may also need __attribute__((__unused__)) so that linker doesn't eliminate it.

Would initializing in a module constructor make it eager-initializing again?

The goals here are:

tolerate cases when the krb5 .so is not present, as it happens fairly often in containers.

delay initialization until used as this API is relatively rare on Linux.

I am sorry, I don't know what I was thinking. Of course you are right. I wonder though - both the openssl and ICU shims have initialization functions that are explicitly called by the managed code (CryptoNative_EnsureOpenSslInitialized / GlobalizationNative_LoadICU), . The benefit is that the failure to load the library happens at a single point and not at a random call to a function from the library. It seems it would be nice to use the same way here.

GSS/KRB5 use patterns do not seem to have explicit initialization. When used, the API is expected to be present, otherwise the app fails.

It seem acceptable and I do not think we have a too strong case to change that.

The problem with singlefile is that the app fails even when the API is not used, which is a regression from non-singlefile case.
On-demand loading fixes just that part.

It is also not a mainline scenario. If the app really needs the API, the user should not have it removed.

Basically - I think a smaller change that delays the failure would be sufficient in this case.

OpenSSL is the same case. The initialization is triggered by this code:

runtime/src/libraries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.Initialization.cs

Lines 30 to 54 in 57bfe47

internal static class CryptoInitializer

{

static CryptoInitializer()

{

if (EnsureOpenSslInitialized() != 0)

{

// Ideally this would be a CryptographicException, but we use

// OpenSSL in libraries lower than System.Security.Cryptography.

// It's not a big deal, though: this will already be wrapped in a

// TypeLoadException, and this failing means something is very

// wrong with the system's configuration and any code using

// these libraries will be unable to operate correctly.

throw new InvalidOperationException();

}

}

internal static void Initialize()

{

// No-op that exists to provide a hook for other static constructors.

}

[DllImport(Libraries.AndroidCryptoNative, EntryPoint = "CryptoNative_EnsureOpenSslInitialized")]

private static extern int EnsureOpenSslInitialized();

}

}

So when the System.Security.Cryptography managed assembly is loaded, the native shim is initialized by the static constructor. When the app doesn't use that assembly, the native shim is not initialized and no functions exported by the native library are called.
A benefit of this approach is that instead of abort from the native code when the library is not installed and the app wants to use it, you'll get an unhandled exception with managed stack trace.

Ah, yes, the static constructor trick. That should be equally non-disruptive to the overall use of the API and have a better failure mode.

Let me see if we can use it here.

Pushed a change that switches to static constructor scheme.

It looks like, while highly improbable, we can have concurrent initialization because IsNtlmInstalled could live in its own class via a file include. I can't rule out the need for atomic things, so I kept them.

janvorli · 2021-07-08T23:20:55Z

src/libraries/Native/Unix/System.Net.Security.Native/pal_gssapi.c

+static gss_shim_t s_gss_shim;
+
+// reference to the shim storage.
+static gss_shim_t* volatile s_gss_shim_ptr = NULL;


A nit - now that the shim is guaranteed to be initialized before the first use, we could just use a static instance of the gss_shim_t or just make the pointers standalone global variables without a wrapper struct like we do in the ICU and OpenSSL shims.

Right, the indirection no longer serves the purpose of intercepting and initializing. I thought it was still useful for ordering the reads - to make sure individual proxies are not loaded before checking that the whole thing is initialized (on ARM64 and like that), but I guess I can just use an acquiring read, since I no longer need to check on every invocation.

Actually I am not sure we need a flag - just let it initialize more than once. We should not have too many classes running this initializer anyways.
As I see SSL does not try optimizing redundant initializations either, and it is a lot heavier there.

janvorli

LGTM, thank you!

VSadov · 2021-07-09T14:28:55Z

Thanks!!

VSadov added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Jul 1, 2021

dotnet-issue-labeler bot added the area-System.Net.Security label Jul 1, 2021

VSadov force-pushed the krbShim branch from a8636d1 to a08f9a0 Compare July 2, 2021 21:11

dotnet deleted a comment from azure-pipelines bot Jul 5, 2021

VSadov changed the title ~~[WIP] Shim gss api on Linux to delay loading libgssapi_krb5.so~~ Shim gss api on Linux to delay loading libgssapi_krb5.so Jul 5, 2021

dotnet deleted a comment from azure-pipelines bot Jul 5, 2021

VSadov requested a review from janvorli July 5, 2021 19:57

VSadov removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Jul 5, 2021

VSadov added the area-Single-File label Jul 5, 2021

VSadov added 6 commits July 6, 2021 16:41

shim one function for starters

348e6bb

do all functions

4a8f960

drop static dependency on libgssapi_krb5.so

ef4c398

remap all gss method calls

998e1a9

keep one lib instance open.

7f88c8b

tolerate absence of API in gss_indicate_mechs. It may be used for A…

3dece8f

…PI probing.

VSadov force-pushed the krbShim branch from 20907f8 to 3dece8f Compare July 6, 2021 23:42

janvorli reviewed Jul 8, 2021

View reviewed changes

VSadov added 2 commits July 8, 2021 16:14

init from a static constructor

b2868bf

move the lib name definition to the shim

97ca749

VSadov force-pushed the krbShim branch from c4a2945 to 97ca749 Compare July 8, 2021 23:14

janvorli reviewed Jul 8, 2021

View reviewed changes

VSadov added 2 commits July 8, 2021 17:08

no indirection

adeffde

do not try optimizing multiple initialization attempts

b4e8953

janvorli approved these changes Jul 9, 2021

View reviewed changes

VSadov merged commit 7cfa95b into dotnet:main Jul 9, 2021

VSadov deleted the krbShim branch July 9, 2021 14:30

This was referenced Jul 9, 2021

System shared-library dependencies are not lazily loaded in single-file on Linux #54347

Closed

Publishing release as single file does not include all libraries (libgssapi_krb5) #45720

Closed

runfoapp bot mentioned this pull request Jul 10, 2021

Feed unreliability affecting CI #55449

Closed

karelz added this to the 6.0.0 milestone Jul 15, 2021

ManickaP mentioned this pull request Jul 20, 2021

[QUIC] Remove AppContext switch from S.N.Quic #56027

Merged

karelz mentioned this pull request Jul 28, 2021

[WinHttpHandler] Long running test: ResponseHeadersRead_SynchronizationContextNotUsedByHandler on Win7/Win8 #54034

Closed

ghost locked as resolved and limited conversation to collaborators Aug 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shim gss api on Linux to delay loading libgssapi_krb5.so #55037

Shim gss api on Linux to delay loading libgssapi_krb5.so #55037

VSadov commented Jul 1, 2021 •

edited

Loading

ghost commented Jul 1, 2021

VSadov commented Jul 5, 2021 •

edited

Loading

ghost commented Jul 5, 2021

janvorli Jul 8, 2021

janvorli Jul 8, 2021

VSadov Jul 8, 2021

VSadov Jul 8, 2021

janvorli Jul 8, 2021

VSadov Jul 8, 2021

janvorli Jul 8, 2021

VSadov Jul 8, 2021

VSadov Jul 8, 2021 •

edited

Loading

janvorli Jul 8, 2021

VSadov Jul 8, 2021 •

edited

Loading

VSadov Jul 9, 2021

janvorli left a comment

VSadov commented Jul 9, 2021

	internal static class CryptoInitializer
	{
	static CryptoInitializer()
	{
	if (EnsureOpenSslInitialized() != 0)
	{
	// Ideally this would be a CryptographicException, but we use
	// OpenSSL in libraries lower than System.Security.Cryptography.
	// It's not a big deal, though: this will already be wrapped in a
	// TypeLoadException, and this failing means something is very
	// wrong with the system's configuration and any code using
	// these libraries will be unable to operate correctly.
	throw new InvalidOperationException();
	}
	}

	internal static void Initialize()
	{
	// No-op that exists to provide a hook for other static constructors.
	}

	[DllImport(Libraries.AndroidCryptoNative, EntryPoint = "CryptoNative_EnsureOpenSslInitialized")]
	private static extern int EnsureOpenSslInitialized();
	}
	}

Shim gss api on Linux to delay loading libgssapi_krb5.so #55037

Shim gss api on Linux to delay loading libgssapi_krb5.so #55037

Conversation

VSadov commented Jul 1, 2021 • edited Loading

ghost commented Jul 1, 2021

VSadov commented Jul 5, 2021 • edited Loading

ghost commented Jul 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janvorli left a comment

Choose a reason for hiding this comment

VSadov commented Jul 9, 2021

VSadov commented Jul 1, 2021 •

edited

Loading

VSadov commented Jul 5, 2021 •

edited

Loading

VSadov Jul 8, 2021 •

edited

Loading

VSadov Jul 8, 2021 •

edited

Loading