From 879f0b7f4053a34b44e34bd22c788c6129115175 Mon Sep 17 00:00:00 2001 From: Javier Calvarro Nelson Date: Tue, 13 Jun 2017 09:10:14 -0700 Subject: [PATCH] [Fixes #1133] Limit the path on the nonce and correlation id cookies --- .../OpenIdConnectHandler.cs | 22 ++-- .../OpenIdConnectOptions.cs | 6 + .../TwitterHandler.cs | 4 + .../TwitterOptions.cs | 6 + .../RemoteAuthenticationHandler.cs | 7 ++ .../RemoteAuthenticationOptions.cs | 6 + .../OAuthTests.cs | 65 +++++++++++ .../OpenIdConnect/OpenIdConnectTests.cs | 105 +++++++++++++++++- 8 files changed, 213 insertions(+), 8 deletions(-) diff --git a/src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectHandler.cs b/src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectHandler.cs index f4dcea4c8..1dec54197 100644 --- a/src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectHandler.cs +++ b/src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectHandler.cs @@ -886,16 +886,21 @@ private void WriteNonceCookie(string nonce) throw new ArgumentNullException(nameof(nonce)); } + var options = new CookieOptions + { + HttpOnly = true, + SameSite = Http.SameSiteMode.None, + Path = OriginalPathBase + Options.CallbackPath, + Secure = Request.IsHttps, + Expires = Clock.UtcNow.Add(Options.ProtocolValidator.NonceLifetime) + }; + + Options.ConfigureNonceCookie?.Invoke(Context, options); + Response.Cookies.Append( OpenIdConnectDefaults.CookieNoncePrefix + Options.StringDataFormat.Protect(nonce), NonceProperty, - new CookieOptions - { - HttpOnly = true, - SameSite = Http.SameSiteMode.None, - Secure = Request.IsHttps, - Expires = Clock.UtcNow.Add(Options.ProtocolValidator.NonceLifetime) - }); + options); } /// @@ -924,10 +929,13 @@ private string ReadNonceCookie(string nonce) var cookieOptions = new CookieOptions { HttpOnly = true, + Path = OriginalPathBase + Options.CallbackPath, SameSite = Http.SameSiteMode.None, Secure = Request.IsHttps }; + Options.ConfigureNonceCookie?.Invoke(Context, cookieOptions); + Response.Cookies.Delete(nonceKey, cookieOptions); return nonce; } diff --git a/src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectOptions.cs b/src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectOptions.cs index a199016dd..a42f0eba3 100644 --- a/src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectOptions.cs +++ b/src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectOptions.cs @@ -262,5 +262,11 @@ public override void Validate() /// remote OpenID Connect provider as an authorization/logout request parameter. /// public bool DisableTelemetry { get; set; } + + /// + /// Gets or sets an action that can override the nonce cookie options before the + /// cookie gets added to the response. + /// + public Action ConfigureNonceCookie { get; set; } } } diff --git a/src/Microsoft.AspNetCore.Authentication.Twitter/TwitterHandler.cs b/src/Microsoft.AspNetCore.Authentication.Twitter/TwitterHandler.cs index 0eab83e41..baa4320d1 100644 --- a/src/Microsoft.AspNetCore.Authentication.Twitter/TwitterHandler.cs +++ b/src/Microsoft.AspNetCore.Authentication.Twitter/TwitterHandler.cs @@ -87,6 +87,8 @@ protected override async Task HandleRemoteAuthenticateAsync( Secure = Request.IsHttps }; + Options.ConfigureStateCookie?.Invoke(Context, cookieOptions); + Response.Cookies.Delete(StateCookie, cookieOptions); var accessToken = await ObtainAccessTokenAsync(requestToken, oauthVerifier); @@ -159,6 +161,8 @@ protected override async Task HandleChallengeAsync(AuthenticationProperties prop Expires = Clock.UtcNow.Add(Options.RemoteAuthenticationTimeout), }; + Options.ConfigureStateCookie?.Invoke(Context, cookieOptions); + Response.Cookies.Append(StateCookie, Options.StateDataFormat.Protect(requestToken), cookieOptions); var redirectContext = new TwitterRedirectToAuthorizationEndpointContext(Context, Scheme, Options, properties, twitterAuthenticationEndpoint); diff --git a/src/Microsoft.AspNetCore.Authentication.Twitter/TwitterOptions.cs b/src/Microsoft.AspNetCore.Authentication.Twitter/TwitterOptions.cs index cf1bf4856..8b57f2502 100644 --- a/src/Microsoft.AspNetCore.Authentication.Twitter/TwitterOptions.cs +++ b/src/Microsoft.AspNetCore.Authentication.Twitter/TwitterOptions.cs @@ -58,6 +58,12 @@ public TwitterOptions() /// public ISecureDataFormat StateDataFormat { get; set; } + /// + /// Gets or sets an action that can override the state cookie options before the + /// cookie gets added to the response. + /// + public Action ConfigureStateCookie { get; set; } + /// /// Gets or sets the used to handle authentication events. /// diff --git a/src/Microsoft.AspNetCore.Authentication/RemoteAuthenticationHandler.cs b/src/Microsoft.AspNetCore.Authentication/RemoteAuthenticationHandler.cs index 2ff06062d..69c926cc0 100644 --- a/src/Microsoft.AspNetCore.Authentication/RemoteAuthenticationHandler.cs +++ b/src/Microsoft.AspNetCore.Authentication/RemoteAuthenticationHandler.cs @@ -205,9 +205,12 @@ protected virtual void GenerateCorrelationId(AuthenticationProperties properties HttpOnly = true, SameSite = SameSiteMode.None, Secure = Request.IsHttps, + Path = OriginalPathBase + Options.CallbackPath, Expires = Clock.UtcNow.Add(Options.RemoteAuthenticationTimeout), }; + Options.ConfigureCorrelationIdCookie?.Invoke(Context, cookieOptions); + properties.Items[CorrelationProperty] = correlationId; var cookieName = CorrelationPrefix + Scheme.Name + "." + correlationId; @@ -243,9 +246,13 @@ protected virtual bool ValidateCorrelationId(AuthenticationProperties properties var cookieOptions = new CookieOptions { HttpOnly = true, + Path = OriginalPathBase + Options.CallbackPath, SameSite = SameSiteMode.None, Secure = Request.IsHttps }; + + Options.ConfigureCorrelationIdCookie?.Invoke(Context, cookieOptions); + Response.Cookies.Delete(cookieName, cookieOptions); if (!string.Equals(correlationCookie, CorrelationMarker, StringComparison.Ordinal)) diff --git a/src/Microsoft.AspNetCore.Authentication/RemoteAuthenticationOptions.cs b/src/Microsoft.AspNetCore.Authentication/RemoteAuthenticationOptions.cs index 65cf6f2ec..066ca963a 100644 --- a/src/Microsoft.AspNetCore.Authentication/RemoteAuthenticationOptions.cs +++ b/src/Microsoft.AspNetCore.Authentication/RemoteAuthenticationOptions.cs @@ -87,5 +87,11 @@ public override void Validate() /// the size of the final authentication cookie. /// public bool SaveTokens { get; set; } + + /// + /// Gets or sets an action that can override the correlation id cookie options before the + /// cookie gets added to the response. + /// + public Action ConfigureCorrelationIdCookie { get; set; } } } diff --git a/test/Microsoft.AspNetCore.Authentication.Test/OAuthTests.cs b/test/Microsoft.AspNetCore.Authentication.Test/OAuthTests.cs index 2381fd6cd..dcc96c094 100644 --- a/test/Microsoft.AspNetCore.Authentication.Test/OAuthTests.cs +++ b/test/Microsoft.AspNetCore.Authentication.Test/OAuthTests.cs @@ -4,6 +4,7 @@ using System; using System.Net; using System.Threading.Tasks; +using Microsoft.AspNetCore.Authentication.Cookies; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.Http; @@ -165,6 +166,70 @@ public async Task ThrowsIfSignInSchemeMissing() Assert.Equal(HttpStatusCode.OK, transaction.Response.StatusCode); } + [Fact] + public async Task RedirectToIdentityProvider_SetsCorrelationIdCookiePath_ToCallBackPath() + { + var server = CreateServer( + app => { }, + s => s.AddOAuthAuthentication( + "Weblie", + opt => + { + opt.ClientId = "Test Id"; + opt.ClientSecret = "secret"; + opt.SignInScheme = CookieAuthenticationDefaults.AuthenticationScheme; + opt.AuthorizationEndpoint = "https://example.com/provider/login"; + opt.TokenEndpoint = "https://example.com/provider/token"; + opt.CallbackPath = "/oauth-callback"; + }), + ctx => + { + ctx.ChallengeAsync("Weblie").ConfigureAwait(false).GetAwaiter().GetResult(); + return true; + }); + + var transaction = await server.SendAsync("https://www.example.com/challenge"); + var res = transaction.Response; + + Assert.Equal(HttpStatusCode.Redirect, res.StatusCode); + Assert.NotNull(res.Headers.Location); + var setCookie = Assert.Single(res.Headers, h => h.Key == "Set-Cookie"); + var correlation = Assert.Single(setCookie.Value, v => v.StartsWith(".AspNetCore.Correlation.")); + Assert.Contains("path=/oauth-callback", correlation); + } + + [Fact] + public async Task RedirectToAuthorizeEndpoint_CorrelationIdCookieOptions_CanBeOverriden() + { + var server = CreateServer( + app => { }, + s => s.AddOAuthAuthentication( + "Weblie", + opt => + { + opt.ClientId = "Test Id"; + opt.ClientSecret = "secret"; + opt.SignInScheme = CookieAuthenticationDefaults.AuthenticationScheme; + opt.AuthorizationEndpoint = "https://example.com/provider/login"; + opt.TokenEndpoint = "https://example.com/provider/token"; + opt.CallbackPath = "/oauth-callback"; + opt.ConfigureCorrelationIdCookie = (ctx, options) => options.Path = "/"; + }), + ctx => + { + ctx.ChallengeAsync("Weblie").ConfigureAwait(false).GetAwaiter().GetResult(); + return true; + }); + + var transaction = await server.SendAsync("https://www.example.com/challenge"); + var res = transaction.Response; + + Assert.Equal(HttpStatusCode.Redirect, res.StatusCode); + Assert.NotNull(res.Headers.Location); + var setCookie = Assert.Single(res.Headers, h => h.Key == "Set-Cookie"); + var correlation = Assert.Single(setCookie.Value, v => v.StartsWith(".AspNetCore.Correlation.")); + Assert.Contains("path=/", correlation); + } private static TestServer CreateServer(Action configure, Action configureServices, Func handler) { diff --git a/test/Microsoft.AspNetCore.Authentication.Test/OpenIdConnect/OpenIdConnectTests.cs b/test/Microsoft.AspNetCore.Authentication.Test/OpenIdConnect/OpenIdConnectTests.cs index 99bc1ec8c..6cfda3b85 100644 --- a/test/Microsoft.AspNetCore.Authentication.Test/OpenIdConnect/OpenIdConnectTests.cs +++ b/test/Microsoft.AspNetCore.Authentication.Test/OpenIdConnect/OpenIdConnectTests.cs @@ -60,6 +60,108 @@ public async Task SignOutSettingMessage() OpenIdConnectParameterNames.VersionTelemetry); } + [Fact] + public async Task RedirectToIdentityProvider_SetsNonceCookiePath_ToCallBackPath() + { + var setting = new TestSettings(opt => + { + opt.ClientId = "Test Id"; + opt.SignInScheme = CookieAuthenticationDefaults.AuthenticationScheme; + opt.Configuration = new OpenIdConnectConfiguration + { + AuthorizationEndpoint = "https://example.com/provider/login" + }; + }); + + var server = setting.CreateTestServer(); + + var transaction = await server.SendAsync(DefaultHost + TestServerBuilder.Challenge); + var res = transaction.Response; + + Assert.Equal(HttpStatusCode.Redirect, res.StatusCode); + Assert.NotNull(res.Headers.Location); + var setCookie = Assert.Single(res.Headers, h => h.Key == "Set-Cookie"); + var nonce = Assert.Single(setCookie.Value, v => v.StartsWith(OpenIdConnectDefaults.CookieNoncePrefix)); + Assert.Contains("path=/signin-oidc", nonce); + } + + [Fact] + public async Task RedirectToIdentityProvider_NonceCookieOptions_CanBeOverriden() + { + var setting = new TestSettings(opt => + { + opt.ClientId = "Test Id"; + opt.SignInScheme = CookieAuthenticationDefaults.AuthenticationScheme; + opt.Configuration = new OpenIdConnectConfiguration + { + AuthorizationEndpoint = "https://example.com/provider/login" + }; + opt.ConfigureNonceCookie = (ctx, options) => options.Path = "/"; + }); + + var server = setting.CreateTestServer(); + + var transaction = await server.SendAsync(DefaultHost + TestServerBuilder.Challenge); + var res = transaction.Response; + + Assert.Equal(HttpStatusCode.Redirect, res.StatusCode); + Assert.NotNull(res.Headers.Location); + var setCookie = Assert.Single(res.Headers, h => h.Key == "Set-Cookie"); + var nonce = Assert.Single(setCookie.Value, v => v.StartsWith(OpenIdConnectDefaults.CookieNoncePrefix)); + Assert.Contains("path=/", nonce); + } + + [Fact] + public async Task RedirectToIdentityProvider_SetsCorrelationIdCookiePath_ToCallBackPath() + { + var setting = new TestSettings(opt => + { + opt.ClientId = "Test Id"; + opt.SignInScheme = CookieAuthenticationDefaults.AuthenticationScheme; + opt.Configuration = new OpenIdConnectConfiguration + { + AuthorizationEndpoint = "https://example.com/provider/login" + }; + }); + + var server = setting.CreateTestServer(); + + var transaction = await server.SendAsync(DefaultHost + TestServerBuilder.Challenge); + var res = transaction.Response; + + Assert.Equal(HttpStatusCode.Redirect, res.StatusCode); + Assert.NotNull(res.Headers.Location); + var setCookie = Assert.Single(res.Headers, h => h.Key == "Set-Cookie"); + var correlation = Assert.Single(setCookie.Value, v => v.StartsWith(".AspNetCore.Correlation.")); + Assert.Contains("path=/signin-oidc", correlation); + } + + [Fact] + public async Task RedirectToIdentityProvider_CorrelationIdCookieOptions_CanBeOverriden() + { + var setting = new TestSettings(opt => + { + opt.ClientId = "Test Id"; + opt.SignInScheme = CookieAuthenticationDefaults.AuthenticationScheme; + opt.Configuration = new OpenIdConnectConfiguration + { + AuthorizationEndpoint = "https://example.com/provider/login" + }; + opt.ConfigureCorrelationIdCookie = (ctx, options) => options.Path = "/"; + }); + + var server = setting.CreateTestServer(); + + var transaction = await server.SendAsync(DefaultHost + TestServerBuilder.Challenge); + var res = transaction.Response; + + Assert.Equal(HttpStatusCode.Redirect, res.StatusCode); + Assert.NotNull(res.Headers.Location); + var setCookie = Assert.Single(res.Headers, h => h.Key == "Set-Cookie"); + var correlation = Assert.Single(setCookie.Value, v => v.StartsWith(".AspNetCore.Correlation.")); + Assert.Contains("path=/", correlation); + } + [Fact] public async Task EndSessionRequestDoesNotIncludeTelemetryParametersWhenDisabled() { @@ -173,7 +275,8 @@ public async Task SignOutWith_Specific_RedirectUri_From_Authentication_Properite [Fact] public async Task SignOut_WithMissingConfig_Throws() { - var setting = new TestSettings(opt => { + var setting = new TestSettings(opt => + { opt.ClientId = "Test Id"; opt.Configuration = new OpenIdConnectConfiguration(); });