Set the cookie "Secure" flag iff the ACS post back URL is HTTPS

[ulrichb]

Partially fixes #1091 (for the ASP.NET Core version).

This allows to comply with the new (Chrome >= 80) "Reject insecure SameSite=None cookies" rule (which otherwise would drop the SignInCommand correlation cookie).

Manually tested:

• ASP.NET Core: Chrome 80 (Dev Channel) with enabled "SameSite by default cookies" and "Cookies without SameSite must be secure" flags in chrome://flags/

[ulrichb]

@AndersAbel This is a proposal for #1091 (comment).

I use the AssertionConsumerServiceUrl.Scheme because it's exactly the post back URL and also includes the PublicOrigin (if set).

What do you think?

[ulrichb]

@AndersAbel Could you take a look at this? Chrome 80 will be released in February ....

[AndersAbel]

@ulrichb I had a quick look and I think it looks good, but I need to look a bit more in detail on it before merging.

Is this the only change needed for Chrome 80 to work? I assumed that a SameSite value must also be set (except for that old version of Safari that requires agent sniffing to detect)?

[ulrichb]

Is this the only change needed for Chrome 80 to work?

I successfully tested the login procedure (against an ADFS Server) with Chrome 80 Dev Channel (+ enabled "SameSite by default cookies" and "Cookies without SameSite must be secure" in chrome://flags/).
(Did not test the logout federation.)

---

I assumed that a SameSite value must also be set

For ASP.NET Core SameSite=None is already set. (See CommandResultExtensions).

For Sustainsys.Saml2.Mvc, a target framework change to 4.7.2 would be necessary to set SameSite = None. See https://docs.microsoft.com/en-us/dotnet/api/system.web.httpcookie.samesite.

For Owin, a library update to 4.1.0 seems to be necessary. See aspnet/AspNetKatana#201.

(I only use the ASP.NET Core version.)

[ulrichb]

I updated the description above to "Partially fixes #1091 (for the ASP.NET Core version)."

[mklinke]

@ulrichb What version of ASP.NET Core have you tested against? Also, won't this break local debugging without HTTPS? At least that's possible with the current implementation. Not sure if the change in Chrome 80 alone will already break that...

[ulrichb]

ASP.NET Core 2.2. And oh, I forgot to mention that the explicit SameSite=None emitting only happens when you have ASP.NET Core >= 2.1.14 or ASP.NET Core >= 2.2.8, before these patch versions the "None" value was simply ignored (see dotnet/aspnetcore#13746.)

---

Also, won't this break local debugging without HTTPS?

That's the reason why I used urls.AssertionConsumerServiceUrl.Scheme ... to avoid the secure flag on HTTP hosts (or theoretically PublicOrigin's). But yes, HTTP localhost testing doesn't work in Chrome >= 80 (unless you explicitly disable the flags, which you can do for debugging purposes); but this is a general problem for all non-HTTPS (localhost debugging) cross origin cookies ...

[ulrichb]

@mklinke Do you need any further info? (February is coming ....)

[mklinke]

@ulrichb Not sure, what you're asking. I'm yet another user of the library as of now. My project is currently based on .NET Core 3.0 and I found the issue as a blocker when I tried upgrading to .NET Core 3.1 due to the SameSite breaking change there.

[ulrichb]

Sorry, I meant @AndersAbel :)

[AndersAbel]

@ulrichb @mklinke I'm working on the releases for this right now, please see #1091 for status as that's the main issue.

[ulrichb]

Thanks @AndersAbel !

[AndersAbel]

Note: Need to apply the Secure flag for the redirect to Discovery Service too, I'll fix that when merging.

[ulrichb]

Cool. Thx!

[AndersAbel]

I ended using cherry-pick to integrate this - first to the v1 branch and then to master. So I'm marking this PR as "closed" but it means "merged"

[ulrichb]

@AndersAbel Many thanks!