[COST-7476] Fall back to credential-based auth when storage account key access is disabled#6062
[COST-7476] Fall back to credential-based auth when storage account key access is disabled#6062bacciotti wants to merge 6 commits into
Conversation
github-actions
Bot
added
the
smokes-required
…disabled When a storage account has 'Allow storage account key access' disabled, listKeys succeeds but any subsequent use of the key is rejected with KeyBasedAuthenticationNotPermitted. The previous fallback only triggered when listKeys itself failed, leaving integrations with Storage Account Contributor broken silently. This change validates the key-based client immediately after creation via get_account_information(). If key-based auth is rejected, the code falls back to credential-based BlobServiceClient — the same path used when listKeys is denied outright. Also replaces httpError.message with str(httpError) to safely handle cases where the message attribute is None. Co-authored-by: Cursor <cursoragent@cursor.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the Azure provider's client to support fallback to credential-based authentication when key-based authentication is disabled on a storage account. It also improves error handling by using the string representation of HttpResponseError to avoid potential TypeError issues and includes new unit tests for these scenarios. Feedback suggests avoiding hardcoded endpoint suffixes to support multi-cloud environments and refactoring the exception handler to store the error string in a variable for better readability.
bacciotti
force-pushed
the
COST-7476-key-auth-fallback
branch
from
0a92937 to
7a933de
Compare
bacciotti
added
the
azure-smoke-tests
Co-authored-by: Cursor <cursoragent@cursor.com>
…n_string to avoid real HTTP call Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6062 +/- ##
=====================================
Coverage 94.4% 94.4%
=====================================
Files 362 362
Lines 32101 32105 +4
Branches 3538 3538
=====================================
+ Hits 30292 30296 +4
Misses 1175 1175
Partials 634 634 🚀 New features to boost your workflow:
|
lcouzens
left a comment
lcouzens
left a comment
There was a problem hiding this comment.
a couple of clarifying questions
| except HttpResponseError as httpError: | ||
| err = "does not have authorization to perform action 'Microsoft.Storage/storageAccounts/listKeys/action'" | ||
| if err not in str(httpError): | ||
| error_msg = str(httpError) |
There was a problem hiding this comment.
I feel like we obfuscated these errors before because they would occasionally leak sensitive information into our logs. I could be miss remembering here but we should also check or add in some logic that can recognise sensitive content and obfuscate it.
There was a problem hiding this comment.
i didnt find any obfuscation pattern... Also the original code already logged httpError.message in the same warning. For these errors we check here the messages don't contain secrets, just ids and metadata.
| ) | ||
| return BlobServiceClient.from_connection_string(connect_str) | ||
| client = BlobServiceClient.from_connection_string(connect_str) | ||
| client.get_account_information() |
There was a problem hiding this comment.
Just for my understanding, this is the real line that fixes thingss correct? Essentially this check would cause a error to be raised for the customer and enable the flow to fall back to account access keys?
There was a problem hiding this comment.
correct, without it, the error only surfaces later during blob listing, outside where the fallback can trigger
2 participants
Jira Ticket
COST-7476
Description
This change fixes a silent failure in
AzureClientFactory.blob_service_clientwhen a storage account has "Allow storage account key access" disabled at the storage account level.Root cause
The existing fallback to credential-based auth only triggers when
listKeysfails (i.e. the service principal lacksMicrosoft.Storage/storageAccounts/listKeys/action). However, if the service principal hasStorage Account Contributor,listKeyssucceeds and returns keys — but any attempt to use those keys is rejected by the storage account withKeyBasedAuthenticationNotPermitted. The fallback never kicks in, and the integration silently fails with a 403 during blob listing.This means that having more permission (
Storage Account Contributor) paradoxically breaks the integration when key access is disabled, because the code never reaches the token-based fallback.Fix
After creating the key-based
BlobServiceClient, immediately callget_account_information()to validate that key-based authentication is actually permitted. If it raisesHttpResponseErrorwithKeyBasedAuthenticationNotPermitted, the code falls back to the credential-basedBlobServiceClient— the same path already used whenlistKeysis denied.Also replaces
httpError.message(which can beNone) withstr(httpError)for safer string matching.Testing
Checkout branch
Run the Azure client tests:
test_blob_service_client_key_auth_not_permitted— new test covering the fixed scenariotest_blob_service_client_none_message— new test covering theNonemessage edge caseTo verify end-to-end: configure an Azure integration where the storage account has "Allow storage account key access" disabled and the service principal has
Storage Account Contributor+Storage Blob Data Reader. Data should now ingest successfully without needing to removeStorage Account Contributor.Release Notes
Made with Cursor