From a13f952a57b41d8778c3bda91caec3f9868f1009 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Mon, 21 Jul 2025 10:31:07 +0200 Subject: [PATCH 1/3] fix: opt-in tailscale vpn loop prevention --- App/App.xaml.cs | 2 +- App/Models/Settings.cs | 13 +++++++++++-- App/Services/RpcController.cs | 9 ++++++++- App/ViewModels/SettingsViewModel.cs | 21 ++++++++++++++++++++- App/Views/Pages/SettingsMainPage.xaml | 6 ++++++ App/Views/SettingsWindow.xaml | 4 ++-- 6 files changed, 48 insertions(+), 7 deletions(-) diff --git a/App/App.xaml.cs b/App/App.xaml.cs index a07af43..40c0f26 100644 --- a/App/App.xaml.cs +++ b/App/App.xaml.cs @@ -92,6 +92,7 @@ public App() services.AddSingleton(_ => this); services.AddSingleton(_ => this); + services.AddSingleton, SettingsManager>(); services.AddSingleton(_ => new WindowsCredentialBackend(WindowsCredentialBackend.CoderCredentialsTargetName)); services.AddSingleton(); @@ -120,7 +121,6 @@ public App() // FileSyncListMainPage is created by FileSyncListWindow. services.AddTransient(); - services.AddSingleton, SettingsManager>(); services.AddSingleton(); // SettingsWindow views and view models services.AddTransient(); diff --git a/App/Models/Settings.cs b/App/Models/Settings.cs index ec4c61b..0e8c340 100644 --- a/App/Models/Settings.cs +++ b/App/Models/Settings.cs @@ -34,6 +34,11 @@ public class CoderConnectSettings : ISettings /// public bool ConnectOnLaunch { get; set; } + /// + /// When this is true Coder Connect will not attempt to protect against Tailscale loopback issues. + /// + public bool DisableTailscaleLoopProtection { get; set; } + /// /// CoderConnect current settings version. Increment this when the settings schema changes. /// In future iterations we will be able to handle migrations when the user has @@ -46,17 +51,21 @@ public CoderConnectSettings() Version = VERSION; ConnectOnLaunch = false; + + DisableTailscaleLoopProtection = false; } - public CoderConnectSettings(int? version, bool connectOnLaunch) + public CoderConnectSettings(int? version, bool connectOnLaunch, bool disableTailscaleLoopProtection) { Version = version ?? VERSION; ConnectOnLaunch = connectOnLaunch; + + DisableTailscaleLoopProtection = disableTailscaleLoopProtection; } public CoderConnectSettings Clone() { - return new CoderConnectSettings(Version, ConnectOnLaunch); + return new CoderConnectSettings(Version, ConnectOnLaunch, DisableTailscaleLoopProtection); } } diff --git a/App/Services/RpcController.cs b/App/Services/RpcController.cs index 532c878..b658e6c 100644 --- a/App/Services/RpcController.cs +++ b/App/Services/RpcController.cs @@ -69,6 +69,7 @@ public interface IRpcController : IAsyncDisposable public class RpcController : IRpcController { private readonly ICredentialManager _credentialManager; + private readonly ISettingsManager _settingsManager; private readonly RaiiSemaphoreSlim _operationLock = new(1, 1); private Speaker? _speaker; @@ -76,9 +77,10 @@ public class RpcController : IRpcController private readonly RaiiSemaphoreSlim _stateLock = new(1, 1); private readonly RpcModel _state = new(); - public RpcController(ICredentialManager credentialManager) + public RpcController(ICredentialManager credentialManager, ISettingsManager settingsManager) { _credentialManager = credentialManager; + _settingsManager = settingsManager; } public event EventHandler? StateChanged; @@ -156,6 +158,11 @@ public async Task StartVpn(CancellationToken ct = default) using var _ = await AcquireOperationLockNowAsync(); AssertRpcConnected(); + var coderConnectSettings = await _settingsManager.Read(); + var disableTailscaleLoopProtection = coderConnectSettings.DisableTailscaleLoopProtection; + Debug.WriteLine( + $"Starting VPN with DisableTailscaleLoopProtection={disableTailscaleLoopProtection}"); + var credentials = _credentialManager.GetCachedCredentials(); if (credentials.State != CredentialState.Valid) throw new RpcOperationException( diff --git a/App/ViewModels/SettingsViewModel.cs b/App/ViewModels/SettingsViewModel.cs index 721ea95..9cafc9c 100644 --- a/App/ViewModels/SettingsViewModel.cs +++ b/App/ViewModels/SettingsViewModel.cs @@ -13,6 +13,9 @@ public partial class SettingsViewModel : ObservableObject [ObservableProperty] public partial bool ConnectOnLaunch { get; set; } + [ObservableProperty] + public partial bool DisableTailscaleLoopProtection { get; set; } + [ObservableProperty] public partial bool StartOnLoginDisabled { get; set; } @@ -31,6 +34,7 @@ public SettingsViewModel(ILogger logger, ISettingsManager logger, ISettingsManager + + + diff --git a/App/Views/SettingsWindow.xaml b/App/Views/SettingsWindow.xaml index 512f0f5..4cd09bd 100644 --- a/App/Views/SettingsWindow.xaml +++ b/App/Views/SettingsWindow.xaml @@ -9,8 +9,8 @@ xmlns:winuiex="using:WinUIEx" mc:Ignorable="d" Title="Coder Settings" - Width="600" Height="350" - MinWidth="600" MinHeight="350"> + Width="600" Height="450" + MinWidth="600" MinHeight="450"> From d0f8ba00a3cc508afeee8d7879b5ff71bb3df9e6 Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Mon, 28 Jul 2025 14:13:42 +1000 Subject: [PATCH 2/3] Wire up setting to StartRequest, PR comments --- App/Models/Settings.cs | 10 +++++----- App/Services/CredentialManager.cs | 10 ++++------ App/Services/RpcController.cs | 7 ++----- App/ViewModels/SettingsViewModel.cs | 4 ++-- App/Views/Pages/SettingsMainPage.xaml | 6 +++--- App/Views/SettingsWindow.xaml | 4 ++-- App/Views/TrayWindow.xaml.cs | 24 ++++++------------------ Vpn.Proto/vpn.proto | 3 ++- 8 files changed, 26 insertions(+), 42 deletions(-) diff --git a/App/Models/Settings.cs b/App/Models/Settings.cs index 0e8c340..f1c89ce 100644 --- a/App/Models/Settings.cs +++ b/App/Models/Settings.cs @@ -37,7 +37,7 @@ public class CoderConnectSettings : ISettings /// /// When this is true Coder Connect will not attempt to protect against Tailscale loopback issues. /// - public bool DisableTailscaleLoopProtection { get; set; } + public bool EnableCorporateVpnSupport { get; set; } /// /// CoderConnect current settings version. Increment this when the settings schema changes. @@ -52,20 +52,20 @@ public CoderConnectSettings() ConnectOnLaunch = false; - DisableTailscaleLoopProtection = false; + EnableCorporateVpnSupport = false; } - public CoderConnectSettings(int? version, bool connectOnLaunch, bool disableTailscaleLoopProtection) + public CoderConnectSettings(int? version, bool connectOnLaunch, bool enableCorporateVpnSupport) { Version = version ?? VERSION; ConnectOnLaunch = connectOnLaunch; - DisableTailscaleLoopProtection = disableTailscaleLoopProtection; + EnableCorporateVpnSupport = enableCorporateVpnSupport; } public CoderConnectSettings Clone() { - return new CoderConnectSettings(Version, ConnectOnLaunch, DisableTailscaleLoopProtection); + return new CoderConnectSettings(Version, ConnectOnLaunch, EnableCorporateVpnSupport); } } diff --git a/App/Services/CredentialManager.cs b/App/Services/CredentialManager.cs index 6868ae7..c8030c3 100644 --- a/App/Services/CredentialManager.cs +++ b/App/Services/CredentialManager.cs @@ -223,8 +223,9 @@ private async Task LoadCredentialsInner(CancellationToken ct) }; } - // Grab the lock again so we can update the state. - using (await _opLock.LockAsync(ct)) + // Grab the lock again so we can update the state. Don't use the CT + // here as it may have already been canceled. + using (await _opLock.LockAsync(TimeSpan.FromSeconds(5), CancellationToken.None)) { // Prevent new LoadCredentials calls from returning this task. if (_loadCts != null) @@ -242,11 +243,8 @@ private async Task LoadCredentialsInner(CancellationToken ct) if (latestCreds is not null) return latestCreds; } - // If there aren't any latest credentials after a cancellation, we - // most likely timed out and should throw. - ct.ThrowIfCancellationRequested(); - UpdateState(model); + ct.ThrowIfCancellationRequested(); return model; } } diff --git a/App/Services/RpcController.cs b/App/Services/RpcController.cs index b658e6c..f7abe79 100644 --- a/App/Services/RpcController.cs +++ b/App/Services/RpcController.cs @@ -158,11 +158,7 @@ public async Task StartVpn(CancellationToken ct = default) using var _ = await AcquireOperationLockNowAsync(); AssertRpcConnected(); - var coderConnectSettings = await _settingsManager.Read(); - var disableTailscaleLoopProtection = coderConnectSettings.DisableTailscaleLoopProtection; - Debug.WriteLine( - $"Starting VPN with DisableTailscaleLoopProtection={disableTailscaleLoopProtection}"); - + var coderConnectSettings = await _settingsManager.Read(ct); var credentials = _credentialManager.GetCachedCredentials(); if (credentials.State != CredentialState.Valid) throw new RpcOperationException( @@ -182,6 +178,7 @@ public async Task StartVpn(CancellationToken ct = default) { CoderUrl = credentials.CoderUrl?.ToString(), ApiToken = credentials.ApiToken, + TunnelUseSoftNetIsolation = coderConnectSettings.EnableCorporateVpnSupport, }, }, ct); } diff --git a/App/ViewModels/SettingsViewModel.cs b/App/ViewModels/SettingsViewModel.cs index 9cafc9c..58b31f2 100644 --- a/App/ViewModels/SettingsViewModel.cs +++ b/App/ViewModels/SettingsViewModel.cs @@ -34,7 +34,7 @@ public SettingsViewModel(ILogger logger, ISettingsManager - diff --git a/App/Views/SettingsWindow.xaml b/App/Views/SettingsWindow.xaml index 4cd09bd..a2fe886 100644 --- a/App/Views/SettingsWindow.xaml +++ b/App/Views/SettingsWindow.xaml @@ -9,8 +9,8 @@ xmlns:winuiex="using:WinUIEx" mc:Ignorable="d" Title="Coder Settings" - Width="600" Height="450" - MinWidth="600" MinHeight="450"> + Width="600" Height="500" + MinWidth="600" MinHeight="500"> diff --git a/App/Views/TrayWindow.xaml.cs b/App/Views/TrayWindow.xaml.cs index 72ab6cc..de944c2 100644 --- a/App/Views/TrayWindow.xaml.cs +++ b/App/Views/TrayWindow.xaml.cs @@ -5,11 +5,9 @@ using Coder.Desktop.App.Views.Pages; using CommunityToolkit.Mvvm.Input; using Microsoft.UI; -using Microsoft.UI.Input; using Microsoft.UI.Windowing; using Microsoft.UI.Xaml; using Microsoft.UI.Xaml.Controls; -using Microsoft.UI.Xaml.Documents; using Microsoft.UI.Xaml.Media.Animation; using System; using System.Collections.Generic; @@ -19,6 +17,7 @@ using Windows.Graphics; using Windows.System; using Windows.UI.Core; +using Microsoft.UI.Input; using WinRT.Interop; using WindowActivatedEventArgs = Microsoft.UI.Xaml.WindowActivatedEventArgs; @@ -41,7 +40,6 @@ public sealed partial class TrayWindow : Window private readonly IRpcController _rpcController; private readonly ICredentialManager _credentialManager; - private readonly ISyncSessionController _syncSessionController; private readonly IUpdateController _updateController; private readonly IUserNotifier _userNotifier; private readonly TrayWindowLoadingPage _loadingPage; @@ -51,15 +49,13 @@ public sealed partial class TrayWindow : Window public TrayWindow( IRpcController rpcController, ICredentialManager credentialManager, - ISyncSessionController syncSessionController, IUpdateController updateController, - IUserNotifier userNotifier, + IUpdateController updateController, IUserNotifier userNotifier, TrayWindowLoadingPage loadingPage, TrayWindowDisconnectedPage disconnectedPage, TrayWindowLoginRequiredPage loginRequiredPage, TrayWindowMainPage mainPage) { _rpcController = rpcController; _credentialManager = credentialManager; - _syncSessionController = syncSessionController; _updateController = updateController; _userNotifier = userNotifier; _loadingPage = loadingPage; @@ -74,9 +70,7 @@ public TrayWindow( _rpcController.StateChanged += RpcController_StateChanged; _credentialManager.CredentialsChanged += CredentialManager_CredentialsChanged; - _syncSessionController.StateChanged += SyncSessionController_StateChanged; - SetPageByState(_rpcController.GetState(), _credentialManager.GetCachedCredentials(), - _syncSessionController.GetState()); + SetPageByState(_rpcController.GetState(), _credentialManager.GetCachedCredentials()); // Setting these directly in the .xaml doesn't seem to work for whatever reason. TrayIcon.OpenCommand = Tray_OpenCommand; @@ -127,8 +121,7 @@ public TrayWindow( }; } - private void SetPageByState(RpcModel rpcModel, CredentialModel credentialModel, - SyncSessionControllerStateModel syncSessionModel) + private void SetPageByState(RpcModel rpcModel, CredentialModel credentialModel) { if (credentialModel.State == CredentialState.Unknown) { @@ -201,18 +194,13 @@ private void MaybeNotifyUser(RpcModel rpcModel) private void RpcController_StateChanged(object? _, RpcModel model) { - SetPageByState(model, _credentialManager.GetCachedCredentials(), _syncSessionController.GetState()); + SetPageByState(model, _credentialManager.GetCachedCredentials()); MaybeNotifyUser(model); } private void CredentialManager_CredentialsChanged(object? _, CredentialModel model) { - SetPageByState(_rpcController.GetState(), model, _syncSessionController.GetState()); - } - - private void SyncSessionController_StateChanged(object? _, SyncSessionControllerStateModel model) - { - SetPageByState(_rpcController.GetState(), _credentialManager.GetCachedCredentials(), model); + SetPageByState(_rpcController.GetState(), model); } // Sadly this is necessary because Window.Content.SizeChanged doesn't diff --git a/Vpn.Proto/vpn.proto b/Vpn.Proto/vpn.proto index 11a481c..61c9978 100644 --- a/Vpn.Proto/vpn.proto +++ b/Vpn.Proto/vpn.proto @@ -62,7 +62,7 @@ message ServiceMessage { StartResponse start = 2; StopResponse stop = 3; Status status = 4; // either in reply to a StatusRequest or broadcasted - StartProgress start_progress = 5; // broadcasted during startup + StartProgress start_progress = 5; // broadcasted during startup (used exclusively by Windows) } } @@ -214,6 +214,7 @@ message NetworkSettingsResponse { // StartResponse. message StartRequest { int32 tunnel_file_descriptor = 1; + bool tunnel_use_soft_net_isolation = 8; string coder_url = 2; string api_token = 3; // Additional HTTP headers added to all requests From ab7cea5d6bbd00deecb31c2ca8990ee3e6d0a152 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Mon, 28 Jul 2025 17:04:12 +0200 Subject: [PATCH 3/3] fixed test to expect a result after the LoadCredentials graceful failure change --- Tests.App/Services/CredentialManagerTest.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Tests.App/Services/CredentialManagerTest.cs b/Tests.App/Services/CredentialManagerTest.cs index 9f1b0df..3f9c4cb 100644 --- a/Tests.App/Services/CredentialManagerTest.cs +++ b/Tests.App/Services/CredentialManagerTest.cs @@ -317,7 +317,10 @@ public async Task SetDuringLoad(CancellationToken ct) var loadTask = manager.LoadCredentials(ct); // Then fully perform a set. await manager.SetCredentials(TestServerUrl, TestApiToken, ct).WaitAsync(ct); - // The load should have been cancelled. - Assert.ThrowsAsync(() => loadTask); + + // The load should complete with the new valid credentials + var result = await loadTask; + Assert.That(result.State, Is.EqualTo(CredentialState.Valid)); + Assert.That(result.CoderUrl?.ToString(), Is.EqualTo(TestServerUrl)); } }