Implementing NegoState.Unix.cs by shrutigarg · Pull Request #5773 · dotnet/corefx

shrutigarg · 2016-01-29T09:14:39Z

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 · 2016-01-29T09:15:28Z

following up #5726

vijaykota · 2016-01-29T10:15:25Z

Adding reference to #2483

shrutigarg · 2016-02-01T05:56:55Z

ping @stephentoub @bartonjs @vijaykota

stephentoub · 2016-02-01T20:34:00Z

Ping

@shrutigarg, I'm confused by all of the PRs. I commented on #5772, but its three commits appear to be completely a part of this one. Then there are this PR (#5773) and the fallback logic PR (#5770) which have those three commits plus one more that appears to be the same, and then they each differ by one commit. What are we actually supposed to be reviewing?

kkurni · 2016-02-10T22:53:08Z

+            Exception exception = null;
+            if (message != s_emptyMessage)
+            {
+                message = GetOutgoingBlob(message, ref exception);


@shrutigarg Does it mean that in Unix we will use the same NTAuthentication.cs as well to GetOutgoingBlob ?

This NTAuthentication.GetOutgoinBlob essentially what System.Data need to get SSPI message for Integrated feature. Is there any plan to make this available to public ?

Thanks

Yes.. Unix will use same NTAuthentication.cs There is no plan to make GetOutgoingBlob public

vijaykota · 2016-02-17T06:39:30Z

@shrutigarg , till we resolve #5890 add a Debug.Write and GlobalLog.Print to the catch block in EstablishSecurityContext

shrutigarg · 2016-02-17T08:52:59Z

@vijaykota added log and marked with a todo statement to keep track on reverting.

shrutigarg · 2016-02-17T09:41:34Z

@vijaykota @stephentoub @bartonjs updated this PR after syncing with dev/negotiatestream branch and incorporating all the comments.
Tested locally.

shrutigarg · 2016-02-17T13:25:40Z

@dotnet-bot test this please

stephentoub · 2016-02-17T16:16:23Z

+
+namespace System.Net.Security
+{
+    internal sealed class SafeFreeNegoCredentials : SafeFreeCredentials


What value is this type providing? It seems like it exists purely to wrap a SafeGssCredHandle... why not just have the caller use a SafeGssCredHandle?

@bartonjs just told me that this exists purely to satisfy the PAL contract... is that true? If so, we can leave it as-is for now, but we should revisit that contract, which sounds flawed.

I'm happy to be wrong about that, but I feel that when I've had similar confusion on other things that was the answer.

Yes.. the platform-agnostic code requires a SafeFreeCredentials object since on Windows it is same for SslStream and NegotiateStream. On Unix this is different. For fallback to NTLM logic, you will actually see more logic inside SafeFreeNegoCredentials.

shrutigarg · 2016-02-19T13:17:21Z

rebase done.

stephentoub · 2016-02-19T22:40:38Z

A few more comments, then LGTM. (Most importantly the DangerousRelease thing needs to be addressed.)

shrutigarg · 2016-02-22T10:10:32Z

@dotnet-bot test this please

vijaykota · 2016-02-22T11:18:53Z

-                }
                Debug.Write("Exception Caught. - " + ex);
+                Debug.Assert((null != credential), "Null credential in SafeDeleteContext");
+                Dispose();


@shrutigarg we need to (implement) and call base.Dispose which should also set _credential to null to prevent double-free

Calling Dispose here is right, but in Dispose implementation below you're calling _sslContext.Dispose, and if the ctor throws in the call to AllocateSslContext, _sslContext will be null, and the call to Dispose will dereference null. You need to check in Dispose that _sslContext is not null before disposing it. Then you should set it back to null.

ok .. will add check .. like in other dispose.

stephentoub · 2016-02-22T14:01:36Z

This can be reviewed again in context as part of a later merge to master. For now, LGTM.

shrutigarg · 2016-02-22T14:07:43Z

These checks seem to have stuck ..seeing it since 5-6 hours.. ,, is some known issue? Should I wait for them to finish?

stephentoub · 2016-02-22T14:09:02Z

@dotnet-bot test this please

stephentoub · 2016-02-22T14:55:40Z

d:\j\workspace\ubuntu_debug_---bbc18e49\src\Common\src\Interop\Unix\libssl\SecuritySafeHandles.cs(308,21): error CS0191: A readonly field cannot be assigned to (except in a constructor or a variable initializer) [d:\j\workspace\ubuntu_debug_---bbc18e49\src\System.Net.Security\src\System.Net.Security.csproj]

stephentoub · 2016-02-22T14:56:58Z

With the build break, that implies the latest version of the code wasn't run through tests. Since there aren't automated ones yet in CI, can you please make sure to do so locally before this is merged?

shrutigarg · 2016-02-22T17:17:57Z

@stephentoub seems unrelated failure now.. I have fixed the issue and tested.

stephentoub · 2016-02-22T17:20:19Z

@mmitche, we're hitting these Azure VM disk full conditions fairly often. We can continue to delete them when they occur, but is there nothing more proactive we can do to prevent them in the first place and avoid the random leg failures?

mmitche · 2016-02-22T18:25:00Z

@stephentoub That actually wasn't a disk space issue, that was it not making the disk writeable. I am trying to find out why. Let me know if you see another instance.

Implementing NegoState.Unix.cs

dnfclas added the cla-already-signed label Jan 29, 2016

dotnet-bot added the 2 - In Progress label Jan 29, 2016

vijaykota mentioned this pull request Jan 29, 2016

Adding fallback logic to NTLM #5770

Closed

shrutigarg mentioned this pull request Jan 29, 2016

Implementing NegoState.Unix.cs #5726

Closed

shrutigarg mentioned this pull request Feb 2, 2016

Refactoring the Windows specific code in NegotiateStream #5772

Merged

shrutigarg referenced this pull request in shrutigarg/corefx Feb 3, 2016

Add fallback logic to NTLM

9466812

shrutigarg referenced this pull request in shrutigarg/corefx Feb 8, 2016

Updating NegoState Pal Layer and other refactoring

f43a1fb

iamjasonp mentioned this pull request Feb 10, 2016

Support NegotiateStream after it lights up in dotnet/corefx dotnet/wcf#592

Closed

2 tasks

kkurni added the area-System.Net label Feb 10, 2016

kkurni reviewed Feb 10, 2016
View reviewed changes

kapilash mentioned this pull request Feb 16, 2016

NegotiateStream Tests on Unix #6139

Merged

shrutigarg force-pushed the devNegoUnix branch from c4fd392 to 26b5f21 Compare February 17, 2016 09:39

stephentoub reviewed Feb 17, 2016
View reviewed changes

shrutigarg force-pushed the devNegoUnix branch from e285e18 to 1dd9c4c Compare February 19, 2016 13:14

shrutigarg force-pushed the devNegoUnix branch from 05e7762 to aec307e Compare February 22, 2016 09:14

vijaykota reviewed Feb 22, 2016
View reviewed changes

shrutigarg force-pushed the devNegoUnix branch from 05f0631 to 78ea444 Compare February 22, 2016 14:04

stephentoub closed this Feb 22, 2016

stephentoub reopened this Feb 22, 2016

stephentoub removed the 2 - In Progress label Feb 22, 2016

dnfclas added the cla-already-signed label Feb 22, 2016

stephentoub added the 2 - In Progress label Feb 22, 2016

shrutigarg force-pushed the devNegoUnix branch from 35edd02 to 78ea444 Compare February 22, 2016 17:33

Implementing NegoState.Unix.cs and SafeHandles

e8cf9aa

shrutigarg force-pushed the devNegoUnix branch from 58b062c to e8cf9aa Compare February 22, 2016 18:02

stephentoub added a commit that referenced this pull request Feb 22, 2016

Merge pull request #5773 from shrutigarg/devNegoUnix

68670f3

Implementing NegoState.Unix.cs

stephentoub merged commit 68670f3 into dotnet:dev/negotiatestream Feb 22, 2016

stephentoub removed the 2 - In Progress label Feb 22, 2016

karelz added this to the 1.0.0-rtm milestone Jan 21, 2017

vijaykota mentioned this pull request Jan 31, 2020

Return more error information for SslStream and NegotiateStream from *Pal.Unix.cs dotnet/runtime#16292

Closed

Conversation

shrutigarg commented Jan 29, 2016

Uh oh!

shrutigarg commented Jan 29, 2016

Uh oh!

vijaykota commented Jan 29, 2016

Uh oh!

shrutigarg commented Feb 1, 2016

Uh oh!

stephentoub commented Feb 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vijaykota commented Feb 17, 2016

Uh oh!

shrutigarg commented Feb 17, 2016

Uh oh!

shrutigarg commented Feb 17, 2016

Uh oh!

shrutigarg commented Feb 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shrutigarg commented Feb 19, 2016

Uh oh!

stephentoub commented Feb 19, 2016

Uh oh!

shrutigarg commented Feb 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Feb 22, 2016

Uh oh!

shrutigarg commented Feb 22, 2016

Uh oh!

stephentoub commented Feb 22, 2016

Uh oh!

stephentoub commented Feb 22, 2016

Uh oh!

stephentoub commented Feb 22, 2016

Uh oh!

shrutigarg commented Feb 22, 2016

Uh oh!

stephentoub commented Feb 22, 2016

Uh oh!

mmitche commented Feb 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants