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.

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

Jump to bottom

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

Merged
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 6 commits
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
8 changes: 7 additions & 1 deletion 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 @@ -19,5 +19,11 @@ macro(append_extra_security_libs NativeLibsExtra)
endif()
endif()

list(APPEND ${NativeLibsExtra} ${LIBGSS})
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")
Copy link
Member

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()
endmacro()
153 changes: 152 additions & 1 deletion 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,6 +20,11 @@
#include <assert.h>
#include <string.h>

#if defined(GSS_DYNAMIC_LIB)
#include <dlfcn.h>
#include "pal_atomic.h"
#endif

c_static_assert(PAL_GSS_C_DELEG_FLAG == GSS_C_DELEG_FLAG);
c_static_assert(PAL_GSS_C_MUTUAL_FLAG == GSS_C_MUTUAL_FLAG);
c_static_assert(PAL_GSS_C_REPLAY_FLAG == GSS_C_REPLAY_FLAG);
Expand Down Expand Up @@ -48,6 +53,152 @@ static gss_OID_desc gss_mech_ntlm_OID_desc = {.length = ARRAY_SIZE(gss_ntlm_oid_
.elements = gss_ntlm_oid_value};
#endif

#if defined(GSS_DYNAMIC_LIB)

#define FOR_ALL_GSS_FUNCTIONS \
PER_FUNCTION_BLOCK(gss_accept_sec_context) \
PER_FUNCTION_BLOCK(gss_acquire_cred) \
PER_FUNCTION_BLOCK(gss_acquire_cred_with_password) \
PER_FUNCTION_BLOCK(gss_delete_sec_context) \
PER_FUNCTION_BLOCK(gss_display_name) \
PER_FUNCTION_BLOCK(gss_display_status) \
PER_FUNCTION_BLOCK(gss_import_name) \
PER_FUNCTION_BLOCK(gss_indicate_mechs) \
PER_FUNCTION_BLOCK(gss_init_sec_context) \
PER_FUNCTION_BLOCK(gss_inquire_context) \
PER_FUNCTION_BLOCK(gss_mech_krb5) \
PER_FUNCTION_BLOCK(gss_oid_equal) \
PER_FUNCTION_BLOCK(gss_release_buffer) \
PER_FUNCTION_BLOCK(gss_release_cred) \
PER_FUNCTION_BLOCK(gss_release_name) \
PER_FUNCTION_BLOCK(gss_release_oid_set) \
PER_FUNCTION_BLOCK(gss_unwrap) \
PER_FUNCTION_BLOCK(gss_wrap) \
PER_FUNCTION_BLOCK(GSS_C_NT_USER_NAME) \
PER_FUNCTION_BLOCK(GSS_C_NT_HOSTBASED_SERVICE)

#if HAVE_GSS_KRB5_CRED_NO_CI_FLAGS_X

#define FOR_ALL_GSS_FUNCTIONS FOR_ALL_GSS_FUNCTIONS \
PER_FUNCTION_BLOCK(gss_set_cred_option)

#endif //HAVE_GSS_KRB5_CRED_NO_CI_FLAGS_X

typedef struct gss_shim_t
{
// define indirection pointers for all functions, like
// TYPEOF(gss_accept_sec_context)* gss_accept_sec_context_ptr;
#define PER_FUNCTION_BLOCK(fn) \
TYPEOF(fn)* fn##_ptr;

FOR_ALL_GSS_FUNCTIONS
#undef PER_FUNCTION_BLOCK
} gss_shim_t;

// static storage for all method pointers
static gss_shim_t s_gss_shim;

static void* volatile s_gssLib = NULL;

// reference to the shim storage.
// NOTE: the shim reference is published after all indirection pointers are initialized.
// when we read the indirection pointers, we do that via the shim reference.
// 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;
Copy link
Member

Choose a reason for hiding this comment

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

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.

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.

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


static void init_gss_shim()
Copy link
Member

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

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

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

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

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

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

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(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))
{
dlclose(lib);
}

// 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", 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", GSS_DYNAMIC_LIB, dlerror()); abort(); }

FOR_ALL_GSS_FUNCTIONS
#undef PER_FUNCTION_BLOCK

// publish the shim pointer
__atomic_store_n(&s_gss_shim_ptr, &s_gss_shim, __ATOMIC_RELEASE);
}

static gss_shim_t* get_gss_shim()
{
gss_shim_t* ptr = s_gss_shim_ptr;
if (ptr == NULL)
{
init_gss_shim();
return s_gss_shim_ptr;
}

return ptr;
}

// remap gss function use to use indirection pointers
#define gss_accept_sec_context(...) get_gss_shim()->gss_accept_sec_context_ptr(__VA_ARGS__)
#define gss_acquire_cred(...) get_gss_shim()->gss_acquire_cred_ptr(__VA_ARGS__)
#define gss_acquire_cred_with_password(...) get_gss_shim()->gss_acquire_cred_with_password_ptr(__VA_ARGS__)
#define gss_delete_sec_context(...) get_gss_shim()->gss_delete_sec_context_ptr(__VA_ARGS__)
#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_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__)
#define gss_release_buffer(...) get_gss_shim()->gss_release_buffer_ptr(__VA_ARGS__)
#define gss_release_cred(...) get_gss_shim()->gss_release_cred_ptr(__VA_ARGS__)
#define gss_release_name(...) get_gss_shim()->gss_release_name_ptr(__VA_ARGS__)
#define gss_release_oid_set(...) get_gss_shim()->gss_release_oid_set_ptr(__VA_ARGS__)
#define gss_unwrap(...) get_gss_shim()->gss_unwrap_ptr(__VA_ARGS__)
#define gss_wrap(...) get_gss_shim()->gss_wrap_ptr(__VA_ARGS__)

#if HAVE_GSS_KRB5_CRED_NO_CI_FLAGS_X
#define gss_set_cred_option(...) get_gss_shim()->gss_set_cred_option_ptr(__VA_ARGS__)
#endif //HAVE_GSS_KRB5_CRED_NO_CI_FLAGS_X


#define GSS_C_NT_USER_NAME *get_gss_shim()->GSS_C_NT_USER_NAME_ptr
#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

// 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 @@ -545,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