diff --git a/docker-compose.yml b/docker-compose.yml index d4d80ea..887e6a7 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -4,10 +4,13 @@ services: context: . dockerfile: Dockerfile ports: - - "8888:5000" + - "127.0.0.1:8888:5000" environment: ASPNETCORE_ENVIRONMENT: ${ASPNETCORE_ENVIRONMENT:-Development} ASPNETCORE_URLS: ${ASPNETCORE_URLS:-http://+:5000} + AUTH_MODE: ${AUTH_MODE:-demo} + SECUREFIX_DEMO_TOKEN: ${SECUREFIX_DEMO_TOKEN:-} + SECUREFIX_DEMO_REVIEWER_TOKEN: ${SECUREFIX_DEMO_REVIEWER_TOKEN:-} DATABASE_URL: ${DATABASE_URL:-sqlite:securefix.db} DATABASE_PROVIDER: ${DATABASE_PROVIDER:-sqlite} AI_PROVIDER: ${AI_PROVIDER:-mock} diff --git a/docs/runbook.md b/docs/runbook.md index d2bcd87..4a60720 100644 --- a/docs/runbook.md +++ b/docs/runbook.md @@ -21,8 +21,10 @@ - **Rotate/verify configuration**: confirm `AUTH_MODE=entra` and `AzureAd__TenantId`, `AzureAd__ClientId`, `AzureAd__Audience` are set in the deployment environment. The API fails to start in `Production` if `AUTH_MODE` is not `entra` (fail closed). -- **Local/dev only**: `AUTH_MODE=demo` uses a static bearer token (`SECUREFIX_DEMO_TOKEN`) - and a self-asserted `X-User-Role` header — never use this mode outside local development. +- **Local/dev only**: `AUTH_MODE=demo` uses `SECUREFIX_DEMO_TOKEN` for the fixed Developer + identity and, optionally, `SECUREFIX_DEMO_REVIEWER_TOKEN` for the fixed SecurityReviewer + identity. Set distinct, non-empty, high-entropy values; role and identity headers are ignored. + The Compose port is bound to `127.0.0.1`. Never use this mode outside local development. ## Failure handling diff --git a/docs/security-model.md b/docs/security-model.md index e3058a0..db03605 100644 --- a/docs/security-model.md +++ b/docs/security-model.md @@ -21,9 +21,12 @@ The API supports two authentication modes, selected via `AUTH_MODE`: using Microsoft.Identity.Web. Tokens must have the correct issuer, audience, signature, and expiration. Roles are asserted by Entra ID in the token's `roles` claim, which Microsoft.Identity.Web maps to `ClaimTypes.Role` for use by `[Authorize(Roles = ...)]`. -- `demo` (local/dev only): a static bearer token plus a self-asserted `X-User-Role` header. - This mode is intentionally weak — it exists only for offline demos — and the API fails - to start if `ASPNETCORE_ENVIRONMENT=Production` and `AUTH_MODE` is not `entra` (fail closed). +- `demo` (local/dev only): explicitly configured `SECUREFIX_DEMO_TOKEN` and optional + `SECUREFIX_DEMO_REVIEWER_TOKEN` bearer tokens map to fixed Developer and SecurityReviewer + identities. Client-supplied identity and role headers are ignored. Configure distinct, + non-empty, high-entropy tokens for local testing; requests are unauthenticated when a token + is unset. Compose publishes only on loopback by default. The API fails to start if + `ASPNETCORE_ENVIRONMENT=Production` and `AUTH_MODE` is not `entra` (fail closed). App roles are defined once on the Entra ID app registration (see `infra/entra-app-registration.sh`) and assigned to users/groups from the Enterprise @@ -40,4 +43,3 @@ Application's "Users and groups" blade — never granted by the application itse Secrets must come from environment variables or managed secret storage in production. No credentials are committed to source control. - diff --git a/src/SecureFix.Api/AuthenticationConfigurator.cs b/src/SecureFix.Api/AuthenticationConfigurator.cs index 01355a4..f341863 100644 --- a/src/SecureFix.Api/AuthenticationConfigurator.cs +++ b/src/SecureFix.Api/AuthenticationConfigurator.cs @@ -10,8 +10,8 @@ namespace SecureFix.Api; /// - "entra" (recommended, required in Production): validates Microsoft Entra ID issued /// JWTs via Microsoft.Identity.Web. App roles (Admin, SecurityReviewer, Developer, Viewer) /// are read from the token's "roles" claim and mapped to ClaimTypes.Role automatically. -/// - "demo" (local/dev only): uses a static bearer token + X-User-Role header. This mode -/// is intentionally weak and must never be enabled in Production. +/// - "demo" (local/dev only): uses explicitly configured developer and reviewer bearer +/// tokens with fixed identities and roles. This mode must never be enabled in Production. /// public static class AuthenticationConfigurator { diff --git a/src/SecureFix.Api/Controllers/WorkflowsController.cs b/src/SecureFix.Api/Controllers/WorkflowsController.cs index a10ecd7..ace4fec 100644 --- a/src/SecureFix.Api/Controllers/WorkflowsController.cs +++ b/src/SecureFix.Api/Controllers/WorkflowsController.cs @@ -88,7 +88,7 @@ public async Task GetWorkflowStatus(string id) /// Only SecurityReviewer role is authorized. /// /// Alert/Workflow ID. - /// Approval details (reviewer, optional reason). + /// Approval decision and optional reason. /// Updated workflow status. /// Workflow approved successfully. /// Invalid request (validation failed, already decided, workflow not found). @@ -126,22 +126,16 @@ public async Task ApproveWorkflow( }); } - var reviewerRole = User.FindFirstValue(ClaimTypes.Role) ?? request.ReviewerRole ?? "SecurityReviewer"; - if (string.IsNullOrWhiteSpace(request.Reviewer)) + if (!TryGetAuthenticatedReviewer(out var reviewer, out var reviewerRole)) { - return BadRequest(new ProblemDetails - { - Title = "Invalid Reviewer", - Status = StatusCodes.Status400BadRequest, - Detail = "Reviewer identity is required." - }); + return Forbid(); } try { - _logger.LogInformation("Approving workflow {WorkflowId} by {Reviewer} as {Role}", id, request.Reviewer, reviewerRole); + _logger.LogInformation("Approving workflow {WorkflowId} by {Reviewer} as {Role}", id, reviewer, reviewerRole); - var updatedStatus = await _approvalService.ApproveAlertAsync(id, request.Reviewer, request.Reason, reviewerRole); + var updatedStatus = await _approvalService.ApproveAlertAsync(id, reviewer, request.Reason, reviewerRole); return Ok(updatedStatus); } @@ -203,7 +197,7 @@ public async Task ApproveWorkflow( /// Only SecurityReviewer role is authorized. /// /// Alert/Workflow ID. - /// Rejection details (reviewer, optional reason). + /// Rejection decision and optional reason. /// Updated workflow status (Rejected). /// Workflow rejected successfully. /// Invalid request (validation failed, already decided, workflow not found). @@ -241,22 +235,16 @@ public async Task RejectWorkflow( }); } - var reviewerRole = User.FindFirstValue(ClaimTypes.Role) ?? request.ReviewerRole ?? "SecurityReviewer"; - if (string.IsNullOrWhiteSpace(request.Reviewer)) + if (!TryGetAuthenticatedReviewer(out var reviewer, out var reviewerRole)) { - return BadRequest(new ProblemDetails - { - Title = "Invalid Reviewer", - Status = StatusCodes.Status400BadRequest, - Detail = "Reviewer identity is required." - }); + return Forbid(); } try { - _logger.LogInformation("Rejecting workflow {WorkflowId} by {Reviewer} as {Role}", id, request.Reviewer, reviewerRole); + _logger.LogInformation("Rejecting workflow {WorkflowId} by {Reviewer} as {Role}", id, reviewer, reviewerRole); - var updatedStatus = await _approvalService.RejectAlertAsync(id, request.Reviewer, request.Reason, reviewerRole); + var updatedStatus = await _approvalService.RejectAlertAsync(id, reviewer, request.Reason, reviewerRole); return Ok(updatedStatus); } @@ -312,4 +300,13 @@ public async Task RejectWorkflow( }); } } + + private bool TryGetAuthenticatedReviewer(out string reviewer, out string reviewerRole) + { + reviewer = User.FindFirstValue(ClaimTypes.NameIdentifier) ?? User.Identity?.Name ?? string.Empty; + reviewerRole = User.FindFirstValue(ClaimTypes.Role) ?? string.Empty; + + return !string.IsNullOrWhiteSpace(reviewer) + && (reviewerRole is "SecurityReviewer" or "Admin"); + } } diff --git a/src/SecureFix.Api/DemoAuthenticationHandler.cs b/src/SecureFix.Api/DemoAuthenticationHandler.cs index 55e7c2a..5d69759 100644 --- a/src/SecureFix.Api/DemoAuthenticationHandler.cs +++ b/src/SecureFix.Api/DemoAuthenticationHandler.cs @@ -26,14 +26,20 @@ protected override Task HandleAuthenticateAsync() } var token = authorization["Bearer ".Length..].Trim(); - var expectedToken = Environment.GetEnvironmentVariable("SECUREFIX_DEMO_TOKEN") ?? "securefix-demo-token"; - if (!string.Equals(token, expectedToken, StringComparison.Ordinal)) + var developerToken = Environment.GetEnvironmentVariable("SECUREFIX_DEMO_TOKEN"); + var reviewerToken = Environment.GetEnvironmentVariable("SECUREFIX_DEMO_REVIEWER_TOKEN"); + var isDeveloper = !string.IsNullOrWhiteSpace(developerToken) + && string.Equals(token, developerToken, StringComparison.Ordinal); + var isReviewer = !string.IsNullOrWhiteSpace(reviewerToken) + && string.Equals(token, reviewerToken, StringComparison.Ordinal); + + if ((!isDeveloper && !isReviewer) || (isDeveloper && isReviewer)) { - return Task.FromResult(AuthenticateResult.Fail("Invalid demo token.")); + return Task.FromResult(AuthenticateResult.Fail("Invalid or misconfigured demo credentials.")); } - var userId = Request.Headers["X-User-Id"].FirstOrDefault() ?? "demo-user"; - var role = Request.Headers["X-User-Role"].FirstOrDefault() ?? "Developer"; + var userId = isReviewer ? "demo-security-reviewer" : "demo-developer"; + var role = isReviewer ? "SecurityReviewer" : "Developer"; var claims = new[] { diff --git a/src/SecureFix.Api/SecureFix.Api.csproj b/src/SecureFix.Api/SecureFix.Api.csproj index a99c0b6..1a13467 100644 --- a/src/SecureFix.Api/SecureFix.Api.csproj +++ b/src/SecureFix.Api/SecureFix.Api.csproj @@ -9,7 +9,7 @@ - + diff --git a/src/SecureFix.Core/Models/ApprovalRequestDto.cs b/src/SecureFix.Core/Models/ApprovalRequestDto.cs index 606acd8..125b1d9 100644 --- a/src/SecureFix.Core/Models/ApprovalRequestDto.cs +++ b/src/SecureFix.Core/Models/ApprovalRequestDto.cs @@ -8,15 +8,13 @@ namespace SecureFix.Core.Models; public class ApprovalRequestDto { /// - /// Reviewer identity (user ID or email). + /// Legacy client field; ignored. Reviewer identity comes from authenticated claims. /// - [Required] [StringLength(255)] - public string Reviewer { get; set; } = null!; + public string? Reviewer { get; set; } /// - /// Role of the reviewer submitting the decision. - /// Only SecurityReviewer and Admin are allowed to approve or reject workflow actions. + /// Legacy client field; ignored. Reviewer role comes from authenticated claims. /// [StringLength(100)] public string? ReviewerRole { get; set; } diff --git a/tests/SecureFix.Tests/WorkflowsControllerSecurityTests.cs b/tests/SecureFix.Tests/WorkflowsControllerSecurityTests.cs new file mode 100644 index 0000000..0d524c1 --- /dev/null +++ b/tests/SecureFix.Tests/WorkflowsControllerSecurityTests.cs @@ -0,0 +1,129 @@ +namespace SecureFix.Tests; + +using System.Security.Claims; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using SecureFix.Api.Controllers; +using SecureFix.Core.Models; +using SecureFix.Core.Services; + +public class WorkflowsControllerSecurityTests +{ + [Fact] + public async Task ApproveWorkflow_UsesAuthenticatedReviewerInsteadOfRequestIdentity() + { + var response = new WorkflowStatusResponse(); + var approvalService = new Mock(); + approvalService + .Setup(service => service.ApproveAlertAsync( + "workflow-1", + "authenticated-reviewer", + "reviewed", + "SecurityReviewer")) + .ReturnsAsync(response); + var controller = CreateController(approvalService, "authenticated-reviewer", "SecurityReviewer"); + + var result = await controller.ApproveWorkflow("workflow-1", new ApprovalRequestDto + { + Decision = "approved", + Reviewer = "forged-reviewer", + ReviewerRole = "Admin", + Reason = "reviewed" + }); + + Assert.Same(response, Assert.IsType(result).Value); + approvalService.VerifyAll(); + } + + [Fact] + public async Task RejectWorkflow_UsesAuthenticatedReviewerInsteadOfRequestIdentity() + { + var response = new WorkflowStatusResponse(); + var approvalService = new Mock(); + approvalService + .Setup(service => service.RejectAlertAsync( + "workflow-1", + "authenticated-reviewer", + "needs more testing", + "SecurityReviewer")) + .ReturnsAsync(response); + var controller = CreateController(approvalService, "authenticated-reviewer", "SecurityReviewer"); + + var result = await controller.RejectWorkflow("workflow-1", new ApprovalRequestDto + { + Decision = "rejected", + Reviewer = "forged-reviewer", + ReviewerRole = "Admin", + Reason = "needs more testing" + }); + + Assert.Same(response, Assert.IsType(result).Value); + approvalService.VerifyAll(); + } + + [Fact] + public async Task ApproveWorkflow_RejectsCallerWithoutAuthenticatedReviewerIdentity() + { + var approvalService = new Mock(); + var controller = CreateController(approvalService, null, "SecurityReviewer"); + + var result = await controller.ApproveWorkflow("workflow-1", new ApprovalRequestDto + { + Decision = "approved", + Reviewer = "forged-reviewer", + ReviewerRole = "SecurityReviewer" + }); + + Assert.IsType(result); + approvalService.VerifyNoOtherCalls(); + } + + [Fact] + public async Task ApproveWorkflow_RejectsDeveloperDespiteReviewerRoleInRequest() + { + var approvalService = new Mock(); + var controller = CreateController(approvalService, "authenticated-developer", "Developer"); + + var result = await controller.ApproveWorkflow("workflow-1", new ApprovalRequestDto + { + Decision = "approved", + Reviewer = "authenticated-developer", + ReviewerRole = "SecurityReviewer" + }); + + Assert.IsType(result); + approvalService.VerifyNoOtherCalls(); + } + + private static WorkflowsController CreateController( + Mock approvalService, + string? reviewerIdentity, + string role) + { + var claims = new List + { + new(ClaimTypes.Role, role) + }; + if (reviewerIdentity is not null) + { + claims.Add(new Claim(ClaimTypes.NameIdentifier, reviewerIdentity)); + } + + var controller = new WorkflowsController( + approvalService.Object, + NullLogger.Instance) + { + ControllerContext = new ControllerContext + { + HttpContext = new DefaultHttpContext + { + User = new ClaimsPrincipal(new ClaimsIdentity(claims, "test")) + } + } + }; + + return controller; + } +}