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

Fix usage of GSS_KRB5_CRED_NO_CI_FLAGS_X #70447

Merged
merged 5 commits into from
Jun 14, 2022
Merged

Fix usage of GSS_KRB5_CRED_NO_CI_FLAGS_X #70447

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
023ec28
Fix build detection of GSS_KRB5_CRED_NO_CI_FLAGS_X
filipnavara Jun 8, 2022
3490f24
Don't use GSS_KRB5_CRED_NO_CI_FLAGS_X on NTLM
filipnavara Jun 8, 2022
2f3e77c
Handle GSS_S_UNAVAILABLE error from gss_set_cred_option (NTLM wrapped…
filipnavara Jun 8, 2022
65b5ef7
Make the GSSAPI shim work with krb5 1.13
filipnavara Jun 9, 2022
2f7906a
Preserve the gss_acquire_cred minor status
filipnavara Jun 9, 2022
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
68 changes: 53 additions & 15 deletions src/native/libs/System.Net.Security.Native/pal_gssapi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,21 @@ static gss_OID_desc gss_mech_ntlm_OID_desc = {.length = STRING_LENGTH(gss_ntlm_o

#if defined(GSS_SHIM)

#define FOR_ALL_GSS_FUNCTIONS \
#if HAVE_GSS_KRB5_CRED_NO_CI_FLAGS_X

#define FOR_ALL_OPTIONAL_GSS_FUNCTIONS \
PER_FUNCTION_BLOCK(gss_set_cred_option) \
PER_FUNCTION_BLOCK(GSS_KRB5_CRED_NO_CI_FLAGS_X)

#define GSS_KRB5_CRED_NO_CI_FLAGS_X_AVAILABLE (gss_set_cred_option_ptr != NULL && GSS_KRB5_CRED_NO_CI_FLAGS_X_ptr != NULL)

#else

#define FOR_ALL_OPTIONAL_GSS_FUNCTIONS

#endif //HAVE_GSS_KRB5_CRED_NO_CI_FLAGS_X

#define FOR_ALL_REQUIRED_GSS_FUNCTIONS \
PER_FUNCTION_BLOCK(gss_accept_sec_context) \
PER_FUNCTION_BLOCK(gss_acquire_cred) \
PER_FUNCTION_BLOCK(gss_acquire_cred_with_password) \
Expand All @@ -78,14 +92,11 @@ static gss_OID_desc gss_mech_ntlm_OID_desc = {.length = STRING_LENGTH(gss_ntlm_o
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
PER_FUNCTION_BLOCK(GSS_C_NT_HOSTBASED_SERVICE) \

#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
#define FOR_ALL_GSS_FUNCTIONS \
FOR_ALL_REQUIRED_GSS_FUNCTIONS \
FOR_ALL_OPTIONAL_GSS_FUNCTIONS

// define indirection pointers for all functions, like
// static TYPEOF(gss_accept_sec_context)* gss_accept_sec_context_ptr;
Expand Down Expand Up @@ -118,6 +129,7 @@ static void* volatile s_gssLib = NULL;

#if HAVE_GSS_KRB5_CRED_NO_CI_FLAGS_X
#define gss_set_cred_option(...) gss_set_cred_option_ptr(__VA_ARGS__)
#define GSS_KRB5_CRED_NO_CI_FLAGS_X (*GSS_KRB5_CRED_NO_CI_FLAGS_X_ptr)
#endif //HAVE_GSS_KRB5_CRED_NO_CI_FLAGS_X


Expand All @@ -138,19 +150,27 @@ static int32_t ensure_gss_shim_initialized()
dlclose(lib);
}

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

FOR_ALL_GSS_FUNCTIONS
FOR_ALL_REQUIRED_GSS_FUNCTIONS
#undef PER_FUNCTION_BLOCK
// for optional functions skip the error check
#define PER_FUNCTION_BLOCK(fn) \
fn##_ptr = (TYPEOF(fn)*)dlsym(s_gssLib, #fn);
FOR_ALL_OPTIONAL_GSS_FUNCTIONS
#undef PER_FUNCTION_BLOCK

return 0;
}

#else // GSS_SHIM

#define GSS_KRB5_CRED_NO_CI_FLAGS_X_AVAILABLE 1

#endif // GSS_SHIM

// transfers ownership of the underlying data from gssBuffer to PAL_GssBuffer
Expand Down Expand Up @@ -183,10 +203,19 @@ static uint32_t AcquireCredSpNego(uint32_t* minorStatus,

// call gss_set_cred_option with GSS_KRB5_CRED_NO_CI_FLAGS_X to support Kerberos Sign Only option from *nix client against a windows server
#if HAVE_GSS_KRB5_CRED_NO_CI_FLAGS_X
if (majorStatus == GSS_S_COMPLETE)
if (majorStatus == GSS_S_COMPLETE && GSS_KRB5_CRED_NO_CI_FLAGS_X_AVAILABLE)
{
GssBuffer emptyBuffer = GSS_C_EMPTY_BUFFER;
majorStatus = gss_set_cred_option(minorStatus, outputCredHandle, GSS_KRB5_CRED_NO_CI_FLAGS_X, &emptyBuffer);
uint32_t tempMinorStatus;
majorStatus = gss_set_cred_option(&tempMinorStatus, outputCredHandle, GSS_KRB5_CRED_NO_CI_FLAGS_X, &emptyBuffer);
if (majorStatus == GSS_S_UNAVAILABLE)
{
majorStatus = GSS_S_COMPLETE;
}
else
{
*minorStatus = tempMinorStatus;
}
Copy link
Member

@am11 am11 Jun 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This can be simplified, since minorStatus value is unused.

Suggested change
uint32_t tempMinorStatus;
majorStatus = gss_set_cred_option(&tempMinorStatus, outputCredHandle, GSS_KRB5_CRED_NO_CI_FLAGS_X, &emptyBuffer);
if (majorStatus == GSS_S_UNAVAILABLE)
{
majorStatus = GSS_S_COMPLETE;
}
else
{
*minorStatus = tempMinorStatus;
}
majorStatus = gss_set_cred_option(&minorStatus, outputCredHandle, GSS_KRB5_CRED_NO_CI_FLAGS_X, &emptyBuffer);
if (majorStatus == GSS_S_UNAVAILABLE)
{
majorStatus = GSS_S_COMPLETE;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not unused, it is passed to the managed code (where it is indeed unused but that may change).

Copy link
Member

Choose a reason for hiding this comment

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

We usually delete the dead code and add code when needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this particular case, it's problematic to change the native prototypes because dotnet/sqlclient has a dependency on it. We are trying to improve the situation going forward by introducing the NegotiateAuthentication public API but that work didn't land yet.

Copy link
Member

Choose a reason for hiding this comment

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

As minimum, I would suggest to put the minor error code to tracing. e.g. make use of it for debugging.

Copy link
Member Author

Choose a reason for hiding this comment

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

I double-checked and it's passed in exception already:

runtime/src/libraries/Common/src/Microsoft/Win32/SafeHandles/GssSafeHandles.cs

Line 126 in 6de7147

throw new Interop.NetSecurityNative.GssApiException(status, minorStatus);

Copy link
Member Author

@filipnavara filipnavara Jun 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Your proposed change always overrides the minor_status from gss_acquire_cred if this code path is taken. The idea was to preserve full gss_acquire_cred status (major+minor) if gss_set_cred_option returns GSS_S_UNAVAILABLE or GSS_S_COMPLETE.

Apparently somewhere along the way I incorrectly dropped the GSS_S_COMPLETE from the first if. I'll fix that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that fixing the if makes no difference since the managed code ignores minorStatus when the major one is GSS_S_COMPLETE but I want to make the intention clear. That is, preserve the full gss_acquire_cred status if gss_set_cred_option succeeds or is unavailable. In all other cases return the full status from gss_set_cred_option.

Copy link
Member

Choose a reason for hiding this comment

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

should we handle GSS_S_UNAVAILABLE in managed code? It seems little bit strange to do the mapping here in PAL.

Copy link
Member Author

@filipnavara filipnavara Jun 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We can map the whole call in PAL and call it from managed code. I don't think it's worth it, to be honest, but it would not be hard to do so. The downside is that SqlClient would not get this fix for free.

Essentially, gss_set_cred_option will succeed only on Kerberos and Negotiate-over-Kerberos where it does a pass-through gss_set_cred_option call to the underlying Kerberos GSSAPI provider. The GSS_S_UNAVAILABLE case happens when there's no Kerberos and GSSAPI maps to Negotiate-over-NTLM, forwards to NTLM provider and that one doesn't recognize the GSS_KRB5_CRED_NO_CI_FLAGS_X option.

}
#endif

Expand Down Expand Up @@ -606,10 +635,19 @@ static uint32_t AcquireCredWithPassword(uint32_t* minorStatus,

// call gss_set_cred_option with GSS_KRB5_CRED_NO_CI_FLAGS_X to support Kerberos Sign Only option from *nix client against a windows server
#if HAVE_GSS_KRB5_CRED_NO_CI_FLAGS_X
if (majorStatus == GSS_S_COMPLETE)
if (!isNtlm && majorStatus == GSS_S_COMPLETE && GSS_KRB5_CRED_NO_CI_FLAGS_X_AVAILABLE)
{
GssBuffer emptyBuffer = GSS_C_EMPTY_BUFFER;
majorStatus = gss_set_cred_option(minorStatus, outputCredHandle, GSS_KRB5_CRED_NO_CI_FLAGS_X, &emptyBuffer);
uint32_t tempMinorStatus;
majorStatus = gss_set_cred_option(&tempMinorStatus, outputCredHandle, GSS_KRB5_CRED_NO_CI_FLAGS_X, &emptyBuffer);
if (majorStatus == GSS_S_UNAVAILABLE)
{
majorStatus = GSS_S_COMPLETE;
}
else
{
*minorStatus = tempMinorStatus;
}
}
#endif

Expand Down
13 changes: 13 additions & 0 deletions src/native/libs/configure.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1030,6 +1030,17 @@ check_include_files(
GSS/GSS.h
HAVE_GSSFW_HEADERS)

if (HAVE_GSSFW_HEADERS)
Copy link
Member Author

@filipnavara filipnavara Jun 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I tagged you as a resident CMake expert. Honestly I have no idea what am I doing here and if I am doing it right...

I basically took that block and reduced it to minimum necessary to get the checks working. I'm not sure if moving it is possible since the extra_libs.cmake is also needed for the single-file host builds where everything is linked statically. Maybe including the extra_libs.cmake is an option? I didn't see that pattern anywhere though.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I forgot about apphost path.

find_library(LIBGSS NAMES GSS)
elseif (HAVE_HEIMDAL_HEADERS)
find_library(LIBGSS NAMES gssapi)
else ()
find_library(LIBGSS NAMES gssapi_krb5)
endif ()

set (PREVIOUS_CMAKE_REQUIRED_LIBRARIES ${CMAKE_REQUIRED_LIBRARIES})
set (CMAKE_REQUIRED_LIBRARIES ${LIBGSS})

if (HAVE_GSSFW_HEADERS)
check_symbol_exists(
GSS_SPNEGO_MECHANISM
Expand All @@ -1054,6 +1065,8 @@ else ()
HAVE_GSS_KRB5_CRED_NO_CI_FLAGS_X)
endif ()

set (CMAKE_REQUIRED_LIBRARIES ${PREVIOUS_CMAKE_REQUIRED_LIBRARIES})

check_symbol_exists(getauxval sys/auxv.h HAVE_GETAUXVAL)
check_include_files(crt_externs.h HAVE_CRT_EXTERNS_H)

Expand Down