Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Implementing NegoState.Unix.cs#5726

Closed
shrutigarg wants to merge 5 commits into
dotnet:masterfrom
shrutigarg:NegoUnix1
Closed

Implementing NegoState.Unix.cs#5726
shrutigarg wants to merge 5 commits into
dotnet:masterfrom
shrutigarg:NegoUnix1

Conversation

@shrutigarg
Copy link
Copy Markdown
Contributor

It includes the implementation on NegoState.Unix pal on top of the common refactoring changes( #5581)
Please refer to the Commit 4 of this PR for reviewing.

Includes:
Implementation of NegoState.Unix.cs Methods.
Implementaion of SafeHandles to be used.

Tested locally as Linux client with windows server.

cc: @stephentoub , @bartonjs @vijaykota

@shrutigarg shrutigarg changed the title Nego unix1 Implementing NegoState.Unix.cs Jan 27, 2016
@vijaykota
Copy link
Copy Markdown
Contributor

cc: @joshfree for tracking #2483

Pre-requisite for merging this PR is #5500 and #5581. Please ignore the 1st 3 commits which are from #5581. We are trying to get early feedback on the xplat implementation without gating on merge of the those PRs.

Note that Unix build is expected to fail since the .csproj includes files that are a part of #5500.

@davidsh
Copy link
Copy Markdown
Contributor

davidsh commented Jan 27, 2016

cc: @CIPop

@bartonjs
Copy link
Copy Markdown
Member

There really need to be automated tests for this. At minimum for the golden path. And they need to be enabled before the code gets merged. PlatformNotSupportedException is (to me) better than it being horribly broken... and even if it works in this set of commits there's nothing to say that the next one won't break it, unless there's a test.

@vijaykota
Copy link
Copy Markdown
Contributor

cc: @CIPop

Just to be clear, please review only the Unix portions of the code ie. 4th commit onwards. Anything impacting Windows will be part of #5581

@vijaykota
Copy link
Copy Markdown
Contributor

and is just a container-wrapper for a SafeGssCredHandle

2 reasons for this:

  1. Both SecureChannel.cs and NTAuthentication.cs need the common classes SafeFreeCredential and SafeDeleteContext but have different functionality. So we need this base class definition and it doesn't make sense to make SafeGssCredHandle derive from it
  2. When the NTLM changes are put in, SafeGssCredHandle will hold other info like username, domain etc.

@vijaykota
Copy link
Copy Markdown
Contributor

There really need to be automated tests for this

Agreed. Had a few offline discussions regarding this. We are going to move the development to a private branch. We expect to merge test code there once we finalize the test strategy.

And to ensure master is never broken, the contract is that we can merge from the private branch only after sufficient automated test coverage is there.

@vijaykota
Copy link
Copy Markdown
Contributor

When the NTLM changes are put in, SafeGssCredHandle will hold other info

Sorry.. that should read "SafeFreeNegoCredentials"

@shrutigarg
Copy link
Copy Markdown
Contributor Author

moved this to #5773 in dev/NegotiateStream branch .

@shrutigarg shrutigarg closed this Jan 29, 2016
@dotnet-bot dotnet-bot changed the title Implementing NegoState.Unix.cs Implementing NegoState.Unix.cs Jan 29, 2016
@karelz karelz modified the milestone: 1.0.0-rtm Dec 3, 2016
@karelz karelz added the os-linux Linux OS (any supported distro) label Mar 8, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Net os-linux Linux OS (any supported distro)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants