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

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
do all functions
  • Loading branch information
@VSadov
VSadov committed Jul 6, 2021
commit 4a8f9609e43e108f6cb5e85869b0c7968c932a4a
80 changes: 52 additions & 28 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 @@ -22,7 +22,7 @@

#ifdef TARGET_LINUX
#include <dlfcn.h>
#include "pal_atomic.h"
#include <stdatomic.h>
#endif

c_static_assert(PAL_GSS_C_DELEG_FLAG == GSS_C_DELEG_FLAG);
Expand Down Expand Up @@ -58,23 +58,70 @@ static gss_OID_desc gss_mech_ntlm_OID_desc = {.length = ARRAY_SIZE(gss_ntlm_oid_

#define libraryName "libgssapi_krb5.so"

#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)

#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
{
TYPEOF(gss_accept_sec_context)* gss_accept_sec_context_ptr;
// 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;

// 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

@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.

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

@VSadov VSadov Jul 9, 2021

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

@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(libraryName, RTLD_LAZY);
if (lib == NULL) { fprintf(stderr, "Cannot load library %s \nError: %s\n", libraryName, dlerror()); abort(); }

s_gss_shim.gss_accept_sec_context_ptr = (TYPEOF(gss_accept_sec_context)*)dlsym(lib, "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", libraryName, dlerror()); abort(); }
// initialize indirection pointers for all functions, like:
// s_gss_shim.gss_accept_sec_context_ptr = (TYPEOF(gss_accept_sec_context)*)dlsym(lib, "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", libraryName, dlerror()); abort(); }
#define PER_FUNCTION_BLOCK(fn) \
s_gss_shim.fn##_ptr = (TYPEOF(fn)*)dlsym(lib, #fn); \
if (s_gss_shim.fn##_ptr == NULL) { fprintf(stderr, "Cannot get symbol " #fn " from %s \nError: %s\n", libraryName, dlerror()); abort(); }

FOR_ALL_GSS_FUNCTIONS
#undef PER_FUNCTION_BLOCK

pal_atomic_cas_ptr((void* volatile *)&s_gss_shim_ptr, &s_gss_shim, NULL);
// publish the shim pointer
__atomic_store_n(&s_gss_shim_ptr, &s_gss_shim, __ATOMIC_RELEASE);
dlclose(lib);
}

Expand All @@ -92,29 +139,6 @@ static gss_shim_t* get_gss_shim()

#define gss_accept_sec_context(...) get_gss_shim()->gss_accept_sec_context_ptr(__VA_ARGS__)

// gss_accept_sec_context
// gss_acquire_cred
// gss_acquire_cred_with_password
// gss_delete_sec_context
// gss_display_name
// gss_display_status
// gss_import_name
// gss_indicate_mechs
// gss_init_sec_context
// gss_inquire_context
// gss_mech_krb5
// gss_oid_equal
// gss_release_buffer
// gss_release_cred
// gss_release_name
// gss_release_oid_set
// gss_unwrap
// gss_wrap

#if HAVE_GSS_KRB5_CRED_NO_CI_FLAGS_X
// gss_set_cred_option
#endif

#endif // TARGET_LINUX

// transfers ownership of the underlying data from gssBuffer to PAL_GssBuffer
Expand Down