From 947ed7e9ec71a44a4e8bec6b5a0df75d0ad80c95 Mon Sep 17 00:00:00 2001 From: Ulrich Buchgraber Date: Mon, 16 Dec 2019 11:12:58 +0100 Subject: [PATCH] Set the cookie "Secure" flag iff the ACS post back URL is HTTPS This allows to comply with the new (Chrome >= 80) "Reject insecure SameSite=None cookies" rule (which otherwise would drop the correlation cookie). --- .../CommandResultExtensions.cs | 1 + .../CommandResultHttpExtensionsShared.cs | 3 +- .../CommandResultExtensions.cs | 1 + Sustainsys.Saml2/WebSSO/CommandResult.cs | 5 +++ Sustainsys.Saml2/WebSSO/SignInCommand.cs | 2 ++ .../CommandResultExtensionsTests.cs | 4 ++- .../CommandResultHttpTests.cs | 4 ++- .../CommandResultExtensionsTests.cs | 5 +-- .../Tests.Shared/WebSSO/SignInCommandTests.cs | 36 +++++++++++++++++++ 9 files changed, 56 insertions(+), 5 deletions(-) diff --git a/Sustainsys.Saml2.AspNetCore2/CommandResultExtensions.cs b/Sustainsys.Saml2.AspNetCore2/CommandResultExtensions.cs index 85944e61b..ead12f472 100644 --- a/Sustainsys.Saml2.AspNetCore2/CommandResultExtensions.cs +++ b/Sustainsys.Saml2.AspNetCore2/CommandResultExtensions.cs @@ -35,6 +35,7 @@ static class CommandResultExtensions new CookieOptions() { HttpOnly = true, + Secure = commandResult.SetCookieSecureFlag, // We are expecting a different site to POST back to us, // so the ASP.Net Core default of Lax is not appropriate in this case SameSite = SameSiteMode.None, diff --git a/Sustainsys.Saml2.HttpModule/CommandResultHttpExtensionsShared.cs b/Sustainsys.Saml2.HttpModule/CommandResultHttpExtensionsShared.cs index c645dc273..b94c3cfc2 100644 --- a/Sustainsys.Saml2.HttpModule/CommandResultHttpExtensionsShared.cs +++ b/Sustainsys.Saml2.HttpModule/CommandResultHttpExtensionsShared.cs @@ -42,7 +42,8 @@ public static void ApplyCookies(this CommandResult commandResult, HttpResponseBa commandResult.SetCookieName, protectedData) { - HttpOnly = true + HttpOnly = true, + Secure = commandResult.SetCookieSecureFlag, }); } diff --git a/Sustainsys.Saml2.Owin/CommandResultExtensions.cs b/Sustainsys.Saml2.Owin/CommandResultExtensions.cs index 6f26f627d..47ca41c48 100644 --- a/Sustainsys.Saml2.Owin/CommandResultExtensions.cs +++ b/Sustainsys.Saml2.Owin/CommandResultExtensions.cs @@ -71,6 +71,7 @@ private static void ApplyCookies(CommandResult commandResult, IOwinContext conte new CookieOptions() { HttpOnly = true, + Secure = commandResult.SetCookieSecureFlag, }); } diff --git a/Sustainsys.Saml2/WebSSO/CommandResult.cs b/Sustainsys.Saml2/WebSSO/CommandResult.cs index e2d800f87..d79f63706 100644 --- a/Sustainsys.Saml2/WebSSO/CommandResult.cs +++ b/Sustainsys.Saml2/WebSSO/CommandResult.cs @@ -67,6 +67,11 @@ public class CommandResult /// public string SetCookieName { get; set; } + /// + /// Value of the "Secure" flag for the cookie (relevant if != null). + /// + public bool SetCookieSecureFlag { get; set; } + /// /// SAML RelayState value /// diff --git a/Sustainsys.Saml2/WebSSO/SignInCommand.cs b/Sustainsys.Saml2/WebSSO/SignInCommand.cs index 63274da78..eae721eec 100644 --- a/Sustainsys.Saml2/WebSSO/SignInCommand.cs +++ b/Sustainsys.Saml2/WebSSO/SignInCommand.cs @@ -157,6 +157,8 @@ private static CommandResult InitiateLoginToIdp(IOptions options, IDictionary(co => co.HttpOnly && co.SameSite == SameSiteMode.None)); + "Saml2.123", expectedCookieData, Arg.Is( + co => co.HttpOnly && co.Secure && co.SameSite == SameSiteMode.None)); context.Response.Cookies.Received().Delete("Clear-Cookie"); diff --git a/Tests/HttpModule.Tests/CommandResultHttpTests.cs b/Tests/HttpModule.Tests/CommandResultHttpTests.cs index 6d10c7222..20248f742 100644 --- a/Tests/HttpModule.Tests/CommandResultHttpTests.cs +++ b/Tests/HttpModule.Tests/CommandResultHttpTests.cs @@ -52,6 +52,7 @@ public void CommandResultHttp_Apply_SetsCookie() new CommandResult() { SetCookieName = "CookieName", + SetCookieSecureFlag = true, RequestState = new StoredRequestState( new EntityId("http://idp.example.com"), null, @@ -64,7 +65,8 @@ public void CommandResultHttp_Apply_SetsCookie() c.Name == "CookieName" && c.Value.All(ch => ch != '/' && ch != '+' && ch != '=') && new StoredRequestState(DecryptCookieData(c.Value)).Idp.Id == "http://idp.example.com" - && c.HttpOnly == true)); + && c.HttpOnly == true + && c.Secure == true)); } private byte[] DecryptCookieData(string data) diff --git a/Tests/Owin.Tests/CommandResultExtensionsTests.cs b/Tests/Owin.Tests/CommandResultExtensionsTests.cs index 89071e265..aee6eab84 100644 --- a/Tests/Owin.Tests/CommandResultExtensionsTests.cs +++ b/Tests/Owin.Tests/CommandResultExtensionsTests.cs @@ -62,7 +62,8 @@ public void CommandResultExtensions_Apply_Cookie() new Uri("http://sp.example.com/loggedout"), new Saml2Id("id123"), null), - SetCookieName = "CookieName" + SetCookieName = "CookieName", + SetCookieSecureFlag = true, }; var context = OwinTestHelpers.CreateOwinContext(); @@ -75,7 +76,7 @@ public void CommandResultExtensions_Apply_Cookie() var protectedData = HttpRequestData.ConvertBinaryData( StubDataProtector.Protect(cr.GetSerializedRequestState())); - var expected = $"CookieName={protectedData}; path=/; HttpOnly"; + var expected = $"CookieName={protectedData}; path=/; secure; HttpOnly"; setCookieHeader.Should().Be(expected); } diff --git a/Tests/Tests.Shared/WebSSO/SignInCommandTests.cs b/Tests/Tests.Shared/WebSSO/SignInCommandTests.cs index 403c11019..f3d869a86 100644 --- a/Tests/Tests.Shared/WebSSO/SignInCommandTests.cs +++ b/Tests/Tests.Shared/WebSSO/SignInCommandTests.cs @@ -412,5 +412,41 @@ public void SignInCommand_Run_RedirectToDsWorksWithoutSpecifiedReturnPath() a.Should().NotThrow(); } + + [TestMethod] + public void SignInCommand_WithHttpUrl_DoesNotSetSecureCookieFlag() + { + var options = StubFactory.CreateOptions(); + var httpRequest = new HttpRequestData("GET", new Uri("http://localhost")); + + var actual = SignInCommand.Run(options.IdentityProviders.Default.EntityId, null, httpRequest, options, null); + + actual.SetCookieName.Should().StartWith(StoredRequestState.CookieNameBase); + actual.SetCookieSecureFlag.Should().BeFalse(); + } + + [TestMethod] + public void SignInCommand_WithHttpsUrl_SetsSecureCookieFlag() + { + var options = StubFactory.CreateOptions(); + var httpRequest = new HttpRequestData("GET", new Uri("https://localhost")); + + var actual = SignInCommand.Run(options.IdentityProviders.Default.EntityId, null, httpRequest, options, null); + + actual.SetCookieName.Should().StartWith(StoredRequestState.CookieNameBase); + actual.SetCookieSecureFlag.Should().BeTrue(); + } + + [TestMethod] + public void SignInCommand_WithHttpsPublicOrigin_SetsSecureCookieFlag() + { + var options = StubFactory.CreateOptionsPublicOrigin(new Uri("https://my.public.origin:8443")); + var httpRequest = new HttpRequestData("GET", new Uri("http://localhost")); + + var actual = SignInCommand.Run(options.IdentityProviders.Default.EntityId, null, httpRequest, options, null); + + actual.SetCookieName.Should().StartWith(StoredRequestState.CookieNameBase); + actual.SetCookieSecureFlag.Should().BeTrue(); + } } } \ No newline at end of file