Skip to content

Azure Storage Accounts PoLP (Principle of least privilege) Approach #2284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Dec 5, 2024
Merged

Azure Storage Accounts PoLP (Principle of least privilege) Approach #2284

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ea4b5e5
Changed Storage Account HealthChecks Logic Order for PoLP
Aug 8, 2024
9d5c501
blob checkpoint
Aug 8, 2024
fa8a0d1
queue checkpoint
Aug 8, 2024
5665fdc
table storage healthchecks
Aug 8, 2024
52f519c
Adjusted naming up to standards
Aug 8, 2024
a1c64a0
Merge remote-tracking branch 'upstream/master' into StorageAccount-PoLP
adamsitnik Dec 5, 2024
9b949cb
address code review feedback to get this PR merged for 9.0: restore d…
adamsitnik Dec 5, 2024
1a6324c
remove unused using
adamsitnik Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions src/HealthChecks.Azure.Data.Tables/AzureTableServiceHealthCheck.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,24 +31,27 @@ public async Task<HealthCheckResult> CheckHealthAsync(HealthCheckContext context
{
try
{
// Note: TableServiceClient.GetPropertiesAsync() cannot be used with only the role assignment
// "Storage Table Data Contributor," so TableServiceClient.QueryAsync() and
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a valuable comment and it should be moved rather than deleted.

// TableClient.QueryAsync<T>() are used instead to probe service health.
await _tableServiceClient
.QueryAsync(filter: "false", cancellationToken: cancellationToken)
.GetAsyncEnumerator(cancellationToken)
.MoveNextAsync()
.ConfigureAwait(false);

if (!string.IsNullOrEmpty(_options.TableName))
{
// Note: PoLP (Principle of least privilege)
// This can be used having at least the role assignment "Storage Table Data Reader" at table level.
var tableClient = _tableServiceClient.GetTableClient(_options.TableName);
await tableClient
.QueryAsync<TableEntity>(filter: "false", cancellationToken: cancellationToken)
.GetAsyncEnumerator(cancellationToken)
.MoveNextAsync()
.ConfigureAwait(false);
}
else
{
// Note: PoLP (Principle of least privilege)
// This can can be used with only the role assignment "Storage Table Data Reader" at storage account level.
await _tableServiceClient
.QueryAsync(filter: "false", cancellationToken: cancellationToken)
.GetAsyncEnumerator(cancellationToken)
.MoveNextAsync()
.ConfigureAwait(false);
}

return HealthCheckResult.Healthy();
}
Expand Down
25 changes: 15 additions & 10 deletions src/HealthChecks.Azure.Storage.Blobs/AzureBlobStorageHealthCheck.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,21 +31,26 @@ public async Task<HealthCheckResult> CheckHealthAsync(HealthCheckContext context
{
try
{
// Note: BlobServiceClient.GetPropertiesAsync() cannot be used with only the role assignment
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, this comment should not be removed.

// "Storage Blob Data Contributor," so BlobServiceClient.GetBlobContainersAsync() is used instead to probe service health.
// However, BlobContainerClient.GetPropertiesAsync() does have sufficient permissions.
await _blobServiceClient
.GetBlobContainersAsync(cancellationToken: cancellationToken)
.AsPages(pageSizeHint: 1)
.GetAsyncEnumerator(cancellationToken)
.MoveNextAsync()
.ConfigureAwait(false);

if (!string.IsNullOrEmpty(_options.ContainerName))
{
// Note: PoLP (Principle of least privilege)
// This can be used having at least the role assignment "Storage Blob Data Reader" at container level or at least "Storage Blob Data Reader" at storage account level.
// See <see href="https://docs.microsoft.com/en-us/azure/storage/common/storage-auth-aad-app?tabs=dotnet#configure-permissions-for-access-to-blob-and-queue-data">Configure permissions for access to blob and queue data</see>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the XML syntax should be used only for /// comments

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/xmldoc/recommended-tags

Suggested change
// See <see href="https://docs.microsoft.com/en-us/azure/storage/common/storage-auth-aad-app?tabs=dotnet#configure-permissions-for-access-to-blob-and-queue-data">Configure permissions for access to blob and queue data</see>
// See https://docs.microsoft.com/en-us/azure/storage/common/storage-auth-aad-app?tabs=dotnet#configure-permissions-for-access-to-blob-and-queue-data

var containerClient = _blobServiceClient.GetBlobContainerClient(_options.ContainerName);
await containerClient.GetPropertiesAsync(cancellationToken: cancellationToken).ConfigureAwait(false);
}
else
{
// Note: PoLP (Principle of least privilege)
// This can be used having at least "Storage Blob Data Reader" at storage account level.
// See <see href="https://docs.microsoft.com/en-us/azure/storage/common/storage-auth-aad-app?tabs=dotnet#configure-permissions-for-access-to-blob-and-queue-data">Configure permissions for access to blob and queue data</see>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Suggested change
// See <see href="https://docs.microsoft.com/en-us/azure/storage/common/storage-auth-aad-app?tabs=dotnet#configure-permissions-for-access-to-blob-and-queue-data">Configure permissions for access to blob and queue data</see>
// See https://docs.microsoft.com/en-us/azure/storage/common/storage-auth-aad-app?tabs=dotnet#configure-permissions-for-access-to-blob-and-queue-data

await _blobServiceClient
.GetBlobContainersAsync(cancellationToken: cancellationToken)
.AsPages(pageSizeHint: 1)
.GetAsyncEnumerator(cancellationToken)
.MoveNextAsync()
.ConfigureAwait(false);
}

return HealthCheckResult.Healthy();
}
Expand Down
25 changes: 15 additions & 10 deletions src/HealthChecks.Azure.Storage.Queues/AzureQueueStorageHealthCheck.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,21 +31,26 @@ public async Task<HealthCheckResult> CheckHealthAsync(HealthCheckContext context
{
try
{
// Note: QueueServiceClient.GetPropertiesAsync() cannot be used with only the role assignment
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

// "Storage Queue Data Contributor," so QueueServiceClient.GetQueuesAsync() is used instead to probe service health.
// However, QueueClient.GetPropertiesAsync() does have sufficient permissions.
await _queueServiceClient
.GetQueuesAsync(cancellationToken: cancellationToken)
.AsPages(pageSizeHint: 1)
.GetAsyncEnumerator(cancellationToken)
.MoveNextAsync()
.ConfigureAwait(false);

if (!string.IsNullOrEmpty(_options.QueueName))
{
// Note: PoLP (Principle of least privilege)
// This can be used having at least the role assignment "Storage Queue Data Reader" at container level or at least "Storage Queue Data Reader" at storage account level.
// See <see href="https://learn.microsoft.com/en-us/rest/api/storageservices/get-queue-metadata#authorization">Configure permissions for access to queue data</see>
var queueClient = _queueServiceClient.GetQueueClient(_options.QueueName);
await queueClient.GetPropertiesAsync(cancellationToken).ConfigureAwait(false);
}
else
{
// Note: PoLP (Principle of least privilege)
// This can be used having at least "Storage Queue Data Reader" at storage account level.
// See <see href="https://learn.microsoft.com/en-us/rest/api/storageservices/get-queue-metadata#authorization">Configure permissions for access to queue data</see>
await _queueServiceClient
.GetQueuesAsync(cancellationToken: cancellationToken)
.AsPages(pageSizeHint: 1)
.GetAsyncEnumerator(cancellationToken)
.MoveNextAsync()
.ConfigureAwait(false);
}

return HealthCheckResult.Healthy();
}
Expand Down
107 changes: 57 additions & 50 deletions ...les.Tests/TableServiceHealthCheckTests.cs → ...ests/AzureTableStorageHealthCheckTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using Azure;
using Azure.Data.Tables;
using Azure.Data.Tables.Models;
using Microsoft.AspNetCore.Mvc.RazorPages;
using NSubstitute;
using NSubstitute.ExceptionExtensions;

Expand Down Expand Up @@ -55,36 +56,26 @@ public async Task return_healthy_when_only_checking_healthy_service()
}

[Fact]
public async Task return_healthy_when_checking_healthy_service_and_table()
public async Task return_healthy_when_checking_healthy_service_table()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: previously it was checking both service and table, now just table, so I would not use "service" in the name

Suggested change
public async Task return_healthy_when_checking_healthy_service_table()
public async Task return_healthy_when_checking_healthy_table()

{
using var tokenSource = new CancellationTokenSource();

_tableServiceClient
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note to other reviewers: we already have a test that is testing the code path where _tableServiceClient is being used:

AspNetCore.Diagnostics.HealthChecks/test/HealthChecks.Azure.Data.Tables.Tests/TableServiceHealthCheckTests.cs

Lines 36 to 55 in f587a63

public async Task return_healthy_when_only_checking_healthy_service()
{
using var tokenSource = new CancellationTokenSource();
_tableServiceClient
.QueryAsync(filter: "false", cancellationToken: tokenSource.Token)
.Returns(AsyncPageable<TableItem>.FromPages(Array.Empty<Page<TableItem>>()));
var actual = await _healthCheck.CheckHealthAsync(_context, tokenSource.Token);
_tableServiceClient
.Received(1)
.QueryAsync(filter: "false", cancellationToken: tokenSource.Token);
_tableClient
.DidNotReceiveWithAnyArgs()
.QueryAsync<TableEntity>(default(string), default, default, default);
actual.Status.ShouldBe(HealthStatus.Healthy);
}

(this change looks like we test only 1 out of 2, but we keep testing both)

.QueryAsync(filter: "false", cancellationToken: tokenSource.Token)
.Returns(AsyncPageable<TableItem>.FromPages(Array.Empty<Page<TableItem>>()));

_tableClient
.QueryAsync<TableEntity>(filter: "false", cancellationToken: tokenSource.Token)
.Returns(AsyncPageable<TableEntity>.FromPages(Array.Empty<Page<TableEntity>>()));

_options.TableName = TableName;
var actual = await _healthCheck.CheckHealthAsync(_context, tokenSource.Token);

_tableServiceClient
.Received(1)
.QueryAsync(filter: "false", cancellationToken: tokenSource.Token);

_tableClient
.Received(1)
.QueryAsync<TableEntity>(filter: "false", cancellationToken: tokenSource.Token);

actual.Status.ShouldBe(HealthStatus.Healthy);
}

[Theory]
[InlineData(false)]
[InlineData(true)]
public async Task return_unhealthy_when_checking_unhealthy_service(bool checkTable)
[Fact]
public async Task return_unhealthy_when_checking_unhealthy_service()
{
using var tokenSource = new CancellationTokenSource();

Expand All @@ -103,7 +94,6 @@ public async Task return_unhealthy_when_checking_unhealthy_service(bool checkTab
.MoveNextAsync()
.ThrowsAsync(new RequestFailedException((int)HttpStatusCode.Unauthorized, "Unable to authorize access."));

_options.TableName = checkTable ? TableName : null;
var actual = await _healthCheck.CheckHealthAsync(_context, tokenSource.Token);

_tableServiceClient
Expand All @@ -129,47 +119,44 @@ await enumerator
}

[Fact]
public async Task return_unhealthy_when_checking_unhealthy_container()
public async Task return_unhealthy_when_checking_unhealthy_service_queue()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

queue? did you mean "table"?

Suggested change
public async Task return_unhealthy_when_checking_unhealthy_service_queue()
public async Task return_unhealthy_when_checking_unhealthy_table()

{
using var tokenSource = new CancellationTokenSource();

var pageable = Substitute.For<AsyncPageable<TableEntity>>();
var enumerator = Substitute.For<IAsyncEnumerator<TableEntity>>();

_tableServiceClient
.QueryAsync(filter: "false", cancellationToken: tokenSource.Token)
.Returns(AsyncPageable<TableItem>.FromPages(Array.Empty<Page<TableItem>>()));

_tableClient
.QueryAsync<TableEntity>(filter: "false", cancellationToken: tokenSource.Token)
.Returns(pageable);
.Throws(new RequestFailedException((int)HttpStatusCode.Unauthorized, "Unable to authorize access."));

pageable
.GetAsyncEnumerator(tokenSource.Token)
.Returns(enumerator);

enumerator
.MoveNextAsync()
.ThrowsAsync(new RequestFailedException((int)HttpStatusCode.NotFound, "Table not found"));

_options.TableName = TableName;
var actual = await _healthCheck.CheckHealthAsync(_context, tokenSource.Token);

_tableServiceClient
.Received(1)
.QueryAsync(filter: "false", cancellationToken: tokenSource.Token);

_tableClient
.Received(1)
.QueryAsync<TableEntity>(filter: "false", cancellationToken: tokenSource.Token);

pageable
.Received(1)
.GetAsyncEnumerator(tokenSource.Token);
actual.Status.ShouldBe(HealthStatus.Unhealthy);
actual
.Exception!.ShouldBeOfType<RequestFailedException>()
.Status.ShouldBe((int)HttpStatusCode.Unauthorized);
}

await enumerator
[Fact]
public async Task return_unhealthy_when_checking_unhealthy_table()
{
using var tokenSource = new CancellationTokenSource();

_tableClient
.QueryAsync<TableEntity>(filter: "false", cancellationToken: tokenSource.Token)
.Throws(new RequestFailedException((int)HttpStatusCode.NotFound, "Table not found"));

_options.TableName = TableName;
var actual = await _healthCheck.CheckHealthAsync(_context, tokenSource.Token);

_tableClient
.Received(1)
.MoveNextAsync();
.QueryAsync<TableEntity>(filter: "false", cancellationToken: tokenSource.Token);


actual.Status.ShouldBe(HealthStatus.Unhealthy);
actual
Expand All @@ -184,19 +171,15 @@ public async Task return_unhealthy_when_invoked_from_healthcheckservice()
.AddSingleton(_tableServiceClient)
.AddLogging()
.AddHealthChecks()
.AddAzureTable(optionsFactory: _ => new AzureTableServiceHealthCheckOptions() { TableName = TableName }, name: HealthCheckName)
.AddAzureTable(optionsFactory: _ => new AzureTableServiceHealthCheckOptions(), name: HealthCheckName)
.Services
.BuildServiceProvider();

var pageable = Substitute.For<AsyncPageable<TableEntity>>();
var enumerator = Substitute.For<IAsyncEnumerator<TableEntity>>();
var pageable = Substitute.For<AsyncPageable<TableItem>>();
var enumerator = Substitute.For<IAsyncEnumerator<TableItem>>();

_tableServiceClient
.QueryAsync(filter: "false", cancellationToken: Arg.Any<CancellationToken>())
.Returns(AsyncPageable<TableItem>.FromPages(Array.Empty<Page<TableItem>>()));

_tableClient
.QueryAsync<TableEntity>(filter: "false", cancellationToken: Arg.Any<CancellationToken>())
.Returns(pageable);

pageable
Expand All @@ -214,10 +197,6 @@ public async Task return_unhealthy_when_invoked_from_healthcheckservice()
.Received(1)
.QueryAsync(filter: "false", cancellationToken: Arg.Any<CancellationToken>());

_tableClient
.Received(1)
.QueryAsync<TableEntity>(filter: "false", cancellationToken: Arg.Any<CancellationToken>());

pageable
.Received(1)
.GetAsyncEnumerator(Arg.Any<CancellationToken>());
Expand All @@ -230,4 +209,32 @@ await enumerator
actual.Status.ShouldBe(HealthStatus.Unhealthy);
actual.Exception!.ShouldBeOfType<RequestFailedException>();
}


[Fact]
public async Task return_unhealthy_when_invoked_from_healthcheckservice_for_table()
{
using var provider = new ServiceCollection()
.AddSingleton(_tableServiceClient)
.AddLogging()
.AddHealthChecks()
.AddAzureTable(optionsFactory: _ => new AzureTableServiceHealthCheckOptions() { TableName = TableName }, name: HealthCheckName)
.Services
.BuildServiceProvider();

_tableClient
.QueryAsync<TableEntity>(filter: "false", cancellationToken: Arg.Any<CancellationToken>())
.Throws(new RequestFailedException((int)HttpStatusCode.NotFound, "Table not found"));

var service = provider.GetRequiredService<HealthCheckService>();
var report = await service.CheckHealthAsync();

_tableClient
.Received(1)
.QueryAsync<TableEntity>(filter: "false", cancellationToken: Arg.Any<CancellationToken>());

var actual = report.Entries[HealthCheckName];
actual.Status.ShouldBe(HealthStatus.Unhealthy);
actual.Exception!.ShouldBeOfType<RequestFailedException>();
}
}
0 ...ta.Tables.Tests/TablesConformanceTests.cs → ...les.Tests/TableStorageConformanceTests.cs
View file
Open in desktop
File renamed without changes.
Loading