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

Qualizorg · 2024-08-08T15:24:48Z

Here's the completed PR template for your draft pull request:

What this PR does / why we need it:

This PR modifies the health checks for Azure Storage Account (blob, queue, and table storage) to work with a least privilege approach. Previously, the health checks required roles at the storage account level, which caused failures when permissions were assigned at the resource level (e.g., tables, queues, containers) via RBAC. The new implementation allows health checks to be performed with roles assigned at the specific resource level, adhering to the principle of least privilege.

Which issue(s) this PR fixes:
None that I'm aware of.

Special notes for your reviewer:

The health check methods have been updated to handle permissions at the resource level correctly.
Comments have been added to explain the rationale behind the changes and to provide guidance on required roles.

Does this PR introduce a user-facing change?:

No, this PR does not introduce a user-facing change.

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Qualizorg · 2024-08-08T15:27:08Z

@dotnet-policy-service agree company="Qualizorg B.V"

Qualizorg · 2024-08-08T15:27:26Z

@dotnet-policy-service agree company="Qualizorg B.V"

xInfinitYz · 2024-08-14T17:35:33Z

Can we merge this?

Qualizorg · 2024-08-20T18:35:50Z

@Alirexaa Could we push this?

adamsitnik

@Qualizorg thank you for your contribution! We just need some polishing before merging it (PTAL at my comments)!

adamsitnik · 2024-12-02T12:19:03Z

src/HealthChecks.Azure.Storage.Blobs/AzureBlobStorageHealthCheck.cs

            {
+                // 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>


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

adamsitnik · 2024-12-02T12:19:37Z

src/HealthChecks.Azure.Storage.Blobs/AzureBlobStorageHealthCheck.cs

+            {
+                // 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>


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

adamsitnik · 2024-12-02T12:20:51Z

src/HealthChecks.Azure.Data.Tables/AzureTableServiceHealthCheck.cs

        try
        {
-            // Note: TableServiceClient.GetPropertiesAsync() cannot be used with only the role assignment
-            // "Storage Table Data Contributor," so TableServiceClient.QueryAsync() and


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

adamsitnik · 2024-12-02T12:21:13Z

src/HealthChecks.Azure.Storage.Blobs/AzureBlobStorageHealthCheck.cs

    {
        try
        {
-            // Note: BlobServiceClient.GetPropertiesAsync() cannot be used with only the role assignment


Same as above, this comment should not be removed.

adamsitnik · 2024-12-02T12:21:44Z

src/HealthChecks.Azure.Storage.Queues/AzureQueueStorageHealthCheck.cs

    {
        try
        {
-            // Note: QueueServiceClient.GetPropertiesAsync() cannot be used with only the role assignment


adamsitnik · 2024-12-02T12:48:28Z

test/HealthChecks.Azure.Data.Tables.Tests/AzureTableStorageHealthCheckTests.cs


    [Fact]
-    public async Task return_healthy_when_checking_healthy_service_and_table()
+    public async Task return_healthy_when_checking_healthy_service_table()


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()

adamsitnik · 2024-12-02T12:49:55Z

test/HealthChecks.Azure.Data.Tables.Tests/AzureTableStorageHealthCheckTests.cs


    [Fact]
-    public async Task return_unhealthy_when_checking_unhealthy_container()
+    public async Task return_unhealthy_when_checking_unhealthy_service_queue()


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()

adamsitnik · 2024-12-02T12:52:00Z

test/HealthChecks.Azure.Data.Tables.Tests/AzureTableStorageHealthCheckTests.cs

    {
        using var tokenSource = new CancellationTokenSource();

-        _tableServiceClient


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)

# Conflicts: # test/HealthChecks.Azure.Data.Tables.Tests/AzureTableStorageHealthCheckTests.cs # test/HealthChecks.Azure.Storage.Blobs.Tests/AzureBlobStorageHealthCheckTests.cs # test/HealthChecks.Azure.Storage.Queues.Tests/AzureQueueStorageHealthCheckTests.cs

…eleted comments, remove duplicated test

codecov-commenter · 2024-12-05T14:05:21Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.68%. Comparing base (72d9abf) to head (1a6324c).
Report is 8 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2284      +/-   ##
==========================================
- Coverage   66.88%   66.68%   -0.21%     
==========================================
  Files         268      261       -7     
  Lines        8730     8647      -83     
  Branches      631      621      -10     
==========================================
- Hits         5839     5766      -73     
+ Misses       2723     2716       -7     
+ Partials      168      165       -3

Flag	Coverage Δ
AzureBlobStorage	`27.46% <100.00%> (+1.03%)`	⬆️
AzureEventHubs	`?`
AzureFileStorage	`?`
AzureQueueStorage	`27.46% <100.00%> (+1.03%)`	⬆️
AzureTableStorage	`28.96% <100.00%> (+0.99%)`	⬆️
Dapr	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

adamsitnik

I want to include these improvements in 9.0 release, since the PR was stale for a long time (due to lack of code review from our side) I've decided to simply push some minor changes to your branch and solve the merge conflicts.

@Qualizorg thank you for your contribution!

Oleksandr Kyselov added 5 commits August 8, 2024 09:03

Changed Storage Account HealthChecks Logic Order for PoLP

ea4b5e5

blob checkpoint

9d5c501

queue checkpoint

fa8a0d1

table storage healthchecks

5665fdc

Adjusted naming up to standards

52f519c

github-actions bot added test azure labels Aug 8, 2024

adamsitnik reviewed Dec 2, 2024

View reviewed changes

adamsitnik added 3 commits December 5, 2024 14:50

address code review feedback to get this PR merged for 9.0: restore d…

9b949cb

…eleted comments, remove duplicated test

remove unused using

1a6324c

adamsitnik approved these changes Dec 5, 2024

View reviewed changes

adamsitnik merged commit 1947824 into Xabaril:master Dec 5, 2024
4 checks passed

adamsitnik self-assigned this Dec 5, 2024

adamsitnik added this to the 9.0 milestone Dec 5, 2024

	// 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

	public async Task return_healthy_when_checking_healthy_service_table()
	public async Task return_healthy_when_checking_healthy_table()

	public async Task return_unhealthy_when_checking_unhealthy_service_queue()
	public async Task return_unhealthy_when_checking_unhealthy_table()

	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);
	}

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

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

Uh oh!

Conversation

Qualizorg commented Aug 8, 2024

Uh oh!

Qualizorg commented Aug 8, 2024

Uh oh!

Qualizorg commented Aug 8, 2024

Uh oh!

xInfinitYz commented Aug 14, 2024

Uh oh!

Qualizorg commented Aug 20, 2024

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Dec 5, 2024

Codecov Report

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants