Security: remove API key auth by gitri-ms · Pull Request #92 · microsoft/chat-copilot

Gina Triolo (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 😄

Gina Triolo (gitri-ms) · 2023-08-03T21:21:19Z


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

Gina Triolo (gitri-ms) · 2023-08-03T21:23:38Z

    [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?

Gil LaHaye (glahaye) · 2023-08-07T21:15:13Z

+
+    [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

Gil LaHaye (glahaye) · 2023-08-07T21:20:51Z

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

Gil LaHaye (glahaye) · 2023-08-07T21:21:48Z

 : "${WEB_APP_SVC_SKU:="B1"}"
 : "${WEB_APP_REGION:="westus2"}"
+: "${AZURE_AD_INSTANCE:="https://login.microsoftonline.com/"}"
+: "${AZURE_AD_TENANT_ID:="common"}"


Gil LaHaye (glahaye) · 2023-08-07T22:39:28Z

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


Default again...

Gina Triolo (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]>

feat [AB#53]: inject QSearchService

Gina Triolo (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

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

Gina Triolo (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

Gina Triolo (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

Gina Triolo (gitri-ms) removed the PR: ready for review label Aug 3, 2023

Gina Triolo (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

Gina Triolo (gitri-ms) removed the PR: ready for review label Aug 3, 2023

Gina Triolo (gitri-ms) commented Aug 3, 2023

View reviewed changes

Gina Triolo (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

Gina Triolo (gitri-ms) commented Aug 4, 2023

View reviewed changes

Comment thread webapi/Auth/AuthInfo.cs Resolved

Desmond Howard (dehoward) mentioned this pull request Aug 5, 2023

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

Closed

3 of 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

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

Gil LaHaye (glahaye) reviewed Aug 7, 2023

View reviewed changes

Gina Triolo (gitri-ms) removed the PR: ready for review label Aug 7, 2023

Gil LaHaye (glahaye) reviewed Aug 7, 2023

View reviewed changes

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

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

Gina Triolo (gitri-ms) requested review from Tao Chen (TaoChenOSU) and Ben Thomas (alliscode) August 8, 2023 17:59

Gina Triolo (gitri-ms) mentioned this pull request Aug 8, 2023

Chat Copilot Security Improvements #126

Merged

3 of 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

Gina Triolo (gitri-ms) closed this Aug 8, 2023

Jonny Toombs (jdtoombs) pushed a commit to jdtoombs/chat-copilot that referenced this pull request Apr 10, 2025

Merge pull request microsoft#92 from Quartech/inject-search-service

75ac47b

feat [AB#53]: inject QSearchService

Navigation Menu

Repository navigation

Security: remove API key auth - #92

Security: remove API key auth#92
Gina Triolo (gitri-ms) wants to merge 16 commits into
microsoft:mainfrom
gitri-ms:security

Gina Triolo (gitri-ms) commented Aug 2, 2023 •

edited

Loading

Uh oh!

Gina Triolo (gitri-ms) Aug 3, 2023 •

edited

Loading

Uh oh!

Gina Triolo (gitri-ms) Aug 3, 2023

Uh oh!

Gil LaHaye (glahaye) Aug 7, 2023

Uh oh!

Uh oh!

Gil LaHaye (glahaye) Aug 7, 2023

Uh oh!

Gina Triolo (gitri-ms) Aug 7, 2023

Uh oh!

Gil LaHaye (glahaye) Aug 7, 2023

Uh oh!

Gil LaHaye (glahaye) Aug 7, 2023

Uh oh!

Gil LaHaye (glahaye) Aug 7, 2023

Uh oh!

Gina Triolo (gitri-ms) commented Aug 8, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Gina Triolo (gitri-ms) commented Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Description

Contribution Checklist

Uh oh!

Gina Triolo (gitri-ms) Aug 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gina Triolo (gitri-ms) Aug 3, 2023

Choose a reason for hiding this comment

Uh oh!

Gil LaHaye (glahaye) Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Gil LaHaye (glahaye) Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

Gina Triolo (gitri-ms) Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

Gil LaHaye (glahaye) Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

Gil LaHaye (glahaye) Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

Gil LaHaye (glahaye) Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

Gina Triolo (gitri-ms) commented Aug 8, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Gina Triolo (gitri-ms) commented Aug 2, 2023 •

edited

Loading

Gina Triolo (gitri-ms) Aug 3, 2023 •

edited

Loading