[Release 3.1] | Address Shim gss api on Linux to delay loading libgssapi_krb5.so while using Net6

[JRahnama]

Ports dotnet/sqlclient#1411 to add shim gss api on Linux following changes in dotnet/runtime#55037

Summary

When working with Kerberos authentication on Unix with recent changes in Net6 users fail to get a connection from Sql server. There is no exception thrown as well. It just crashes.

Customer Impact

This will prevent users from upgrading to Net6 while using Kerberos authentication in Unix.

Testing

Adding tests require some special setup on CI pipelines to install Net6 TFS and enabled Kerberos authentication.

cc: @danmoseley @saurabh500 @David-Engel

[danmoseley]

I didn't notice you were putting this in 3.1. I believe this is needed in 6.0. That would be the dotnet/runtime repo, in the release/6.0 branch.

[danmoseley]

linking dotnet/SqlClient#1390

[David-Engel]

I didn't notice you were putting this in 3.1. I believe this is needed in 6.0. That would be the dotnet/runtime repo, in the release/6.0 branch.

@danmoseley

I thought System.Data.SqlClient was removed from runtime after 3.1 and this was the last servicing branch that would contain it. This is where we've backported all SDS fixes in the past:
https://github.com/dotnet/corefx/pulls?q=is%3Apr+author%3Acheenamalhotra
And there are zero SDS PRs in dotnet/runtime:
https://github.com/dotnet/runtime/pulls?q=is%3Aopen+is%3Apr+label%3Aarea-System.Data.SqlClient

Service releases of SDS from this 3.1 branch rely on continued backwards compatibility in .NET 5+.

[danmoseley]

My assumption was that the fix needed to be in the libraries themselves.

It is possible I didn't think about this clearly (moving too fast). Let's discuss in dotnet/runtime#65469

[David-Engel]

You might be right. I didn't realize the code change was not in a SqlClient-specific file. There is probably a nuance I'm missing, too.

[carlossanlop]

@David-Engel @danmoseley the branch is open for servicing changes. What should we do about this PR?

[David-Engel]

What should we do about this PR?

System.Data.SqlClient still needs the fix. But I think the PR needs to be updated. From jkotas' comment:

The delta in this PR affects both the core runtime assemblies and System.Data.SqlClient. The change in core runtime assemblies is unnecessary - it comes with unnecessary risk. If you go with adding the workaround to release/3.1, it should be authored to affect System.Data.SqlClient only.

@JRahnama It sounds like the fix needs to be moved into an appropriate file that only affects System.Data.SqlClient.

[ericstj]

This PR needs changes and needs tactics approval before merging.

[David-Engel]

This PR needs changes and needs tactics approval before merging.

@ericstj @danmoseley - Changes have been made. What's involved with getting tactics approval for this? We have one PR approval from Jan. Do we need any others?

[ericstj]

What's involved with getting tactics approval for this? We have one PR approval from Jan. Do we need any others?

Once it's ready for approval you need to add the servicing-consider label and someone needs to represent in the tactics meeting. When you're ready, reach out to Dan or myself over email and we can share the invite.

[David-Engel]

/azp run

[ericstj]

@ericstj I wonder whether we should consider whether we should move this code elsewhere (perhaps runtime in the 7.0 branch) so we can fully retire corefx when we move the ASP.NET relevant bits from 2.1 also?

Some history here: we had set the precedent in 1.x that packages would be maintained in main so that we could keep servicing costs down. Want an update? - just move to latest. In 2.0 we removed many of the packages that were absorbed by the shared framework and netstandard because we knew we'd never need to ship them again - they contained no serviceable code since the implementations moved into the frameworks (.NETCore shared framework and .NETFramework). It was later that some packages were removed that were not absorbed by frameworks - or not all frameworks. These were removed based on the premise that they were "done" and would go out of support when their branch when out of support. It sounds like that's not the case here. What's changed since then? Why are we changing the POR?

[jkotas]

we move the ASP.NET relevant bits from 2.1

Where are we moving the ASP.NET relevant bits from 2.1?

I am wondering whether we need to have dotnet/runtime-graveyard repo for packages that are in a perpetual servicing-only mode and that does not really make sense to carry in dotnet/runtime.

[danmoseley]

@jkotas I was referring to discussion about moving the few bits in this repo that are supported indefinitely as part of ASP.NET 2.1 on .NET Framework into the aspnet repo soon so any servicing can happen in one place. @ericstj is the owner of this plan and can speak to it and correct me if needed.

[bartonjs]

I share in the upset that a NuGet package is depending directly on an inbox shim, but this isn't introducing the problem.

NetSecurityNative_EnsureGssInitialized looks like it is, observationally, idempotent. So, the initializer being called twice won't break things. (It doesn't have the possibility of any fn_ptr being temporarily reassigned to nullptr that the crypto shim had at one point)

[carlossanlop]

@JRahnama @DavoudEshtehari please add the OOB package authoring changes so this can merge.

I think none of the test failures are related, but can you please also double check?

[JRahnama]

please add the OOB package authoring changes so this can merge.

I am not sure if I know what to do to accomplish this. Can you explain a bit more please?

I think none of the test failures are related, but can you please also double check?

The code is inside corefx and as far as I was able to see nothing was related to this change.

[carlossanlop]

@JRahnama you need to include changes that are very similar to the ones @ericstj shared above: ad72601

The idea is to bump the version of the package that needs to be generated for your assembly, to ensure it gets properly shipped.

[JRahnama]

@carlossanlop I added required OOB for System.Data.SqlClient.

[danmoseley]

Can someone signoff on the packaging change?

[carlossanlop]

@JRahnama the changes are incomplete. You also need to:

• Bump the "StableVersions" number for Microsoft.Windows.Compatibility in packageIndex.json, line 430. Do not bump the "BaselineVersion" under it.
• Bump the "PackageVersion" and "ServiceModelVersion" numbers in Microsoft.Windows.Compatibility.pkgproj.
• Bump "PackageVersion" and "AssemblyVersion" in src/System.Data.SqlClient/Directory.Build.props.
• Add the two project includes in src/package.builds.

Basically, make sure to match the same changes that were made in the old PR.

[JRahnama]

ok, thanks for pointing out the missing parts. I have updated them now.

[danmoseley]

This was probably mentioned already, but do we expect to push this out aligned with core .NET bits (presumably Nov 8th at this point) or just push out on its own? I assume the latter?

[danmoseley]

Test failures are (of course) unrelated. Some bit rot in unrelated tests, not worth fixing at this point.

[David-Engel]

This was probably mentioned already, but do we expect to push this out aligned with core .NET bits (presumably Nov 8th at this point) or just push out on its own? I assume the latter?

We have no preference on this item. Whatever is more convenient for the servicing process.

[carlossanlop]

@JRahnama @David-Engel there is still one more question that needs an answer. I unresolved it.

[David-Engel]

there is still one more question that needs an answer. I unresolved it.

@carlossanlop
We don't know the answer to the question in pkg/Microsoft.Private.PackageBaseline/packageIndex.json. We'll defer to @ericstj and @ViktorHofer .

[carlossanlop]

Thanks @ericstj. Merging.

[danmoseley]

Do we need to do anything particular to ship this now, or does it "join the flow"? (I feel a moral obligation to ensure we don't trip over ourselves somehow since I am partially responsible for the delays here earlier in the year!)

[danmoseley]

@carlossanlop curious about the q above?

[joperezr]

These changes were made in the wrong ItemGroup, they should have instead been made in the one bellow which is the one used for servicing.

corefx/src/packages.builds

Lines 27 to 35 in c6f58cb

	<ItemGroup Condition="'$(BuildAllPackages)' == 'false' AND '$(SkipManagedPackageBuild)' != 'true'" >
	<Project Include="$(MSBuildThisFileDirectory)..\pkg\Microsoft.Private.PackageBaseline\Microsoft.Private.PackageBaseline.builds">
	<AdditionalProperties>$(AdditionalProperties)</AdditionalProperties>
	</Project>
	<Project Include="$(MSBuildThisFileDirectory)..\pkg\Microsoft.Private.CoreFx.NETCoreApp\Microsoft.Private.CoreFx.NETCoreApp.builds">
	<AdditionalProperties>$(AdditionalProperties)</AdditionalProperties>
	</Project>
	<!-- add specific builds / pkgproj's here to include in servicing builds -->
	</ItemGroup>

The result is that these two packages are not going to be built as part of the official build, so in order for the packages to get serviced that will need to be fixed.

[JRahnama]

@joperezr if I make a new PR right now and move those newly added projects to the below ItemGroup do we need to wait for the .Net Tactics to approve the changes and then move forward?

[carlossanlop]

No, it's infra only, we can take it without requesting additional Tactics approval.

[carlossanlop]

FYI 4.8.4 has been published, which includes this fix: https://www.nuget.org/packages/System.Data.SqlClient/4.8.4