diff --git a/src/Microsoft.Owin.Security.OpenIdConnect/Microsoft.Owin.Security.OpenIdConnect.csproj b/src/Microsoft.Owin.Security.OpenIdConnect/Microsoft.Owin.Security.OpenIdConnect.csproj index 24e5f2d2f..3715c8304 100644 --- a/src/Microsoft.Owin.Security.OpenIdConnect/Microsoft.Owin.Security.OpenIdConnect.csproj +++ b/src/Microsoft.Owin.Security.OpenIdConnect/Microsoft.Owin.Security.OpenIdConnect.csproj @@ -86,6 +86,7 @@ + diff --git a/src/Microsoft.Owin.Security.OpenIdConnect/OAuthConstants.cs b/src/Microsoft.Owin.Security.OpenIdConnect/OAuthConstants.cs new file mode 100644 index 000000000..3d744b6b4 --- /dev/null +++ b/src/Microsoft.Owin.Security.OpenIdConnect/OAuthConstants.cs @@ -0,0 +1,30 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +namespace Microsoft.Owin.Security.OpenIdConnect +{ + [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", MessageId = "Auth", + Justification = "OAuth2 is a valid word.")] + internal static class OAuthConstants + { + /// + /// code_verifier defined in https://tools.ietf.org/html/rfc7636 + /// + public const string CodeVerifierKey = "code_verifier"; + + /// + /// code_challenge defined in https://tools.ietf.org/html/rfc7636 + /// + public const string CodeChallengeKey = "code_challenge"; + + /// + /// code_challenge_method defined in https://tools.ietf.org/html/rfc7636 + /// + public const string CodeChallengeMethodKey = "code_challenge_method"; + + /// + /// S256 defined in https://tools.ietf.org/html/rfc7636 + /// + public const string CodeChallengeMethodS256 = "S256"; + } +} \ No newline at end of file diff --git a/src/Microsoft.Owin.Security.OpenIdConnect/OpenIdConnectAuthenticationOptions.cs b/src/Microsoft.Owin.Security.OpenIdConnect/OpenIdConnectAuthenticationOptions.cs index 1fe8fc432..00c5f398b 100644 --- a/src/Microsoft.Owin.Security.OpenIdConnect/OpenIdConnectAuthenticationOptions.cs +++ b/src/Microsoft.Owin.Security.OpenIdConnect/OpenIdConnectAuthenticationOptions.cs @@ -47,6 +47,7 @@ public OpenIdConnectAuthenticationOptions() /// TokenValidationParameters: new with AuthenticationType = authenticationType. /// UseTokenLifetime: true. /// RedeemCode: false. + /// UsePkce: true. /// /// will be used to when creating the for the AuthenticationType property. [SuppressMessage("Microsoft.Globalization", "CA1303:Do not pass literals as localized parameters", MessageId = "Microsoft.Owin.Security.OpenIdConnect.OpenIdConnectAuthenticationOptions.set_Caption(System.String)", Justification = "Not a LOC field")] @@ -71,6 +72,7 @@ public OpenIdConnectAuthenticationOptions(string authenticationType) UseTokenLifetime = true; CookieManager = new CookieManager(); RedeemCode = false; + UsePkce = true; } /// @@ -322,5 +324,15 @@ public bool UseTokenLifetime /// This property is set to false by default. /// public bool RedeemCode { get; set; } + + /// + /// Enables or disables the use of the Proof Key for Code Exchange (PKCE) standard. + /// This only applies when the is set to . + /// See https://tools.ietf.org/html/rfc7636. + /// The default value is `true`. + /// + [SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", MessageId = "Pkce", + Justification = "Pkce is a valid acronym.")] + public bool UsePkce { get; set; } } -} \ No newline at end of file +} diff --git a/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs b/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs index 0d54beb37..2ec402162 100644 --- a/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs +++ b/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs @@ -16,6 +16,7 @@ using Microsoft.IdentityModel.Protocols.OpenIdConnect; using Microsoft.IdentityModel.Tokens; using Microsoft.Owin.Logging; +using Microsoft.Owin.Security.DataHandler.Encoder; using Microsoft.Owin.Security.Infrastructure; using Microsoft.Owin.Security.Notifications; @@ -155,10 +156,31 @@ protected override async Task ApplyResponseChallengeAsync() RequestType = OpenIdConnectRequestType.Authentication, Resource = Options.Resource, ResponseType = Options.ResponseType, - Scope = Options.Scope, - State = OpenIdConnectAuthenticationDefaults.AuthenticationPropertiesKey + "=" + Uri.EscapeDataString(Options.StateDataFormat.Protect(properties)), + Scope = Options.Scope }; + // https://tools.ietf.org/html/rfc7636 + if (Options.UsePkce && Options.ResponseType == OpenIdConnectResponseType.Code) + { + using (RandomNumberGenerator randomNumberGenerator = RandomNumberGenerator.Create()) + using (HashAlgorithm hash = SHA256.Create()) + { + byte[] bytes = new byte[32]; + randomNumberGenerator.GetBytes(bytes); + string codeVerifier = TextEncodings.Base64Url.Encode(bytes); + + // Store this for use during the code redemption. + properties.Dictionary.Add(OAuthConstants.CodeVerifierKey, codeVerifier); + byte[] challengeBytes = hash.ComputeHash(Encoding.UTF8.GetBytes(codeVerifier)); + string codeChallenge = TextEncodings.Base64Url.Encode(challengeBytes); + + openIdConnectMessage.Parameters.Add(OAuthConstants.CodeChallengeKey, codeChallenge); + openIdConnectMessage.Parameters.Add(OAuthConstants.CodeChallengeMethodKey, OAuthConstants.CodeChallengeMethodS256); + } + } + + openIdConnectMessage.State = OpenIdConnectAuthenticationDefaults.AuthenticationPropertiesKey + "=" + Uri.EscapeDataString(Options.StateDataFormat.Protect(properties)); + // Omitting the response_mode parameter when it already corresponds to the default // response_mode used for the specified response_type is recommended by the specifications. // See http://openid.net/specs/oauth-v2-multiple-response-types-1_0.html#ResponseModes @@ -397,6 +419,15 @@ protected override async Task AuthenticateCoreAsync() RedirectUri = tokenEndpointRequest.RedirectUri, TokenEndpointRequest = tokenEndpointRequest }; + + // PKCE https://tools.ietf.org/html/rfc7636#section-4.5 + string codeVerifier; + if (properties.Dictionary.TryGetValue(OAuthConstants.CodeVerifierKey, out codeVerifier)) + { + tokenEndpointRequest.Parameters.Add(OAuthConstants.CodeVerifierKey, codeVerifier); + properties.Dictionary.Remove(OAuthConstants.CodeVerifierKey); + } + await Options.Notifications.AuthorizationCodeReceived(authorizationCodeReceivedNotification); if (authorizationCodeReceivedNotification.HandledResponse) { diff --git a/tests/Katana.Sandbox.WebServer/Startup.cs b/tests/Katana.Sandbox.WebServer/Startup.cs index 34389d875..0d5e882bc 100644 --- a/tests/Katana.Sandbox.WebServer/Startup.cs +++ b/tests/Katana.Sandbox.WebServer/Startup.cs @@ -10,6 +10,7 @@ using System.Security.Principal; using System.Threading.Tasks; using Katana.Sandbox.WebServer; +using Microsoft.IdentityModel.Protocols.OpenIdConnect; using Microsoft.Owin; using Microsoft.Owin.Extensions; using Microsoft.Owin.Host.SystemWeb; @@ -135,9 +136,9 @@ public void Configuration(IAppBuilder app) app.UseOpenIdConnectAuthentication(new Microsoft.Owin.Security.OpenIdConnect.OpenIdConnectAuthenticationOptions() { - // https://github.com/IdentityServer/IdentityServer4.Demo/blob/master/src/IdentityServer4Demo/Config.cs - ClientId = "hybrid", - ClientSecret = "secret", // for code flow + // https://github.com/IdentityServer/IdentityServer4.Demo/blob/main/src/IdentityServer4Demo/Config.cs + ClientId = "interactive.confidential.short", // client requires pkce + ClientSecret = "secret", Authority = "https://demo.identityserver.io/", RedirectUri = "https://localhost:44318/signin-oidc", /* @@ -146,11 +147,11 @@ public void Configuration(IAppBuilder app) ClientSecret = Environment.GetEnvironmentVariable("oidc:clientsecret"),*/ // CookieManager = new SystemWebCookieManager(), CookieManager = new SameSiteCookieManager(), - //ResponseType = "code", - //ResponseMode = "query", - //SaveTokens = true, - //Scope = "openid profile offline_access", - //RedeemCode = true, + ResponseType = OpenIdConnectResponseType.Code, + ResponseMode = OpenIdConnectResponseMode.Query, + SaveTokens = true, + Scope = "openid profile offline_access", + RedeemCode = true, //Notifications = new Microsoft.Owin.Security.OpenIdConnect.OpenIdConnectAuthenticationNotifications //{ // AuthorizationCodeReceived = async n => @@ -166,6 +167,7 @@ public void Configuration(IAppBuilder app) // n.HandleCodeRedemption(message); // } //} + UsePkce = true, }); /* diff --git a/tests/Microsoft.Owin.Security.Tests/Microsoft.Owin.Security.Tests.csproj b/tests/Microsoft.Owin.Security.Tests/Microsoft.Owin.Security.Tests.csproj index 94d867374..49943bb1c 100644 --- a/tests/Microsoft.Owin.Security.Tests/Microsoft.Owin.Security.Tests.csproj +++ b/tests/Microsoft.Owin.Security.Tests/Microsoft.Owin.Security.Tests.csproj @@ -43,6 +43,12 @@ ..\..\packages\Microsoft.IdentityModel.Logging.5.3.0\lib\net45\Microsoft.IdentityModel.Logging.dll True + + ..\..\packages\Microsoft.IdentityModel.Protocols.5.3.0\lib\net45\Microsoft.IdentityModel.Protocols.dll + + + ..\..\packages\Microsoft.IdentityModel.Protocols.OpenIdConnect.5.3.0\lib\net45\Microsoft.IdentityModel.Protocols.OpenIdConnect.dll + ..\..\packages\Microsoft.IdentityModel.Tokens.5.3.0\lib\net45\Microsoft.IdentityModel.Tokens.dll True @@ -89,6 +95,7 @@ + diff --git a/tests/Microsoft.Owin.Security.Tests/OpenIdConnect/OpenIdConnectMiddlewareTests.cs b/tests/Microsoft.Owin.Security.Tests/OpenIdConnect/OpenIdConnectMiddlewareTests.cs new file mode 100644 index 000000000..e93cd71e0 --- /dev/null +++ b/tests/Microsoft.Owin.Security.Tests/OpenIdConnect/OpenIdConnectMiddlewareTests.cs @@ -0,0 +1,182 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Net; +using System.Net.Http; +using System.Threading.Tasks; +using System.Xml.Linq; +using Microsoft.IdentityModel.Protocols.OpenIdConnect; +using Microsoft.Owin.Security.Cookies; +using Microsoft.Owin.Security.OpenIdConnect; +using Microsoft.Owin.Testing; +using Owin; +using Xunit; +using Xunit.Extensions; + +namespace Microsoft.Owin.Security.Tests.OpenIdConnect +{ + public class OpenIdConnectMiddlewareTests + { + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task ChallengeIncludesPkceIfRequested(bool include) + { + var options = new OpenIdConnectAuthenticationOptions() + { + Authority = "https://demo.identityserver.io", + ClientId = "Test Client Id", + ClientSecret = "Test Client Secret", + UsePkce = include, + ResponseType = OpenIdConnectResponseType.Code + }; + var server = CreateServer( + app => app.UseOpenIdConnectAuthentication(options), + context => + { + context.Authentication.Challenge("OpenIdConnect"); + return true; + }); + + var transaction = await SendAsync(server, "http://example.com/challenge"); + + var res = transaction.Response; + Assert.Equal(HttpStatusCode.Redirect, res.StatusCode); + Assert.NotNull(res.Headers.Location); + + if (include) + { + Assert.Contains("code_challenge=", res.Headers.Location.Query); + Assert.Contains("code_challenge_method=S256", res.Headers.Location.Query); + } + else + { + Assert.DoesNotContain("code_challenge=", res.Headers.Location.Query); + Assert.DoesNotContain("code_challenge_method=", res.Headers.Location.Query); + } + } + + [Theory] + [InlineData(OpenIdConnectResponseType.Token)] + [InlineData(OpenIdConnectResponseType.IdToken)] + [InlineData(OpenIdConnectResponseType.CodeIdToken)] + public async Task ChallengeDoesNotIncludePkceForOtherResponseTypes(string responseType) + { + var options = new OpenIdConnectAuthenticationOptions() + { + Authority = "https://demo.identityserver.io", + ClientId = "Test Client Id", + ClientSecret = "Test Client Secret", + UsePkce = true, + ResponseType = responseType + }; + var server = CreateServer( + app => app.UseOpenIdConnectAuthentication(options), + context => + { + context.Authentication.Challenge("OpenIdConnect"); + return true; + }); + + var transaction = await SendAsync(server, "http://example.com/challenge"); + + var res = transaction.Response; + Assert.Equal(HttpStatusCode.Redirect, res.StatusCode); + Assert.NotNull(res.Headers.Location); + + Assert.DoesNotContain("code_challenge=", res.Headers.Location.Query); + Assert.DoesNotContain("code_challenge_method=", res.Headers.Location.Query); + } + + + private static TestServer CreateServer(Action configure, Func handler) + { + return TestServer.Create(app => + { + app.Properties["host.AppName"] = "OpenIdConnect.Owin.Security.Tests"; + app.UseCookieAuthentication(new CookieAuthenticationOptions + { + AuthenticationType = "External" + }); + app.SetDefaultSignInAsAuthenticationType("External"); + if (configure != null) + { + configure(app); + } + app.Use(async (context, next) => + { + if (handler == null || !handler(context)) + { + await next(); + } + }); + }); + } + + private static async Task SendAsync(TestServer server, string uri, string cookieHeader = null) + { + var request = new HttpRequestMessage(HttpMethod.Get, uri); + if (!string.IsNullOrEmpty(cookieHeader)) + { + request.Headers.Add("Cookie", cookieHeader); + } + var transaction = new Transaction + { + Request = request, + Response = await server.HttpClient.SendAsync(request), + }; + if (transaction.Response.Headers.Contains("Set-Cookie")) + { + transaction.SetCookie = transaction.Response.Headers.GetValues("Set-Cookie").ToList(); + } + transaction.ResponseText = await transaction.Response.Content.ReadAsStringAsync(); + + if (transaction.Response.Content != null && + transaction.Response.Content.Headers.ContentType != null && + transaction.Response.Content.Headers.ContentType.MediaType == "text/xml") + { + transaction.ResponseElement = XElement.Parse(transaction.ResponseText); + } + return transaction; + } + + private class Transaction + { + public HttpRequestMessage Request { get; set; } + public HttpResponseMessage Response { get; set; } + public IList SetCookie { get; set; } + public string ResponseText { get; set; } + public XElement ResponseElement { get; set; } + + public string AuthenticationCookieValue + { + get + { + if (SetCookie != null && SetCookie.Count > 0) + { + var authCookie = SetCookie.SingleOrDefault(c => c.Contains(".AspNet.External=")); + if (authCookie != null) + { + return authCookie.Substring(0, authCookie.IndexOf(';')); + } + } + + return null; + } + } + + public string FindClaimValue(string claimType) + { + XElement claim = ResponseElement.Elements("claim").SingleOrDefault(elt => elt.Attribute("type").Value == claimType); + if (claim == null) + { + return null; + } + return claim.Attribute("value").Value; + } + } + } +} diff --git a/tests/Microsoft.Owin.Security.Tests/packages.config b/tests/Microsoft.Owin.Security.Tests/packages.config index ba008f48d..e810e7063 100644 --- a/tests/Microsoft.Owin.Security.Tests/packages.config +++ b/tests/Microsoft.Owin.Security.Tests/packages.config @@ -2,6 +2,8 @@ + +