Security: remove API key auth #92

gitri-ms · 2023-08-02T16:35:46Z

Motivation and Context

This PR addresses microsoft/semantic-kernel#1639

Description

Remove API key authorization
Use "AzureAD" as default authentication configuration for deployments, "None" for running locally (Note: UI changes to disable sign in flow for the latter case are still forthcoming)
Enable auth policy on controllers that checks if the user is part of the conversation they are trying to access
Update routes to better align to Microsoft API design principles

Breaking change: This PR changes the contract between the frontend and backend around how user IDs are communicated. Users who have been signing into the frontend with AAD will now only see their chats if the backend is also gated by AAD authentication, which was not the case previously.

Contribution Checklist

The code builds clean without any errors or warnings
The PR follows the Contribution Guidelines and the pre-submission formatting script raises no violations
All unit tests pass, and I have added new tests where possible
I didn't break anyone 😄

gitri-ms · 2023-08-03T21:21:19Z

deploy/README.md

@@ -112,8 +112,6 @@ This will get you to the CORS page where you can add your allowed hosts.

 ## Authorization
 All of endpoints (except `/healthz`) require authorization to access.
-By default, an API key is required for access which can be found in the `Authorization:ApiKey` configuration setting.


TODO: update this section. Also update deployment commands

gitri-ms · 2023-08-03T21:23:38Z

deploy/deploy-webapp.ps1

    [string]
    # Authority for client applications that are not configured as multi-tenant.
-    $Authority="https://login.microsoftonline.com/common"
+    $Authority = "https://login.microsoftonline.com/common"


There is some weirdness here -- the backend expects instance + tenant ID, while the frontend expects authority. However the values need to match. This could be confusing for the user. Perhaps change to make them more consistent (e.g. both use instance + tenant id)?

What's the format expected from the client side?

webapi/Auth/AuthInfo.cs

glahaye · 2023-08-07T21:15:13Z

deploy/deploy-azure.ps1

+
+    [string]
+    # Azure AD tenant ID for authenticating users
+    $AzureAdTenantId = "common",


As discussed during bug bash, we might want to not put "common" as the default and force users to provide a tenant

Yup, I am currently going through and changing all of these

glahaye · 2023-08-07T21:20:51Z

deploy/deploy-azure.sh

    echo "  -ai, --ai-service AI_SERVICE_TYPE          Type of AI service to use (i.e., OpenAI or AzureOpenAI)"
    echo "  -aikey, --ai-service-key AI_SERVICE_KEY    API key for existing Azure OpenAI resource or OpenAI account"
    echo "  -aiend, --ai-endpoint AI_ENDPOINT          Endpoint for existing Azure OpenAI resource"
    echo "  -rg, --resource-group RESOURCE_GROUP       Resource group to which to make the deployment (default: \"rg-\$DEPLOYMENT_NAME\")"
    echo "  -r, --region REGION                        Region to which to make the deployment (default: \"South Central US\")"
    echo "  -wr, --web-app-region WEB_APP_REGION       Region to deploy to the static web app into. This must be a region that supports static web apps. (default: \"West US 2\")"
    echo "  -a, --app-service-sku WEB_APP_SVC_SKU      SKU for the Azure App Service plan (default: \"B1\")"
+    echo "  -i, --instance AZURE_AD_INSTANCE           Azure AD cloud instance for authenticating users"
+    echo "                                             (default: \"https://login.microsoftonline.com/\")"
+    echo "  -t, --tenant-id AZURE_AD_TENANT_ID         Azure AD tenant ID for authenticating users (default: \"common\")"


Again, might want to make this mandatory...

glahaye · 2023-08-07T21:21:48Z

deploy/deploy-azure.sh

@@ -171,20 +179,24 @@ az account set -s "$SUBSCRIPTION"
 : "${REGION:="southcentralus"}"
 : "${WEB_APP_SVC_SKU:="B1"}"
 : "${WEB_APP_REGION:="westus2"}"
+: "${AZURE_AD_INSTANCE:="https://login.microsoftonline.com/"}"
+: "${AZURE_AD_TENANT_ID:="common"}"


glahaye · 2023-08-07T22:39:28Z

deploy/main.bicep

+param azureAdInstance string = environment().authentication.loginEndpoint
+
+@description('Azure AD tenant ID for authenticating users')
+param azureAdTenantId string = 'common'


Default again...

gitri-ms · 2023-08-08T18:19:22Z

Closing this PR as it is now covered by #126

### Motivation and Context This PR addresses microsoft/semantic-kernel#1639. It is a combination of PRs #92 and #110 ### Description #### Backend changes - Remove API key authorization - Use "AzureAD" as default authentication configuration for deployments, "None" for running locally (Note: UI changes to disable sign in flow for the latter case are still forthcoming) - Enable auth policy on controllers that checks if the user is part of the conversation they are trying to access This PR changes the contract between the frontend and backend around how user IDs are communicated. Users who have been signing into the frontend with AAD will now only see their chats if the backend is also gated by AAD authentication, which was not the case previously. #### Frontend changes - adds `REACT_APP_AUTH_TYPE` and changes AAD variables in `.env` to be optional - adds `AuthHelper.IsAuthAAD` to conditionally render different elements throughout the app - changes user settings menu popup to instead just show as a settings button: ![image](https://github.com/microsoft/chat-copilot/assets/52973358/342f977d-d011-464d-b122-5eff5f8222ac) Existing users will need to uncomment `REACT_APP_AUTH_TYPE=AzureAd` in `webapp/.env` to continue using AAD as their authorization type. ### Contribution Checklist  - [x] The code builds clean without any errors or warnings - [x] The PR follows the [Contribution Guidelines](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md) and the [pre-submission formatting script](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md#development-scripts) raises no violations - [ ] All unit tests pass, and I have added new tests where possible - [x] I didn't break anyone 😄 --------- Co-authored-by: Desmond Howard <[email protected]>

### Motivation and Context This PR addresses microsoft/semantic-kernel#1639. It is a combination of PRs microsoft#92 and microsoft#110 ### Description #### Backend changes - Remove API key authorization - Use "AzureAD" as default authentication configuration for deployments, "None" for running locally (Note: UI changes to disable sign in flow for the latter case are still forthcoming) - Enable auth policy on controllers that checks if the user is part of the conversation they are trying to access This PR changes the contract between the frontend and backend around how user IDs are communicated. Users who have been signing into the frontend with AAD will now only see their chats if the backend is also gated by AAD authentication, which was not the case previously. #### Frontend changes - adds `REACT_APP_AUTH_TYPE` and changes AAD variables in `.env` to be optional - adds `AuthHelper.IsAuthAAD` to conditionally render different elements throughout the app - changes user settings menu popup to instead just show as a settings button: ![image](https://github.com/microsoft/chat-copilot/assets/52973358/342f977d-d011-464d-b122-5eff5f8222ac) Existing users will need to uncomment `REACT_APP_AUTH_TYPE=AzureAd` in `webapp/.env` to continue using AAD as their authorization type. ### Contribution Checklist  - [x] The code builds clean without any errors or warnings - [x] The PR follows the [Contribution Guidelines](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md) and the [pre-submission formatting script](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md#development-scripts) raises no violations - [ ] All unit tests pass, and I have added new tests where possible - [x] I didn't break anyone 😄 --------- Co-authored-by: Desmond Howard <[email protected]>

gitri-ms added 6 commits August 1, 2023 09:15

Begin porting security fixes - does not build

183792b

Security updates to backend webapi

b61abe1

Update ChatParticipantsController

8232749

Update routes on FE

d81c067

Merge branch 'main' into security

0ee5c96

Code cleanup

65de3b9

github-actions bot added webapp Pull requests that update Typescript code webapi Pull requests that update .net code deployment Issues related to deploying Chat-Copilot PR: ready for review github actions Pull requests that update GitHub Actions code labels Aug 2, 2023

gitri-ms added PR: in progress and removed PR: ready for review labels Aug 2, 2023

gitri-ms self-assigned this Aug 2, 2023

Fix claim names, remove api key references

59aaeb0

github-actions bot added documentation Improvements or additions to documentation PR: ready for review labels Aug 2, 2023

gitri-ms added 3 commits August 2, 2023 16:16

Fix policy and unblock patch requests

369f60c

Merge branch 'main' into security

045784b

Merge branch 'main' into security

c2535f9

gitri-ms removed the PR: ready for review label Aug 3, 2023

gitri-ms added 2 commits August 3, 2023 14:16

Update deploy scripts to use AAD auth by default

2079c04

Merge branch 'main' into security

cf65b19

github-actions bot added the PR: ready for review label Aug 3, 2023

gitri-ms removed the PR: ready for review label Aug 3, 2023

gitri-ms commented Aug 3, 2023

View reviewed changes

gitri-ms added 3 commits August 3, 2023 16:44

Update deployment pipeline

cb5bb18

Update deploy scripts to set API scopes in env, fix bug in policy

a637834

Merge branch 'main' into security

7e9f5e5

github-actions bot added the PR: ready for review label Aug 4, 2023

Fix bicep lint error, include tenant id in user id on server side

43bf249

gitri-ms commented Aug 4, 2023

View reviewed changes

webapi/Auth/AuthInfo.cs Show resolved Hide resolved

dehoward mentioned this pull request Aug 5, 2023

Change AAD authorization to be opt-in within webapp #110

Closed

4 tasks

This was linked to issues Aug 7, 2023

CC: Restrict API key auth for service-to-service microsoft/semantic-kernel#1639

Closed

copilot-chat ChatHistory controller neither authenticates nor validates message ownership microsoft/semantic-kernel#827

Closed

gitri-ms added the PR: breaking change Pull requests that introduce breaking changes label Aug 7, 2023

glahaye reviewed Aug 7, 2023

View reviewed changes

gitri-ms removed the PR: ready for review label Aug 7, 2023

glahaye reviewed Aug 7, 2023

View reviewed changes

gitri-ms added PR: ready for review and removed PR: in progress labels Aug 8, 2023

gitri-ms marked this pull request as ready for review August 8, 2023 05:28

gitri-ms requested review from TaoChenOSU and alliscode August 8, 2023 17:59

gitri-ms mentioned this pull request Aug 8, 2023

Chat Copilot Security Improvements #126

Merged

4 tasks

This was unlinked from issues Aug 8, 2023

copilot-chat ChatHistory controller neither authenticates nor validates message ownership microsoft/semantic-kernel#827

Closed

CC: Restrict API key auth for service-to-service microsoft/semantic-kernel#1639

Closed

gitri-ms closed this Aug 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Security: remove API key auth #92

Security: remove API key auth #92

gitri-ms commented Aug 2, 2023 •

edited

Loading

gitri-ms Aug 3, 2023 •

edited

Loading

gitri-ms Aug 3, 2023

glahaye Aug 7, 2023

glahaye Aug 7, 2023

gitri-ms Aug 7, 2023

glahaye Aug 7, 2023

glahaye Aug 7, 2023

glahaye Aug 7, 2023

gitri-ms commented Aug 8, 2023

Security: remove API key auth #92

Security: remove API key auth #92

Conversation

gitri-ms commented Aug 2, 2023 • edited Loading

Motivation and Context

Description

Contribution Checklist

gitri-ms Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

gitri-ms Aug 3, 2023

Choose a reason for hiding this comment

glahaye Aug 7, 2023

Choose a reason for hiding this comment

glahaye Aug 7, 2023

Choose a reason for hiding this comment

gitri-ms Aug 7, 2023

Choose a reason for hiding this comment

glahaye Aug 7, 2023

Choose a reason for hiding this comment

glahaye Aug 7, 2023

Choose a reason for hiding this comment

glahaye Aug 7, 2023

Choose a reason for hiding this comment

gitri-ms commented Aug 8, 2023

gitri-ms commented Aug 2, 2023 •

edited

Loading

gitri-ms Aug 3, 2023 •

edited

Loading