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] Do not throw PNSE exception from NegotiateAuthentication constructor #91753

Merged
merged 2 commits into from
Sep 14, 2023

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented Sep 7, 2023

Fixes #91131

This is a stripped down version of 9.0 (main) PR #91160 with more targeted fix and same unit test.
Unlike #91160 it doesn't try to fix which status code is reported and always reports Unsupported. This is in line with the .NET 7 behavior.

Customer Impact

Regression against 7.0.
Due to refactoring in the space, on Android and tvOS we started throwing PlatformNotSupportedException for CredentialCache.DefaultCredentials instead of returning StatusCode = HttpStatusCode.Unauthorized (401) as we did in 7.0.

For context:

  • It applies only to Android and tvOS, because those are the only platforms using managed NTLM implementation (introduced in .NET 7.0).
  • In 6.0 and earlier we already threw PlatformNotSupportedException for this case, but it was inconsistent across platforms - it sometimes threw Win32Exception or even internal GssApiException.
  • Refactoring in 8.0 removed several layers of abstractions in NTLM authentication and simplified the space, but as a side-effect removed general try-catch which allowed HttpClient to handle these exceptions and return 401 HTTP status code. How it should behave, depends if one looks at it from the low-level API perspective (NegotiateAuthentication), or from higher-level API perspective (HttpClient) - see discussion in Android NTLM: Empty Credentials now throws PlatformNotSupportedException #91131.

Testing

Unit test is added to simulate the customer scenario. It fails without the fix and succeeds with it.
E2E validation by customer on private build of 8.0 branch - see #91131 (comment)

Risk

Low

Affects only managed NTLM implementation, which is used only on Android and tvOS. Changes only error handling, not main code path.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 7, 2023
@ghost
Copy link

ghost commented Sep 7, 2023

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

Issue Details

Fixes #91131. This is a stripped down version of PR #91160 with more targeted fix and same unit test. Unlike #91160 it doesn't try to fix which status code is reported and always reports Unsupported. This is in line with the .NET 7 behavior.

[TBD: Fill in the template; this is currently a draft to get a CI run]

Author: filipnavara
Assignees: -
Labels:

area-System.Net.Security

Milestone: -

@filipnavara filipnavara changed the title Do not throw PNSE exception from NegotiateAuthentication constructor, report Unsupported status instead [release/8.0] Do not throw PNSE exception from NegotiateAuthentication constructor Sep 7, 2023
@karelz karelz added this to the 8.0.0 milestone Sep 8, 2023
@karelz karelz requested a review from rzikm September 8, 2023 08:45
@filipnavara filipnavara marked this pull request as ready for review September 8, 2023 11:07
@karelz
Copy link
Member

karelz commented Sep 8, 2023

@rzikm
Copy link
Member

rzikm commented Sep 8, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@carlossanlop
Copy link
Member

@karelz I see you approved this, but then also self-assigned it. Do you want to do something additional with it, or can we add the servicing-approved label and merge it?

@karelz
Copy link
Member

karelz commented Sep 12, 2023

@carlossanlop we are still waiting on E2E customer validation - see #91131 (comment)

@karelz karelz added the Servicing-consider Issue for next servicing release review label Sep 13, 2023
@karelz
Copy link
Member

karelz commented Sep 13, 2023

@artl93 another issue ready for your approval - regression against 7.0 - see context in the template.

@artl93
Copy link
Member

artl93 commented Sep 13, 2023

M2 Approved.

@artl93 artl93 added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 13, 2023
Copy link
Member

@artl93 artl93 left a comment

Choose a reason for hiding this comment

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

M2 approved.

@rzikm
Copy link
Member

rzikm commented Sep 14, 2023

Fix confirmed to work, see #91131 (comment)

@carlossanlop carlossanlop merged commit 851a936 into dotnet:release/8.0 Sep 14, 2023
106 checks passed
@radical radical mentioned this pull request Sep 26, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Security community-contribution Indicates that the PR has been added by a community member Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants