This repository was archived by the owner on Jan 23, 2023. It is now read-only.
Implementing NegoState.Unix.cs #5773
There are no files selected for viewing
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System; | ||
| using System.Diagnostics; | ||
| using System.Runtime.InteropServices; | ||
| using System.Text; | ||
| using Microsoft.Win32.SafeHandles; | ||
|
|
||
| namespace System.Net.Security | ||
| { | ||
| internal sealed class SafeFreeNegoCredentials : SafeFreeCredentials | ||
| { | ||
| private SafeGssCredHandle _credential; | ||
|
|
||
| public SafeGssCredHandle GssCredential | ||
| { | ||
| get { return _credential; } | ||
| } | ||
|
|
||
| public SafeFreeNegoCredentials(string username, string password, string domain) : base(IntPtr.Zero, true) | ||
| { | ||
| bool ignore = false; | ||
| _credential = SafeGssCredHandle.Create(username, password, domain); | ||
| _credential.DangerousAddRef(ref ignore); | ||
| } | ||
|
|
||
| public override bool IsInvalid | ||
| { | ||
| get { return (null == _credential); } | ||
| } | ||
|
|
||
| protected override bool ReleaseHandle() | ||
| { | ||
| _credential.DangerousRelease(); | ||
| _credential = null; | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| internal sealed class SafeDeleteNegoContext : SafeDeleteContext | ||
|
Member
There was a problem hiding this comment. Same question as above... why is this type needed? It seems to just be a container for two other safe handles. (And if it is necessary for some reason, does it need to be managing the lifetime of the handles it contains, as in the other one?)
Contributor
There was a problem hiding this comment. There is a parent/child relationship between SafeDeleteContext and SafeFreeCredentials. You can see the same pattern in Windows code of SslStream/NegotiateStream and Unix version of SslStream |
||
| { | ||
| private SafeGssNameHandle _targetName; | ||
| private SafeGssContextHandle _context; | ||
|
|
||
| public SafeGssNameHandle TargetName | ||
| { | ||
| get { return _targetName; } | ||
| } | ||
|
|
||
| public SafeGssContextHandle GssContext | ||
| { | ||
| get { return _context; } | ||
| } | ||
|
|
||
| public SafeDeleteNegoContext(SafeFreeNegoCredentials credential, string targetName) | ||
| : base(credential) | ||
| { | ||
| try | ||
| { | ||
| _targetName = SafeGssNameHandle.CreatePrincipal(targetName); | ||
| } | ||
| catch | ||
| { | ||
| Debug.Assert((null != credential), "Null credential in SafeDeleteNegoContext"); | ||
| Dispose(); | ||
|
Contributor
There was a problem hiding this comment. @shrutigarg see other comment at SafeDeleteSslContext |
||
| throw; | ||
| } | ||
| } | ||
|
|
||
| public void SetGssContext(SafeGssContextHandle context) | ||
| { | ||
| Debug.Assert(!context.IsInvalid, "Invalid context passed to SafeDeleteNegoContext"); | ||
| _context = context; | ||
| } | ||
|
|
||
| protected override void Dispose(bool disposing) | ||
| { | ||
| if (disposing) | ||
| { | ||
| if (null != _context) | ||
| { | ||
| _context.Dispose(); | ||
| _context = null; | ||
| } | ||
|
|
||
| if (_targetName != null) | ||
| { | ||
| _targetName.Dispose(); | ||
| _targetName = null; | ||
| } | ||
| } | ||
| base.Dispose(disposing); | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,12 +56,19 @@ protected override bool ReleaseHandle() | |
| // Implementation of handles dependable on FreeCredentialsHandle | ||
| // | ||
| #if DEBUG | ||
| internal abstract class SafeFreeCredentials : DebugSafeHandle | ||
| { | ||
| #else | ||
| internal abstract class SafeFreeCredentials : SafeHandle | ||
| { | ||
| #endif | ||
| protected SafeFreeCredentials(IntPtr handle, bool ownsHandle) : base(handle, ownsHandle) | ||
| { | ||
| } | ||
| } | ||
|
|
||
| internal sealed class SafeFreeSslCredentials : SafeFreeCredentials | ||
| { | ||
| private SafeX509Handle _certHandle; | ||
| private SafeEvpPKeyHandle _certKeyHandle; | ||
| private SslProtocols _protocols = SslProtocols.None; | ||
|
|
@@ -87,7 +94,7 @@ internal EncryptionPolicy Policy | |
| get { return _policy; } | ||
| } | ||
|
|
||
| public SafeFreeSslCredentials(X509Certificate certificate, SslProtocols protocols, EncryptionPolicy policy) | ||
| : base(IntPtr.Zero, true) | ||
| { | ||
| Debug.Assert( | ||
|
|
@@ -207,14 +214,50 @@ protected override bool ReleaseHandle() | |
| } | ||
|
|
||
| #if DEBUG | ||
| internal abstract class SafeDeleteContext : DebugSafeHandle | ||
| { | ||
| #else | ||
| internal abstract class SafeDeleteContext : SafeHandle | ||
| { | ||
| #endif | ||
| private SafeFreeCredentials _credential; | ||
|
|
||
| protected SafeDeleteContext(SafeFreeCredentials credential) | ||
| : base(IntPtr.Zero, true) | ||
| { | ||
| Debug.Assert((null != credential), "Invalid credential passed to SafeDeleteContext"); | ||
|
|
||
| // When a credential handle is first associated with the context we keep credential | ||
| // ref count bumped up to ensure ordered finalization. The credential properties | ||
| // are used in the SSL/NEGO data structures and should survive the lifetime of | ||
| // the SSL/NEGO context | ||
| bool ignore = false; | ||
| _credential = credential; | ||
| _credential.DangerousAddRef(ref ignore); | ||
| } | ||
|
|
||
| public override bool IsInvalid | ||
| { | ||
| get { return (null == _credential); } | ||
| } | ||
|
|
||
| protected override bool ReleaseHandle() | ||
| { | ||
| Debug.Assert((null != _credential), "Null credential in SafeDeleteContext"); | ||
| _credential.DangerousRelease(); | ||
| _credential = null; | ||
| return true; | ||
| } | ||
|
|
||
| public override string ToString() | ||
| { | ||
| return handle.ToString(); | ||
| } | ||
| } | ||
|
|
||
| internal sealed class SafeDeleteSslContext : SafeDeleteContext | ||
| { | ||
| private SafeSslHandle _sslContext; | ||
|
|
||
| public SafeSslHandle SslContext | ||
| { | ||
|
|
@@ -224,18 +267,10 @@ public SafeSslHandle SslContext | |
| } | ||
| } | ||
|
|
||
| public SafeDeleteSslContext(SafeFreeSslCredentials credential, bool isServer, bool remoteCertRequired) | ||
| : base(credential) | ||
| { | ||
| Debug.Assert((null != credential) && !credential.IsInvalid, "Invalid credential used in SafeDeleteSslContext"); | ||
|
|
||
| try | ||
| { | ||
|
|
@@ -249,11 +284,8 @@ public SafeDeleteContext(SafeFreeCredentials credential, bool isServer, bool rem | |
| } | ||
| catch(Exception ex) | ||
| { | ||
| Debug.Write("Exception Caught. - " + ex); | ||
| Dispose(); | ||
|
Contributor
There was a problem hiding this comment. @shrutigarg we need to (implement) and call base.Dispose which should also set _credential to null to prevent double-free
Member
There was a problem hiding this comment. 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.
Contributor
Author
There was a problem hiding this comment. ok .. will add check .. like in other dispose. |
||
| throw; | ||
| } | ||
| } | ||
|
|
@@ -266,26 +298,23 @@ public override bool IsInvalid | |
| } | ||
| } | ||
|
|
||
| protected override void Dispose(bool disposing) | ||
| { | ||
| if (disposing) | ||
| { | ||
| if (null != _sslContext) | ||
| { | ||
| _sslContext.Dispose(); | ||
| _sslContext = null; | ||
| } | ||
| } | ||
|
|
||
| base.Dispose(disposing); | ||
| } | ||
|
|
||
| public override string ToString() | ||
| { | ||
| return handle.ToString(); | ||
| } | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System; | ||
|
|
||
| namespace System.Net | ||
| { | ||
| internal static class ContextFlagsAdapterPal | ||
| { | ||
| private struct ContextFlagMapping | ||
| { | ||
| public readonly Interop.NetSecurityNative.GssFlags GssFlags; | ||
| public readonly ContextFlagsPal ContextFlag; | ||
|
|
||
| public ContextFlagMapping(Interop.NetSecurityNative.GssFlags gssFlag, ContextFlagsPal contextFlag) | ||
| { | ||
| GssFlags = gssFlag; | ||
| ContextFlag = contextFlag; | ||
| } | ||
| } | ||
|
|
||
| private static readonly ContextFlagMapping[] s_contextFlagMapping = new[] | ||
| { | ||
| // GSS_C_INTEG_FLAG is set if either AcceptIntegrity (used by server) or InitIntegrity (used by client) is set | ||
| new ContextFlagMapping(Interop.NetSecurityNative.GssFlags.GSS_C_INTEG_FLAG, ContextFlagsPal.AcceptIntegrity | ContextFlagsPal.InitIntegrity), | ||
| new ContextFlagMapping(Interop.NetSecurityNative.GssFlags.GSS_C_CONF_FLAG, ContextFlagsPal.Confidentiality), | ||
| new ContextFlagMapping(Interop.NetSecurityNative.GssFlags.GSS_C_IDENTIFY_FLAG, ContextFlagsPal.InitIdentify), | ||
| new ContextFlagMapping(Interop.NetSecurityNative.GssFlags.GSS_C_MUTUAL_FLAG, ContextFlagsPal.MutualAuth), | ||
| new ContextFlagMapping(Interop.NetSecurityNative.GssFlags.GSS_C_REPLAY_FLAG, ContextFlagsPal.ReplayDetect), | ||
| new ContextFlagMapping(Interop.NetSecurityNative.GssFlags.GSS_C_SEQUENCE_FLAG, ContextFlagsPal.SequenceDetect) | ||
| }; | ||
|
|
||
|
|
||
| internal static ContextFlagsPal GetContextFlagsPalFromInterop(Interop.NetSecurityNative.GssFlags gssFlags) | ||
| { | ||
| ContextFlagsPal flags = ContextFlagsPal.Zero; | ||
| foreach (ContextFlagMapping mapping in s_contextFlagMapping) | ||
| { | ||
| if ((gssFlags & mapping.GssFlags) != 0) | ||
| { | ||
| flags |= mapping.ContextFlag; | ||
| } | ||
| } | ||
|
|
||
| return flags; | ||
| } | ||
|
|
||
| internal static Interop.NetSecurityNative.GssFlags GetInteropFromContextFlagsPal(ContextFlagsPal flags) | ||
| { | ||
| Interop.NetSecurityNative.GssFlags gssFlags = (Interop.NetSecurityNative.GssFlags)0; | ||
| foreach (ContextFlagMapping mapping in s_contextFlagMapping) | ||
| { | ||
| if ((flags & mapping.ContextFlag) != 0) | ||
|
Member
There was a problem hiding this comment. This is going to map in GSS_C_INTEG_FLAG for either AcceptIntegrity or InitIntegrity (not requiring them both to be set). If you need them both to be set, this should be == mapping.ContextFlag instead of != 0. If "either" is what you want, it might be worthy of a comment. (And if "both" is what you want, that might also warrant a comment :))
Contributor
There was a problem hiding this comment. "either" is what is needed since Accept is used by server and Init by client. @shrutigarg please add a comment to that effect |
||
| { | ||
| gssFlags |= mapping.GssFlags; | ||
| } | ||
| } | ||
|
|
||
| return gssFlags; | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
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.
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.
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?
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.
@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.
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.
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.
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.
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.