Skip to content

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

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.

Sign up for GitHub

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

Merged
VSadov merged 10 commits into from
Jul 9, 2021
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
348e6bb
shim one function for starters
VSadov Jul 1, 2021
4a8f960
do all functions
VSadov Jul 2, 2021
ef4c398
drop static dependency on libgssapi_krb5.so
VSadov Jul 2, 2021
998e1a9
remap all gss method calls
VSadov Jul 2, 2021
7f88c8b
keep one lib instance open.
VSadov Jul 5, 2021
3dece8f
tolerate absence of API in `gss_indicate_mechs`. It may be used for A…
VSadov Jul 6, 2021
b2868bf
init from a static constructor
VSadov Jul 8, 2021
97ca749
move the lib name definition to the shim
VSadov Jul 8, 2021
adeffde
no indirection
VSadov Jul 8, 2021
b4e8953
do not try optimizing multiple initialization attempts
VSadov Jul 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
tolerate absence of API in gss_indicate_mechs. It may be used for A… 
…PI probing.
  • Loading branch information
@VSadov
VSadov committed Jul 6, 2021
commit 3dece8fef9da3bee1eca5986aca5c85fa0ce3831
3 changes: 1 addition & 2 deletions src/libraries/Native/Unix/System.Net.Security.Native/extra_libs.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@ macro(append_extra_security_libs NativeLibsExtra)
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_SHIM)
add_definitions(-DgssLibraryName="libgssapi_krb5.so")
add_definitions(-DGSS_DYNAMIC_LIB="libgssapi_krb5.so")
Copy link
Member

@janvorli janvorli Jul 8, 2021

Choose a reason for hiding this comment

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

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.

else()
list(APPEND ${NativeLibsExtra} ${LIBGSS})
endif()
Expand Down
44 changes: 35 additions & 9 deletions src/libraries/Native/Unix/System.Net.Security.Native/pal_gssapi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
#include <assert.h>
#include <string.h>

#if defined(GSS_SHIM)
#if defined(GSS_DYNAMIC_LIB)
#include <dlfcn.h>
#include "pal_atomic.h"
#endif
Expand Down Expand Up @@ -53,7 +53,7 @@ static gss_OID_desc gss_mech_ntlm_OID_desc = {.length = ARRAY_SIZE(gss_ntlm_oid_
.elements = gss_ntlm_oid_value};
#endif

#if defined(GSS_SHIM)
#if defined(GSS_DYNAMIC_LIB)

#define FOR_ALL_GSS_FUNCTIONS \
PER_FUNCTION_BLOCK(gss_accept_sec_context) \
Expand Down Expand Up @@ -108,8 +108,8 @@ static gss_shim_t* volatile s_gss_shim_ptr = NULL;

static void init_gss_shim()
Copy link
Member

@janvorli janvorli Jul 8, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@VSadov VSadov Jul 8, 2021

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@VSadov VSadov Jul 8, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

@janvorli janvorli Jul 8, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@VSadov VSadov Jul 8, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

@janvorli janvorli Jul 8, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@VSadov VSadov Jul 8, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@VSadov VSadov Jul 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

{
void* lib = dlopen(gssLibraryName, RTLD_LAZY);
if (lib == NULL) { fprintf(stderr, "Cannot load library %s \nError: %s\n", gssLibraryName, dlerror()); abort(); }
void* lib = dlopen(GSS_DYNAMIC_LIB, RTLD_LAZY);
if (lib == NULL) { fprintf(stderr, "Cannot load library %s \nError: %s\n", GSS_DYNAMIC_LIB, dlerror()); abort(); }

// check is someone else has opened and published s_gssLib already
if (!pal_atomic_cas_ptr(&s_gssLib, lib, NULL))
Expand All @@ -119,10 +119,10 @@ static void init_gss_shim()

// initialize indirection pointers for all functions, like:
// s_gss_shim.gss_accept_sec_context_ptr = (TYPEOF(gss_accept_sec_context)*)dlsym(s_gssLib, "gss_accept_sec_context");
// if (s_gss_shim.gss_accept_sec_context_ptr == NULL) { fprintf(stderr, "Cannot get symbol %s from %s \nError: %s\n", "gss_accept_sec_context", gssLibraryName, dlerror()); abort(); }
// if (s_gss_shim.gss_accept_sec_context_ptr == NULL) { fprintf(stderr, "Cannot get symbol %s from %s \nError: %s\n", "gss_accept_sec_context", GSS_DYNAMIC_LIB, dlerror()); abort(); }
#define PER_FUNCTION_BLOCK(fn) \
s_gss_shim.fn##_ptr = (TYPEOF(fn)*)dlsym(s_gssLib, #fn); \
if (s_gss_shim.fn##_ptr == NULL) { fprintf(stderr, "Cannot get symbol " #fn " from %s \nError: %s\n", gssLibraryName, dlerror()); abort(); }
if (s_gss_shim.fn##_ptr == NULL) { fprintf(stderr, "Cannot get symbol " #fn " from %s \nError: %s\n", GSS_DYNAMIC_LIB, dlerror()); abort(); }

FOR_ALL_GSS_FUNCTIONS
#undef PER_FUNCTION_BLOCK
Expand Down Expand Up @@ -151,7 +151,6 @@ static gss_shim_t* get_gss_shim()
#define gss_display_name(...) get_gss_shim()->gss_display_name_ptr(__VA_ARGS__)
#define gss_display_status(...) get_gss_shim()->gss_display_status_ptr(__VA_ARGS__)
#define gss_import_name(...) get_gss_shim()->gss_import_name_ptr(__VA_ARGS__)
#define gss_indicate_mechs(...) get_gss_shim()->gss_indicate_mechs_ptr(__VA_ARGS__)
#define gss_init_sec_context(...) get_gss_shim()->gss_init_sec_context_ptr(__VA_ARGS__)
#define gss_inquire_context(...) get_gss_shim()->gss_inquire_context_ptr(__VA_ARGS__)
#define gss_oid_equal(...) get_gss_shim()->gss_oid_equal_ptr(__VA_ARGS__)
Expand All @@ -171,7 +170,34 @@ static gss_shim_t* get_gss_shim()
#define GSS_C_NT_HOSTBASED_SERVICE *get_gss_shim()->GSS_C_NT_HOSTBASED_SERVICE_ptr
#define gss_mech_krb5 *get_gss_shim()->gss_mech_krb5_ptr

#endif // GSS_SHIM
// NB: Managed side may call IsNtlmInstalled, which in turn calls `gss_indicate_mechs` to probe for support and
// treat all all exceptions same as `false`. Our own tests and platform detection do that.
// So we will not abort if API is not there for `gss_indicate_mechs_ptr`, and return a failure code instead.
static bool probe_gss_api()
{
if (s_gss_shim_ptr)
{
return true;
}

void* lib = dlopen(GSS_DYNAMIC_LIB, RTLD_LAZY);
if (lib == NULL)
{
return false;
}

// check is someone else has opened and published s_gssLib already
if (!pal_atomic_cas_ptr(&s_gssLib, lib, NULL))
{
dlclose(lib);
}

return true;
}

#define gss_indicate_mechs(...) (probe_gss_api() ? get_gss_shim()->gss_indicate_mechs_ptr(__VA_ARGS__) : GSS_S_UNAVAILABLE)

#endif // GSS_DYNAMIC_LIB

// transfers ownership of the underlying data from gssBuffer to PAL_GssBuffer
static void NetSecurityNative_MoveBuffer(gss_buffer_t gssBuffer, PAL_GssBuffer* targetBuffer)
Expand Down Expand Up @@ -670,7 +696,7 @@ uint32_t NetSecurityNative_IsNtlmInstalled()

uint32_t majorStatus;
uint32_t minorStatus;
gss_OID_set mechSet;
gss_OID_set mechSet = NULL;
gss_OID_desc oid;
uint32_t foundNtlm = 0;

Expand Down