Updated to use common Azure auth logic #972

ItalyPaleAle · 2021-06-23T21:28:44Z

TLDR

This PR fixes #970 and it generally tries to improve the authentication logic for (eventually) all Azure services, making it consistent and allowing Azure AD + RBAC wherever possible. Using Azure AD allows greater security and simplified management, such as via MSI. In the process, this PR also adds support for sovereign Azure clouds (Gov, China, etc).

Description

Currently, Dapr authenticates with Azure services primarily via "shared keys". For example, for Azure Storage it uses the "account key", which is not recommended as it's harder to manage the access token securely and it's less auditable. Quoting from the Azure Storage documentation:

Microsoft recommends using Azure AD when possible for maximum security and ease of use

This PR adds support for authenticating with Azure services through Azure AD (eventually, supporting all those who support this). This enables authentication via Azure AD "service principals", including client credentials (using a client secret), client certificate credentials (using a client certificate), and MSI. It then enables authorization via Azure RBAC, which can be fine-tuned by admins for better access control.

While I was implementing this, I also enabled the usage of other Azure clouds where possible (Azure China, Azure Gov, etc). For Azure Storage, the goal is to be able to support emulators too for simpler local development (like Azurite).

The code is partly based on the authorizer file that already existed for Azure Key Vault; I have expanded it to support Azure Storage and other services, and I have made it possible to support sovereign clouds.

Example

Example of using state.azure.blobstorage using client credentials:

apiVersion: dapr.io/v1alpha1
kind: Component
metadata:
  name: azblob
spec:
  type: state.azure.blobstorage
  version: v1
  metadata:
    - name: accountName
      value: xx
    - name: spnClientId
      value: xxx
    - name: spnClientSecret
      value: xxx
    - name: spnTenantId
      value: xxx
    - name: containerName
      value: xxx

Naming consistency

The new authorizer uses key names spn* to be consistent with what the Azure Key Vault authorizer was doing before.

Within Dapr, other modules (such as bindings/azure/eventgrid) use tenantId, clientId, and clientSecret. We should likely standardize on one style (I personally have a preference for the Event Grid's one, omitting spn because that's an Azure-specific name that is unclear in the OAuth world). For backwards-compatibility, the "deprecated" names can be kept as aliases (such as spnClientId -> clientId)

Work status

The PR is not complete yet. I am opening it as a draft for a review of the underlying authorization logic, and I will complete the support for the remaining modules then (unless someone wants to help :) )

Here's the list of work that's been done and still to do:

artursouza · 2021-07-09T20:01:44Z

@msfussell @yaron2 FYI

artursouza

Overall is on a good path. I would ask to keep backwards compatibility and also keep support for the existing (although non-recommended) authentication via secret key and connection strings. This can be handy for local development and also to give existing apps some room to migrate.

secretstores/azure/keyvault/keyvault.go

state/azure/blobstorage/blobstorage.go

ItalyPaleAle · 2021-07-09T20:21:16Z

Thanks for the first round of reviews. I'll continue iterating on this.

Understood on maintaining support for the "legacy" out, agree it should be preserved.

daixiang0 · 2021-07-16T08:19:53Z

Could move task list to issue, and separate PR as small as possible, it would help track and review.

ItalyPaleAle · 2021-07-16T20:12:35Z

@daixiang0 I think this would make sense. I could trim down this PR to only what's implemented today (a few things need to be updated/fixed, I'll get to that soon - just need to find some time :) ).

The other component updates could be moved to a separate issue, and we could open a PR for each.

daixiang0 · 2021-07-19T01:22:54Z

thanks for your contributions! please fix the conflict.

berndverst · 2021-08-03T19:51:45Z

@ItalyPaleAle I think this contribution is great and exactly what we need.

I like that you centralized and standardized the Azure Auth. Also, I approve of the KV related changes (I was working on those independently myself).

- Currently implemented on secretstores/azure/keyvault and state/azure/blobstorage - Supports Azure AD via service principal (client credentials, client certificate, MSI) - based on the previous authorizer for AKV - Allows using other Azure clouds (China, Germany, etc) - For Blob Storage state, supports using custom endpoints (like emulators like Azurite)

berndverst · 2021-08-06T00:14:27Z

@artursouza feel free to review this now. I think this is in a state where we can merge it. Right now this only updates behaviors for State Store from Azure Storage and Secret Store with Keyvault. It does however generally put the common Azure Auth logic in place. We can in subsequent PRs switch the other Azure components to use this logic.

This change here is also fully backwards compatible.

I introduced new Environment variables as alternatives to the old spn prefixes ones. Both will work. This is to ensure consistency with the official Azure Go SDK (and other Azure auth libraries).

…omponents-contrib into common-azure-auth

yaron2 · 2021-08-06T20:25:06Z

@artursouza feel free to review this now. I think this is in a state where we can merge it. Right now this only updates behaviors for State Store from Azure Storage and Secret Store with Keyvault. It does however generally put the common Azure Auth logic in place. We can in subsequent PRs switch the other Azure components to use this logic.

This change here is also fully backwards compatible.

I introduced new Environment variables as alternatives to the old spn prefixes ones. Both will work. This is to ensure consistency with the official Azure Go SDK (and other Azure auth libraries).

It's important to make sure this is backward compatible. I'm happy with this change.

yaron2 · 2021-08-06T20:25:22Z

Workflow is running now.

ItalyPaleAle · 2021-08-06T20:28:07Z

@yaron2 I'm taking one more pass to review this PR and also the following changes. Working synchronously with @berndverst as I'm writing this. We should be done soon.

berndverst · 2021-08-06T21:12:47Z

Given that the various settings are specified as metadata in the component YAMLs we decided to name them in a more YAML standard way with camelcase. The naming is still derived from the naming in the other Azure SDKs, but is camelcase with an azure prefix to disambiguate.

ItalyPaleAle · 2021-08-06T21:13:19Z

@yaron2 we're done. The last changes include:

Cleaning up the name of the keys that can be used in the metadata. As per my "Naming consistency" point above, some components were using naming like "spnClientId", some were using "azureClientId", and some "clientId". This now supports all three, although the preferred one should be "azureClientId" (and similar).
MSI should be fixed now.

I think this is ready for review at this point. Some notes:

Backwards compatibility should have been maintained at all times, for all components. Everything that works today will work with this new PR. if that's not the case, please let me know as it should be considered a bug.
As discussed, this was scoped down for now and it only updates secretstores/azure/keyvault and state/azure/blobstorage. The common auth layer is now generic enough that we should be able to leverage it for every other component (excluding those that use Table Storage because of lack of upstream support), but that work will need to happen in separate PRs.

ItalyPaleAle · 2021-08-06T21:24:38Z

authentication/azure/storage.go

+	}
+	var tokenRefresher azblob.TokenRefresher = func(credential azblob.TokenCredential) time.Duration {
+		log.Debug("Refreshing Azure Storage auth token")
+		fmt.Println("Refreshing Azure Storage auth token")


I just noticed there are 2 log statements here. Perhaps only one of the two should be kept (or neither). Same on lines 53-54. Not sure what the standard for logging in Dapr is, maybe you could decide how to fix this please?

We shouldn't use fmt.Println

I already removed it before I saw your comment @yaron2 :)

berndverst · 2021-08-10T04:16:50Z

@artursouza @yaron2 can you please take a look and review? :)

codecov · 2021-08-10T04:17:41Z

Codecov Report

Merging #972 (b411017) into master (253ef85) will increase coverage by 0.12%.
The diff coverage is 46.19%.

@@            Coverage Diff             @@
##           master     #972      +/-   ##
==========================================
+ Coverage   34.53%   34.65%   +0.12%     
==========================================
  Files         132      132              
  Lines       10870    10907      +37     
==========================================
+ Hits         3754     3780      +26     
- Misses       6736     6740       +4     
- Partials      380      387       +7

Impacted Files	Coverage Δ
authentication/azure/storage.go	`0.00% <0.00%> (ø)`
state/azure/tablestorage/tablestorage.go	`13.20% <0.00%> (ø)`
state/redis/redis.go	`42.22% <33.33%> (-0.64%)`	⬇️
state/azure/blobstorage/blobstorage.go	`24.46% <50.00%> (+0.01%)`	⬆️
authentication/azure/auth.go	`57.02% <57.02%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa7d2ee...b411017. Read the comment docs.

artursouza

Just a comment on a test that might affect backwards compatibility.

artursouza · 2021-08-10T16:57:21Z

state/azure/blobstorage/blobstorage_test.go

@@ -50,13 +50,11 @@ func TestGetBlobStorageMetaData(t *testing.T) {
 	t.Run("All parameters passed and parsed", func(t *testing.T) {
 		m := make(map[string]string)
 		m["accountName"] = "acc"
-		m["accountKey"] = "key"


Why key is not parsed anymore?

The accountKey property doesn't exist anymore in the object (see line 59 deleted in the test below).

Instead, the shared key is read in the method that returns the azblob.Credential object:

components-contrib/authentication/azure/storage.go

Line 26 in b411017

accountKey, ok := metadata[storageAccountKeyKey]

dapr#972 accidentally introduced a backwards-incompatible change with a feature added in 1.3. Before, it was possible to specify an Azure environment for the AKV secret store by passing a FQDN as "vaultName" property that included the suffix for the Azure environment. dapr#972 introduced a better way to handle this (using the "azureEnvironment" metadata property), but accidentally broke the behavior added in 1.3 This patch restores full compatibility with 1.3. Although that behavior should be considered deprecated and thus discouraged (and it will be removed from docs), it will still be supported.

* Restored backwards compatibility with 1.3 #972 accidentally introduced a backwards-incompatible change with a feature added in 1.3. Before, it was possible to specify an Azure environment for the AKV secret store by passing a FQDN as "vaultName" property that included the suffix for the Azure environment. #972 introduced a better way to handle this (using the "azureEnvironment" metadata property), but accidentally broke the behavior added in 1.3 This patch restores full compatibility with 1.3. Although that behavior should be considered deprecated and thus discouraged (and it will be removed from docs), it will still be supported. * Lint

* Restored backwards compatibility with 1.3 dapr#972 accidentally introduced a backwards-incompatible change with a feature added in 1.3. Before, it was possible to specify an Azure environment for the AKV secret store by passing a FQDN as "vaultName" property that included the suffix for the Azure environment. dapr#972 introduced a better way to handle this (using the "azureEnvironment" metadata property), but accidentally broke the behavior added in 1.3 This patch restores full compatibility with 1.3. Although that behavior should be considered deprecated and thus discouraged (and it will be removed from docs), it will still be supported. * Lint

* Restored backwards compatibility with 1.3 #972 accidentally introduced a backwards-incompatible change with a feature added in 1.3. Before, it was possible to specify an Azure environment for the AKV secret store by passing a FQDN as "vaultName" property that included the suffix for the Azure environment. #972 introduced a better way to handle this (using the "azureEnvironment" metadata property), but accidentally broke the behavior added in 1.3 This patch restores full compatibility with 1.3. Although that behavior should be considered deprecated and thus discouraged (and it will be removed from docs), it will still be supported. * Lint Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>

* Common Azure auth logic - Currently implemented on secretstores/azure/keyvault and state/azure/blobstorage - Supports Azure AD via service principal (client credentials, client certificate, MSI) - based on the previous authorizer for AKV - Allows using other Azure clouds (China, Germany, etc) - For Blob Storage state, supports using custom endpoints (like emulators like Azurite) * Add environment variable aliases * Address linter warnings * another lint thing * Fixed typo in method description * Updated metadata key names so they're more consistent * Fix test * Some more linter things Co-authored-by: Bernd Verst <me@bernd.dev> Co-authored-by: Yaron Schneider <yaronsc@microsoft.com> Co-authored-by: Bernd Verst <berndverst@users.noreply.github.com>

* Restored backwards compatibility with 1.3 dapr#972 accidentally introduced a backwards-incompatible change with a feature added in 1.3. Before, it was possible to specify an Azure environment for the AKV secret store by passing a FQDN as "vaultName" property that included the suffix for the Azure environment. dapr#972 introduced a better way to handle this (using the "azureEnvironment" metadata property), but accidentally broke the behavior added in 1.3 This patch restores full compatibility with 1.3. Although that behavior should be considered deprecated and thus discouraged (and it will be removed from docs), it will still be supported. * Lint

artursouza self-assigned this Jul 9, 2021

artursouza reviewed Jul 9, 2021

View reviewed changes

secretstores/azure/keyvault/keyvault.go Show resolved Hide resolved

state/azure/blobstorage/blobstorage.go Show resolved Hide resolved

state/azure/blobstorage/blobstorage.go Show resolved Hide resolved

berndverst mentioned this pull request Aug 3, 2021

Certify Azure Key Vault w/ Managed Identity as GA in secret store #959

Closed

ItalyPaleAle and others added 4 commits August 5, 2021 14:23

Add environment variable aliases

fe08c74

Address linter warnings

ac9a38b

another lint thing

cdb578a

ItalyPaleAle marked this pull request as ready for review August 6, 2021 14:34

ItalyPaleAle requested review from a team as code owners August 6, 2021 14:34

yaron2 and others added 3 commits August 6, 2021 13:23

Merge branch 'master' into common-azure-auth

72d0a79

Fixed typo in method description

e5e5574

Merge branch 'common-azure-auth' of https://github.com/ItalyPaleAle/c…

7019ba2

…omponents-contrib into common-azure-auth

Updated metadata key names so they're more consistent

9e88174

ItalyPaleAle commented Aug 6, 2021

View reviewed changes

berndverst and others added 3 commits August 6, 2021 14:51

Fix test

a0578ec

Some more linter things

1afc0b7

Merge branch 'master' into common-azure-auth

b411017

artursouza requested changes Aug 10, 2021

View reviewed changes

artursouza approved these changes Aug 10, 2021

View reviewed changes

artursouza merged commit d0816e3 into dapr:master Aug 10, 2021

ItalyPaleAle deleted the common-azure-auth branch August 12, 2021 12:45

This was referenced Aug 28, 2021

Update all other Azure components to use the common auth layer (and support Azure AD auth) #1103

Closed

docs for dapr/components-contrib #972 dapr/docs#1755

Closed

ItalyPaleAle mentioned this pull request Sep 10, 2021

Restored backwards compatibility with 1.3 #1131

Merged

3 tasks

artursouza mentioned this pull request Sep 10, 2021

Cherry-pick: Restored backwards compatibility with 1.3 (#1131) #1133

Merged

3 tasks

ItalyPaleAle mentioned this pull request Mar 4, 2022

REQUEST: New membership for ItalyPaleAle dapr/community#152

Closed

4 tasks

berndverst mentioned this pull request May 26, 2022

REQUEST: Approver Status for @italypaleale in Contrib dapr/community#177

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated to use common Azure auth logic #972

Updated to use common Azure auth logic #972

ItalyPaleAle commented Jun 23, 2021 •

edited

artursouza commented Jul 9, 2021

artursouza left a comment

ItalyPaleAle commented Jul 9, 2021

daixiang0 commented Jul 16, 2021

ItalyPaleAle commented Jul 16, 2021

daixiang0 commented Jul 19, 2021

berndverst commented Aug 3, 2021

berndverst commented Aug 6, 2021

yaron2 commented Aug 6, 2021

yaron2 commented Aug 6, 2021

ItalyPaleAle commented Aug 6, 2021

berndverst commented Aug 6, 2021

ItalyPaleAle commented Aug 6, 2021

ItalyPaleAle Aug 6, 2021

yaron2 Aug 6, 2021

berndverst Aug 6, 2021

berndverst commented Aug 10, 2021

codecov bot commented Aug 10, 2021

artursouza left a comment

artursouza Aug 10, 2021

ItalyPaleAle Aug 10, 2021

Updated to use common Azure auth logic #972

Updated to use common Azure auth logic #972

Conversation

ItalyPaleAle commented Jun 23, 2021 • edited

TLDR

Description

Example

Naming consistency

Work status

artursouza commented Jul 9, 2021

artursouza left a comment

Choose a reason for hiding this comment

ItalyPaleAle commented Jul 9, 2021

daixiang0 commented Jul 16, 2021

ItalyPaleAle commented Jul 16, 2021

daixiang0 commented Jul 19, 2021

berndverst commented Aug 3, 2021

berndverst commented Aug 6, 2021

yaron2 commented Aug 6, 2021

yaron2 commented Aug 6, 2021

ItalyPaleAle commented Aug 6, 2021

berndverst commented Aug 6, 2021

ItalyPaleAle commented Aug 6, 2021

ItalyPaleAle Aug 6, 2021

Choose a reason for hiding this comment

yaron2 Aug 6, 2021

Choose a reason for hiding this comment

berndverst Aug 6, 2021

Choose a reason for hiding this comment

berndverst commented Aug 10, 2021

codecov bot commented Aug 10, 2021

Codecov Report

artursouza left a comment

Choose a reason for hiding this comment

artursouza Aug 10, 2021

Choose a reason for hiding this comment

ItalyPaleAle Aug 10, 2021

Choose a reason for hiding this comment

ItalyPaleAle commented Jun 23, 2021 •

edited