diff --git a/src/Dapr/AdminPanel.Host/Dockerfile b/src/Dapr/AdminPanel.Host/Dockerfile index c3381c368..22d8b7b24 100644 --- a/src/Dapr/AdminPanel.Host/Dockerfile +++ b/src/Dapr/AdminPanel.Host/Dockerfile @@ -18,5 +18,8 @@ RUN dotnet publish "MUnique.OpenMU.AdminPanel.Host.csproj" -c Release -o /app/pu FROM base AS final WORKDIR /app COPY --from=publish /app/publish . +# The container runs as a non-root user, but /app belongs to root. The data protection +# key ring, which protects the admin panel sessions, needs a writable directory. +RUN mkdir -p /app/data-protection-keys && chmod 777 /app/data-protection-keys USER $APP_UID ENTRYPOINT ["dotnet", "MUnique.OpenMU.AdminPanel.Host.dll"] diff --git a/src/Persistence/EntityFramework/AdminAuth/AdminUserRepository.cs b/src/Persistence/EntityFramework/AdminAuth/AdminUserRepository.cs index 90470772d..7f190eca6 100644 --- a/src/Persistence/EntityFramework/AdminAuth/AdminUserRepository.cs +++ b/src/Persistence/EntityFramework/AdminAuth/AdminUserRepository.cs @@ -16,9 +16,25 @@ namespace MUnique.OpenMU.Persistence.EntityFramework.AdminAuth; /// public class AdminUserRepository : IAdminUserRepository { + /// + /// The time after which a connection attempt to the database server is given up. + /// + private static readonly TimeSpan ConnectTimeout = TimeSpan.FromSeconds(3); + + /// + /// The time after which the creation of the schema is given up. + /// + private static readonly TimeSpan MigrationTimeout = TimeSpan.FromSeconds(30); + + /// + /// The time to wait before the storage is probed again after a failed attempt. + /// + private static readonly TimeSpan RetryDelay = TimeSpan.FromSeconds(30); + private readonly ILogger _logger; private readonly AsyncLock _storageLock = new(); private bool _isStorageReady; + private DateTime _nextProbeAt = DateTime.MinValue; /// /// Initializes a new instance of the class. @@ -37,17 +53,40 @@ public async ValueTask EnsureStorageAsync(CancellationToken cancellationTo return true; } + // The authorization of every request asks whether a user exists, so a database which is not + // reachable must not be retried on each of them - otherwise the whole panel waits for a + // connection which is going to time out anyway. + if (DateTime.UtcNow < this._nextProbeAt) + { + return false; + } + using var l = await this._storageLock.LockAsync(cancellationToken).ConfigureAwait(false); if (this._isStorageReady) { return true; } + if (DateTime.UtcNow < this._nextProbeAt) + { + return false; + } + try { await using var context = new AdminPanelContext(); - await context.Database.MigrateAsync(cancellationToken).ConfigureAwait(false); - this._isStorageReady = true; + + // Connecting is checked separately and with a short timeout, because the configured + // command timeout of the connection string is far too long to block a request on. + using var connectCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + connectCts.CancelAfter(ConnectTimeout); + if (await context.Database.CanConnectAsync(connectCts.Token).ConfigureAwait(false)) + { + using var migrationCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + migrationCts.CancelAfter(MigrationTimeout); + await context.Database.MigrateAsync(migrationCts.Token).ConfigureAwait(false); + this._isStorageReady = true; + } } catch (Exception ex) { @@ -55,6 +94,13 @@ public async ValueTask EnsureStorageAsync(CancellationToken cancellationTo // The admin panel then falls back to the configured bootstrap user. this._logger.LogInformation(ex, "The admin user storage is not available (yet)."); } + finally + { + if (!this._isStorageReady) + { + this._nextProbeAt = DateTime.UtcNow + RetryDelay; + } + } return this._isStorageReady; } diff --git a/src/Startup/Dockerfile b/src/Startup/Dockerfile index 5f8003493..cbbec880c 100644 --- a/src/Startup/Dockerfile +++ b/src/Startup/Dockerfile @@ -45,7 +45,10 @@ WORKDIR /app COPY --from=publish /app/publish . -RUN mkdir -p /app/logs && chmod 777 /app/logs +# The container runs as a non-root user, but /app belongs to root. Directories the +# application writes to have to be created and made writable beforehand - the data +# protection key ring among them, which protects the admin panel sessions. +RUN mkdir -p /app/logs /app/data-protection-keys && chmod 777 /app/logs /app/data-protection-keys ARG APP_UID=1000 USER ${APP_UID} diff --git a/src/Web/AdminPanel/Auth/AdminPanelAuthExtensions.cs b/src/Web/AdminPanel/Auth/AdminPanelAuthExtensions.cs index c2cf9058e..5ddb6070f 100644 --- a/src/Web/AdminPanel/Auth/AdminPanelAuthExtensions.cs +++ b/src/Web/AdminPanel/Auth/AdminPanelAuthExtensions.cs @@ -16,6 +16,7 @@ namespace MUnique.OpenMU.Web.AdminPanel.Auth; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Logging; using MUnique.OpenMU.Persistence.AdminAuth; /// @@ -58,13 +59,7 @@ public static IServiceCollection AddAdminPanelAuth(this IServiceCollection servi options.BootstrapUser = authOptions.BootstrapUser; }); - // The key ring protects the authentication cookies and the authenticator keys. It has to be - // persisted, otherwise a restart invalidates all sessions and makes all stored authenticator - // keys unreadable. In docker, the directory should be a mounted volume. - var keyPath = configuration["AdminPanel:Auth:DataProtectionKeyPath"] ?? "data-protection-keys"; - services.AddDataProtection() - .SetApplicationName("MUnique.OpenMU.AdminPanel") - .PersistKeysToFileSystem(new DirectoryInfo(Path.Combine(Directory.GetCurrentDirectory(), keyPath))); + services.AddSingleton(ConfigureDataProtection(services, configuration)); // The hosting application registers the real storage; this is just a fallback which lets // the panel start in its initial setup mode instead of failing to resolve its services. @@ -131,6 +126,18 @@ public static IServiceCollection AddAdminPanelAuth(this IServiceCollection servi /// The same instance, to allow chaining of further calls. public static IApplicationBuilder UseAdminPanelAuth(this IApplicationBuilder app) { + if (app.ApplicationServices.GetService() is { Error: { } error } status) + { + app.ApplicationServices.GetRequiredService() + .CreateLogger(typeof(AdminPanelAuthExtensions)) + .LogWarning( + error, + "The data protection keys can't be stored at '{Path}', so they are only kept in memory: " + + "everybody is signed out when the application restarts, and stored authenticator keys " + + "become unreadable. Make sure the directory exists and is writable by the user which runs the application.", + status.Path); + } + app.UseAuthentication(); app.UseAuthorization(); return app; @@ -170,6 +177,45 @@ public static IApplicationBuilder UseAuthorizedPath(this IApplicationBuilder app }); } + /// + /// Sets the storage of the data protection key ring up. + /// + /// The service collection. + /// The configuration. + /// The result, which is logged when the request pipeline is built. + /// + /// The key ring protects the authentication cookies and the authenticator keys, so it has to be + /// persisted - otherwise a restart invalidates all sessions and makes all stored authenticator + /// keys unreadable. In docker, the directory should be a mounted volume. + /// It's deliberately not fatal when the directory can't be used: the containers run as a + /// non-root user, so a directory which wasn't prepared in the image would otherwise make every + /// page of the panel fail with an error as soon as a key is needed. + /// + private static DataProtectionKeyStorageStatus ConfigureDataProtection(IServiceCollection services, IConfiguration configuration) + { + var keyPath = configuration["AdminPanel:Auth:DataProtectionKeyPath"] ?? "data-protection-keys"; + var directory = new DirectoryInfo(Path.Combine(Directory.GetCurrentDirectory(), keyPath)); + var dataProtection = services.AddDataProtection().SetApplicationName("MUnique.OpenMU.AdminPanel"); + + try + { + directory.Create(); + + // Creating an existing directory succeeds even without write access to it, so the + // access is checked explicitly - a mounted volume may belong to another user. + var probeFilePath = Path.Combine(directory.FullName, ".write-probe"); + File.WriteAllBytes(probeFilePath, Array.Empty()); + File.Delete(probeFilePath); + + dataProtection.PersistKeysToFileSystem(directory); + return new DataProtectionKeyStorageStatus(directory.FullName, null); + } + catch (Exception ex) + { + return new DataProtectionKeyStorageStatus(directory.FullName, ex); + } + } + private static void ApplyEnvironmentVariables(AdminPanelAuthOptions options) { var loginName = Environment.GetEnvironmentVariable(BootstrapUserVariableName); diff --git a/src/Web/AdminPanel/Auth/AdminUserAvailabilityService.cs b/src/Web/AdminPanel/Auth/AdminUserAvailabilityService.cs index 873be77be..c93f96d0e 100644 --- a/src/Web/AdminPanel/Auth/AdminUserAvailabilityService.cs +++ b/src/Web/AdminPanel/Auth/AdminUserAvailabilityService.cs @@ -17,6 +17,11 @@ namespace MUnique.OpenMU.Web.AdminPanel.Auth; /// public class AdminUserAvailabilityService { + /// + /// The time for which the answer is reused before the storage is asked again. + /// + private static readonly TimeSpan CheckInterval = TimeSpan.FromSeconds(10); + private readonly IAdminUserRepository _repository; private readonly BootstrapAdminUserProvider _bootstrapUserProvider; private readonly SemaphoreSlim _semaphore = new(1, 1); @@ -40,24 +45,23 @@ public AdminUserAvailabilityService(IAdminUserRepository repository, BootstrapAd /// /// The cancellation token. /// true, if at least one user exists; otherwise, false. + /// + /// This is called by the authorization of every request, so it must never wait for the + /// database: when another caller is already asking, or when the last answer is still fresh, + /// the known value is returned right away. + /// public async ValueTask AnyUserExistsAsync(CancellationToken cancellationToken = default) { - if (this._bootstrapUserProvider.User is not null) - { - return true; - } - - if (this._anyUserExists) + if (this._bootstrapUserProvider.User is not null || this._anyUserExists) { return true; } - if (DateTime.UtcNow < this._nextCheck) + if (DateTime.UtcNow < this._nextCheck || !await this._semaphore.WaitAsync(0, cancellationToken).ConfigureAwait(false)) { - return false; + return this._anyUserExists; } - await this._semaphore.WaitAsync(cancellationToken).ConfigureAwait(false); try { if (this._anyUserExists || DateTime.UtcNow < this._nextCheck) @@ -65,10 +69,15 @@ public async ValueTask AnyUserExistsAsync(CancellationToken cancellationTo return this._anyUserExists; } - this._anyUserExists = await this._repository.GetCountAsync(cancellationToken).ConfigureAwait(false) > 0; + if (await this._repository.EnsureStorageAsync(cancellationToken).ConfigureAwait(false)) + { + this._anyUserExists = await this._repository.GetCountAsync(cancellationToken).ConfigureAwait(false) > 0; + } - // The database might not be reachable yet, so don't hammer it on every render. - this._nextCheck = DateTime.UtcNow.AddSeconds(5); + // When the storage isn't available, we can't tell - the previous answer is kept, which + // is the initial setup mode on a fresh installation. The installation needs the database + // as well, so there is nothing to protect at that point anyway. + this._nextCheck = DateTime.UtcNow.Add(CheckInterval); return this._anyUserExists; } finally diff --git a/src/Web/AdminPanel/Auth/DataProtectionKeyStorageStatus.cs b/src/Web/AdminPanel/Auth/DataProtectionKeyStorageStatus.cs new file mode 100644 index 000000000..fceb3c6f2 --- /dev/null +++ b/src/Web/AdminPanel/Auth/DataProtectionKeyStorageStatus.cs @@ -0,0 +1,18 @@ +// +// Licensed under the MIT License. See LICENSE file in the project root for full license information. +// + +namespace MUnique.OpenMU.Web.AdminPanel.Auth; + +/// +/// The result of setting the storage of the data protection key ring up. +/// +/// The path at which the keys should be stored. +/// The error which prevented the usage of that path; null, if it works. +/// +/// The key ring protects the authentication cookies and the stored authenticator keys. When it +/// can't be persisted, the admin panel still works - but everybody is signed out after a restart +/// and the stored authenticator keys become unreadable. That's worth a warning, but it's not worth +/// taking the whole panel down for, which is what an exception at this point would do. +/// +public record DataProtectionKeyStorageStatus(string Path, Exception? Error); diff --git a/tests/MUnique.OpenMU.Web.Tests/AdminAuth/AdminAuthenticationTests.cs b/tests/MUnique.OpenMU.Web.Tests/AdminAuth/AdminAuthenticationTests.cs index 5f37bdcbf..697911871 100644 --- a/tests/MUnique.OpenMU.Web.Tests/AdminAuth/AdminAuthenticationTests.cs +++ b/tests/MUnique.OpenMU.Web.Tests/AdminAuth/AdminAuthenticationTests.cs @@ -4,8 +4,10 @@ namespace MUnique.OpenMU.Web.Tests.AdminAuth; +using System.IO; using Microsoft.AspNetCore.DataProtection; using Microsoft.AspNetCore.Identity; +using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -290,6 +292,129 @@ public void EffectiveRolesIncludeTheLessPrivilegedOnes() Assert.That(AdminRoles.GetEffectiveRoles("Unknown"), Is.Empty); } + /// + /// Tests that an unreachable storage is not asked again on every authorization check. + /// + /// + /// The authorization of every request asks whether a user exists. When that answer required a + /// database round trip each time, an unreachable database made the whole admin panel wait for + /// connection attempts which were going to time out anyway - it never finished loading. + /// + [Test] + public async Task UnavailableStorageIsNotProbedOnEveryCheckAsync() + { + var repository = new UnavailableAdminUserRepository(); + var service = new AdminUserAvailabilityService( + repository, + this._serviceProvider.GetRequiredService()); + + for (var i = 0; i < 20; i++) + { + Assert.That(await service.AnyUserExistsAsync().ConfigureAwait(false), Is.False); + } + + Assert.That(repository.EnsureStorageCallCount, Is.EqualTo(1)); + } + + /// + /// Tests that an unusable directory for the data protection keys is reported instead of throwing. + /// + /// + /// The docker images run as a non-root user while /app belongs to root, so a directory which + /// wasn't prepared in the image can't be created. That used to surface as an error page on + /// every page of the panel, as soon as a key was needed for an antiforgery token or a cookie. + /// + [Test] + public void UnusableDataProtectionDirectoryIsReportedAndDoesNotThrow() + { + // A file at the place of the directory is a portable way to make its creation fail. + var blockingFilePath = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); + File.WriteAllText(blockingFilePath, string.Empty); + try + { + var provider = BuildAuthServices(blockingFilePath); + var status = provider.GetRequiredService(); + + Assert.That(status.Error, Is.Not.Null); + Assert.That(status.Path, Is.EqualTo(blockingFilePath)); + + // The keys are what an antiforgery token and the authentication cookie are protected + // with, so this is what used to fail on every page of the panel. + var protector = provider.GetRequiredService().CreateProtector("test"); + Assert.That(() => protector.Protect("payload"), Throws.Nothing); + } + finally + { + File.Delete(blockingFilePath); + } + } + + /// + /// Tests that a usable directory for the data protection keys is accepted. + /// + [Test] + public void UsableDataProtectionDirectoryIsAccepted() + { + var keyDirectoryPath = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); + try + { + var status = BuildAuthServices(keyDirectoryPath).GetRequiredService(); + + Assert.That(status.Error, Is.Null); + Assert.That(Directory.Exists(keyDirectoryPath), Is.True); + } + finally + { + if (Directory.Exists(keyDirectoryPath)) + { + Directory.Delete(keyDirectoryPath, true); + } + } + } + + /// + /// Tests that an authorization check doesn't wait for an availability probe which is already running. + /// + /// + /// This is the regression test for an admin panel which never finished loading: the + /// authorization of every request asks whether a user exists, and while the first request was + /// stuck in the connection timeout of an unreachable database, every other request queued up + /// behind it - including the ones which render the page. + /// + [Test] + public async Task AvailabilityCheckDoesNotWaitForARunningProbeAsync() + { + var repository = new BlockingAdminUserRepository(); + var service = new AdminUserAvailabilityService( + repository, + this._serviceProvider.GetRequiredService()); + + var blockedCall = Task.Run(async () => await service.AnyUserExistsAsync().ConfigureAwait(false)); + await repository.ProbeStarted.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false); + + var concurrentCall = service.AnyUserExistsAsync(); + Assert.That(concurrentCall.IsCompleted, Is.True, "A check must not wait for a probe which is already running."); + Assert.That(await concurrentCall.ConfigureAwait(false), Is.False); + + repository.Release(); + Assert.That(await blockedCall.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false), Is.False); + } + + /// + /// Tests that the answer is cached once a user exists, so the database isn't queried again. + /// + [Test] + public async Task ExistingUserIsRememberedAsync() + { + await this.CreateUserAsync("tester").ConfigureAwait(false); + var service = new AdminUserAvailabilityService( + this._repository, + this._serviceProvider.GetRequiredService()); + + Assert.That(await service.AnyUserExistsAsync().ConfigureAwait(false), Is.True); + Assert.That(await service.AnyUserExistsAsync().ConfigureAwait(false), Is.True); + } + /// /// Tests that the role names and the role enum stay in sync, since the roles are stored /// and compared as strings, but selected as an enum in the user interface. @@ -324,6 +449,21 @@ public async Task ClaimsContainTheEffectiveRolesAsync() Is.True); } + private static ServiceProvider BuildAuthServices(string dataProtectionKeyPath) + { + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["AdminPanel:Auth:DataProtectionKeyPath"] = dataProtectionKeyPath, + }) + .Build(); + + var services = new ServiceCollection(); + services.AddLogging(builder => builder.SetMinimumLevel(LogLevel.Warning)); + services.AddAdminPanelAuth(configuration); + return services.BuildServiceProvider(); + } + private AdminLoginService GetLoginService() { return this._serviceProvider.CreateScope().ServiceProvider.GetRequiredService(); diff --git a/tests/MUnique.OpenMU.Web.Tests/AdminAuth/BlockingAdminUserRepository.cs b/tests/MUnique.OpenMU.Web.Tests/AdminAuth/BlockingAdminUserRepository.cs new file mode 100644 index 000000000..af095ed46 --- /dev/null +++ b/tests/MUnique.OpenMU.Web.Tests/AdminAuth/BlockingAdminUserRepository.cs @@ -0,0 +1,60 @@ +// +// Licensed under the MIT License. See LICENSE file in the project root for full license information. +// + +namespace MUnique.OpenMU.Web.Tests.AdminAuth; + +using System.Threading; +using MUnique.OpenMU.Persistence.AdminAuth; + +/// +/// An whose availability check blocks until it's released, +/// like a database server which is not reachable and runs into its connection timeout. +/// +internal class BlockingAdminUserRepository : IAdminUserRepository +{ + private readonly TaskCompletionSource _probeStarted = new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly TaskCompletionSource _release = new(TaskCreationOptions.RunContinuationsAsynchronously); + + /// + /// Gets a task which completes as soon as the availability check has been entered. + /// + public Task ProbeStarted => this._probeStarted.Task; + + /// + /// Lets the blocked availability check continue. + /// + public void Release() => this._release.TrySetResult(); + + /// + public async ValueTask EnsureStorageAsync(CancellationToken cancellationToken = default) + { + this._probeStarted.TrySetResult(); + await this._release.Task.ConfigureAwait(false); + return false; + } + + /// + public ValueTask GetCountAsync(CancellationToken cancellationToken = default) => ValueTask.FromResult(0); + + /// + public ValueTask> GetAllAsync(CancellationToken cancellationToken = default) + => ValueTask.FromResult>(new List()); + + /// + public ValueTask GetByIdAsync(Guid id, CancellationToken cancellationToken = default) + => ValueTask.FromResult(null); + + /// + public ValueTask GetByNormalizedLoginNameAsync(string normalizedLoginName, CancellationToken cancellationToken = default) + => ValueTask.FromResult(null); + + /// + public ValueTask AddAsync(AdminUser user, CancellationToken cancellationToken = default) => throw new InvalidOperationException(); + + /// + public ValueTask UpdateAsync(AdminUser user, CancellationToken cancellationToken = default) => throw new InvalidOperationException(); + + /// + public ValueTask DeleteAsync(AdminUser user, CancellationToken cancellationToken = default) => throw new InvalidOperationException(); +} diff --git a/tests/MUnique.OpenMU.Web.Tests/AdminAuth/UnavailableAdminUserRepository.cs b/tests/MUnique.OpenMU.Web.Tests/AdminAuth/UnavailableAdminUserRepository.cs new file mode 100644 index 000000000..c87682b5c --- /dev/null +++ b/tests/MUnique.OpenMU.Web.Tests/AdminAuth/UnavailableAdminUserRepository.cs @@ -0,0 +1,52 @@ +// +// Licensed under the MIT License. See LICENSE file in the project root for full license information. +// + +namespace MUnique.OpenMU.Web.Tests.AdminAuth; + +using System.Threading; +using MUnique.OpenMU.Persistence.AdminAuth; + +/// +/// An which behaves like an unreachable database and counts +/// how often it was asked for its availability. +/// +internal class UnavailableAdminUserRepository : IAdminUserRepository +{ + /// + /// Gets the number of calls to . + /// + public int EnsureStorageCallCount { get; private set; } + + /// + public ValueTask EnsureStorageAsync(CancellationToken cancellationToken = default) + { + this.EnsureStorageCallCount++; + return ValueTask.FromResult(false); + } + + /// + public ValueTask GetCountAsync(CancellationToken cancellationToken = default) + => throw new InvalidOperationException("The storage is not available, so it must not be queried."); + + /// + public ValueTask> GetAllAsync(CancellationToken cancellationToken = default) + => ValueTask.FromResult>(new List()); + + /// + public ValueTask GetByIdAsync(Guid id, CancellationToken cancellationToken = default) + => ValueTask.FromResult(null); + + /// + public ValueTask GetByNormalizedLoginNameAsync(string normalizedLoginName, CancellationToken cancellationToken = default) + => ValueTask.FromResult(null); + + /// + public ValueTask AddAsync(AdminUser user, CancellationToken cancellationToken = default) => throw new InvalidOperationException(); + + /// + public ValueTask UpdateAsync(AdminUser user, CancellationToken cancellationToken = default) => throw new InvalidOperationException(); + + /// + public ValueTask DeleteAsync(AdminUser user, CancellationToken cancellationToken = default) => throw new InvalidOperationException(); +}