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

[release/8.0] Fix implementation of NegotiateAuthentication.Wrap for Kerberos on Windows #91311

Merged
merged 2 commits into from
Sep 11, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 30, 2023

Backport of #91152 to release/8.0

Fixes PowerShell/PowerShell#20168

/cc @rzikm @filipnavara

Customer Impact

Regression against 7.0 - Establishing NTLM (Kerberos) authenticated connection can fail with "The encryption operation failed" (see PowerShell/PowerShell#20168).

The story behind the regression:

The regression is caused by changes in .NET Kerberos/NTLM authentication which started in 7.0 and which are finishing now in 8.0. The goal was to introduce public Kerberos/NTLM authentication APIs (NegotiateAuthenticaton) for higher-level frameworks and applications (e.g. ASP.NET needed it to avoid using private Reflection - see #29270).
In 6.0 and earlier, the Kerberos/NTLM authentication (NTAuthentication) was internal only code and was compiled into multiple Networking assemblies (e.g. System.Net.Security, System.Net.Mail, etc.) to avoid using Reflection. Therefore, it also had negative impact on .NET binaries size due to compiled code duplication (while the source code was shared).

In 7.0, we introduced the new public API NegotiateAuthenticaton and we migrated a few internal usages of NTAuthentication to the new public API (e.g. Mail), but not all of them.
One of the public APIs (NegotiateAuthentication.Wrap) had a bug on Windows only that was not exposed until 8.0, when we migrated also NegotiateStream to the public APIs in PR #86948. NegotiateStream support of Kerberos requires more flexibility in encryption padding, which NTLM didn't need, and the new API didn't fully provide it.

This PR brings parity of old internal functionality in NegotiateStreamPal.Encrypt to the new public API NegotiateAuthentication.Wrap.

Testing

The change was tested on affected scenario reported in PowerShell/PowerShell#20168.
Additional validation was performed using a custom/manual NegotiateStream client-server setup between Windows Server 2019 server machine and Windows 11 client machine.

Note: NTLM has good unit test coverage. Kerberos has also good unit test coverage on Linux via Kerberos.NET. However, Kerberos on Windows requires complicated multi-machine setup, therefore it is not automated. We will evaluate feasibility of adding Kerberos test endpoint for CI during 9.0 to address the test gap.

Risk

Low to Medium.

The change affects encryption and signing of data transferred through NegotiateStream. There's no other internal consumer of the NegotiateAuthentication.Wrap API (with exception of single message in SMTP GSSAPI authentication which is covered by tests). Given that this is very advanced API introduced in 7.0, we do not expect there to be any external usages of the API either. And if they are, they would not be ok with the buggy behavior in 7.0.
Only two encryption protocols are supported - NTLM and Kerberos, and NTLM is covered by tests. The Kerberos use case was reported to be broken, and this restores the affected code to mimic the .NET 7 behavior.

@ghost
Copy link

ghost commented Aug 30, 2023

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

Issue Details

Backport of #91152 to release/8.0

/cc @rzikm @filipnavara

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Net.Security

Milestone: -

@rzikm
Copy link
Member

rzikm commented Aug 30, 2023

@filipnavara would you be able to provide text for the Testing and Risk sections as well?

cc: @karelz

@filipnavara

This comment was marked as duplicate.

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @filipnavara

@carlossanlop carlossanlop added the Servicing-consider Issue for next servicing release review label Aug 30, 2023
@carlossanlop carlossanlop added this to the 8.0.0 milestone Aug 30, 2023
@carlossanlop
Copy link
Member

@karelz do you approve this for RC2?

@karelz
Copy link
Member

karelz commented Sep 7, 2023

I approve, it is E2E regression - @artl93 it is ready for you

@artl93
Copy link
Contributor

artl93 commented Sep 7, 2023

M2 approved.

@artl93 artl93 added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 7, 2023
@carlossanlop carlossanlop merged commit 217be6c into release/8.0 Sep 11, 2023
107 of 116 checks passed
@carlossanlop carlossanlop deleted the backport/pr-91152-to-release/8.0 branch September 11, 2023 23:09
@radical radical mentioned this pull request Sep 26, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 12, 2023
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.

6 participants