Skip to content

Commit

Permalink
PipeOptions.CurrentUserOnly: remove dir on Unix and add negative test…
Browse files Browse the repository at this point in the history
…s for Windows (dotnet/corefx#27573)

* Initial unpolished change

Cleanup impersonation implementation

Remove extra dir added for CurrentUserOnly on Unix

* PR feedback plus NETFX fix


Commit migrated from dotnet/corefx@df5bf81
  • Loading branch information
Paulo Janotti authored Mar 1, 2018
1 parent 2c0eae5 commit b330391
Show file tree
Hide file tree
Showing 8 changed files with 185 additions and 32 deletions.
3 changes: 0 additions & 3 deletions src/libraries/System.IO.Pipes/src/System.IO.Pipes.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -182,9 +182,6 @@
<Compile Include="$(CommonPath)\Interop\Unix\Interop.IOErrors.cs">
<Link>Common\Interop\Unix\Interop.IOErrors.cs</Link>
</Compile>
<Compile Include="$(CommonPath)\Interop\Unix\System.Native\Interop.ChMod.cs">
<Link>Common\Interop\Unix\Interop.ChMod.cs</Link>
</Compile>
<Compile Include="$(CommonPath)\Interop\Unix\System.Native\Interop.Close.cs">
<Link>Common\Interop\Unix\Interop.Close.cs</Link>
</Compile>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public NamedPipeClientStream(String serverName, String pipeName, PipeDirection d
IsCurrentUserOnly = true;
}

_normalizedPipePath = GetPipePath(serverName, pipeName, IsCurrentUserOnly);
_normalizedPipePath = GetPipePath(serverName, pipeName);
_direction = direction;
_inheritability = inheritability;
_impersonationLevel = impersonationLevel;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ private void Create(string pipeName, PipeDirection direction, int maxNumberOfSer
// We don't have a good way to enforce maxNumberOfServerInstances across processes; we only factor it in
// for streams created in this process. Between processes, we behave similarly to maxNumberOfServerInstances == 1,
// in that the second process to come along and create a stream will find the pipe already in existence and will fail.
_instance = SharedServer.Get(GetPipePath(".", pipeName, IsCurrentUserOnly), maxNumberOfServerInstances);
_instance = SharedServer.Get(GetPipePath(".", pipeName), maxNumberOfServerInstances);

_direction = direction;
_options = options;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public abstract partial class PipeStream : Stream
/// <summary>Prefix to prepend to all pipe names.</summary>
private static readonly string s_pipePrefix = Path.Combine(Path.GetTempPath(), "CoreFxPipe_");

internal static string GetPipePath(string serverName, string pipeName, bool isCurrentUserOnly)
internal static string GetPipePath(string serverName, string pipeName)
{
if (serverName != "." && serverName != Interop.Sys.GetHostName())
{
Expand Down Expand Up @@ -59,18 +59,6 @@ internal static string GetPipePath(string serverName, string pipeName, bool isCu
// naming scheme used as that breaks the ability for code running on an older
// runtime to connect to code running on the newer runtime. That means we're stuck
// with a tmp file for the lifetime of the server stream.
if (isCurrentUserOnly)
{
string directory = Path.Combine(Path.GetTempPath(), ".NET" + Interop.Sys.GetEUid());
Directory.CreateDirectory(directory);
if (Interop.Sys.ChMod(directory, (int)Interop.Sys.Permissions.S_IRWXU) == -1)
{
throw CreateExceptionForLastError();
}

return Path.Combine(directory, pipeName);
}

return s_pipePrefix + pipeName;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ public abstract partial class PipeStream : Stream
internal const bool CheckOperationsRequiresSetHandle = true;
internal ThreadPoolBoundHandle _threadPoolBinding;

// In Windows we don't use isCurrentUserOnly flag for the path because we set an ACL to the pipe.
internal static string GetPipePath(string serverName, string pipeName, bool isCurrentUserOnly)
internal static string GetPipePath(string serverName, string pipeName)
{
string normalizedPipePath = Path.GetFullPath(@"\\" + serverName + @"\pipe\" + pipeName);
if (String.Equals(normalizedPipePath, @"\\.\pipe\" + AnonymousPipeName, StringComparison.OrdinalIgnoreCase))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
// 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 Microsoft.Win32.SafeHandles;
using System.ComponentModel;
using System.DirectoryServices.AccountManagement;
using System.Runtime.InteropServices;
using System.Security.Cryptography;
using System.Security.Principal;
using System.Threading;
using System.Threading.Tasks;
using Xunit;

namespace System.IO.Pipes.Tests
{
// Class to be used as xUnit fixture to avoid creating the user, an relatively slow operation (couple of seconds), multiple times.
public class TestAccountImpersonator : IDisposable
{
private const string TestAccountName = "CorFxTst0uZa"; // Random suffix to avoid matching any other account by accident, but const to avoid leaking it.
private SafeAccessTokenHandle _testAccountTokenHandle;

public TestAccountImpersonator()
{
string testAccountPassword;
using (RandomNumberGenerator rng = new RNGCryptoServiceProvider())
{
var randomBytes = new byte[33];
rng.GetBytes(randomBytes);

// Add special chars to ensure it satisfies password requirements.
testAccountPassword = Convert.ToBase64String(randomBytes) + "_-As@!%*(1)4#2";
}

DateTime accountExpirationDate = DateTime.UtcNow + TimeSpan.FromMinutes(2);
using (var principalCtx = new PrincipalContext(ContextType.Machine))
{
bool needToCreate = false;
using (var foundUserPrincipal = UserPrincipal.FindByIdentity(principalCtx, TestAccountName))
{
if (foundUserPrincipal == null)
{
needToCreate = true;
}
else
{
// Somehow the account leaked from previous runs, however, password is lost, reset it.
foundUserPrincipal.SetPassword(testAccountPassword);
foundUserPrincipal.AccountExpirationDate = accountExpirationDate;
foundUserPrincipal.Save();
}
}

if (needToCreate)
{
using (var userPrincipal = new UserPrincipal(principalCtx))
{
userPrincipal.SetPassword(testAccountPassword);
userPrincipal.AccountExpirationDate = accountExpirationDate;
userPrincipal.Name = TestAccountName;
userPrincipal.DisplayName = TestAccountName;
userPrincipal.Description = TestAccountName;
userPrincipal.Save();
}
}
}

const int LOGON32_PROVIDER_DEFAULT = 0;
const int LOGON32_LOGON_INTERACTIVE = 2;

if (!LogonUser(TestAccountName, ".", testAccountPassword, LOGON32_LOGON_INTERACTIVE, LOGON32_PROVIDER_DEFAULT, out _testAccountTokenHandle))
{
_testAccountTokenHandle = null;
throw new Exception($"Failed to get SafeAccessTokenHandle for test account {TestAccountName}", new Win32Exception());
}
}

public void Dispose()
{
if (_testAccountTokenHandle == null)
return;

_testAccountTokenHandle.Dispose();
_testAccountTokenHandle = null;

using (var principalCtx = new PrincipalContext(ContextType.Machine))
using (var userPrincipal = UserPrincipal.FindByIdentity(principalCtx, TestAccountName))
{
if (userPrincipal == null)
throw new Exception($"Failed to get user principal to delete test account {TestAccountName}");

try
{
userPrincipal.Delete();
}
catch (InvalidOperationException)
{
// TODO: Investigate, it always throw this exception with "Can't delete object already deleted", but it actually deletes it.
}
}
}

// This method asserts if it impersonates the current identity, i.e.: it ensures that an actual impersonation happens
public void RunImpersonated(Action action)
{
using (WindowsIdentity serverIdentity = WindowsIdentity.GetCurrent())
{
WindowsIdentity.RunImpersonated(_testAccountTokenHandle, () =>
{
using (WindowsIdentity clientIdentity = WindowsIdentity.GetCurrent())
Assert.NotEqual(serverIdentity.Name, clientIdentity.Name);
action();
});
}
}

[DllImport("advapi32.dll", SetLastError = true, CharSet = CharSet.Unicode)]
private static extern bool LogonUser(string userName, string domain, string password, int logonType, int logonProvider, out SafeAccessTokenHandle safeAccessTokenHandle);
}

/// <summary>
/// Tests for the constructors for NamedPipeClientStream
/// </summary>
public class NamedPipeTest_CurrentUserOnly_Windows : NamedPipeTestBase, IClassFixture<TestAccountImpersonator>
{
private TestAccountImpersonator _testAccountImpersonator;

public NamedPipeTest_CurrentUserOnly_Windows(TestAccountImpersonator testAccountImpersonator)
{
_testAccountImpersonator = testAccountImpersonator;
}

[OuterLoop]
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsWindowsAndElevated))]
[InlineData(PipeOptions.None, PipeOptions.CurrentUserOnly)]
[InlineData(PipeOptions.CurrentUserOnly, PipeOptions.None)]
public void Connection_UnderDifferentUser_SingleSide_CurrentUserOnly_Fails(PipeOptions serverPipeOptions, PipeOptions clientPipeOptions)
{
string name = GetUniquePipeName();
using (var cts = new CancellationTokenSource())
using (var server = new NamedPipeServerStream(name, PipeDirection.InOut, 1, PipeTransmissionMode.Byte, serverPipeOptions | PipeOptions.Asynchronous))
{
Task serverTask = server.WaitForConnectionAsync(cts.Token);

_testAccountImpersonator.RunImpersonated(() =>
{
using (var client = new NamedPipeClientStream(".", name, PipeDirection.InOut, clientPipeOptions))
{
Assert.Throws<UnauthorizedAccessException>(() => client.Connect());
}
});

// Server is expected to not have received any request.
cts.Cancel();
var e = Assert.Throws<AggregateException>(() => serverTask.Wait(10_000));
Assert.IsType(typeof(TaskCanceledException), e.InnerException);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,17 +41,22 @@ public static void CreateServer_ConnectClient()
}
}

[Fact]
[PlatformSpecific(TestPlatforms.AnyUnix)] // On Unix domain socket should have different location in this case.
public static void CreateServerNotCurrentUserOnly_ClientCurrentUserOnly_ThrowsTimeout_OnUnix()
[Theory]
[InlineData(PipeOptions.None, PipeOptions.CurrentUserOnly)]
[InlineData(PipeOptions.CurrentUserOnly, PipeOptions.None)]
public static void Connection_UnderSameUser_SingleSide_CurrentUserOnly_Works(PipeOptions serverPipeOptions, PipeOptions clientPipeOptions)
{
string name = GetUniquePipeName();
using (var server = new NamedPipeServerStream(name, PipeDirection.InOut, 1, PipeTransmissionMode.Byte))
using (var server = new NamedPipeServerStream(name, PipeDirection.InOut, 1, PipeTransmissionMode.Byte, serverPipeOptions))
using (var client = new NamedPipeClientStream(".", name, PipeDirection.InOut, clientPipeOptions))
{
using (var client = new NamedPipeClientStream(".", name, PipeDirection.InOut, PipeOptions.CurrentUserOnly))
{
Assert.Throws<TimeoutException>(() => client.Connect(1));
}
Task[] tasks = new[]
{
Task.Run(() => server.WaitForConnection()),
Task.Run(() => client.Connect())
};

Assert.True(Task.WaitAll(tasks, 20_000));
}
}

Expand Down Expand Up @@ -99,4 +104,4 @@ public static void CreateMultipleServers_ConnectMultipleClients_MultipleThreads(
Task.WaitAll(tasks.ToArray());
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,14 @@
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netcoreapp-Windows_NT-Debug|AnyCPU'" />
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netcoreapp-Windows_NT-Release|AnyCPU'" />
<ItemGroup>
<Compile Include="NamedPipeTests\NamedPipeTest.cs" />
<Compile Include="PipeTest.cs" />
<Compile Include="AnonymousPipeTests\AnonymousPipeTest.CreateServer.cs" />
<Compile Include="AnonymousPipeTests\AnonymousPipeTest.CreateClient.cs" />
<Compile Include="AnonymousPipeTests\AnonymousPipeTest.CrossProcess.cs" />
<Compile Include="AnonymousPipeTests\AnonymousPipeTestBase.cs" />
<Compile Include="AnonymousPipeTests\AnonymousPipeTest.Read.cs" />
<Compile Include="AnonymousPipeTests\AnonymousPipeTest.Specific.cs" />
<Compile Include="AnonymousPipeTests\AnonymousPipeTest.Write.cs" />
<Compile Include="NamedPipeTests\NamedPipeTest.cs" />
<Compile Include="NamedPipeTests\NamedPipeTest.CreateServer.cs" />
<Compile Include="NamedPipeTests\NamedPipeTest.CreateClient.cs" />
<Compile Include="NamedPipeTests\NamedPipeTest.CrossProcess.cs" />
Expand All @@ -32,6 +31,7 @@
<Compile Include="NamedPipeTests\NamedPipeTest.Write.cs" />
<Compile Include="NamedPipeTests\NamedPipeTest.Specific.cs" />
<Compile Include="NamedPipeTests\NamedPipeTest.Simple.cs" />
<Compile Include="PipeTest.cs" />
<Compile Include="PipeTestBase.cs" />
<Compile Include="PipeTest.Read.cs" />
<Compile Include="PipeTest.Write.cs" />
Expand All @@ -42,6 +42,9 @@
<Compile Include="PipeTest.Read.netcoreapp.cs" />
<Compile Include="PipeTest.Write.netcoreapp.cs" />
</ItemGroup>
<ItemGroup Condition="'$(TargetGroup)' == 'netcoreapp' and '$(TargetsWindows)' == 'true'">
<Compile Include="NamedPipeTests\NamedPipeTest.CurrentUserOnly.netcoreapp.Windows.cs" />
</ItemGroup>
<ItemGroup Condition="'$(TargetsWindows)' == 'true'">
<Compile Include="Interop.Windows.cs" />
<Compile Include="NamedPipeTests\NamedPipeTest.RunAsClient.Windows.cs" />
Expand All @@ -58,4 +61,4 @@
</ProjectReference>
</ItemGroup>
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
</Project>
</Project>

0 comments on commit b330391

Please sign in to comment.