-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Fix security issues in copilot chat app #1090
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c23fa92
f352042
142ca11
6d10c37
7f76bad
6719f2f
03d7ea9
dd74c84
a7b6953
91b333b
8b4fb97
7e0b1ae
7014e91
f14886d
93b88d8
63d54c6
33bb80a
c7ec92c
eae5abe
6b4b6a7
2e81779
5ca3f27
4a9cbb0
5e35abc
669e882
935eafb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| // Copyright (c) Microsoft. All rights reserved. | ||
|
|
||
| using System; | ||
| using Azure.Identity; | ||
| using Microsoft.AspNetCore.Http; | ||
| using Microsoft.IdentityModel.JsonWebTokens; | ||
|
|
||
| namespace SemanticKernel.Service.Auth; | ||
|
|
||
| /// <summary> | ||
| /// Class which provides validated security information for use in controllers. | ||
| /// </summary> | ||
| public class AuthInfo : IAuthInfo | ||
| { | ||
| private record struct AuthData( | ||
| string UserId, | ||
| string UserName); | ||
|
|
||
| private readonly Lazy<AuthData> _data; | ||
|
|
||
| public AuthInfo(IHttpContextAccessor httpContextAccessor) | ||
| { | ||
| this._data = new Lazy<AuthData>(() => | ||
| { | ||
| var user = httpContextAccessor.HttpContext?.User; | ||
| if (user is null) | ||
| { | ||
| throw new InvalidOperationException("HttpContext must be present to inspect auth info."); | ||
| } | ||
| var userIdClaim = user.FindFirst("oid") | ||
| ?? user.FindFirst(JwtRegisteredClaimNames.Sub); | ||
|
|
||
| if (userIdClaim is null) | ||
| { | ||
| throw new CredentialUnavailableException("User Id was not present in the request token."); | ||
| } | ||
|
|
||
| var userNameClaim = user.FindFirst(JwtRegisteredClaimNames.Name); | ||
| if (userNameClaim is null) | ||
| { | ||
| throw new CredentialUnavailableException("User name was not present in the request token."); | ||
| } | ||
|
|
||
| return new AuthData | ||
| { | ||
| UserId = userIdClaim.Value, | ||
| UserName = userNameClaim.Value, | ||
| }; | ||
| }, isThreadSafe: false); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// The authenticated user's unique ID. | ||
| /// </summary> | ||
| public string UserId => this._data.Value.UserId; | ||
|
|
||
| /// <summary> | ||
| /// The authenticated user's name. | ||
| /// </summary> | ||
| public string Name => this._data.Value.UserName; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| // Copyright (c) Microsoft. All rights reserved. | ||
|
|
||
| namespace SemanticKernel.Service.Auth; | ||
|
|
||
| public interface IAuthInfo | ||
| { | ||
| /// <summary> | ||
| /// The authenticated user's unique ID. | ||
| /// </summary> | ||
| public string UserId { get; } | ||
|
|
||
| /// <summary> | ||
| /// The authenticated user's name. | ||
| /// </summary> | ||
| public string Name { get; } | ||
| } |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,15 +3,17 @@ | |
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.AspNetCore.Authorization; | ||
| using Microsoft.AspNetCore.Http; | ||
| using Microsoft.AspNetCore.Mvc; | ||
| using Microsoft.Extensions.Logging; | ||
| using Microsoft.SemanticKernel; | ||
| using Microsoft.SemanticKernel.AI; | ||
| using Microsoft.SemanticKernel.Orchestration; | ||
| using Microsoft.SemanticKernel.SkillDefinition; | ||
| using SemanticKernel.Service.Auth; | ||
| using SemanticKernel.Service.CopilotChat.Storage; | ||
| using SemanticKernel.Service.Models; | ||
| using SemanticKernel.Service.Utilities; | ||
|
|
||
| namespace SemanticKernel.Service.Controllers; | ||
|
|
||
|
|
@@ -34,34 +36,45 @@ public SemanticKernelController(ILogger<SemanticKernelController> logger) | |
| /// and attempt to invoke the function with the given name. | ||
| /// </remarks> | ||
| /// <param name="kernel">Semantic kernel obtained through dependency injection</param> | ||
| /// <param name="askConverter">Converter to use for converting Asks.</param> | ||
| /// <param name="chatSessionRepository">Storage for chat sessions.</param> | ||
| /// <param name="authInfo">Authenticated info about the user for the current request.</param> | ||
| /// <param name="ask">Prompt along with its parameters</param> | ||
| /// <param name="skillName">Skill in which function to invoke resides</param> | ||
| /// <param name="functionName">Name of function to invoke</param> | ||
| /// <returns>Results consisting of text generated by invoked function along with the variable in the Semantic Kernel that generated it</returns> | ||
| [Authorize] | ||
|
DavidParks8 marked this conversation as resolved.
|
||
| [Route("skills/{skillName}/functions/{functionName}/invoke")] | ||
| [HttpPost] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| public async Task<ActionResult<AskResult>> InvokeFunctionAsync( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This controller is a generic controller to invoke SK functions. It should not contain any chat related components.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yet, it must contain the minimum components required to validate that the user is allowed to pass specific context variables. Otherwise, users will be able to invoke skills for chats they do not own. Validation needs to be here as well as within skills as part of a defense in depth strategy.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This controller will not be used for any chat related features. Maybe we need to be explicit about it.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regardless of what it will be used for in the app, it still allows for remote execution of the chat skill if a bad actor constructs the proper request. This must be guarded against. When it comes to backend security, the frontend's default usecase is not relevant. What one could do when calling the backend is what matters. We see people take tokens out of browsers and automate attacks against backends all the time.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally agreed! Need further discussion with @adrianwyatt and @hathind-ms |
||
| [FromServices] IKernel kernel, | ||
| [FromServices] AskConverter askConverter, | ||
| [FromServices] ChatSessionRepository chatSessionRepository, | ||
| [FromServices] IAuthInfo authInfo, | ||
| [FromBody] Ask ask, | ||
| string skillName, string functionName) | ||
| { | ||
| this._logger.LogDebug("Received call to invoke {SkillName}/{FunctionName}", skillName, functionName); | ||
|
|
||
| if (string.IsNullOrWhiteSpace(ask.Input)) | ||
| const string chatIdKey = "chatId"; | ||
| var chatIdFromContext = ask.Variables.FirstOrDefault(x => x.Key == chatIdKey); | ||
| if (chatIdFromContext.Key is chatIdKey) | ||
| { | ||
| return this.BadRequest("Input is required."); | ||
| var chat = await chatSessionRepository.FindByIdAsync(chatIdFromContext.Value); | ||
| if (chat == null) | ||
| { | ||
| return this.NotFound("Failed to find chat session for the chatId specified in variables."); | ||
| } | ||
| if (chat.UserId != authInfo.UserId) | ||
| { | ||
| return this.Unauthorized("User does not have access to the chatId specified in variables."); | ||
| } | ||
| } | ||
|
|
||
| // Put ask's variables in the context we will use. | ||
| var contextVariables = new ContextVariables(ask.Input); | ||
| foreach (var input in ask.Variables) | ||
| { | ||
| contextVariables.Set(input.Key, input.Value); | ||
| } | ||
| var contextVariables = askConverter.GetContextVariables(ask); | ||
|
|
||
| // Get the function to invoke | ||
| ISKFunction? function = null; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| // Copyright (c) Microsoft. All rights reserved. | ||
|
|
||
| namespace SemanticKernel.Service.CopilotChat.Auth; | ||
|
|
||
| /// <summary> | ||
| /// Holds the policy names for custom authorization policies. | ||
| /// </summary> | ||
| public static class AuthPolicyName | ||
| { | ||
| public const string RequireChatOwner = "RequireChatOwner"; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| // Copyright (c) Microsoft. All rights reserved. | ||
|
|
||
| using System.Threading.Tasks; | ||
| using Microsoft.AspNetCore.Authorization; | ||
| using Microsoft.AspNetCore.Http; | ||
| using Microsoft.AspNetCore.Routing; | ||
| using SemanticKernel.Service.Auth; | ||
| using SemanticKernel.Service.CopilotChat.Storage; | ||
|
|
||
| namespace SemanticKernel.Service.CopilotChat.Auth; | ||
|
|
||
| /// <summary> | ||
| /// Class implementing "authorization" that validates the user has access to a chat. | ||
| /// </summary> | ||
| public class ChatOwnerAuthorizationHandler : AuthorizationHandler<ChatOwnerRequirement, HttpContext> | ||
| { | ||
| private readonly IAuthInfo _authInfo; | ||
| private readonly ChatSessionRepository _chatSessionRepository; | ||
|
|
||
| /// <summary> | ||
| /// Constructor | ||
| /// </summary> | ||
| public ChatOwnerAuthorizationHandler( | ||
| IAuthInfo authInfo, | ||
| ChatSessionRepository chatSessionRepository) : base() | ||
| { | ||
| this._authInfo = authInfo; | ||
| this._chatSessionRepository = chatSessionRepository; | ||
| } | ||
|
|
||
| protected override async Task HandleRequirementAsync( | ||
| AuthorizationHandlerContext context, | ||
| ChatOwnerRequirement requirement, | ||
| HttpContext resource) | ||
| { | ||
| string? chatId = resource.GetRouteValue("chatId")?.ToString(); | ||
| if (chatId == null) | ||
| { | ||
| // delegate to downstream validation | ||
| context.Succeed(requirement); | ||
| return; | ||
| } | ||
|
|
||
| var session = await this._chatSessionRepository.FindByIdAsync(chatId); | ||
| if (session == null) | ||
| { | ||
| // delegate to downstream validation | ||
| context.Succeed(requirement); | ||
| return; | ||
| } | ||
|
|
||
| if (session.UserId != this._authInfo.UserId) | ||
| { | ||
| context.Fail(new AuthorizationFailureReason(this, "User does not have access to the requested chat.")); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| // Copyright (c) Microsoft. All rights reserved. | ||
|
|
||
| using Microsoft.AspNetCore.Authorization; | ||
|
|
||
| namespace SemanticKernel.Service.CopilotChat.Auth; | ||
|
|
||
| /// <summary> | ||
| /// Used to require the chat to be owned by the authenticated user. | ||
| /// </summary> | ||
| public class ChatOwnerRequirement : IAuthorizationRequirement | ||
| { | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.