Managed implementation of NTLM for Android and tvOS #66879
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
the
community-contribution
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
|
Tagging subscribers to this area: @dotnet/ncl, @vcsjones Issue DetailsContributes to #62264 This is based on @wfurt's managed NTLM code and extended to cover couple more scenarios (SPNs, channel bindings, message integrity code). It integrates it into HTTP authentication for SocketsHttpHandler for Android and tvOS platforms. Notably, it does not integrate into SMTP client and NegotiateStream implementations but it would be rather trivial to add after the basic code is merged. TODO:
|
This was referenced Mar 21, 2022
bartonjs
reviewed
Mar 21, 2022
| } | ||
| } | ||
|
|
||
| public void Transform(ReadOnlySpan<byte> input, Span<byte> output) |
There was a problem hiding this comment.
It looks to me like Transform is only ever called once on any given instance. Rather than an instantiable class, this should just be
internal static class RC4
{
internal static void Transform(ReadOnlySpan<byte> key, ReadOnlySpan<byte> input, Span<byte> output)
{
Span<byte> state = stackalloc byte[256];
...
}
}There was a problem hiding this comment.
That's the case for the MIC calculation but it would be reused for Encrypt/Decrypt if we implement it later (for SMTP authentication and NegotiateStream).
bartonjs
reviewed
Mar 21, 2022
src/libraries/Common/src/System/Net/NTAuthentication.Managed.cs
Outdated
Show resolved
Hide resolved
bartonjs
reviewed
Mar 21, 2022
| // as NTLM has only one challenege message. | ||
| if (!NtlmOid.Equals(mech)) | ||
| { | ||
| throw new Win32Exception(NTE_FAIL, $"'{mech}' mechanism is not supported"); |
There was a problem hiding this comment.
Exception messages should be in resx (and thus use format placeholders instead of interpolation)
There was a problem hiding this comment.
Yep, I have this one in the TODO in the initial comment.
filipnavara
commented
Mar 22, 2022
src/libraries/Common/src/System/Net/NTAuthentication.Managed.cs
Outdated
Show resolved
Hide resolved
filipnavara
force-pushed
the
managed_ntlm
branch
from
54dc4d1 to
efedd2a
Compare
wfurt
reviewed
Mar 25, 2022
src/libraries/Common/src/System/Net/NTAuthentication.Managed.cs
Outdated
Show resolved
Hide resolved
wfurt
reviewed
Mar 25, 2022
wfurt
reviewed
Mar 25, 2022
src/libraries/Common/src/System/Net/NTAuthentication.Managed.cs
Outdated
Show resolved
Hide resolved
wfurt
reviewed
Mar 25, 2022
There was a problem hiding this comment.
generally LGTM.
Thanks @filipnavara.
This looks like great start.
|
enterprise tests failures are tracked by #66970 |
Some of the failures are/were from the temporary test I added. I'll remove it on next pass. |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
On tvOS devices, it's crashing pretty early. Quite likely not related to this change directly, but we do want to enable System.Net.Security as a result. |
|
Should we disable tvOS for now and focus on Android? It seems like it is failing as well but from the logs I cannot see what is wrong. |
…ommit. Read NegTokenResp explicitly. Add mechListMIC reading and verification.
filipnavara
force-pushed
the
managed_ntlm
branch
from
8188f65 to
50a8fc6
Compare
|
The System.Net.Security.Tests crashes are completely unrelated to these changes. In fact, the only relevant test in that batch is the MD4 one for the managed implementation. I did NOT enable the implementation of |
|
Tests may be #68206 |
directhex
pushed a commit
to directhex/runtime
that referenced
this pull request
Apr 21, 2022
* Move MD4 implementation into Common/src/System/Net/Security * Add minimal RC4 implementation * WIP: Integrate managed Ntlm implementation into SocketsHttpHandler Co-authored-by: Tomas Weinfurt <[email protected]> * WIP: Makeshift tests for NTLM/Negotiate authentication * Fix compilation, clean up some of the hashing * Avoid using a temporary buffer * Add computation of signing keys, sealing keys and mechListMIC * Various cleanups * Send SPN in target information * Add some validation, mark spots with missing validation * Clean up some of the memory offset manipulation * Move NTLM version into static variable * Add support for channel bindings, clean up * Fix hash calculation in makeNtlm2Hash accidentally broken with last commit. Read NegTokenResp explicitly. Add mechListMIC reading and verification. * Verify last authentication token in HTTP Negotiate authentication * Address feedback * Fix tvOS builds by making few methods static * Enable System.Net.Security tests on Android and iOS Co-authored-by: Tomas Weinfurt <[email protected]> Co-authored-by: Steve Pfister <[email protected]>
|
@filipnavara Thank you for adding a native RC4 implementation to .NET. Would you consider making the class public so it can be used? I have some old code that uses RC4 (not related to NTLM for Android and tvOS) and it would be nice to be able to call into .NET directly. |
|
@EatonZ Every new API would have to go through API review process. RC4 is unlikely to pass through that process because the algorithm is insecure and deprecated. Feel free to copy the implementation under the current MIT license though, it is really small. |
|
@filipnavara Thanks for the fast response. That is what I was going to do anyway, but thought I'd ask. |
ghost
locked as resolved and limited conversation to collaborators
bartonjs
added
needs-further-triage
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contributes to #62264
This is based on @wfurt's managed NTLM code and extended to cover couple more scenarios (SPNs, channel bindings, message integrity code). It integrates it into HTTP authentication for SocketsHttpHandler for Android and tvOS platforms. Notably, it does not integrate into SMTP client and NegotiateStream implementations but it would be rather trivial to add after the basic code is merged.
TODO: