diff --git a/src/PostIt.Tests/LoginPageViewModelTests.cs b/src/PostIt.Tests/LoginPageViewModelTests.cs index c04618c2..e469c011 100644 --- a/src/PostIt.Tests/LoginPageViewModelTests.cs +++ b/src/PostIt.Tests/LoginPageViewModelTests.cs @@ -42,6 +42,83 @@ public class LoginPageViewModelTests $"Login reported error: {vm.StatusMessage}"); } + [Fact] + public async Task LoginAsync_refuses_to_call_OidcClient_when_Authority_is_empty() + { + // Regression: when no user settings file exists and the embedded + // default somehow fails to load (e.g. resource stripped at publish + // time), the ViewModel must NOT hand a blank Authority to + // OidcClient — IdentityModel would build a bogus authorize URL + // like "http://127.0.0.1:1/" which the browser rejects with a + // confusing error. Surface a clear, actionable message instead. + // + // SettingsLoadOverride is set to a no-op so the test fixture's + // pre-loaded Settings object survives the call to LoginAsync. + var settings = new PostIt.Settings + { + Authentication = new AuthenticationSettings + { + Authority = "", + ClientId = "postit-tests", + }, + RedirectUri = "http://127.0.0.1:7890/", + Scopes = new[] { "openid" }, + }; + + var browserInvoked = false; + var vm = new LoginPageViewModel(settings, () => + { + browserInvoked = true; + return null; + }) + { + // Skip the disk / embedded read so the Authority stays empty. + SettingsLoadOverride = () => System.Threading.Tasks.Task.CompletedTask, + }; + + await vm.LoginAsync(); + + Assert.False( + browserInvoked, + "Browser factory was invoked even though Authority was empty."); + Assert.NotNull(vm.StatusMessage); + Assert.Contains("Configuration manquante", vm.StatusMessage); + Assert.Contains("postit-settings.json", vm.StatusMessage); + Assert.True(string.IsNullOrEmpty(vm.AccessToken)); + } + + [Fact] + public async Task LoginAsync_works_when_authority_has_trailing_slash() + { + // Regression: with Authority ending in "/" (the production + // postit-settings.json shape for https://yavsc.pschneider.fr/), + // the discovery URL OidcClient computes must NOT contain a + // double slash before /.well-known/openid-configuration. The + // stub advertises itself without the trailing slash; OidcClient + // must bridge. + using var authority = await OidcStubAuthority.StartAsync(); + var browser = new FakeAuthorizingBrowser(authority.LoopbackRedirectUri); + + var settings = new PostIt.Settings + { + Authentication = new AuthenticationSettings + { + Authority = authority.Issuer + "/", + ClientId = "postit-tests" + }, + RedirectUri = authority.LoopbackRedirectUri, + Scopes = new[] { "openid" } + }; + + var vm = new LoginPageViewModel(settings, browser.CreateBrowser); + + await vm.LoginAsync(); + + Assert.True( + !string.IsNullOrEmpty(vm.AccessToken), + $"Login with trailing slash failed. StatusMessage={vm.StatusMessage ?? ""}"); + } + [Fact] public void RegisterUrl_and_ForgotPasswordUrl_are_derived_from_authority() { diff --git a/src/PostIt.Tests/LoopbackBrowserTests.cs b/src/PostIt.Tests/LoopbackBrowserTests.cs new file mode 100644 index 00000000..de6ebb9d --- /dev/null +++ b/src/PostIt.Tests/LoopbackBrowserTests.cs @@ -0,0 +1,92 @@ +using System.Net; +using System.Net.Sockets; +using PostIt.Services; + +namespace PostIt.Tests; + +/// +/// Regression coverage for the loopback browser that PostIt uses to +/// receive the OIDC authorization-code callback on a local port. +/// Specifically: the listener must always be released, even when the +/// flow is abandoned (timeout or caller cancellation). Without this, +/// the next PostIt launch fails with "Failed to listen on prefix +/// http://127.0.0.1:7890/ because it conflicts with an existing +/// registration on the machine." +/// +public class LoopbackBrowserTests +{ + [Fact] + public async Task InvokeAsync_releases_listener_when_no_browser_responds_within_timeout() + { + // Pick a free port for this test (don't reuse 7890 — it could be + // bound by a real PostIt running on the developer's machine). + var port = GetFreePort(); + var prefix = $"http://127.0.0.1:{port}/"; + + var browser = new LoopbackBrowser(); + var options = new IdentityModel.OidcClient.Browser.BrowserOptions( + "http://127.0.0.1:1/", // never reached + prefix); + + // The internal wait timeout is 5 minutes; we don't want the test + // to actually wait that long. Instead we cancel via the outer + // token and verify the listener is released. + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(10)); + + var result = await browser.InvokeAsync(options, cts.Token); + + // The cancellation propagates as Timeout because the outer token + // fires first (the test is faster than the 5-minute internal wait). + // We don't care which BrowserResultType is returned here — only + // that the port is free afterwards. + Assert.NotNull(result); + + // Critical assertion: the port is free. If the listener leaked, + // a TcpListener binding to the same port would throw. + using var probe = new TcpListener(IPAddress.Loopback, port); + probe.Start(); + probe.Stop(); + } + + [Fact] + public async Task InvokeAsync_releases_listener_when_browser_actually_responds() + { + var port = GetFreePort(); + var prefix = $"http://127.0.0.1:{port}/"; + + var browser = new LoopbackBrowser(); + var options = new IdentityModel.OidcClient.Browser.BrowserOptions( + "http://127.0.0.1:1/", // never reached (we respond directly below) + prefix); + + // Race the listener against a fake browser callback. + var browserTask = browser.InvokeAsync(options); + + // Give the listener a moment to bind. + await Task.Delay(50); + + // Simulate the browser returning the redirect with code + state. + using var http = new HttpClient(); + var response = await http.GetAsync($"{prefix.TrimEnd('/')}/?code=***&state=***"); + // We don't care about the response body; just that the request + // was accepted (otherwise the listener hadn't bound yet). + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + var result = await browserTask; + Assert.Equal(IdentityModel.OidcClient.Browser.BrowserResultType.Success, result.ResultType); + + // Listener should be released now. + using var probe = new TcpListener(IPAddress.Loopback, port); + probe.Start(); + probe.Stop(); + } + + private static int GetFreePort() + { + var l = new TcpListener(IPAddress.Loopback, 0); + l.Start(); + var port = ((IPEndPoint)l.LocalEndpoint).Port; + l.Stop(); + return port; + } +} diff --git a/src/PostIt/PostIt/Services/LoopbackBrowser.cs b/src/PostIt/PostIt/Services/LoopbackBrowser.cs index c9dd06b1..c49aec83 100644 --- a/src/PostIt/PostIt/Services/LoopbackBrowser.cs +++ b/src/PostIt/PostIt/Services/LoopbackBrowser.cs @@ -29,7 +29,16 @@ namespace PostIt.Services; { Process.Start(new ProcessStartInfo(options.StartUrl) { UseShellExecute = true }); - var context = await listener.GetContextAsync().ConfigureAwait(false); + // Bound the wait so the port is released even if the user + // closes the browser without completing the flow. Without + // this, a crashed/abandoned login keeps the HttpListener + // bound and the next PostIt launch fails with + // "Failed to listen on prefix … because it conflicts with + // an existing registration". + using var waitCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + waitCts.CancelAfter(TimeSpan.FromMinutes(5)); + + var context = await listener.GetContextAsync().WaitAsync(waitCts.Token).ConfigureAwait(false); var response = context.Response; var responseString = "Authentication complete. You can close this window."; var buffer = Encoding.UTF8.GetBytes(responseString); @@ -44,13 +53,28 @@ namespace PostIt.Services; Response = raw }; } + catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) + { + // Bound hit: user did not complete the flow within 5 minutes. + return new BrowserResult + { + ResultType = BrowserResultType.Timeout, + Error = "Timed out waiting for the browser to return the authorization code.", + }; + } catch (Exception ex) { return new BrowserResult { ResultType = BrowserResultType.UnknownError, Error = ex.Message }; } finally { + // Stop() aborts GetContextAsync (releases the bound port); + // Close() disposes the underlying socket. Both are idempotent + // and safe to call after Stop() already succeeded, so calling + // both covers cases where one path throws before the other + // gets a chance (e.g. process-level socket cleanup on Linux). try { listener.Stop(); } catch { } + try { listener.Close(); } catch { } } } } diff --git a/src/PostIt/PostIt/Settings/Settings.cs b/src/PostIt/PostIt/Settings/Settings.cs index 7aac405f..1d8e59c2 100644 --- a/src/PostIt/PostIt/Settings/Settings.cs +++ b/src/PostIt/PostIt/Settings/Settings.cs @@ -59,20 +59,11 @@ public partial class Settings : ObservableObject /// (no client secret). The browser implementation should be supplied /// per-platform by the caller. /// - /// - /// is normalised by - /// trimming any trailing slash before being handed to OidcClient. - /// OidcClient derives the discovery URL from - /// Authority + "/.well-known/openid-configuration"; leaving a - /// trailing slash in place would produce a double-slash URL that some - /// servers reject with 404. - /// internal OidcClientOptions GetOidcClientOptions(IdentityModel.OidcClient.Browser.IBrowser? browser = null) { - var authority = Authentication.Authority?.TrimEnd('/') ?? string.Empty; var options = new OidcClientOptions { - Authority = authority, + Authority = Authentication.Authority, ClientId = Authentication.ClientId, RedirectUri = RedirectUri, Scope = string.Join(' ', this.Scopes), diff --git a/src/PostIt/PostIt/ViewModels/LoginPageViewModel.cs b/src/PostIt/PostIt/ViewModels/LoginPageViewModel.cs index 91535763..800be830 100644 --- a/src/PostIt/PostIt/ViewModels/LoginPageViewModel.cs +++ b/src/PostIt/PostIt/ViewModels/LoginPageViewModel.cs @@ -94,6 +94,14 @@ public partial class LoginPageViewModel : ViewModelBase /// public Func? BrowserFactoryOverride { get; set; } + /// + /// Optional override used by tests. When set, this delegate replaces + /// the call to at the start of + /// , so tests can inject a Settings object + /// without it being overwritten by the user/embedded default. + /// + public Func? SettingsLoadOverride { get; set; } + public LoginPageViewModel() : this(new Settings(), browserFactoryOverride: null) { // Load settings eagerly so RegisterUrl / ForgotPasswordUrl are @@ -121,7 +129,22 @@ public partial class LoginPageViewModel : ViewModelBase try { this.IsBusy = true; - Settings.Load().Wait(); + (SettingsLoadOverride ?? Settings.Load)().Wait(); + + // Guard: if the authority is empty (no user settings file and + // the embedded default couldn't be loaded for any reason), + // refuse to call OidcClient. IdentityModel would otherwise + // build a bogus authorize URL like "http://127.0.0.1:1/" + // from an empty Authority, which the browser then refuses to + // open with a confusing "Cette adresse est interdite" + // (or equivalent) message. Tell the operator exactly what + // to fix instead. + if (string.IsNullOrWhiteSpace(Settings.Authentication?.Authority)) + { + this.IsBusy = false; + StatusMessage = $"Configuration manquante — édite {SettingsFileHint()} et renseigne Authentication.Authority"; + return; + } // The platform project picks the right redirect URI and browser // implementation; we don't reference any UI toolkit from here. @@ -174,4 +197,15 @@ public partial class LoginPageViewModel : ViewModelBase StatusMessage = $"Error: {ex.Message}{suffix}"; } } + + /// + /// XDG-compliant path to the user settings file. Surfaced in the + /// "Configuration manquante" message so the operator knows exactly + /// which file to edit without having to dig through docs. + /// + private static string SettingsFileHint() + { + var appData = Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData); + return System.IO.Path.Combine(appData, "PostIt", "postit-settings.json"); + } } diff --git a/src/Yavsc.Org/Directory.Packages.props b/src/Yavsc.Org/Directory.Packages.props index c5f856a2..6cd62331 100644 --- a/src/Yavsc.Org/Directory.Packages.props +++ b/src/Yavsc.Org/Directory.Packages.props @@ -6,6 +6,7 @@ + diff --git a/src/Yavsc.Org/Extensions/HostingExtensions.cs b/src/Yavsc.Org/Extensions/HostingExtensions.cs index c29540a5..a439b7f1 100644 --- a/src/Yavsc.Org/Extensions/HostingExtensions.cs +++ b/src/Yavsc.Org/Extensions/HostingExtensions.cs @@ -1,6 +1,7 @@ using System.Globalization; using System.IdentityModel.Tokens.Jwt; using System.Reflection; +using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; using Google.Apis.Util.Store; using IdentityModel; @@ -18,6 +19,15 @@ using Microsoft.AspNetCore.Localization; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Razor; using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.FileProviders; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using Org.BouncyCastle.Crypto; +using Org.BouncyCastle.Crypto.Parameters; +using Org.BouncyCastle.OpenSsl; +using Microsoft.IdentityModel.Tokens; +using Org.BouncyCastle.Security; using Microsoft.Extensions.FileProviders; using Microsoft.Extensions.Localization; using Microsoft.Extensions.Options; @@ -350,31 +360,149 @@ public static class HostingExtensions "Production IdentityServer requires a signing certificate. " + "Configure Kestrel:Endpoints:Https:Certificate:{Path,KeyPath}."); } - // CreateFromPemFile loads the leaf cert + its private key from - // PEM files without writing to the Windows certificate store - // (irrelevant on Linux, but keeps the call cross-platform). - var signingCert = X509Certificate2.CreateFromPemFile(certPath, keyPath); - // Pick the JWT signing algorithm from the cert's key type. Let's - // Encrypt may issue either RSA or ECDSA certificates depending on - // the ACME account's preferred chain; IdentityServer would 500 if - // we forced RS256 against an ECDSA key. - var algorithm = signingCert.GetECDsaPrivateKey() is not null ? "ES256" : "RS256"; - identityServerBuilder.AddSigningCredential(signingCert, algorithm); + // Load the leaf cert and extract its private key for signing. + // The previous attempts (X509Certificate2.CreateFromPemFile, + // the 3-arg ctor with X509KeyStorageFlags, and BC + CopyWithPrivateKey) + // all loaded the cert successfully, but CreateJwkDocumentAsync + // still raised NullReferenceException on the first GET /jwks + // request: IdentityServer8's key material service reads the + // private key off the X509Certificate2 at runtime, and on Linux + // the key handle is not retained across that boundary. + // + // The reliable pattern is to pass a SigningCredentials that + // wraps a SecurityKey built directly from the BC-parsed key + // parameters. The SecurityKey is a managed object whose Key + // property is a live AsymmetricAlgorithm, which survives every + // read IdentityServer does (token signing, JWKS publish). + var signingCredentials = LoadSigningCredentials(certPath, keyPath); + identityServerBuilder.AddSigningCredential(signingCredentials); } - // Override the advertised jwks_uri to the canonical - // /.well-known/jwks endpoint that UseIdentityServer() actually - // mounts. IdentityServer8's default convention here is - // /.well-known/openid-configuration/jwks, which is not what most - // OIDC clients (including IdentityModel.OidcClient) expect, and - // would otherwise need a parallel route to be wired up. - identityServerBuilder.Services.Configure(options => - { - options.Discovery.CustomEntries["jwks_uri"] = "/.well-known/jwks"; - }); + // Note: IdentityServer8 does NOT expose the JWKS at /.well-known/jwks. + // The default jwks_uri is /.well-known/openid-configuration/jwks, + // which is what DiscoveryKeyEndpoint serves. Earlier revisions of + // this file tried to override CustomEntries["jwks_uri"], but + // IdentityServer8 reserves that key and rejects the override with + // "Discovery custom entry jwks_uri cannot be added, because it + // already exists." The default endpoint works once the signing + // credential's private key is attached (see above). return identityServerBuilder; } + /// + /// Load the signing credentials (algorithm + private key) from the + /// configured PEM files. Returns a + /// whose Key is a managed built + /// directly from the BouncyCastle-parsed key parameters — which keeps + /// the private key alive for every read IdentityServer8 does (token + /// signing, JWKS publish), unlike X509Certificate2.CopyWithPrivateKey + /// which loses the handle on Linux when IdentityServer8's key material + /// service reads it back at runtime. + /// + private static SigningCredentials LoadSigningCredentials(string certPath, string keyPath) + { + // Pre-flight read so permission / missing-file errors surface with + // the actual path instead of being wrapped as an opaque + // InvalidOperationException by the cert / key parsers. + try + { + return LoadSigningCredentialsInner(certPath, keyPath); + } + catch (Exception ex) + { + Console.Error.WriteLine( + $"[yavsc] Failed to load signing credentials from {certPath} / {keyPath}:"); + Console.Error.WriteLine(ex.ToString()); + throw new InvalidOperationException( + $"Failed to load signing credentials from {certPath} / {keyPath}. " + + "See stderr for the underlying managed exception (likely a " + + "PEM format mismatch between the cert and private key).", + ex); + } + } + + private static SigningCredentials LoadSigningCredentialsInner(string certPath, string keyPath) + { + // Validate the cert is readable (used downstream for token + // audience/subject validation; signing itself uses the key). + _ = new X509Certificate2(certPath); + string keyPem = File.ReadAllText(keyPath); + + // BouncyCastle's PemReader accepts every flavour of unencrypted + // private key PEM that ACME clients produce (PKCS#1 with + // BEGIN EC/RSA PRIVATE KEY, PKCS#8 with BEGIN PRIVATE KEY, both + // EC and RSA), and returns the right AsymmetricKeyParameter + // subtype without the SIGABRTs we saw when forcing the + // System.Security.Cryptography path on the production EC Let's + // Encrypt cert. + using var sr = new StringReader(keyPem); + var pemReader = new PemReader(sr); + var keyObj = pemReader.ReadObject() + ?? throw new InvalidOperationException( + $"PEM reader returned null for {keyPath}"); + + AsymmetricKeyParameter bcKey = keyObj switch + { + AsymmetricCipherKeyPair pair => pair.Private, + AsymmetricKeyParameter param => param, + _ => throw new InvalidOperationException( + $"Unexpected PEM object type '{keyObj.GetType().FullName}' " + + $"in {keyPath}; expected a private key."), + }; + + // Build the SecurityKey + SigningCredentials. RsaSecurityKey / + // ECDsaSecurityKey wrap managed AsymmetricAlgorithm objects whose + // Key is the live private key — IdentityServer8 can call Sign on + // these repeatedly without losing the key handle. + switch (bcKey) + { + case RsaPrivateCrtKeyParameters rsa: + { + var rsaDotNet = DotNetUtilities.ToRSA(rsa); + var key = new RsaSecurityKey(rsaDotNet); + return new SigningCredentials(key, SecurityAlgorithms.RsaSha256); + } + case ECPrivateKeyParameters ec: + { + var ecParams = new ECParameters + { + Curve = LoadEcCurve(ec.Parameters), + D = ec.D.ToByteArrayUnsigned(), + }; + var ecdsa = ECDsa.Create(); + ecdsa.ImportParameters(ecParams); + var key = new ECDsaSecurityKey(ecdsa); + return new SigningCredentials(key, SecurityAlgorithms.EcdsaSha256); + } + default: + throw new InvalidOperationException( + $"Unsupported private key algorithm '{bcKey.GetType().Name}' " + + $"in {keyPath}; expected RSA or EC."); + } + } + + /// + /// Map a BouncyCastle to a + /// that + /// understands. Handles the curves Let's Encrypt issues (P-256, + /// P-384, P-521); other curves throw. + /// + private static ECCurve LoadEcCurve(ECDomainParameters bcCurve) + { + // bcCurve.N is the order of the generator; its bit length is the + // canonical fingerprint for NIST curves (256, 384, 521 bits). + var orderBits = bcCurve.N.BitLength; + return orderBits switch + { + 256 => ECCurve.NamedCurves.nistP256, + 384 => ECCurve.NamedCurves.nistP384, + 521 => ECCurve.NamedCurves.nistP521, + _ => throw new InvalidOperationException( + $"Unsupported EC curve with order bit length {orderBits}; " + + "expected P-256, P-384 or P-521."), + }; + } + private static bool UsesInMemoryProvider(string connectionString) { return string.Equals(connectionString, InMemoryProviderName, StringComparison.OrdinalIgnoreCase); diff --git a/src/Yavsc.Org/Yavsc.Org.csproj b/src/Yavsc.Org/Yavsc.Org.csproj index 76be5bff..d5340f84 100644 --- a/src/Yavsc.Org/Yavsc.Org.csproj +++ b/src/Yavsc.Org/Yavsc.Org.csproj @@ -38,6 +38,7 @@ +