-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Android NTLM: Empty Credentials now throws PlatformNotSupportedException #91131
Comments
Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones Issue DetailsDescriptionIf an empty network credential is used a recent change is now causing the exception Reproduction StepsOn .NET Android, run the folllowing code: string url = "https://url.to/my-iwa-secured-server";
var handler = new SocketsHttpHandler();
var cre = handler.Credentials;
handler.Credentials = CredentialCache.DefaultCredentials; // this is empty string username+password
//OR: handler.Credentials = new NetworkCredential();
HttpClient client = new HttpClient(handler);
var result = await client.SendAsync(new HttpRequestMessage(HttpMethod.Get, url)); // Throws Expected behaviorReturns 401 like .NET 7 would. Actual behavior
Regression?Yes Known WorkaroundsCheck whether the credential is empty before setting, but has effect on already released libraries. Configuration.NET 8 Preview 7 (Android) Other information/CC @wfurt @filipnavara as discussed
|
You cannot check that. The value is a placeholder that's always empty. The question is what is the expected behavior when you specify .NET 6 on Linux/macOS could throw
On API level, the .NET 8 change is that We can change HttpClient to handle |
My suggestion would be to special-case empty credentials and treat them as if they weren't set. They are in way the same thing. |
By that definition What I am proposing is to swallow the PNSE in |
If you're going out of your way to specify what credentials are used, and those credentials aren't supported, it should throw. The platform in question doesn't support default credentials. Having it translate to an application-level error is just weird. handler.Credentials = CredentialCache.DefaultCredentials; // this is empty string username+password Additionally, since it's NTLM, I actually think it should always throw PNSE for |
On the contrary I'm going out of my way not to write platform specific code. If the platform doesn't support default credentials, why does CredentialCache.DefaultCredentials not return null then? |
Firstly, it was never defined that way. It cannot be changed to return |
If I have invalid credentials a 401 seems like the right response, which is what .net7 did |
I see where you are coming from, and I tend to agree to a point. The problem with That said, I believe the current behavior of |
@wfurt I am curious what you think. Being in line with .NET 7 behavior makes sense to me. |
I agree when it comes to the observable behavior on I'm not sure I agree when it comes to the observable behavior on
This will always fail on any non-Windows platform. Would you expect this to throw |
Not sure I get the sql client argument. If I talk to a webserver that I can't authenticate with, I expect a 401 response code. Having to deal with certain platforms now throwing in certain cases for specific credentials makes no sense to me and makes it really hard to write libraries that run on multiple platforms. |
It's an argument about non-HTTP scenario. In case of
With <= .NET 6 you could actually get I would argue that anyone using TL;DR: I am arguing for |
The alternative to PNSE in context of |
In Windows we'll return SEC_E_NO_CREDENTIALS in these sorts of situations. That is our explicit error to signal CREDUI or bail. GSS is also going to return a similar error. UnknownCredentials is probably the equivalent? But also now you're mixing error behaviors between status codes and exceptions.
The follow up question I have is, one way or another this must be dealt with in the error case otherwise the request just won't work, so what exactly is the OP plan to prompt for creds?
There are genuinely two failure modes: client can't use what it has (because it has nothing or the cred is messed up) and the server accepting can't use what it was given for unknown reasons. The former can provide a TON of information about the failure, and the latter can tell you next to nothing. Filip's point early on was that you'd be trading a ton of useful diagnostic information for something that is nearly useless to debug. That is a silly tradeoff.
…________________________________
From: Filip Navara ***@***.***>
Sent: Saturday, August 26, 2023 8:40:42 AM
To: dotnet/runtime ***@***.***>
Cc: Comment ***@***.***>
Subject: Re: [dotnet/runtime] Android NTLM: Empty Credentials now throws PlatformNotSupportedException (Issue #91131)
The alternative to PNSE in context of NegotiateAuthentication is to map to the closest NegotiateAuthenticationStatus, which in this case would likely be UnknownCredentials.
—
Reply to this email directly, view it on GitHub<#91131 (comment)> or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAJHTYNPDDZ23RO7QA347CDXXIKHXBFKMF2HI4TJMJ2XIZLTS6BKK5TBNR2WLJDUOJ2WLJDOMFWWLO3UNBZGKYLEL5YGC4TUNFRWS4DBNZ2F6YLDORUXM2LUPGBKK5TBNR2WLJDUOJ2WLJDOMFWWLLTXMF2GG2C7MFRXI2LWNF2HTAVFOZQWY5LFUVUXG43VMWSG4YLNMWVXI2DSMVQWIX3UPFYGLAVFOZQWY5LFVIYTMNZVGM4DKNZXHCSG4YLNMWUWQYLTL5WGCYTFNSBKK5TBNR2WLKRRGY3TSMRTGU4TINVENZQW2ZNJNBQXGX3MMFRGK3ECUV3GC3DVMWVDCOJSGA4TMNBYGE32I3TBNVS2S2DBONPWYYLCMVWIFJLWMFWHKZNKGI2TMMRUGUYDGNJZURXGC3LFVFUGC427NRQWEZLMVRZXKYTKMVRXIX3UPFYGLLCJONZXKZKDN5WW2ZLOOSTHI33QNFRXHFUCUR2HS4DFVJZGK4DPONUXI33SPGSXMYLMOVS2SMRRGA3TCNRQGA2YFJDUPFYGLJLJONZXKZNFOZQWY5LFVIYTQNRXGUZDOOJXGSBKI5DZOBS2K3DBMJSWZJLWMFWHKZNKGE3DONJTHA2TONZYQKSHI6LQMWSWYYLCMVWKK5TBNR2WLKRRGY3TSMRTGU4TINUCUR2HS4DFUVWGCYTFNSSXMYLMOVS2UMJZGIYDSNRUHAYTPAVEOR4XAZNFNRQWEZLMUV3GC3DVMWVDENJWGI2DKMBTGU42O5DSNFTWOZLSUZRXEZLBORSQ>.
You are receiving this email because you commented on the thread.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Correct.
The question is whether it makes sense to "emulate Windows" here, or throw an exception outright. In case of the low-level
|
I look at it from a developer point of view. Ie, to me it seems weird that I have to deal with things different: handler.Credential = null; // returns 401
handler.Credential = new NetworkCredential("invalid user", "invalid password"); // returns 401
handler.Credential = new NetworkCredential("", ""); // Throws platform not supported on some platforms because user accepted sign-in dialog before entering anything
handler.Credential = CredentialCache.DefaultCredentials; // Throws platform not supported In all of these cases, I need to do the exact same thing: I need to prompt the user for a valid set of credentials. Just like I do for any other 401 response. But the way I do it is currently in .NET 8 different on different platforms. |
Reading the thread I would probably be inclined to support the idea that the low-level API returns specific error e.g. I'm not big fan of throwing/catching/ignoring. I can live with 401 for cases when the password happens to be empty. The explicit use of From that prospective I'm probably in favor of #91160 |
After writing both versions of the code I am inclined to agree that returning status codes is less disrupting for the current API consumers. That said, we should probably have a documentation on how to enable the detailed tracing. Right now the steps are just scattered around in several issues and random snippets at gist.github.com, which is far from ideal. |
I think we should take this one as well for 8.0 @karelz as it can be viewed as 8.0 regression. |
Reopening to track the backport. |
Just wanted to follow-up on this bit. I tried on a domain-joined MacOS with net7.0-maccatalyst, and it works just fine there. The default credential correctly authenticates with the server using the provided repo. |
I talked with @filipnavara and we agreed to create a targeted fix for 8.0 - just try-catch around all managed NTLM invocations. cc @rzikm - I will need your help with the private bits here |
Sure. Bring it! |
@dotMorten There it is, it's from a local Debug build since we don't publish Android artifacts in the pipelines. Should work with both x64 and arm architectures as managed dlls are not arch specific (only OS-specific). I am not familiar with the Android publishing process, but for regular desktop bits, the process is
An alternative is to modify your installed .net sdk bits (overwrite dlls in |
@dotMorten will you have time to test it out on your side please? Thanks! |
@rzikm is Lastly I don't see this DLL (or any of the other DLLs) in the generated Android package. |
Uh... I am not that familiar with how Android packages get build. But my intuition tells me to try replace the one which has "android" and .net 8 preview version in path (maybe all of them) As for size, my guess is that published DLLs are distributed "ready to run", i.e. with pre-jitted machine code, which is not being done for dev builds. But I may be wrong. @simonrozsival is what we are attempting the right way of testing private bits on Android? If not, can you please help? |
@rzikm @dotMorten In order to validate, you should be able to replace the assemblies in the android runtime packs that are installed via I don't think you'll see the actual assemblies in your apk because the android sdk team has a crafty resource bundling scheme. |
@rzikm unlike desktop, by default mobile configurations produce self contained apps. They will not use the shared framework. |
Got it! I guess I didn't clean up enough the first time around. I can confirm that the fix works! |
Description
If an empty network credential is used a recent change is now causing the exception
System.PlatformNotSupportedException: 'NTLM authentication is not possible with default credentials on this platform.'
to be thrown rather than treating the empty credential as null / not set.This is a regression from .NET 7 behavior.
Reproduction Steps
On .NET Android, run the folllowing code:
Expected behavior
Returns 401 like .NET 7 would.
Actual behavior
Regression?
Yes
Known Workarounds
Check whether the credential is empty before setting, but has effect on already released libraries or cross-platform code.
Configuration
.NET 8 Preview 7 (Android)
Other information
/CC @wfurt @filipnavara as discussed
The text was updated successfully, but these errors were encountered: