-
Notifications
You must be signed in to change notification settings - Fork 528
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
[Mono.Android] NTLM support in AndroidMessageHandler #6999
[Mono.Android] NTLM support in AndroidMessageHandler #6999
Conversation
313205a
to
a4a1868
Compare
....Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64SimpleDotNet.apkdesc
Outdated
Show resolved
Hide resolved
5156931
to
2b259af
Compare
This feature increases app sizes noticeably so it shouldn't be included if the customers don't need it. Unfortunately, I ran into problems while I was trying to figure out trimming for this PR and I need to consult it with you before I spend more time on it. First, here's how customers will use NT authentication in their apps: var cache = new CredentialCache ();
cache.Add (uri, "Negotiate", new NetworkCredential(username, password, domain));
cache.Add (uri, "NTLM", new NetworkCredential(username, password, domain));
var handler = new AndroidMessageHandler { Credentials = cache };
var client = new HttpClient (handler);
var response = await client.GetAsync (uri);
// -> 200 OK I have two ideas how we could go about linking out the feature when it's not used but neither of them is great IMO.
Is there some other options that I haven't considered yet? I think that the second option is better but I don't know if the poor DX is acceptable. As far as I understand it NTLM is quite a niche feature that most apps don't need but it could be a big deal for some customers. |
I'd lean lightly towards I have a couple of PRs on dotnet/runtime that improve the testing infrastructure but once that's processed I'll proceed with submitting the |
@simonrozsival given that this feature wouldn't be used in a lot of apps, can you use a linker "feature switch"? The name could be We can document the flag in a couple places, like this file is synced to the ms-docs website: |
xamarin-macios uses native |
@filipnavara the |
It will be used in |
@filipnavara I see. Will it make sense to have a feature switch for negotiate authentication in |
It's the exact same dependency chain that gets pulled in to the final app, so I would say it makes sense to enable the same level of trimming. |
src/Mono.Android/Xamarin.Android.Net/NegotiateAuthenticationHelper.cs
Outdated
Show resolved
Hide resolved
…o android-message-handler-ntlm
tests/Mono.Android-Tests/Runtime-Microsoft.Android.Sdk/Mono.Android.NET-Tests.csproj
Outdated
Show resolved
Hide resolved
.../Mono.Android-Tests/Xamarin.Android.Net/AndroidMessageHandlerNegotiateAuthenticationTests.cs
Outdated
Show resolved
Hide resolved
} | ||
|
||
[Test] | ||
public async Task RequestWithCredentialsSucceeds () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a "complete" NTLM test suite somewhere? Something that checks for "invalid" hostnames, "valid but dodgy-looking" hostnames, escape characters, ../../../
path escape fragments, etc.? Something to help verify that there are no latent security-related or -adjacent issues in this NTLM authentication?
"Obviously" running tests against a "fake NTLM server" is not a complete "real-world" unit test…
Draft (and woefully short) commit message: [Mono.Android] Optional NTLM support in AndroidMessageHandler (#6999)
Context: https://github.com/dotnet/runtime/issues/62264
Context? https://github.com/wfurt/Ntlm
Update `Xamarin.Android.Net.AndroidMessageHandler` to *optionally*
support NTLM authentication.
NTLM authentication can be enabled by setting the
`$(AndroidUseNegotiateAuthentication)` MSBuild property to True.
If this property is False or isn't set, then NTLM support is linked
away during the package build. |
…and while trying a quick search for NTLM & security concerns, ran across:
Sounds like Kerberos-based auth should be preferred in some environments? Then there's Microsoft documentation: https://docs.microsoft.com/en-us/windows-server/security/kerberos/ntlm-overview#BKMK_APP
Then there's this beauty: https://docs.microsoft.com/en-us/archive/blogs/authentication/ntlms-time-has-passed
🤔 Also, is this PR adding NTLM or NTLMv2? (That feels kinda important?) |
It is NTLMv2. NTLM is not great and it should be considered as compat feature for legacy apps @jonpryor As far as testing, runtime (and default handlers) use container-based approach via special enterprise-test pipeline. I'm not sure if that is practical for this repo. @filipnavara is also working on extending test coverage using Kerberos.NET. That should hopefully land before 7.0 ships. |
The Specifically for Android the dotnet/runtime implementation of NegotiateAuthentication supports only NTLM and Negotiate (wrapping just the NTLM). The NTLM implementation is quite strict NTLMv2 with integrity checks that are always enabled. Kerberos can be added in future on the dotnet/runtime side but it would either have to be opt-in (due to size) or lot of code would have to be reshuffled to make it smaller (eg. it requires DNS SRV lookups that have no public API yet, large part of the crypto stack, etc.). Testing all this stuff is hard... there were latent bugs for many years in code that had no test coverage or insufficient coverage on some platforms. That said, I think we should focus on the actual authentication exchange being tested in the dotnet/runtime side and on the Android side it's sufficient to test the HTTP flow and Authorization headers. |
@@ -19,6 +19,7 @@ | |||
<_MonoAndroidTestPackage>Mono.Android.NET_Tests</_MonoAndroidTestPackage> | |||
<PlotDataLabelSuffix>-$(TestsFlavor)NET6</PlotDataLabelSuffix> | |||
<WarningsAsErrors>IL2037</WarningsAsErrors> | |||
<AndroidUseNegotiateAuthentication>true</AndroidUseNegotiateAuthentication> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simonrozsival : thank you for this change.
@simonrozsival, @filipnavara : would appreciate review of the draft commit message. Is there anything that should be mentioned wrt usage? What's supported? What isn't supported? (NTLM v1 should probably be called out as explicitly not supported, presumably?) When & why you would want to use this option, or what stack trace/error message people would encounter which is a sign that they should enable this feature? Links to other sources of documentation? |
@jonpryor The only reason why you would want to use this option is IMO that you need to use it. I think we should mention that we recommend using NTLM only for authentication against legacy servers. We don't throw any exceptions if it's not enabled, the requests will all just result in 401 responses. Some customers could be confused when they specify the credentials, yet the user isn't authenticated. The commit message you suggested explicitly mentions that the NTLM support is linked away when the feature isn't enabled though. I can't find any reasonable documentation we could link to (@filipnavara, @wfurt suggestions for existing docs are welcome). I would add a short example usage instead. I suggest changing the commit message to something like:
|
@simonrozsival : thank you, and sorry for the delay. Hopefully final "squash-and-merge" commit message: [Mono.Android] Optional NTLM support in AndroidMessageHandler (#6999)
Context: https://github.com/dotnet/runtime/issues/62264
Context? https://github.com/wfurt/Ntlm
Update `Xamarin.Android.Net.AndroidMessageHandler` to *optionally*
support NTLMv2 authentication in .NET 7+. This authentication method
is recommended only for legacy services that do not provide any more
secure options.
If an endpoint requires NTLMv2 authentication and NTMLv2 is not
enabled, then the endpoint will return HTTP-401 errors.
NTLMv2 authentication can be enabled by setting the
`$(AndroidUseNegotiateAuthentication)` MSBuild property to True.
If this property is False or isn't set, then NTLMv2 support is linked
away during the package build.
Example `.csproj` changes to enable NTLMv2 support:
<PropertyGroup>
<AndroidUseNegotiateAuthentication>true</AndroidUseNegotiateAuthentication>
</PropertyGroup>
Example C# `HttpClient` usage using NTLMv2 authentication:
var cache = new CredentialCache ();
cache.Add (serverUri, "Negotiate", new NetworkCredential(username, password, domain));
var handler = new AndroidMessageHandler {
Credentials = cache,
};
var client = new HttpClient (handler);
var response = await client.GetAsync (requestUri); // 200 OK; 401 is NTLMv2 isn't enabled |
* main: LEGO: Merge pull request 7221 LEGO: Merge pull request 7219 [Xamarin.Android.Build.Tasks] use `ToJniName(type, cache)` (dotnet#7211) [docs] Synchronize with MicrosoftDocs/xamarin-docs (dotnet#7208) [Mono.Android] Remove System.Linq usage (dotnet#7210) Bump to Android NDK r25 (dotnet#6764) Bump to mono/opentk@daa9b2d5 (dotnet#7192) [Mono.Android] Optional NTLMv2 support in AndroidMessageHandler (dotnet#6999) Bump to dotnet/installer@53587f9 7.0.100-rc.1.22374.1 (dotnet#7198)
* mm-codegen: LEGO: Merge pull request 7221 LEGO: Merge pull request 7219 [Xamarin.Android.Build.Tasks] use `ToJniName(type, cache)` (dotnet#7211) [docs] Synchronize with MicrosoftDocs/xamarin-docs (dotnet#7208) [Mono.Android] Remove System.Linq usage (dotnet#7210) Bump to Android NDK r25 (dotnet#6764) Bump to mono/opentk@daa9b2d5 (dotnet#7192) [Mono.Android] Optional NTLMv2 support in AndroidMessageHandler (dotnet#6999) Bump to dotnet/installer@53587f9 7.0.100-rc.1.22374.1 (dotnet#7198)
This PR contributes to dotnet/runtime#62264
I attempted to implement the authentication as AuthModules similarly to the Basic and Digest modules first, but it wasn't very suitable for the challenge/response auth schemes. I tried replicating SocketHttpHandler's AuthenticationHelper.NtAuth.cs in the new
NegotiateAuthenticationHelper
static class. NTLM/Negotiate support is available only for .NET 7+ because it uses its new managed implementation of NTLM/Negotiate and it uses a new public classNegotiateAuthentication
.This feature has dependencies on several dlls that aren't used by most apps so the code is behind a feature flag
$(AndroidUseNegotiateAuthentication)
/Xamarin.Android.Net.UseNegotiateAuthentication
. If this flag isn't set totrue
, the helper class and all its dependencies are linked away.Any suggestions for the implementation itself and for the feature flag documentation are welcome. I'm aware that the rudimentary mention of the feature flag in the docs isn't sufficient and I'll try to improve it.