Skip to content

Commit f21a3e2

Browse files
debdevopsdebdevops
authored
Merge pull request #138 from debdevops/fix/deep-dive-critical-issues
feat: implement tenant isolation for anomalies and namespaces, enhanc…
2 parents 6e9c81f + ce86a64 commit f21a3e2

9 files changed

Lines changed: 388 additions & 4 deletions

File tree

‎services/api/src/ServiceHub.Api/Controllers/V1/AnomaliesController.cs‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,24 @@ public async Task<ActionResult<AnomalyInfo>> GetById(
144144
return ToActionResult<AnomalyInfo>(result.Error);
145145
}
146146

147+
// TENANT ISOLATION: an anomaly is only visible to the owner of the namespace
148+
// it was detected in. Return 404 (not 403) on mismatch to avoid leaking that
149+
// the anomaly ID exists.
150+
var namespaceResult = await _namespaceRepository.GetByIdAsync(result.Value.NamespaceId, cancellationToken);
151+
if (namespaceResult.IsFailure
152+
&& namespaceResult.Error.Type != ServiceHub.Shared.Results.ErrorType.NotFound)
153+
{
154+
return ToActionResult<AnomalyInfo>(namespaceResult.Error);
155+
}
156+
157+
if (namespaceResult.IsFailure
158+
|| !string.Equals(namespaceResult.Value.OwnerId, OwnerId, StringComparison.Ordinal))
159+
{
160+
return ToActionResult<AnomalyInfo>(ServiceHub.Shared.Results.Error.NotFound(
161+
"Anomaly.NotFound",
162+
$"Anomaly with ID '{id}' was not found."));
163+
}
164+
147165
return Ok(MapToAnomalyInfo(result.Value));
148166
}
149167

‎services/api/src/ServiceHub.Api/Controllers/V1/CloudBridgeController.cs‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22
using ServiceHub.Api.Authorization;
33
using ServiceHub.Core.Enums;
44
using ServiceHub.Core.Interfaces;
5+
using ServiceHub.Shared.Constants;
6+
using ServiceHub.Shared.Results;
57

68
namespace ServiceHub.Api.Controllers.V1;
79

@@ -15,16 +17,19 @@ namespace ServiceHub.Api.Controllers.V1;
1517
public sealed class CloudBridgeController : ApiControllerBase
1618
{
1719
private readonly IEnumerable<ICloudMessagingProvider> _providers;
20+
private readonly INamespaceRepository _namespaceRepository;
1821
private readonly ILogger<CloudBridgeController> _logger;
1922

2023
/// <summary>
2124
/// Initialises a new <see cref="CloudBridgeController"/>.
2225
/// </summary>
2326
public CloudBridgeController(
2427
IEnumerable<ICloudMessagingProvider> providers,
28+
INamespaceRepository namespaceRepository,
2529
ILogger<CloudBridgeController> logger)
2630
{
2731
_providers = providers;
32+
_namespaceRepository = namespaceRepository;
2833
_logger = logger;
2934
}
3035

@@ -86,6 +91,9 @@ public async Task<IActionResult> ListEntities(
8691
var resolved = ResolveProvider(provider, out var error);
8792
if (resolved is null) return error!;
8893

94+
var ownershipError = await VerifyNamespaceOwnershipAsync(namespaceId, ct).ConfigureAwait(false);
95+
if (ownershipError is not null) return ownershipError;
96+
8997
var result = await resolved.ListEntitiesAsync(namespaceId, ct).ConfigureAwait(false);
9098
if (!result.IsSuccess)
9199
{
@@ -133,6 +141,9 @@ public async Task<IActionResult> GetVisibilityStatus(
133141
var resolved = ResolveProvider(provider, out var error);
134142
if (resolved is null) return error!;
135143

144+
var ownershipError = await VerifyNamespaceOwnershipAsync(namespaceId, ct).ConfigureAwait(false);
145+
if (ownershipError is not null) return ownershipError;
146+
136147
if (resolved.ProviderType == CloudProviderType.Aws)
137148
{
138149
if (resolved.GetMessageReceiver() is not IVisibilityStatusProvider awsReceiver)
@@ -178,6 +189,27 @@ public async Task<IActionResult> GetVisibilityStatus(
178189
// Helpers
179190
// -------------------------------------------------------------------------
180191

192+
// TENANT ISOLATION: the provider layer resolves connections by namespace ID alone,
193+
// so ownership must be enforced here. Returns 404 (not 403) on mismatch to avoid
194+
// leaking that the ID is in use — same convention as NamespacesController.
195+
private async Task<IActionResult?> VerifyNamespaceOwnershipAsync(Guid namespaceId, CancellationToken ct)
196+
{
197+
var namespaceResult = await _namespaceRepository.GetByIdAsync(namespaceId, ct).ConfigureAwait(false);
198+
if (namespaceResult.IsFailure)
199+
{
200+
return ToActionResult(Result.Failure(namespaceResult.Error));
201+
}
202+
203+
if (!string.Equals(namespaceResult.Value.OwnerId, OwnerId, StringComparison.Ordinal))
204+
{
205+
return ToActionResult(Result.Failure(Error.NotFound(
206+
ErrorCodes.Namespace.NotFound,
207+
$"Namespace with ID '{namespaceId}' was not found.")));
208+
}
209+
210+
return null;
211+
}
212+
181213
private static string SanitizeForLog(string? value)
182214
=> (value ?? string.Empty)
183215
.Replace("\r", string.Empty, StringComparison.Ordinal)

‎services/api/src/ServiceHub.Api/Middleware/EasyAuthMiddleware.cs‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,32 @@ public EasyAuthMiddleware(
4343
// and passes through without setting OwnerId (allowing legacy SPA token path).
4444
// Defaults to enabled to preserve current behavior when setting is absent.
4545
var easyAuthEnabledSetting = configuration["Security:EasyAuth:Enabled"];
46-
_enabled = !bool.TryParse(easyAuthEnabledSetting, out var enabled) || enabled;
46+
var configEnabled = !bool.TryParse(easyAuthEnabledSetting, out var enabled) || enabled;
47+
48+
// SECURITY: X-MS-CLIENT-PRINCIPAL-ID is only unforgeable when Azure's
49+
// authentication layer sits in front of the app and strips the header from
50+
// inbound traffic. On any other host (AWS ECS, GCP Cloud Run, bare Kestrel)
51+
// a client can send the header directly and mint an arbitrary identity.
52+
// Trust it only when Azure Easy Auth is provably active for this worker
53+
// (App Service sets WEBSITE_AUTH_ENABLED=True), or when the operator
54+
// explicitly asserts their front end strips and injects the header.
55+
var behindAzureEasyAuth = string.Equals(
56+
Environment.GetEnvironmentVariable("WEBSITE_AUTH_ENABLED"),
57+
"True",
58+
StringComparison.OrdinalIgnoreCase);
59+
var explicitHeaderTrust = configuration.GetValue(
60+
"Security:EasyAuth:TrustClientPrincipalHeader", false);
61+
62+
_enabled = configEnabled && (behindAzureEasyAuth || explicitHeaderTrust);
63+
64+
if (configEnabled && !_enabled)
65+
{
66+
_logger.LogInformation(
67+
"EasyAuth is enabled in configuration but Azure Easy Auth is not detected " +
68+
"(WEBSITE_AUTH_ENABLED is not set to 'True'). The X-MS-CLIENT-PRINCIPAL-ID header will be " +
69+
"ignored to prevent identity spoofing. If a trusted proxy strips and injects this " +
70+
"header on your platform, set Security:EasyAuth:TrustClientPrincipalHeader=true.");
71+
}
4772
}
4873

4974
/// <summary>

‎services/api/src/ServiceHub.Api/Middleware/RateLimitingMiddleware.cs‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,16 @@ public sealed class RateLimitingMiddleware
1515
// CRITICAL FIX: Bound dictionary size to prevent DoS via memory exhaustion
1616
private const int MaxTrackedClients = 10_000;
1717

18+
// Full-dictionary expiry sweeps are O(n); running one per request would make every
19+
// request pay for the whole client table. Sweep at most once per interval instead.
20+
private static readonly TimeSpan CleanupInterval = TimeSpan.FromSeconds(30);
21+
1822
private readonly RequestDelegate _next;
1923
private readonly ILogger<RateLimitingMiddleware> _logger;
2024
private readonly RateLimitOptions _options;
2125
private readonly HttpHeadersOptions _headersOptions;
2226
private readonly ConcurrentDictionary<string, RateLimitEntry> _clients = new();
27+
private long _lastCleanupTicks;
2328

2429
/// <summary>
2530
/// Initializes a new instance of the <see cref="RateLimitingMiddleware"/> class.
@@ -143,6 +148,18 @@ private static string GetClientIdentifier(HttpContext context)
143148

144149
private void CleanupExpiredEntries(DateTime now)
145150
{
151+
var lastCleanup = Interlocked.Read(ref _lastCleanupTicks);
152+
if (now.Ticks - lastCleanup < CleanupInterval.Ticks)
153+
{
154+
return;
155+
}
156+
157+
// Only one request per interval performs the sweep; losers of the race skip it.
158+
if (Interlocked.CompareExchange(ref _lastCleanupTicks, now.Ticks, lastCleanup) != lastCleanup)
159+
{
160+
return;
161+
}
162+
146163
var expiredKeys = _clients
147164
.Where(kvp => now - kvp.Value.WindowStart > _options.WindowDuration * 2)
148165
.Select(kvp => kvp.Key)

‎services/api/src/ServiceHub.Api/appsettings.Production.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,8 @@
5858
},
5959
"EasyAuth": {
6060
"Enabled": true,
61-
"Description": "Authentication via your hosting provider's identity settings (e.g. Azure Easy Auth, AWS Cognito, or similar). Configure via your hosting environment."
61+
"TrustClientPrincipalHeader": false,
62+
"Description": "Azure App Service Easy Auth. X-MS-CLIENT-PRINCIPAL-ID is only trusted when Azure Easy Auth is detected on the worker (WEBSITE_AUTH_ENABLED=True). On non-Azure hosts the header is ignored unless TrustClientPrincipalHeader is explicitly true AND your front end strips and injects it."
6263
},
6364
"Headers": {
6465
"Enabled": true

‎services/api/tests/ServiceHub.UnitTests/Api/Controllers/V1/AnomaliesControllerTests.cs‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,8 @@ public async Task GetById_Success_ShouldReturnOk()
189189

190190
_aiServiceClient.Setup(a => a.GetAnomalyByIdAsync(anomaly.Id, It.IsAny<CancellationToken>()))
191191
.ReturnsAsync(Result<Anomaly>.Success(anomaly));
192+
_namespaceRepository.Setup(r => r.GetByIdAsync(anomaly.NamespaceId, It.IsAny<CancellationToken>()))
193+
.ReturnsAsync(Result<Namespace>.Success(CreateTestNamespace()));
192194

193195
var result = await _controller.GetById(anomaly.Id);
194196

@@ -197,6 +199,72 @@ public async Task GetById_Success_ShouldReturnOk()
197199
response.Id.Should().Be(anomaly.Id);
198200
}
199201

202+
[Fact]
203+
public async Task GetById_NamespaceOwnedByAnotherTenant_ShouldReturnNotFound()
204+
{
205+
var anomaly = Anomaly.Create(
206+
Guid.NewGuid(),
207+
"test-queue",
208+
AnomalyType.HighMessageVolume,
209+
50,
210+
"Message volume anomaly");
211+
212+
var foreignNamespace = Namespace.Create(
213+
"other-namespace",
214+
"Endpoint=sb://other.servicebus.windows.net/;SharedAccessKeyName=RootManageSharedAccessKey;SharedAccessKey=testkey123456789=",
215+
ownerId: "key_othertenant").Value;
216+
217+
_aiServiceClient.Setup(a => a.GetAnomalyByIdAsync(anomaly.Id, It.IsAny<CancellationToken>()))
218+
.ReturnsAsync(Result<Anomaly>.Success(anomaly));
219+
_namespaceRepository.Setup(r => r.GetByIdAsync(anomaly.NamespaceId, It.IsAny<CancellationToken>()))
220+
.ReturnsAsync(Result<Namespace>.Success(foreignNamespace));
221+
222+
var result = await _controller.GetById(anomaly.Id);
223+
224+
result.Result.Should().BeOfType<NotFoundObjectResult>();
225+
}
226+
227+
[Fact]
228+
public async Task GetById_NamespaceNotFound_ShouldReturnNotFound()
229+
{
230+
var anomaly = Anomaly.Create(
231+
Guid.NewGuid(),
232+
"test-queue",
233+
AnomalyType.HighMessageVolume,
234+
50,
235+
"Message volume anomaly");
236+
237+
_aiServiceClient.Setup(a => a.GetAnomalyByIdAsync(anomaly.Id, It.IsAny<CancellationToken>()))
238+
.ReturnsAsync(Result<Anomaly>.Success(anomaly));
239+
_namespaceRepository.Setup(r => r.GetByIdAsync(anomaly.NamespaceId, It.IsAny<CancellationToken>()))
240+
.ReturnsAsync(Result<Namespace>.Failure(Error.NotFound("NOT_FOUND", "Namespace not found")));
241+
242+
var result = await _controller.GetById(anomaly.Id);
243+
244+
result.Result.Should().BeOfType<NotFoundObjectResult>();
245+
}
246+
247+
[Fact]
248+
public async Task GetById_NamespaceLookupFails_ShouldPropagateError()
249+
{
250+
var anomaly = Anomaly.Create(
251+
Guid.NewGuid(),
252+
"test-queue",
253+
AnomalyType.HighMessageVolume,
254+
50,
255+
"Message volume anomaly");
256+
257+
_aiServiceClient.Setup(a => a.GetAnomalyByIdAsync(anomaly.Id, It.IsAny<CancellationToken>()))
258+
.ReturnsAsync(Result<Anomaly>.Success(anomaly));
259+
_namespaceRepository.Setup(r => r.GetByIdAsync(anomaly.NamespaceId, It.IsAny<CancellationToken>()))
260+
.ReturnsAsync(Result<Namespace>.Failure(Error.Internal("DB_ERR", "Database unavailable")));
261+
262+
var result = await _controller.GetById(anomaly.Id);
263+
264+
var objectResult = result.Result.Should().BeOfType<ObjectResult>().Subject;
265+
objectResult.StatusCode.Should().Be(StatusCodes.Status500InternalServerError);
266+
}
267+
200268
[Fact]
201269
public async Task GetById_NotFound_ShouldReturnNotFound()
202270
{

‎services/api/tests/ServiceHub.UnitTests/Api/Controllers/V1/CloudBridgeControllerTests.cs‎

Lines changed: 82 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using Microsoft.Extensions.Logging;
55
using Moq;
66
using ServiceHub.Api.Controllers.V1;
7+
using ServiceHub.Core.Entities;
78
using ServiceHub.Core.Enums;
89
using ServiceHub.Core.Interfaces;
910
using ServiceHub.Core.Models;
@@ -14,12 +15,19 @@ namespace ServiceHub.UnitTests.Api.Controllers.V1;
1415
public sealed class CloudBridgeControllerTests
1516
{
1617
private readonly Mock<ILogger<CloudBridgeController>> _loggerMock = new();
18+
private readonly Mock<INamespaceRepository> _namespaceRepositoryMock = new();
1719
private readonly List<ICloudMessagingProvider> _providers = new();
1820
private readonly CloudBridgeController _controller;
1921

2022
public CloudBridgeControllerTests()
2123
{
22-
_controller = new CloudBridgeController(_providers, _loggerMock.Object)
24+
// Default: any namespace lookup succeeds and belongs to the SPA owner
25+
// (the OwnerId ApiControllerBase falls back to when none is set).
26+
_namespaceRepositoryMock
27+
.Setup(r => r.GetByIdAsync(It.IsAny<Guid>(), It.IsAny<CancellationToken>()))
28+
.ReturnsAsync((Guid _, CancellationToken _) => Result<Namespace>.Success(CreateNamespace(Namespace.SpaOwnerId)));
29+
30+
_controller = new CloudBridgeController(_providers, _namespaceRepositoryMock.Object, _loggerMock.Object)
2331
{
2432
ControllerContext = new ControllerContext
2533
{
@@ -28,6 +36,15 @@ public CloudBridgeControllerTests()
2836
};
2937
}
3038

39+
private static Namespace CreateNamespace(string ownerId)
40+
{
41+
return Namespace.Create(
42+
"aws-queue",
43+
"https://sqs.us-east-1.amazonaws.com/123456789012/my-queue",
44+
provider: CloudProviderType.Aws,
45+
ownerId: ownerId).Value;
46+
}
47+
3148
[Fact]
3249
public void GetProviderStatus_ReturnsCorrectStatus()
3350
{
@@ -148,6 +165,70 @@ public async Task ListEntities_Success_ReturnsOk()
148165
returned.Should().HaveCount(1);
149166
}
150167

168+
[Fact]
169+
public async Task ListEntities_NamespaceOwnedByAnotherTenant_ReturnsNotFound()
170+
{
171+
// Arrange
172+
var nsId = Guid.NewGuid();
173+
var awsProvider = new Mock<ICloudMessagingProvider>();
174+
awsProvider.SetupGet(p => p.ProviderType).Returns(CloudProviderType.Aws);
175+
_providers.Add(awsProvider.Object);
176+
177+
_namespaceRepositoryMock
178+
.Setup(r => r.GetByIdAsync(nsId, It.IsAny<CancellationToken>()))
179+
.ReturnsAsync(Result<Namespace>.Success(CreateNamespace("key_othertenant")));
180+
181+
// Act
182+
var result = await _controller.ListEntities(nsId, "Aws", CancellationToken.None);
183+
184+
// Assert
185+
result.Should().BeOfType<NotFoundObjectResult>();
186+
awsProvider.Verify(
187+
p => p.ListEntitiesAsync(It.IsAny<Guid>(), It.IsAny<CancellationToken>()),
188+
Times.Never);
189+
}
190+
191+
[Fact]
192+
public async Task ListEntities_NamespaceNotFound_ReturnsNotFound()
193+
{
194+
// Arrange
195+
var nsId = Guid.NewGuid();
196+
var awsProvider = new Mock<ICloudMessagingProvider>();
197+
awsProvider.SetupGet(p => p.ProviderType).Returns(CloudProviderType.Aws);
198+
_providers.Add(awsProvider.Object);
199+
200+
_namespaceRepositoryMock
201+
.Setup(r => r.GetByIdAsync(nsId, It.IsAny<CancellationToken>()))
202+
.ReturnsAsync(Result<Namespace>.Failure(Error.NotFound("Namespace.NotFound", "Not found.")));
203+
204+
// Act
205+
var result = await _controller.ListEntities(nsId, "Aws", CancellationToken.None);
206+
207+
// Assert
208+
result.Should().BeOfType<NotFoundObjectResult>();
209+
}
210+
211+
[Fact]
212+
public async Task GetVisibilityStatus_NamespaceOwnedByAnotherTenant_ReturnsNotFound()
213+
{
214+
// Arrange
215+
var nsId = Guid.NewGuid();
216+
var awsProvider = new Mock<ICloudMessagingProvider>();
217+
awsProvider.SetupGet(p => p.ProviderType).Returns(CloudProviderType.Aws);
218+
_providers.Add(awsProvider.Object);
219+
220+
_namespaceRepositoryMock
221+
.Setup(r => r.GetByIdAsync(nsId, It.IsAny<CancellationToken>()))
222+
.ReturnsAsync(Result<Namespace>.Success(CreateNamespace("key_othertenant")));
223+
224+
// Act
225+
var result = await _controller.GetVisibilityStatus(nsId, "my-queue", "Aws", CancellationToken.None);
226+
227+
// Assert
228+
result.Should().BeOfType<NotFoundObjectResult>();
229+
awsProvider.Verify(p => p.GetMessageReceiver(), Times.Never);
230+
}
231+
151232
[Fact]
152233
public async Task GetVisibilityStatus_AwsSuccess_ReturnsOk()
153234
{

0 commit comments

Comments
 (0)