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.

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 krb5 library SO name in the gss api shim #59526

Merged
merged 1 commit into from
Sep 24, 2021

Conversation

janvorli
Copy link
Member

The library name used in the shim is a name of the build time library which
is usually installed only for development purposes. We should use
libgssapi_krb5.so.2 which is the underlying runtime library.

Close #59518

The library name used in the shim is a name of the build time library which
is usually installed only for development purposes. We should use
libgssapi_krb5.so.2 which is the underlying runtime library.
@janvorli janvorli added this to the 6.0.0 milestone Sep 23, 2021
@janvorli janvorli self-assigned this Sep 23, 2021
@ghost
Copy link

ghost commented Sep 23, 2021

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

Issue Details

The library name used in the shim is a name of the build time library which
is usually installed only for development purposes. We should use
libgssapi_krb5.so.2 which is the underlying runtime library.

Close #59518

Author: janvorli
Assignees: janvorli
Labels:

area-System.Net.Security

Milestone: 6.0.0

@karelz karelz requested a review from wfurt September 23, 2021 13:47
@karelz karelz modified the milestones: 6.0.0, 7.0.0 Sep 23, 2021
@VSadov
Copy link
Member

VSadov commented Sep 23, 2021

I assumed it is the purpose of libgssapi_krb5.so - to link to the actual underlying library, assuming there are different implementations of the api with different names, i.e. MIT vs. Heimdall.

Do they both just install “so.2”?

@@ -123,7 +123,7 @@ static void* volatile s_gssLib = NULL;
#define GSS_C_NT_HOSTBASED_SERVICE (*GSS_C_NT_HOSTBASED_SERVICE_ptr)
#define gss_mech_krb5 (*gss_mech_krb5_ptr)

#define gss_lib_name "libgssapi_krb5.so"
#define gss_lib_name "libgssapi_krb5.so.2"
Copy link
Member

Choose a reason for hiding this comment

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

Can we align it with how it's done for numa/ssl?

try dlopen libgssapi_krb5.so.2
then try dlopen libgssapi_krb5.so.2.2
then try dlopen libgssapi_krb5.so

Copy link
Member Author

Choose a reason for hiding this comment

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

For SSL, it is different as there are several incompatible versions of the library we support. We always use a versioned one though. But adding fallback to libgssapi_krb5.so sounds probably fine, even though I am not aware of a distro where it would be named that way and usually a change in the major version of the library means a compatibility break. So loading just the .so if present might result in hard to debug crashes in case e.g. libgssapi_krb5.so.3 was released and had some subtle incompatible changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

@am11, it actually seems that for the NUMA case, attempting to use the versionless .so is also something that we might not want to do for the same reason I've mentioned. And do you remember why you have also added the probe for libnuma.so.1.0.0? Have we found systems where the libnuma.so.1 was not present? The comment on the PR that added the libnuma probing is actually not right, there is also libnuma.so on Fedora, the package is numactl-devel.

Copy link
Member

Choose a reason for hiding this comment

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

@janvorli, good points, I think krb5's .2 and numa's .1 are undisputed, other two variants (version-less and .krb5 2.2 / numa .1.0.0) are probably not too great to probe.

The reason of probe for actual binary (1.0.0) was that at the time I was making a .NET example for nanos unikernel docs https://github.com/nanovms/ops-examples/tree/master/dotnet and trying to reduce extra files in that environment (which strives for minimalism). Now I am realizing we can update nanos' packaging tool (ops) so it follows symlink and add both in the resultant image (which we can boot on azure/aws/google hypervisor).

@janvorli
Copy link
Member Author

I assumed it is the purpose of libgssapi_krb5.so - to link to the actual underlying library

No, in most cases, the .so without version is a dev time library. The linker then stores the actual library SONAME which includes the version to the target binary (when linking the library at build time).

@janvorli
Copy link
Member Author

The only thing I am not sure about is FreeBSD. I have found I have two of the libraries installed there:

/usr/lib/libgssapi_krb5.so
/usr/lib/libgssapi_krb5.so.10
/usr/local/lib/libgssapi_krb5.so
/usr/local/lib/libgssapi_krb5.so.2
/usr/local/lib/libgssapi_krb5.so.2.2

Might be just a result of my past experiments. I wonder if the libgssapi_krb5.so.10 is the one to use on FreeBSD and so we should add probing for it.
@josteink, @Thefrank I can see you are heavily working on FreeBSD .NET stuff, what is the truth here?

@VSadov
Copy link
Member

VSadov commented Sep 23, 2021

It seems like in general the most consistent way would be to mimic the static linker behavior - resolve the link at build time and dlopen the actual versioned .so - not because it is strictly better, but just to match the common practice.

However, since we have just one case like this, if we are sure it is always so.2, then current fix is good enough.

@wfurt
Copy link
Member

wfurt commented Sep 23, 2021

The libgssapi_krb5.so.10 is part of base OS and comes from Heimdal (same as macOS). The libgssapi_krb5.so.2 comes from krb5 package and it is the MIT implementation.
Conceptually, dependency on MIT could work but I would think using base OS would be preferable.

Seems like the packaging strategy is different on Linux. In bot cases for Heimdal and MIT the .so is part of core package not specific to debug. If we load the .so than user can set LD_LIBRARY_PATH to choose implementation. (but I'm not sure how common that would be)

I'm wondering if we should scope this change to Linux only where the claim about development is probably true.

@wfurt
Copy link
Member

wfurt commented Sep 23, 2021

BTW since this is regression, I would be ok if we proceed with this for 6.0.

@Thefrank
Copy link
Contributor

TL;DR: The ports version (aka MIT version, the one found in /usr/local/lib) is what would be preferred.

Long version:
Generally: items that ship with the OS are for the OS and items found in ports should use other items found in ports.

A specific example of this is that FreeBSD ships with a full LLVM suite. This is great until you want to use an lldb plugin only to find out plugins are not supported. The ports version of the very same LLVM suite includes this support. [As a side rant: this makes proper compiler detection a pain a times]

Also, I think we currently list krb5 as an optional package dependency for net5.

@janvorli
Copy link
Member Author

@Thefrank thank you a lot for your quick response! Then the current change is good for FreeBSD too.

@Thefrank
Copy link
Contributor

@janvorli I think this PR is getting bit by the "license/cla missing" bug

@janvorli
Copy link
Member Author

@dotnet/dnceng why am I seeing the license/cla "waiting for status to be reported"? What is the way to fix it? Closing and reopening the PR?

@alexperovich
Copy link
Member

A close and re-open will probably fix it. This is the cla bot that the opensource team manages. Re-opening the PR would re-trigger the web hooks that make the cla thing go. I haven't heard of any current issues with the bot or github.

@janvorli janvorli closed this Sep 23, 2021
@janvorli janvorli reopened this Sep 23, 2021
@janvorli janvorli merged commit 2056905 into dotnet:main Sep 24, 2021
@janvorli janvorli deleted the fix-krb5-shim branch September 24, 2021 12:51
@karelz
Copy link
Member

karelz commented Sep 24, 2021

/backport to release/6.0-rc2

@github-actions
Copy link
Contributor

Started backporting to release/6.0-rc2: https://github.com/dotnet/runtime/actions/runs/1269896275

@karelz karelz changed the title Fix krb5 library SO name in the gcc api shim Fix krb5 library SO name in the gss api shim Sep 24, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kerberos authentication not work on .NET 6 RC1 with official docker images
7 participants