semantic-kernel
semantic-kernel copied to clipboard
microsoft
Fix security issues in copilot chat app
Motivation and Context
Description
api key auth was removed because it was not possible to be secure in a user context. Any user was able to operate on any other user's chats. A new auth policy was introduced to validate userIds coming from tokens rather than request bodies.
Contribution Checklist
- [x] The code builds clean without any errors or warnings
- [x] The PR follows SK Contribution Guidelines (https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
- [x] I didn't break anyone :smile:
Thanks for your PR!!
Just a note for our team, this PR will create a lot of conflicts with the multi-user feature branch. In the multi-user setting, some chat-related authentication logic may need to change from this PR. For example, the data schema for ChatSession will be different in the multi-user setting. We need to talk about which one we want to let in first.
@TaoChenOSU, This PR needs to go in first because if it had been filed via MSRC, your team would have been required to drop everything to fix it immediately. Security takes precedence over new features. I chose to instead save your team all that trouble and, with the goahead from @adrianwyatt, implement it.
@TaoChenOSU, This PR needs to go in first because if it had been filed via MSRC, your team would have been required to drop everything to fix it immediately. Security takes precedence over new features. I chose to instead save your team all that trouble and, with the goahead from @adrianwyatt, implement it.
I am looking at this today and the review will likely bleed into tomorrow. Thank you for your awesome work on this!
Is there anything additional I need to perform before launching the web API and the web app besides the current instructions documented in the ReadMe? The web API throws errors when the web app tries to retrieve chat sessions. I have tried both pass through auth and AAD auth.
Thanks again for the PR and especially outlining the concerns. The team is meeting this week to discuss how to mitigate the issues in line with our near-future planning. We'll be referencing this PR and will send you the plan as an FYI as soon as we have one.
@DavidParks8 and I had a great real-time conversation and we'll be bringing in these changes piecewise over the next week or two. Closing this one in light of a set of incoming PRs.