Refactoring the Windows specific code in NegotiateStream

[shrutigarg]

summary

Renaming NegoState.Windows.cs to InternalNegoState.cs as it had much common code applicable for Unix. (commit1)
Separating out the common code and windows specific code in InternalNegoState and NegoState.Windows.cs respectiviely.
Adding NegotiateInfoClass.cs to be used by Unix impl.

Tested locally both AuthenticateAsServer/Client on Windows.

/cc: @SidharthNabar @stephentoub @davidsh @CIPop @vijaykota

[vijaykota]

cc: @joshfree to help track #2483

[stephentoub]

cc: @bartonjs

[bartonjs]

Why is this in the Interop/.../Native directory?

[vijaykota]

For Windows, it is under Interop\Windows\SspiCli. Any recommendations for Unix? (it is needed for both SPNEGO and NTLM protocols)

[bartonjs]

I guess my question is "does this really vary across OS, or should it be something shared across them?"

[vijaykota]

Sorry.. misunderstood the original question. We can change this to a partial class if that is preferable. The constants can be shared and the constructor containing Windows specific code can remain under SspiCli

[joshfree]

cc @ericeil

[bartonjs]

I see three copies of this currently on my screen. Perhaps factor it out to a method?

[bartonjs]

Actually, all three are doing the same runtime work to send ERROR_TRUST_FAILURE. If that's at all common, maybe it should be a private static readonly field.

[bartonjs]

This could also benefit from being turned into a loop.

[shrutigarg]

@dotnet-bot test this please

[CIPop]

@shrutigarg I'd like to postpone merging this PR given multiple test failures for System.Net.Security: #4467, #5283 #5284.

This is to avoid regressions as the one seen in #5642 especially since a lot of common code is getting refactored.

[vijaykota]

I'd like to postpone merging this PR

@joshfree, please note that actual xplat implementation would then be blocked by this PR

[CIPop]

please note that actual xplat implementation would then be blocked by this PR

@shrutigarg @vijaykota What I meant is that only merging this to master should be blocked until I stabilize and enable the Windows tests.
You can still follow the development pattern where a private branch is used for various PRs and reviews. When we have the tests working we can merge from that. Since this is a big item you may also want to request a branch within dotnet/corefx.

[vijaykota]

Understood, @CIPop We are merely informing @joshfree since he is tracking the final merge to master for consumption by partners.

given multiple test failures for System.Net.Security: #4467, #5283 #5284.

I take it you are actively working on code changes for these issues. Do you have an ETA for your changes getting merged so we can calculate our ETA?

[vijaykota]

@bartonjs, please take a look at the changes made by @shrutigarg and comment if anything not covered by #5617 needs changes.

[vijaykota]

@stephentoub , PTAL There is one comment pending in IsError which @shrutigarg will fix

[stephentoub]

@CIPop, we would like to move the implementation of the unix side of NegotiateStream into a separate branch, and I've created https://github.com/dotnet/corefx/tree/dev/negotiatestream for that purpose. Do you have any plans to make significant changes to the Windows implementation in the near future? If so, it would seem pragmatic to merge into master whatever changes are necessary to refactor that code so that there won't be major conflicts between the work done in master and the work done in the separate branch. Or, are the changes you're talking about only impacting the tests, such that this refactoring can be done in the separate branch without causing significant merge issues?

[CIPop]

Do you have any plans to make significant changes to the Windows implementation in the near future?

@stephentoub As far as I can tell right now, we have everything we need from Desktop and no major changes will be made to the production code.
The only thing that I'll try to get to this week is fixing the test code which, we already know, has some design bugs.

[stephentoub]

Thanks, @CIPop. Sounds then like there should be few if any concerns around merge conflicts (if anywhere they'd be in tests), and we can stabilize everything in the separate branch before merging it into master. @vijaykota, sound good? We can take very frequent merges from master to the dev branch to ensure it's up-to-date, and @CIPop, if we do have any unexpected changes to NegotiateStream that need to get made in the interim, it'd be great if you could help ensure that such merges are done correctly.

[vijaykota]

@vijaykota, sound good? We can take very frequent merges from master to the dev branch to ensure it's up-to-date

Thanks, @stephentoub and @CIPop This should be good enough for us to branch off the work (including this PR) to the dev branch.

[vijaykota]

Added a label to prevent accidental merge while we are moving this to dev/negotiatestream

[shrutigarg]

moved this to #5772 in dev/NegotiateStream branch .