Allow authentication using RBAC instead of API Keys #427

adamdougal · 2023-12-01T13:28:45Z

Required by #391

pamelafox · 2023-12-01T14:31:10Z

Do you also need to add RBAC roles for this to work, like https://github.com/Azure-Samples/azure-search-openai-demo/blob/87d15fc021a2a84c4cbcbec56b2fe0560af05d52/infra/main.bicep#L336 ?

adamdougal · 2023-12-01T14:37:09Z

@pamelafox Yes! Looks like I missed that. Some roles are there already but a few are missing. I'll get them added now.

adamdougal · 2023-12-01T16:24:48Z

@pamelafox Those have now been added

README.md

pamelafox · 2023-12-01T18:11:50Z

infra/core/search/search-services.bicep

@@ -8,6 +8,7 @@ param sku object = {

 param authOptions object = {}
 param semanticSearch string = 'disabled'
+param searchApiKeyEnabled bool = true


cc @jongio who likes to be notified about potential changes to infra/core Bicep files (as we try to reuse the same infra/core across azd templates)

pamelafox · 2023-12-01T18:13:07Z

infra/main.bicep

@@ -271,6 +273,46 @@ module searchRoleBackend 'core/security/role.bicep' = {
  }
 }

+module searchRoleOpenAi 'core/security/role.bicep' = if (!searchApiKeyEnabled) {


I see you only did the system roles, what about local server running? For the other repo with these roles, we also have the equivalent for the user. I don't recall if this repo can be run locally or not.

So I tried locally and it didn't require me to add any roles. I think this is because we're still calling OpenAI with API key. I missed this before, so will make the change and push to this branch/PR along with the associated required role.

We'll want rbac to work for the local user as well.

jongio · 2023-12-04T17:12:55Z

README.md

@@ -114,6 +114,17 @@ const getUserInfoList = async () => {
 }
 ```

+### Authenticate using Azure RBAC
+To authenticate using Azure RBAC instead of the default API Keys follow the steps below:
+1. Ensure role assignments listed on [this page](https://learn.microsoft.com/azure/ai-services/openai/concepts/use-your-data?tabs=ai-search#azure-role-based-access-controls-azure-rbac-for-adding-data-sources) have been created 


Should mention these should be created by azd provision already

jongio · 2023-12-04T17:14:29Z

README.md

@@ -114,6 +114,17 @@ const getUserInfoList = async () => {
 }
 ```

+### Authenticate using Azure RBAC


I would rather the flow of the application take care of all the settings and not require the user to take manual action.

We are experimenting with adding an AZURE_AUTH_TYPE parameter that can get set to rbac or keys and the bicep does the appropriate thing based on that.

It's not finalized, but should be soon.

app.py

jongio · 2023-12-04T17:15:30Z

infra/core/ai/cognitiveservices.bicep

@@ -41,3 +44,5 @@ output id string = account.id
 output name string = account.name
 output skuName string = account.sku.name
 output key string = account.listKeys().key1
+output identityPrincipalId string = account.identity.principalId


This should match the name of the other core files when they provide an id, which is just principalId

jongio · 2023-12-04T17:16:05Z

infra/core/search/search-services.bicep

@@ -17,8 +18,8 @@ resource search 'Microsoft.Search/searchServices@2021-04-01-preview' = {
    type: 'SystemAssigned'
  }
  properties: {
-    authOptions: authOptions
-    disableLocalAuth: false
+    authOptions: searchApiKeyEnabled ? authOptions : null


Param should be something like 'authType' with allowed values of rbac or keys

jongio · 2023-12-04T17:16:14Z

infra/core/search/search-services.bicep

@@ -41,3 +42,4 @@ output endpoint string = 'https://${name}.search.windows.net/'
 output name string = search.name
 output skuName string = sku.name
 output adminKey string = search.listAdminKeys().primaryKey
+output identityPrincipalId string = search.identity.principalId


Same comment about name principalId

jongio · 2023-12-04T17:16:20Z

infra/main.bicep

@@ -26,6 +26,7 @@ param searchContentColumns string = 'content'
 param searchFilenameColumn string = 'filepath'
 param searchTitleColumn string = 'title'
 param searchUrlColumn string = 'url'
+param searchApiKeyEnabled bool = true


jongio · 2023-12-04T17:16:39Z

infra/main.bicep

@@ -271,6 +273,46 @@ module searchRoleBackend 'core/security/role.bicep' = {
  }
 }

+module searchRoleOpenAi 'core/security/role.bicep' = if (!searchApiKeyEnabled) {


We'll want rbac to work for the local user as well.

jongio · 2023-12-04T17:17:42Z

infra/main.bicep

+    principalType: 'ServicePrincipal'
+  }
+}
+


In outputs below we shouldn't be outputting keys. You will get a bicep linter warning. Best to ask the user to manually set them with in env vars at this time until we have a secure way to store them.

Phew, glad to see the keys are out of this output finally! That's been a TODO (in my head) for a long time.

Another option btw is to provide a script that grabs the keys, which is what I've had to do for another repo which unfortunately requires keys:
https://github.com/Azure-Samples/contoso-chat/pull/33/files#diff-c8719e3915d76118aeee543ed85367b315252d46ef1d793635a82eaef96ce7b4R19

But probably okay not to do that here, since we now have the keyless option.

adamdougal · 2023-12-04T18:07:50Z

Heya, thanks for your review comments! I've made some changes. Please note, I've yet to test them out. I'll update on here when I have.

jongio · 2023-12-04T18:31:31Z

infra/main.parameters.json

@@ -43,6 +43,9 @@
    },
    "authClientSecret": {
      "value": "${AUTH_CLIENT_SECRET}"
+    },
+    "authType": {
+      "value": "${AZURE_AUTH_TYPE}"


You'll want to default this to rbac

"value": "${AZURE_AUTH_TYPE=rbac}"

jongio · 2023-12-04T18:32:01Z

app.py

@@ -40,6 +39,7 @@ def assets(path):
 SEARCH_TOP_K = os.environ.get("SEARCH_TOP_K", 5)
 SEARCH_STRICTNESS = os.environ.get("SEARCH_STRICTNESS", 3)
 SEARCH_ENABLE_IN_DOMAIN = os.environ.get("SEARCH_ENABLE_IN_DOMAIN", "true")
+AZURE_AUTH_TYPE = os.environ.get("AZURE_AUTH_TYPE", "apikeys")


we'll use 'keys'

jongio · 2023-12-04T18:32:20Z

infra/core/search/search-services.bicep

@@ -8,7 +8,7 @@ param sku object = {

 param authOptions object = {}
 param semanticSearch string = 'disabled'
-param searchApiKeyEnabled bool = true
+param authType string


add rbac default

jongio · 2023-12-04T18:32:46Z

.env.sample

@@ -1,3 +1,4 @@
+AZURE_AUTH_TYPE


all the others have an =

adamdougal · 2023-12-05T13:02:44Z

I've now tested this deployed to Azure, as well as running locally. I did not have to create any additional role assignments to get this to work locally.

jongio · 2023-12-05T18:19:06Z

I've now tested this deployed to Azure, as well as running locally. I did not have to create any additional role assignments to get this to work locally.

Did you test with both rbac and keys in both local and azure?

adamdougal · 2023-12-06T09:06:41Z

I've now tested this deployed to Azure, as well as running locally. I did not have to create any additional role assignments to get this to work locally.

Did you test with both rbac and keys in both local and azure?

Yep, both worked without any additional roles being required.

app.py

jongio · 2023-12-06T19:19:37Z

app.py

+            default_credential = DefaultAzureCredential()
+            token = default_credential.get_token("https://cognitiveservices.azure.com/.default")
+            openai.api_key = token.token


Can this be refactored to remove dupe code above?

adamdougal · 2023-12-11T15:34:25Z

Heya, I believe this is ready for re-review. Thanks

jongio · 2024-01-02T22:33:24Z

Can we move this to azure-samples github org? Not sure history on why it is in the microsoft org, but most samples go in azure-samples org. https://github.com/azure-samples

jongio · 2024-01-02T22:35:25Z

infra/core/host/appservice.bicep

@@ -132,6 +132,6 @@ resource applicationInsights 'Microsoft.Insights/components@2020-02-02' existing
  name: applicationInsightsName
 }

-output identityPrincipalId string = managedIdentity ? appService.identity.principalId : ''
+output principalId string = managedIdentity ? appService.identity.principalId : ''


I stand corrected, the official name is identityPrincipalId

https://github.com/Azure/azure-dev/blob/main/templates/common/infra/bicep/core/host/appservice.bicep#L121

Please change it back.

jongio · 2024-01-02T22:35:40Z

infra/core/ai/cognitiveservices.bicep

@@ -41,3 +44,5 @@ output id string = account.id
 output name string = account.name
 output skuName string = account.sku.name
 output key string = account.listKeys().key1
+output principalId string = account.identity.principalId


Can you change this to identityPrincipalId? (Sorry)

jongio · 2024-01-02T22:35:57Z

infra/core/ai/cognitiveservices.bicep

@@ -20,6 +20,9 @@ resource account 'Microsoft.CognitiveServices/accounts@2023-05-01' = {
    publicNetworkAccess: publicNetworkAccess


Can you do a PR to update this too? https://github.com/Azure/azure-dev/blob/main/templates/common/infra/bicep/core/ai/cognitiveservices.bicep

What updates are required? Do you just mean to pull from main?

jongio · 2024-01-02T22:36:23Z

infra/core/search/search-services.bicep

@@ -8,6 +8,7 @@ param sku object = {

 param authOptions object = {}
 param semanticSearch string = 'disabled'
+param authType string = 'rbac'


Can you do a PR to update this? https://github.com/Azure/azure-dev/blob/main/templates/common/infra/bicep/core/search/search-services.bicep

What updates are required? Do you just mean to pull from main?

jongio · 2024-01-02T22:37:13Z

infra/main.bicep

+  }
+}
+
+module searchRoleOpenAi 'core/security/role.bicep' = if (authType == 'rbac') {


Any new roles you create will also need to be assigned to the user's principalId, see "user roles" above.

Co-authored-by: Pamela Fox <[email protected]>

adamdougal · 2024-01-08T09:48:26Z

Heya @jongio, thanks for the further comments. I have addressed them and rebased from main.

Unfortunately, I have no information as to why this is not in the azure samples org.

sarah-widder · 2024-01-08T23:27:33Z

Can we move this to azure-samples github org? Not sure history on why it is in the microsoft org, but most samples go in azure-samples org. https://github.com/azure-samples

We have considered moving this to the azure-samples org but haven't taken the action yet. The repo is used in multiple ways (code-first sample, source code for apps deployed from Azure OpenAI studio, storage place for public-facing utility scripts such as data preparation). For now we're keeping it here.

sarah-widder

Looks good to me, thank you very much for the contribution.

adamdougal · 2024-01-15T17:05:12Z

Heya, can this be merged? Thanks

sarah-widder · 2024-01-16T19:30:34Z

app.py

@@ -41,6 +41,7 @@ def assets(path):
 SEARCH_TOP_K = os.environ.get("SEARCH_TOP_K", 5)
 SEARCH_STRICTNESS = os.environ.get("SEARCH_STRICTNESS", 3)
 SEARCH_ENABLE_IN_DOMAIN = os.environ.get("SEARCH_ENABLE_IN_DOMAIN", "true")
+AZURE_AUTH_TYPE = os.environ.get("AZURE_AUTH_TYPE", "rbac")


Can we actually have this default to "keys" and set it to "rbac" from the bicep deployment? The reason to have it default to keys is that for the case of deployment from the Azure OpenAI Studio chat playground, role assignments are not configured by default between the resources, so keys need to be used by default in that scenario.

👍 Good point! I've just pushed that change.

This is required as the OpenAI Studio does not create roles so this change would break deployments from there.

adamdougal · 2024-01-30T11:10:56Z

Heya! Is this ready to be merged now? Thanks

adamdougal · 2024-02-06T09:42:08Z

@sarah-widder Is this still required? It looks like similar changes have been made in #460?

adamdougal · 2024-02-06T09:54:59Z

I guess it's just the infra side of things that's still required?

sarah-widder · 2024-02-06T18:54:53Z

Hey @adamdougal yes, #460 should have already addressed using RBAC in the app backend, but the infra changes you were working on to automatically assign the right roles during deployment are still needed. If you want to rebase and just keep the infra changes I will give it another review. Thanks very much for your contribution!

adamdougal · 2024-02-09T13:26:11Z

I'm going to close this. It looks like the openai version bump has brought significant changes so a fresh PR/branch would probably be a better approach.

pamelafox reviewed Dec 1, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

pamelafox reviewed Dec 1, 2023

View reviewed changes

jongio suggested changes Dec 4, 2023

View reviewed changes

jongio reviewed Dec 4, 2023

View reviewed changes

.env.sample Outdated

@@ -1,3 +1,4 @@

AZURE_AUTH_TYPE

Copy link

jongio Dec 4, 2023

Choose a reason for hiding this comment

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

all the others have an =

adamdougal reacted with thumbs up emoji

jongio reviewed Dec 6, 2023

View reviewed changes

jongio mentioned this pull request Dec 6, 2023

Azdevify chat-with-your-data-solution-accelerator. jongio/chat-with-your-data-solution-accelerator#2

Merged

jongio reviewed Jan 2, 2024

View reviewed changes

adamdougal and others added 10 commits January 6, 2024 23:48

Allow authentication using RBAC instead of API Keys

6c52e81

Add required RBAC roles

bd6fa86

Remove lang from url

eee14a8

Co-authored-by: Pamela Fox <[email protected]>

Allow OpenAI authentication using RABC instead of API Keys

f049d63

Introduce auth type setting

97b9937

Rename identityPrincipalId to principalId

ad80985

Stop outputting keys

f834699

Improve docs

eca07f1

Default to rbac

4b6e7a8

apikeys -> keys

f48cca4

adamdougal added 4 commits January 6, 2024 23:48

Add missing =

477f162

Remove duplication

c3e8a57

principalId -> identityPrincipalId

c8fdc5f

Add roles to user

314aca8

adamdougal force-pushed the feature/rbac branch from 4f21291 to 314aca8 Compare January 8, 2024 09:46

jongio approved these changes Jan 8, 2024

View reviewed changes

sarah-widder approved these changes Jan 8, 2024

View reviewed changes

Merge branch 'main' into feature/rbac

52c460a

sarah-widder requested changes Jan 16, 2024

View reviewed changes

adamdougal and others added 2 commits January 17, 2024 09:15

Default to keys in app.py

dfbd8ad

This is required as the OpenAI Studio does not create roles so this change would break deployments from there.

Merge branch 'main' into feature/rbac

2e96bfd

adamdougal closed this Feb 9, 2024

HXK8 mentioned this pull request Feb 15, 2024

'Azure Search Error: 403, Server responded with status 403. Error message: Authorization failed' when using Azure RBAC instead of API keys to authenticate to Azure AI Search #624

Closed

		@@ -20,6 +20,9 @@ resource account 'Microsoft.CognitiveServices/accounts@2023-05-01' = {
		publicNetworkAccess: publicNetworkAccess

Allow authentication using RBAC instead of API Keys #427

Allow authentication using RBAC instead of API Keys #427

Conversation

adamdougal commented Dec 1, 2023

pamelafox commented Dec 1, 2023

adamdougal commented Dec 1, 2023

adamdougal commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamdougal commented Dec 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamdougal commented Dec 5, 2023

jongio commented Dec 5, 2023

adamdougal commented Dec 6, 2023

Choose a reason for hiding this comment

adamdougal commented Dec 11, 2023

jongio commented Jan 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamdougal commented Jan 8, 2024

sarah-widder commented Jan 8, 2024

sarah-widder left a comment

Choose a reason for hiding this comment

adamdougal commented Jan 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamdougal commented Jan 30, 2024

adamdougal commented Feb 6, 2024

adamdougal commented Feb 6, 2024

sarah-widder commented Feb 6, 2024

adamdougal commented Feb 9, 2024