From 990a97068bd01f3b6a1ae243d9fdf471c141bbab Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Tue, 28 Jan 2025 23:33:00 +1100 Subject: [PATCH 1/4] fix: various fixes for tunnel startup to work Fixes various problems that prevented tunnel startup from succeeding and prevented clients from connecting to the RPC server. Added a new temporary package Vpn.DebugClient to help control the system service component until the UI is ready. Adds utility PS1 scripts for adding the debug binary to a new system service and managing/hotswapping it. --- Coder.Desktop.sln | 20 +++- Vpn.DebugClient/Program.cs | 128 +++++++++++++++++++++ Vpn.DebugClient/Vpn.DebugClient.csproj | 17 +++ Vpn.DebugClient/packages.lock.json | 30 +++++ Vpn.Service/Create-Service.ps1 | 35 ++++++ Vpn.Service/Delete-Service.ps1 | 18 +++ Vpn.Service/Downloader.cs | 2 + Vpn.Service/Manager.cs | 109 ++++++++++++++---- Vpn.Service/ManagerRpcService.cs | 92 ++++++++++----- Vpn.Service/Program.cs | 101 ++++++++++++---- Vpn.Service/Rebuild-Service.ps1 | 3 + Vpn.Service/RegistryConfigurationSource.cs | 2 + Vpn.Service/Restart-Service.ps1 | 13 +++ Vpn.Service/Stop-Service.ps1 | 10 ++ Vpn.Service/TunnelSupervisor.cs | 93 +++++++++++++-- Vpn.Service/Vpn.Service.csproj | 3 + 16 files changed, 593 insertions(+), 83 deletions(-) create mode 100644 Vpn.DebugClient/Program.cs create mode 100644 Vpn.DebugClient/Vpn.DebugClient.csproj create mode 100644 Vpn.DebugClient/packages.lock.json create mode 100644 Vpn.Service/Create-Service.ps1 create mode 100644 Vpn.Service/Delete-Service.ps1 create mode 100644 Vpn.Service/Rebuild-Service.ps1 create mode 100644 Vpn.Service/Restart-Service.ps1 create mode 100644 Vpn.Service/Stop-Service.ps1 diff --git a/Coder.Desktop.sln b/Coder.Desktop.sln index e25c8fa..dccfbf5 100644 --- a/Coder.Desktop.sln +++ b/Coder.Desktop.sln @@ -1,7 +1,7 @@  Microsoft Visual Studio Solution File, Format Version 12.00 # Visual Studio Version 17 -VisualStudioVersion = 17.12.35707.178 d17.12 +VisualStudioVersion = 17.12.35707.178 MinimumVisualStudioVersion = 10.0.40219.1 Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Vpn", "Vpn\Vpn.csproj", "{B342F896-C721-4AA5-A0F6-0BFA8EBAFACB}" EndProject @@ -21,6 +21,8 @@ Project("{C7167F0D-BC9F-4E6E-AFE1-012C56B48DB5}") = "Package", "Package\Package. EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "App", "App\App.csproj", "{800C7E2D-0D86-4554-9903-B879DA6FA2CE}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Vpn.DebugClient", "Vpn.DebugClient\Vpn.DebugClient.csproj", "{1BBFDF88-B25F-4C07-99C2-30DA384DB730}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -185,6 +187,22 @@ Global {800C7E2D-0D86-4554-9903-B879DA6FA2CE}.Release|x64.Build.0 = Release|x64 {800C7E2D-0D86-4554-9903-B879DA6FA2CE}.Release|x86.ActiveCfg = Release|x86 {800C7E2D-0D86-4554-9903-B879DA6FA2CE}.Release|x86.Build.0 = Release|x86 + {1BBFDF88-B25F-4C07-99C2-30DA384DB730}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {1BBFDF88-B25F-4C07-99C2-30DA384DB730}.Debug|Any CPU.Build.0 = Debug|Any CPU + {1BBFDF88-B25F-4C07-99C2-30DA384DB730}.Debug|ARM64.ActiveCfg = Debug|Any CPU + {1BBFDF88-B25F-4C07-99C2-30DA384DB730}.Debug|ARM64.Build.0 = Debug|Any CPU + {1BBFDF88-B25F-4C07-99C2-30DA384DB730}.Debug|x64.ActiveCfg = Debug|Any CPU + {1BBFDF88-B25F-4C07-99C2-30DA384DB730}.Debug|x64.Build.0 = Debug|Any CPU + {1BBFDF88-B25F-4C07-99C2-30DA384DB730}.Debug|x86.ActiveCfg = Debug|Any CPU + {1BBFDF88-B25F-4C07-99C2-30DA384DB730}.Debug|x86.Build.0 = Debug|Any CPU + {1BBFDF88-B25F-4C07-99C2-30DA384DB730}.Release|Any CPU.ActiveCfg = Release|Any CPU + {1BBFDF88-B25F-4C07-99C2-30DA384DB730}.Release|Any CPU.Build.0 = Release|Any CPU + {1BBFDF88-B25F-4C07-99C2-30DA384DB730}.Release|ARM64.ActiveCfg = Release|Any CPU + {1BBFDF88-B25F-4C07-99C2-30DA384DB730}.Release|ARM64.Build.0 = Release|Any CPU + {1BBFDF88-B25F-4C07-99C2-30DA384DB730}.Release|x64.ActiveCfg = Release|Any CPU + {1BBFDF88-B25F-4C07-99C2-30DA384DB730}.Release|x64.Build.0 = Release|Any CPU + {1BBFDF88-B25F-4C07-99C2-30DA384DB730}.Release|x86.ActiveCfg = Release|Any CPU + {1BBFDF88-B25F-4C07-99C2-30DA384DB730}.Release|x86.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE diff --git a/Vpn.DebugClient/Program.cs b/Vpn.DebugClient/Program.cs new file mode 100644 index 0000000..4fc33c0 --- /dev/null +++ b/Vpn.DebugClient/Program.cs @@ -0,0 +1,128 @@ +using System.IO.Pipes; +using Coder.Desktop.Vpn.Proto; + +namespace Coder.Desktop.Vpn.DebugClient; + +public static class Program +{ + private static Speaker? _speaker; + + private static string? _coderUrl; + private static string? _apiToken; + + public static void Main() + { + Console.WriteLine("Type 'exit' to exit the program"); + Console.WriteLine("Type 'connect' to connect to the service"); + Console.WriteLine("Type 'disconnect' to disconnect from the service"); + Console.WriteLine("Type 'configure' to set the parameters"); + Console.WriteLine("Type 'start' to send a start command with the current parameters"); + Console.WriteLine("Type 'stop' to send a stop command"); + while (true) + { + Console.Write("> "); + var input = Console.ReadLine()?.Trim(); + try + { + switch (input) + { + case "exit": + return; + case "connect": + Connect(); + break; + case "disconnect": + Disconnect(); + break; + case "configure": + Configure(); + break; + case "start": + Start(); + break; + case "stop": + Stop(); + break; + } + } + catch (Exception ex) + { + Console.WriteLine($"Error: {ex}"); + } + } + } + + private static void Connect() + { + var client = new NamedPipeClientStream(".", "Coder.Desktop.Vpn", PipeDirection.InOut, PipeOptions.Asynchronous); + client.Connect(); + Console.WriteLine("Connected to named pipe."); + + _speaker = new Speaker(client); + _speaker.Receive += message => { Console.WriteLine($"Received({message.Message.MsgCase}: {message.Message}"); }; + _speaker.Error += exception => + { + Console.WriteLine($"Error: {exception}"); + Disconnect(); + }; + _speaker.StartAsync().Wait(); + Console.WriteLine("Speaker started."); + } + + private static void Disconnect() + { + _speaker?.DisposeAsync().AsTask().Wait(); + _speaker = null; + Console.WriteLine("Disconnected from named pipe"); + } + + private static void Configure() + { + Console.Write("Coder URL: "); + _coderUrl = Console.ReadLine()?.Trim(); + Console.Write("API Token: "); + _apiToken = Console.ReadLine()?.Trim(); + } + + private static void Start() + { + if (_speaker is null) + { + Console.WriteLine("Not connected to Coder.Desktop.Vpn."); + return; + } + + var message = new ClientMessage + { + Start = new StartRequest + { + CoderUrl = _coderUrl, + ApiToken = _apiToken, + }, + }; + Console.WriteLine("Sending start message..."); + var sendTask = _speaker.SendRequestAwaitReply(message).AsTask(); + Console.WriteLine("Start message sent, awaiting reply."); + sendTask.Wait(); + Console.WriteLine($"Received reply: {sendTask.Result.Message}"); + } + + private static void Stop() + { + if (_speaker is null) + { + Console.WriteLine("Not connected to Coder.Desktop.Vpn."); + return; + } + + var message = new ClientMessage + { + Stop = new StopRequest(), + }; + Console.WriteLine("Sending stop message..."); + var sendTask = _speaker.SendRequestAwaitReply(message); + Console.WriteLine("Stop message sent, awaiting reply."); + var reply = sendTask.AsTask().Result; + Console.WriteLine($"Received reply: {reply.Message}"); + } +} diff --git a/Vpn.DebugClient/Vpn.DebugClient.csproj b/Vpn.DebugClient/Vpn.DebugClient.csproj new file mode 100644 index 0000000..b20654b --- /dev/null +++ b/Vpn.DebugClient/Vpn.DebugClient.csproj @@ -0,0 +1,17 @@ + + + + Coder.Desktop.Vpn.DebugClient + Exe + net8.0 + enable + enable + true + + + + + + + + diff --git a/Vpn.DebugClient/packages.lock.json b/Vpn.DebugClient/packages.lock.json new file mode 100644 index 0000000..dfa5c4b --- /dev/null +++ b/Vpn.DebugClient/packages.lock.json @@ -0,0 +1,30 @@ +{ + "version": 1, + "dependencies": { + "net8.0": { + "Google.Protobuf": { + "type": "Transitive", + "resolved": "3.29.1", + "contentHash": "kDFLP72bPu4GwquVN7HtFnfqxhQs4CLbUEyOc/0yV48HB0Pxf7tDK7zIx6ifaQwGp+RSoLY1sz1CXqoAEAu+AQ==" + }, + "System.IO.Pipelines": { + "type": "Transitive", + "resolved": "9.0.0", + "contentHash": "eA3cinogwaNB4jdjQHOP3Z3EuyiDII7MT35jgtnsA4vkn0LUrrSHsU0nzHTzFzmaFYeKV7MYyMxOocFzsBHpTw==" + }, + "vpn": { + "type": "Project", + "dependencies": { + "System.IO.Pipelines": "[9.0.0, )", + "Vpn.Proto": "[1.0.0, )" + } + }, + "vpn.proto": { + "type": "Project", + "dependencies": { + "Google.Protobuf": "[3.29.1, )" + } + } + } + } +} \ No newline at end of file diff --git a/Vpn.Service/Create-Service.ps1 b/Vpn.Service/Create-Service.ps1 new file mode 100644 index 0000000..bd00da5 --- /dev/null +++ b/Vpn.Service/Create-Service.ps1 @@ -0,0 +1,35 @@ +# Elevate to administrator +if (-not ([Security.Principal.WindowsPrincipal]([Security.Principal.WindowsIdentity]::GetCurrent())).IsInRole([Security.Principal.WindowsBuiltInRole]::Administrator)) { + Write-Host "Elevating script to run as administrator..." + Start-Process powershell.exe -ArgumentList "-NoProfile -ExecutionPolicy Bypass -File `"$($MyInvocation.MyCommand.Path)`"" -Verb RunAs + exit +} + +$name = "Coder Desktop (Debug)" +$binaryPath = Join-Path -Path $PSScriptRoot -ChildPath "bin/Debug/net8.0-windows/Vpn.Service.exe" + +try { + Write-Host "Creating service..." + New-Service -Name $name -BinaryPathName "`"$binaryPath`"" -DisplayName $name -StartupType Automatic + + $sddl = & sc.exe sdshow $name + if (-not $sddl) { + throw "Failed to retrieve security descriptor for service '$name'" + } + Write-Host "Current security descriptor: '$sddl'" + $sddl = $sddl.Trim() -replace "D:", "D:(A;;RPWP;;;WD)" # allow everyone to start, stop, pause, and query the service + Write-Host "Setting security descriptor: '$sddl'" + & sc.exe sdset $name $sddl + + Write-Host "Starting service..." + Start-Service -Name $name + + if ((Get-Service -Name $name -ErrorAction Stop).Status -ne "Running") { + throw "Service '$name' is not running" + } + Write-Host "Service '$name' created and started successfully" +} catch { + Write-Host $_ -ForegroundColor Red + Write-Host "Press Return to exit..." + Read-Host +} diff --git a/Vpn.Service/Delete-Service.ps1 b/Vpn.Service/Delete-Service.ps1 new file mode 100644 index 0000000..1a857ba --- /dev/null +++ b/Vpn.Service/Delete-Service.ps1 @@ -0,0 +1,18 @@ +# Elevate to administrator +if (-not ([Security.Principal.WindowsPrincipal]([Security.Principal.WindowsIdentity]::GetCurrent())).IsInRole([Security.Principal.WindowsBuiltInRole]::Administrator)) { + Write-Host "Elevating script to run as administrator..." + Start-Process powershell.exe -ArgumentList "-NoProfile -ExecutionPolicy Bypass -File `"$($MyInvocation.MyCommand.Path)`"" -Verb RunAs + exit +} + +$name = "Coder Desktop (Debug)" + +try { + Stop-Service -Name $name -Force -ErrorAction SilentlyContinue + sc.exe delete $name + Write-Host "Service '$name' deleted" +} catch { + Write-Host $_ -ForegroundColor Red + Write-Host "Press Return to exit..." + Read-Host +} diff --git a/Vpn.Service/Downloader.cs b/Vpn.Service/Downloader.cs index ea4c587..96e6e1c 100644 --- a/Vpn.Service/Downloader.cs +++ b/Vpn.Service/Downloader.cs @@ -42,6 +42,7 @@ public class AuthenticodeDownloadValidator : IDownloadValidator { private readonly string _expectedName; + // ReSharper disable once ConvertToPrimaryConstructor public AuthenticodeDownloadValidator(string expectedName) { _expectedName = expectedName; @@ -79,6 +80,7 @@ public class AssemblyVersionDownloadValidator : IDownloadValidator { private readonly string _expectedAssemblyVersion; + // ReSharper disable once ConvertToPrimaryConstructor public AssemblyVersionDownloadValidator(string expectedAssemblyVersion) { _expectedAssemblyVersion = expectedAssemblyVersion; diff --git a/Vpn.Service/Manager.cs b/Vpn.Service/Manager.cs index 0f11f34..4288007 100644 --- a/Vpn.Service/Manager.cs +++ b/Vpn.Service/Manager.cs @@ -21,12 +21,14 @@ public Task HandleClientRpcMessage(ReplyableRpcMessage _logger; private readonly ITunnelSupervisor _tunnelSupervisor; + private SemVersion? _lastServerVersion; + private StartRequest? _lastStartRequest; // ReSharper disable once ConvertToPrimaryConstructor public Manager(IOptions config, ILogger logger, IDownloader downloader, @@ -52,7 +54,6 @@ public async Task HandleClientRpcMessage(ReplyableRpcMessageTunnel RPC error"); try { _tunnelSupervisor.StopAsync(); + // TODO: this should broadcast an update to all clients } catch (Exception e2) { @@ -171,6 +201,34 @@ private static string SystemArchitecture() }; } + /// + /// Connects to the Coder server to ensure the server version is within the required range and the credentials + /// are valid. + /// + /// Coder server base URL + /// Coder API token + /// Cancellation token + /// The server version + /// The server version is not within the required range + private async ValueTask CheckServerVersionAndCredentials(string baseUrl, string apiToken, + CancellationToken ct = default) + { + var client = new CoderApiClient(baseUrl, apiToken); + + var buildInfo = await client.GetBuildInfo(ct); + _logger.LogInformation("Fetched server version '{ServerVersion}'", buildInfo.Version); + if (buildInfo.Version.StartsWith('v')) buildInfo.Version = buildInfo.Version[1..]; + var serverVersion = SemVersion.Parse(buildInfo.Version); + if (!serverVersion.Satisfies(ServerVersionRange)) + throw new InvalidOperationException( + $"Server version '{serverVersion}' is not within required server version range '{ServerVersionRange}'"); + + var user = await client.GetUser(User.Me, ct); + _logger.LogInformation("Authenticated to server as '{Username}'", user.Username); + + return serverVersion; + } + /// /// Fetches the "/bin/coder-windows-{architecture}.exe" binary from the given base URL and writes it to the /// destination path after validating the signature and checksum. @@ -200,16 +258,17 @@ private async Task DownloadTunnelBinaryAsync(string baseUrl, SemVersion expected _config.TunnelBinaryPath); var req = new HttpRequestMessage(HttpMethod.Get, url); var validators = new CombinationDownloadValidator( - AuthenticodeDownloadValidator.Coder, - new AssemblyVersionDownloadValidator( - $"{expectedVersion.Major}.{expectedVersion.Minor}.{expectedVersion.Patch}.0") + // TODO: re-enable when the binaries are signed and have versions + //AuthenticodeDownloadValidator.Coder, + //new AssemblyVersionDownloadValidator( + //$"{expectedVersion.Major}.{expectedVersion.Minor}.{expectedVersion.Patch}.0") ); var downloadTask = await _downloader.StartDownloadAsync(req, _config.TunnelBinaryPath, validators, ct); // TODO: monitor and report progress when we have a mechanism to do so - // Awaiting this will check the checksum (via the ETag) if provided, - // and will also validate the signature using the validator we supplied. + // Awaiting this will check the checksum (via the ETag) if the file + // exists, and will also validate the signature and version. await downloadTask.Task; } } diff --git a/Vpn.Service/ManagerRpcService.cs b/Vpn.Service/ManagerRpcService.cs index ce2b17e..4d03c2f 100644 --- a/Vpn.Service/ManagerRpcService.cs +++ b/Vpn.Service/ManagerRpcService.cs @@ -1,5 +1,7 @@ using System.Collections.Concurrent; using System.IO.Pipes; +using System.Security.AccessControl; +using System.Security.Principal; using Coder.Desktop.Vpn.Proto; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Logging; @@ -7,17 +9,25 @@ namespace Coder.Desktop.Vpn.Service; +public class ManagerRpcClient(Speaker speaker, Task task) +{ + public Speaker Speaker { get; } = speaker; + public Task Task { get; } = task; +} + /// /// Provides a named pipe server for communication between multiple RpcRole.Client and RpcRole.Manager. /// public class ManagerRpcService : BackgroundService, IAsyncDisposable { - private readonly ConcurrentDictionary _activeClientTasks = new(); + private readonly ConcurrentDictionary _activeClients = new(); private readonly ManagerConfig _config; private readonly CancellationTokenSource _cts = new(); private readonly ILogger _logger; private readonly IManager _manager; + private ulong _lastClientId; + // ReSharper disable once ConvertToPrimaryConstructor public ManagerRpcService(IOptions config, ILogger logger, IManager manager) { _logger = logger; @@ -28,7 +38,7 @@ public ManagerRpcService(IOptions config, ILogger c.Task)); _cts.Dispose(); GC.SuppressFinalize(this); } @@ -36,7 +46,7 @@ public async ValueTask DisposeAsync() public override async Task StopAsync(CancellationToken cancellationToken) { await _cts.CancelAsync(); - while (!_activeClientTasks.IsEmpty) await Task.WhenAny(_activeClientTasks.Values); + while (!_activeClients.IsEmpty) await Task.WhenAny(_activeClients.Values.Select(c => c.Task)); } /// @@ -46,47 +56,59 @@ protected override async Task ExecuteAsync(CancellationToken stoppingToken) { _logger.LogInformation(@"Starting continuous named pipe RPC server at \\.\pipe\{PipeName}", _config.ServiceRpcPipeName); + + // Allow everyone to connect to the named pipe + var pipeSecurity = new PipeSecurity(); + pipeSecurity.AddAccessRule(new PipeAccessRule( + new SecurityIdentifier(WellKnownSidType.WorldSid, null), + PipeAccessRights.FullControl, + AccessControlType.Allow)); + + // Starting a named pipe server is not like a TCP server where you can + // continuously accept new connections. You need to recreate the server + // after accepting a connection in order to accept new connections. using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(stoppingToken, _cts.Token); while (!linkedCts.IsCancellationRequested) { - var pipeServer = new NamedPipeServerStream(_config.ServiceRpcPipeName, PipeDirection.InOut, - NamedPipeServerStream.MaxAllowedServerInstances, PipeTransmissionMode.Byte, PipeOptions.Asynchronous); + var pipeServer = NamedPipeServerStreamAcl.Create(_config.ServiceRpcPipeName, PipeDirection.InOut, + NamedPipeServerStream.MaxAllowedServerInstances, PipeTransmissionMode.Byte, PipeOptions.Asynchronous, 0, + 0, pipeSecurity); try { - try - { - _logger.LogDebug("Waiting for new named pipe client connection"); - await pipeServer.WaitForConnectionAsync(linkedCts.Token); - } - finally - { - await pipeServer.DisposeAsync(); - } + _logger.LogDebug("Waiting for new named pipe client connection"); + await pipeServer.WaitForConnectionAsync(linkedCts.Token); - _logger.LogInformation("Handling named pipe client connection"); - var clientTask = HandleRpcClientAsync(pipeServer, linkedCts.Token); - _activeClientTasks.TryAdd(clientTask.Id, clientTask); - _ = clientTask.ContinueWith(RpcClientContinuation, CancellationToken.None); + var clientId = Interlocked.Add(ref _lastClientId, 1); + _logger.LogInformation("Handling named pipe client connection for client {ClientId}", clientId); + var speaker = new Speaker(pipeServer); + var clientTask = HandleRpcClientAsync(speaker, linkedCts.Token); + _activeClients.TryAdd(clientId, new ManagerRpcClient(speaker, clientTask)); + _ = clientTask.ContinueWith(task => + { + if (task.IsFaulted) + _logger.LogWarning(task.Exception, "Client {ClientId} RPC task faulted", clientId); + _activeClients.TryRemove(clientId, out _); + }, CancellationToken.None); } catch (OperationCanceledException) { + await pipeServer.DisposeAsync(); throw; } catch (Exception e) { _logger.LogWarning(e, "Failed to accept named pipe client"); + await pipeServer.DisposeAsync(); } } } - private async Task HandleRpcClientAsync(NamedPipeServerStream pipeServer, CancellationToken ct) + private async Task HandleRpcClientAsync(Speaker speaker, CancellationToken ct) { var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(ct, _cts.Token); - await using (pipeServer) + await using (speaker) { - await using var speaker = new Speaker(pipeServer); - var tcs = new TaskCompletionSource(); var activeTasks = new ConcurrentDictionary(); speaker.Receive += msg => @@ -101,6 +123,7 @@ private async Task HandleRpcClientAsync(NamedPipeServerStream pipeServer, Cancel }, CancellationToken.None); }; speaker.Error += tcs.SetException; + speaker.Error += exception => { _logger.LogWarning(exception, "Client RPC speaker error"); }; await using (ct.Register(() => tcs.SetCanceled(ct))) { await speaker.StartAsync(ct); @@ -112,17 +135,28 @@ private async Task HandleRpcClientAsync(NamedPipeServerStream pipeServer, Cancel } } - private void RpcClientContinuation(Task task) - { - if (task.IsFaulted) - _logger.LogWarning(task.Exception, "Client RPC task faulted"); - _activeClientTasks.TryRemove(task.Id, out _); - } - private async Task HandleRpcMessageAsync(ReplyableRpcMessage message, CancellationToken ct) { _logger.LogInformation("Received RPC message: {Message}", message.Message); await _manager.HandleClientRpcMessage(message, ct); } + + public async Task BroadcastAsync(ServiceMessage message, CancellationToken ct) + { + foreach (var (clientId, client) in _activeClients) + try + { + var cts = CancellationTokenSource.CreateLinkedTokenSource(ct); + cts.CancelAfter(5 * 1000); + await client.Speaker.SendMessage(message, cts.Token); + } + catch (Exception e) + { + _logger.LogWarning(e, "Failed to send message to client {ClientId}", clientId); + // TODO: this should probably kill the client, but due to the + // async nature of the client handling, calling Dispose + // will not remove the client from the active clients list + } + } } diff --git a/Vpn.Service/Program.cs b/Vpn.Service/Program.cs index 78fbff2..e46e674 100644 --- a/Vpn.Service/Program.cs +++ b/Vpn.Service/Program.cs @@ -1,30 +1,91 @@ -using Coder.Desktop.Vpn.Service; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using Microsoft.Win32; +using Serilog; -var builder = Host.CreateApplicationBuilder(args); +namespace Coder.Desktop.Vpn.Service; -// Configuration sources -builder.Configuration.Sources.Clear(); -(builder.Configuration as IConfigurationBuilder).Add( - new RegistryConfigurationSource(Registry.LocalMachine, @"SOFTWARE\Coder\Coder VPN")); -builder.Configuration.AddEnvironmentVariables("CODER_MANAGER_"); -builder.Configuration.AddCommandLine(args); +public static class Program +{ +#if DEBUG + private const string serviceName = "Coder Desktop (Debug)"; +#else + const string serviceName = "Coder Desktop"; +#endif -// Options types (these get registered as IOptions singletons) -builder.Services.AddOptions() - .Bind(builder.Configuration.GetSection("Manager")) - .ValidateDataAnnotations(); + private static readonly ILogger MainLogger = Log.ForContext("SourceContext", "Coder.Desktop.Vpn.Service.Program"); -// Singletons -builder.Services.AddSingleton(); -builder.Services.AddSingleton(); -builder.Services.AddSingleton(); + public static async Task Main(string[] args) + { + // Configure Serilog. + Log.Logger = new LoggerConfiguration() + .Enrich.FromLogContext() + // TODO: configurable level + .MinimumLevel.Debug() + .WriteTo.Console( + outputTemplate: "[{Timestamp:HH:mm:ss} {Level:u3}] {SourceContext} - {Message:lj}{NewLine}{Exception}") + // TODO: better location + .WriteTo.File(@"C:\CoderDesktop.log", + outputTemplate: + "{Timestamp:yyyy-MM-dd HH:mm:ss.fff zzz} [{Level:u3}] {SourceContext} - {Message:lj}{NewLine}{Exception}") + .CreateLogger(); -// Services -builder.Services.AddHostedService(); -builder.Services.AddHostedService(); + try + { + await BuildAndRun(args); + return 0; + } + catch (Exception ex) + { + MainLogger.Fatal(ex, "Host terminated unexpectedly"); + return 1; + } + finally + { + await Log.CloseAndFlushAsync(); + } + } -builder.Build().Run(); + private static async Task BuildAndRun(string[] args) + { + var builder = Host.CreateApplicationBuilder(args); + + // Configuration sources + builder.Configuration.Sources.Clear(); + (builder.Configuration as IConfigurationBuilder).Add( + new RegistryConfigurationSource(Registry.LocalMachine, @"SOFTWARE\Coder\Coder Desktop")); + builder.Configuration.AddEnvironmentVariables("CODER_MANAGER_"); + builder.Configuration.AddCommandLine(args); + + // Options types (these get registered as IOptions singletons) + builder.Services.AddOptions() + .Bind(builder.Configuration.GetSection("Manager")) + .ValidateDataAnnotations(); + + // Logging + builder.Services.AddSerilog(); + + // Singletons + builder.Services.AddSingleton(); + builder.Services.AddSingleton(); + builder.Services.AddSingleton(); + + // Services + // TODO: is this sound enough to determine if we're a service? + if (!Environment.UserInteractive) + { + MainLogger.Information("Running as a windows service"); + builder.Services.AddWindowsService(options => { options.ServiceName = serviceName; }); + } + else + { + MainLogger.Information("Running as a console application"); + } + + builder.Services.AddHostedService(); + builder.Services.AddHostedService(); + + await builder.Build().RunAsync(); + } +} diff --git a/Vpn.Service/Rebuild-Service.ps1 b/Vpn.Service/Rebuild-Service.ps1 new file mode 100644 index 0000000..617b5b4 --- /dev/null +++ b/Vpn.Service/Rebuild-Service.ps1 @@ -0,0 +1,3 @@ +& $PSScriptRoot/Stop-Service.ps1 +dotnet build -c Debug ./Vpn.Service.csproj +& $PSScriptRoot/Restart-Service.ps1 diff --git a/Vpn.Service/RegistryConfigurationSource.cs b/Vpn.Service/RegistryConfigurationSource.cs index 7ac2764..8e2dd0d 100644 --- a/Vpn.Service/RegistryConfigurationSource.cs +++ b/Vpn.Service/RegistryConfigurationSource.cs @@ -8,6 +8,7 @@ public class RegistryConfigurationSource : IConfigurationSource private readonly RegistryKey _root; private readonly string _subKeyName; + // ReSharper disable once ConvertToPrimaryConstructor public RegistryConfigurationSource(RegistryKey root, string subKeyName) { _root = root; @@ -25,6 +26,7 @@ public class RegistryConfigurationProvider : ConfigurationProvider private readonly RegistryKey _root; private readonly string _subKeyName; + // ReSharper disable once ConvertToPrimaryConstructor public RegistryConfigurationProvider(RegistryKey root, string subKeyName) { _root = root; diff --git a/Vpn.Service/Restart-Service.ps1 b/Vpn.Service/Restart-Service.ps1 new file mode 100644 index 0000000..201aab6 --- /dev/null +++ b/Vpn.Service/Restart-Service.ps1 @@ -0,0 +1,13 @@ +$name = "Coder Desktop (Debug)" + +try { + Restart-Service -Name $name -Force + if ((Get-Service -Name $name -ErrorAction Stop).Status -ne "Running") { + throw "Service '$name' is not running" + } + Write-Host "Service '$name' restarted successfully" +} catch { + Write-Host $_ -ForegroundColor Red + Write-Host "Press Return to exit..." + Read-Host +} diff --git a/Vpn.Service/Stop-Service.ps1 b/Vpn.Service/Stop-Service.ps1 new file mode 100644 index 0000000..afd8540 --- /dev/null +++ b/Vpn.Service/Stop-Service.ps1 @@ -0,0 +1,10 @@ +$name = "Coder Desktop (Debug)" + +try { + Stop-Service -Name $name -Force + Write-Host "Service '$name' stopped successfully" +} catch { + Write-Host $_ -ForegroundColor Red + Write-Host "Press Return to exit..." + Read-Host +} diff --git a/Vpn.Service/TunnelSupervisor.cs b/Vpn.Service/TunnelSupervisor.cs index 9ea5b05..b02d893 100644 --- a/Vpn.Service/TunnelSupervisor.cs +++ b/Vpn.Service/TunnelSupervisor.cs @@ -8,6 +8,8 @@ namespace Coder.Desktop.Vpn.Service; public interface ITunnelSupervisor : IAsyncDisposable { + public bool IsRunning { get; } + /// /// Starts the tunnel subprocess with the given executable path. If the subprocess is already running, this method will /// kill it first. @@ -27,9 +29,26 @@ public Task StartAsync(string binPath, /// /// Stops the tunnel subprocess. If the subprocess is not running, this method does nothing. /// - /// - /// + /// Cancellation token public Task StopAsync(CancellationToken ct = default); + + /// + /// Sends a message to the tunnel that does not expect a reply. + /// + /// Message to send + /// Cancellation token + /// The Speaker is not ready or the tunnel is not running + public Task SendMessage(ManagerMessage message, CancellationToken ct = default); + + /// + /// Send a message to the tunnel and wait for a reply. The reply will be returned and the callback will not be + /// invoked as long as the reply is received before cancellation or termination. + /// + /// Message to send - the Rpc field will be overwritten + /// Cancellation token + /// Received reply + /// The Speaker is not ready or the tunnel is not running + public ValueTask SendRequestAwaitReply(ManagerMessage message, CancellationToken ct = default); } /// @@ -52,6 +71,8 @@ public TunnelSupervisor(ILogger logger) _logger = logger; } + public bool IsRunning => _speaker != null; + public async Task StartAsync(string binPath, Speaker.OnReceiveDelegate messageHandler, Speaker.OnErrorDelegate errorHandler, @@ -76,8 +97,20 @@ public async Task StartAsync(string binPath, ArgumentList = { "vpn-daemon", "run" }, UseShellExecute = false, CreateNoWindow = true, + RedirectStandardError = true, + RedirectStandardOutput = true, }, }; + _subprocess.OutputDataReceived += (_, args) => + { + if (!string.IsNullOrWhiteSpace(args.Data)) + _logger.LogDebug("OUT: {Data}", args.Data); + }; + _subprocess.ErrorDataReceived += (_, args) => + { + if (!string.IsNullOrWhiteSpace(args.Data)) + _logger.LogDebug("ERR: {Data}", args.Data); + }; // Pass the other end of the pipes to the subprocess and dispose // the local copies. @@ -85,11 +118,13 @@ public async Task StartAsync(string binPath, _outPipe.GetClientHandleAsString()); _subprocess.StartInfo.Environment.Add("CODER_VPN_DAEMON_RPC_WRITE_HANDLE", _inPipe.GetClientHandleAsString()); - _outPipe.DisposeLocalCopyOfClientHandle(); - _inPipe.DisposeLocalCopyOfClientHandle(); _logger.LogInformation("StartAsync: starting subprocess"); _subprocess.Start(); + _subprocess.BeginOutputReadLine(); + _subprocess.BeginErrorReadLine(); + _outPipe.DisposeLocalCopyOfClientHandle(); + _inPipe.DisposeLocalCopyOfClientHandle(); _logger.LogInformation("StartAsync: subprocess started"); // We don't use the supplied CancellationToken here because we want it to only apply to the startup @@ -140,6 +175,48 @@ public async Task StopAsync(CancellationToken ct = default) } } + public async Task SendMessage(ManagerMessage message, CancellationToken ct = default) + { + if (!await _operationLock.WaitAsync(0, ct)) + throw new InvalidOperationException("TunnelSupervisor is not running"); + + Task task; + try + { + if (_speaker == null) + throw new InvalidOperationException("Speaker is not ready"); + task = _speaker.SendMessage(message, ct); + } + finally + { + _operationLock.Release(); + } + + // Don't await the task while holding the lock. + await task; + } + + public async ValueTask SendRequestAwaitReply(ManagerMessage message, CancellationToken ct = default) + { + if (!await _operationLock.WaitAsync(0, ct)) + throw new InvalidOperationException("TunnelSupervisor is not running"); + + ValueTask task; + try + { + if (_speaker == null) + throw new InvalidOperationException("Speaker is not ready"); + task = _speaker.SendRequestAwaitReply(message, ct); + } + finally + { + _operationLock.Release(); + } + + // Don't await the task while holding the lock. + return await task; + } + public async ValueTask DisposeAsync() { _cts.Dispose(); @@ -150,13 +227,13 @@ public async ValueTask DisposeAsync() private async Task OnProcessExited(Task task) { if (task.IsFaulted) + _logger.LogError(task.Exception, "OnProcessExited: subprocess task exited with an exception"); + if (!await _operationLock.WaitAsync(0)) { - _logger.LogError(task.Exception, "OnProcessExited: subprocess exited with an exception"); + _logger.LogInformation("OnProcessExited: could not acquire operation lock to perform cleanup"); return; } - if (!await _operationLock.WaitAsync(0)) _logger.LogInformation("OnProcessExited: subprocess exited"); - try { await CleanupAsync(); @@ -170,7 +247,7 @@ private async Task OnProcessExited(Task task) } /// - /// Cleans up the pipes and the subprocess if it's still running. This method should not be called without holding the + /// Cleans up the pipes and the subprocess if it's still running. This method must be called while holding the /// semaphore. /// private async Task CleanupAsync(CancellationToken ct = default) diff --git a/Vpn.Service/Vpn.Service.csproj b/Vpn.Service/Vpn.Service.csproj index e6da70d..44b99c9 100644 --- a/Vpn.Service/Vpn.Service.csproj +++ b/Vpn.Service/Vpn.Service.csproj @@ -14,6 +14,9 @@ + + + From 07d08929939a8be0fdbe3bd541d963c673f806d0 Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Thu, 30 Jan 2025 14:27:17 +1100 Subject: [PATCH 2/4] PR comments --- Coder.Desktop.sln.DotSettings | 21 +--- Vpn.Service/Manager.cs | 150 +++++++++++++++++------------ Vpn.Service/ManagerRpcService.cs | 6 ++ Vpn/Utilities/RaiiSemaphoreSlim.cs | 7 ++ 4 files changed, 103 insertions(+), 81 deletions(-) diff --git a/Coder.Desktop.sln.DotSettings b/Coder.Desktop.sln.DotSettings index 176e490..9804cf1 100644 --- a/Coder.Desktop.sln.DotSettings +++ b/Coder.Desktop.sln.DotSettings @@ -30,7 +30,7 @@ </HasMember> </And> </TypePattern.Match> - + <Entry DisplayName="Fields"> <Entry.Match> <And> @@ -144,10 +144,6 @@ <Kind Is="Delegate" /> </And> </Entry.Match> - - <Entry.SortBy> - <Name /> - </Entry.SortBy> </Entry> <Entry DisplayName="Public Enums" Priority="100"> @@ -157,10 +153,6 @@ <Kind Is="Enum" /> </And> </Entry.Match> - - <Entry.SortBy> - <Name /> - </Entry.SortBy> </Entry> <Entry DisplayName="Static Fields and Constants"> @@ -193,21 +185,12 @@ </Not> </And> </Entry.Match> - - <Entry.SortBy> - <Readonly /> - <Name /> - </Entry.SortBy> </Entry> <Entry DisplayName="Events"> <Entry.Match> <Kind Is="Event" /> </Entry.Match> - - <Entry.SortBy> - <Name /> - </Entry.SortBy> </Entry> <Entry DisplayName="Constructors"> @@ -256,4 +239,4 @@ True True True - True \ No newline at end of file + True diff --git a/Vpn.Service/Manager.cs b/Vpn.Service/Manager.cs index 4288007..3c6906c 100644 --- a/Vpn.Service/Manager.cs +++ b/Vpn.Service/Manager.cs @@ -1,5 +1,6 @@ using System.Runtime.InteropServices; using Coder.Desktop.Vpn.Proto; +using Coder.Desktop.Vpn.Utilities; using CoderSdk; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -27,6 +28,10 @@ public class Manager : IManager private readonly IDownloader _downloader; private readonly ILogger _logger; private readonly ITunnelSupervisor _tunnelSupervisor; + + // TunnelSupervisor already has protections against concurrent operations, + // but all the other stuff before starting the tunnel does not. + private readonly RaiiSemaphoreSlim _tunnelOperationLock = new(1, 1); private SemVersion? _lastServerVersion; private StartRequest? _lastStartRequest; @@ -58,10 +63,18 @@ public async Task HandleClientRpcMessage(ReplyableRpcMessage message, + private async ValueTask HandleClientMessageStart(ClientMessage message, CancellationToken ct) { - try + var opLock = await _tunnelOperationLock.LockAsync(TimeSpan.FromMilliseconds(500), ct); + if (opLock == null) { - var serverVersion = - await CheckServerVersionAndCredentials(message.Message.Start.CoderUrl, message.Message.Start.ApiToken, - ct); - if (_tunnelSupervisor.IsRunning && _lastStartRequest != null && - _lastStartRequest.Equals(message.Message.Start) && _lastServerVersion == serverVersion) + _logger.LogWarning("ClientMessage.Start: Tunnel operation lock timed out"); + return new StartResponse { - // The client is requesting to start an identical tunnel while - // we're already running it. - _logger.LogInformation("ClientMessage.Start: Ignoring duplicate start request"); - await message.SendReply(new ServiceMessage + Success = false, + ErrorMessage = "Could not acquire tunnel operation lock, another operation is in progress", + }; + } + + using (opLock) + { + try + { + var serverVersion = + await CheckServerVersionAndCredentials(message.Start.CoderUrl, message.Start.ApiToken, + ct); + if (_tunnelSupervisor.IsRunning && _lastStartRequest != null && + _lastStartRequest.Equals(message.Start) && _lastServerVersion == serverVersion) { - Start = new StartResponse + // The client is requesting to start an identical tunnel while + // we're already running it. + _logger.LogInformation("ClientMessage.Start: Ignoring duplicate start request"); + return new StartResponse { Success = true, - }, - }, ct); - return; - } - - _lastStartRequest = message.Message.Start; - _lastServerVersion = serverVersion; + }; + } - // Stop the tunnel if it's running so we don't have to worry about - // permissions issues when replacing the binary. - await _tunnelSupervisor.StopAsync(ct); - await DownloadTunnelBinaryAsync(message.Message.Start.CoderUrl, serverVersion, ct); - await _tunnelSupervisor.StartAsync(_config.TunnelBinaryPath, HandleTunnelRpcMessage, HandleTunnelRpcError, - ct); + _lastStartRequest = message.Start; + _lastServerVersion = serverVersion; - var reply = await _tunnelSupervisor.SendRequestAwaitReply(new ManagerMessage - { - Start = message.Message.Start, - }, ct); - if (reply.MsgCase != TunnelMessage.MsgOneofCase.Start) - throw new InvalidOperationException("Tunnel did not reply with a Start response"); + // TODO: each section of this operation needs a timeout + // Stop the tunnel if it's running so we don't have to worry about + // permissions issues when replacing the binary. + await _tunnelSupervisor.StopAsync(ct); + await DownloadTunnelBinaryAsync(message.Start.CoderUrl, serverVersion, ct); + await _tunnelSupervisor.StartAsync(_config.TunnelBinaryPath, HandleTunnelRpcMessage, + HandleTunnelRpcError, + ct); - await message.SendReply(new ServiceMessage - { - Start = reply.Start, - }, ct); - } - catch (Exception e) - { - _logger.LogWarning(e, "ClientMessage.Start: Failed to start VPN client"); - await message.SendReply(new ServiceMessage + var reply = await _tunnelSupervisor.SendRequestAwaitReply(new ManagerMessage + { + Start = message.Start, + }, ct); + if (reply.MsgCase != TunnelMessage.MsgOneofCase.Start) + throw new InvalidOperationException("Tunnel did not reply with a Start response"); + return reply.Start; + } + catch (Exception e) { - Start = new StartResponse + _logger.LogWarning(e, "ClientMessage.Start: Failed to start VPN client"); + return new StartResponse { Success = false, ErrorMessage = e.ToString(), - }, - }, ct); + }; + } } } - private async Task HandleClientMessageStop(ReplyableRpcMessage message, + private async ValueTask HandleClientMessageStop(ClientMessage message, CancellationToken ct) { - try + var opLock = await _tunnelOperationLock.LockAsync(TimeSpan.FromMilliseconds(500), ct); + if (opLock == null) { - // This will handle sending the Stop message to the tunnel for us. - await _tunnelSupervisor.StopAsync(ct); - await message.SendReply(new ServiceMessage + _logger.LogWarning("ClientMessage.Stop: Tunnel operation lock timed out"); + return new StopResponse { - Stop = new StopResponse - { - Success = true, - }, - }, ct); + Success = false, + ErrorMessage = "Could not acquire tunnel operation lock, another operation is in progress", + }; } - catch (Exception e) + + using (opLock) { - _logger.LogWarning(e, "ClientMessage.Stop: Failed to stop VPN client"); - await message.SendReply(new ServiceMessage + try { - Stop = new StopResponse + // This will handle sending the Stop message to the tunnel for us. + await _tunnelSupervisor.StopAsync(ct); + return new StopResponse + { + Success = true, + }; + } + catch (Exception e) + { + _logger.LogWarning(e, "ClientMessage.Stop: Failed to stop VPN client"); + return new StopResponse { Success = false, - ErrorMessage = e.Message, - }, - }, ct); + ErrorMessage = e.ToString(), + }; + } } } diff --git a/Vpn.Service/ManagerRpcService.cs b/Vpn.Service/ManagerRpcService.cs index 4d03c2f..eb3cd0b 100644 --- a/Vpn.Service/ManagerRpcService.cs +++ b/Vpn.Service/ManagerRpcService.cs @@ -144,6 +144,8 @@ private async Task HandleRpcMessageAsync(ReplyableRpcMessage LockAsync(CancellationToken ct = default) return new Lock(_semaphore); } + public async ValueTask LockAsync(TimeSpan timeout, CancellationToken ct = default) + { + if (await _semaphore.WaitAsync(timeout, ct)) return null; + + return new Lock(_semaphore); + } + private class Lock : IDisposable { private readonly SemaphoreSlim _semaphore1; From 4eacfeadc62538d06a3402d7c5a2dc398c1d1a4b Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Thu, 30 Jan 2025 16:45:05 +1100 Subject: [PATCH 3/4] fixup! Merge branch 'main' into dean/debug-client --- App/packages.lock.json | 6 ------ 1 file changed, 6 deletions(-) diff --git a/App/packages.lock.json b/App/packages.lock.json index cd62fd8..14115ab 100644 --- a/App/packages.lock.json +++ b/App/packages.lock.json @@ -26,12 +26,6 @@ "System.Reflection.Metadata": "9.0.0" } }, - "Microsoft.NET.ILLink.Tasks": { - "type": "Direct", - "requested": "[8.0.12, )", - "resolved": "8.0.12", - "contentHash": "FV4HnQ3JI15PHnJ5PGTbz+rYvrih42oLi/7UMIshNwCwUZhTq13UzrggtXk4ygrcMcN+4jsS6hhshx2p/Zd0ig==" - }, "Microsoft.Windows.SDK.BuildTools": { "type": "Direct", "requested": "[10.0.26100.1742, )", From 0f5410a5bcc05fed1b423898c21fc891f97269cb Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Thu, 30 Jan 2025 16:48:58 +1100 Subject: [PATCH 4/4] fixup! Merge branch 'main' into dean/debug-client --- Vpn.DebugClient/Program.cs | 2 +- Vpn.Service/Manager.cs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Vpn.DebugClient/Program.cs b/Vpn.DebugClient/Program.cs index 4fc33c0..9facc85 100644 --- a/Vpn.DebugClient/Program.cs +++ b/Vpn.DebugClient/Program.cs @@ -1,4 +1,4 @@ -using System.IO.Pipes; +using System.IO.Pipes; using Coder.Desktop.Vpn.Proto; namespace Coder.Desktop.Vpn.DebugClient; diff --git a/Vpn.Service/Manager.cs b/Vpn.Service/Manager.cs index 3c6906c..882a75f 100644 --- a/Vpn.Service/Manager.cs +++ b/Vpn.Service/Manager.cs @@ -284,10 +284,10 @@ private async Task DownloadTunnelBinaryAsync(string baseUrl, SemVersion expected _config.TunnelBinaryPath); var req = new HttpRequestMessage(HttpMethod.Get, url); var validators = new CombinationDownloadValidator( - // TODO: re-enable when the binaries are signed and have versions - //AuthenticodeDownloadValidator.Coder, - //new AssemblyVersionDownloadValidator( - //$"{expectedVersion.Major}.{expectedVersion.Minor}.{expectedVersion.Patch}.0") + // TODO: re-enable when the binaries are signed and have versions + //AuthenticodeDownloadValidator.Coder, + //new AssemblyVersionDownloadValidator( + //$"{expectedVersion.Major}.{expectedVersion.Minor}.{expectedVersion.Patch}.0") ); var downloadTask = await _downloader.StartDownloadAsync(req, _config.TunnelBinaryPath, validators, ct);