Skip to content
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

[2.1] Re-implement SameSite for 2019 #13746

Merged
merged 5 commits into from Oct 3, 2019
Merged

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented Sep 5, 2019

2.1 patch for #12125. Similar patches are in flight for System.Web and Microsoft.Owin.

Issue: Chrome is redefining how the SameSite cookie property works in a non-backwards compatible way. Cross site components like OpenIdConnect authentication must opt-out by sending a new "None" value or they won't work anymore.

Customer impact: Customers using OpenIdConnect to log into their web sites will break without this patch starting in January (Chrome 80).

Risk: Medium. The SameSite changes are known to be incompatible with older OSs and browsers, especially iOS 12 and OSX Mojave (latest - 1). These represent a small but influential portion of the web client user base. Updating to the latest OS version addresses the incompatibility.

Mitigations:
A) This patch includes an AppContext switch to revert everything to the old behavior. "Microsoft.AspNetCore.SuppressSameSiteNone"
B) (SameSiteMode)(-1) can be used to exclude the SameSite attribute for cookies on a case by case basis.
C) We're providing browser sniffing samples and guidance to account for the incompatibilities in some old clients.

@blowdart

@Tratcher Tratcher self-assigned this Sep 5, 2019
@Pilchie Pilchie added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Sep 6, 2019
@Tratcher
Copy link
Member Author

Tratcher commented Sep 6, 2019

@aspnet/build what's up with Windows in 2.1?

C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.targets(114,5): warning : The plugin credential provider could not acquire credentials. Authentication may require manual action. Consider re-running the command with --interactive for `dotnet`, /p:NuGetInteractive="true" for MSBuild or removing the -NonInteractive switch for `NuGet` [F:\workspace.5\_work\1\s\build\tasks\RepoTasks.csproj]
C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.targets(114,5): error : Unable to load the service index for source https://vside.myget.org/F/vssdk/api/v3/index.json. [F:\workspace.5\_work\1\s\build\tasks\RepoTasks.csproj]
C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.targets(114,5): error :   Response status code does not indicate success: 401 (Unauthorized). [F:\workspace.5\_work\1\s\build\tasks\RepoTasks.csproj]
C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.targets(114,5): error : NuGet.Protocol.Core.Types.FatalProtocolException: Unable to load the service index for source https://vside.myget.org/F/vssdk/api/v3/index.json. ---> System.Net.Http.HttpRequestException: Response status code does not indicate success: 401 (Unauthorized). [F:\workspace.5\_work\1\s\build\tasks\RepoTasks.csproj]

@dougbu
Copy link
Member

dougbu commented Sep 7, 2019

Hmm, I thought vside.myget.org was not authenticated? In any case, whatever credentials we're using for these downlevel pipelines are either insufficient (i.e. we don't have a token to access everything we need) or expired.

Could you please compare the variable groups mentioned in the pipelines for this branch with those used in (say) 'release/3.0' and let us know if there's a glaring difference?

@Tratcher
Copy link
Member Author

Tratcher commented Sep 9, 2019

@dougbu yes https://vside.myget.org/F/vssdk/api/v3/index.json is authenticated. I'm only finding one variable group and it's not branch specific.

Also, Linux is working for some reason.

@dougbu
Copy link
Member

dougbu commented Sep 9, 2019

Wondering if this was a temporary situation…

@dougbu
Copy link
Member

dougbu commented Sep 9, 2019

/azp run all

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@Tratcher
Copy link
Member Author

Tratcher commented Sep 9, 2019

@dougbu nope

@dougbu
Copy link
Member

dougbu commented Sep 9, 2019

@Tratcher I'll follow up w/ the owner of that feed after lunch. Looks like auth is now required and we can't handle that; there are almost no secrets available in our public builds and adding one here wouldn't be good.

@Tratcher Tratcher requested a review from HaoK September 9, 2019 21:03
@HaoK
Copy link
Member

HaoK commented Sep 9, 2019

Should we pick a more clear name instead of a 2016 date? Maybe UseSameSiteLaxAsDefault ?

@Tratcher
Copy link
Member Author

Tratcher commented Sep 9, 2019

There's a lot more behavior in that 2016 rfc. Most of it affects the parser though.

@HaoK
Copy link
Member

HaoK commented Sep 9, 2019

Sure but specifically if the only thing we are really doing is switching between none/lax with this switch, maybe its better to be more specific with the name

@Tratcher
Copy link
Member Author

Tratcher commented Sep 9, 2019

SuppressSameSiteNone?

@dougbu
Copy link
Member

dougbu commented Sep 9, 2019

Found the right email. We should update any https://vside.myget.org/F/vssdk/api/v3/index.json reference to instead use https://pkgs.dev.azure.com/azure-public/vside/_packaging/vssdk/nuget/v3/index.json Could you do that in this PR and your similar 2.2 update? We don't use the MyGet feeds in more recent branches.

Also, https://vside.myget.org/F/devcore/api/v3/index.json, https://vside.myget.org/F/vsmac/api/v3/index.json and https://vside.myget.org/F/vs-impl/api/v3/index.json have all been deprecated. The replacement for them is https://pkgs.dev.azure.com/azure-public/vside/_packaging/vs-impl/nuget/v3/index.json And, no, I haven't checked how much we use the old feeds in 2.x branches…

@HaoK
Copy link
Member

HaoK commented Sep 9, 2019

sure SuppressSameSiteNone sounds good

@analogrelay analogrelay added this to the 2.1.x milestone Sep 17, 2019
@Tratcher Tratcher added the Servicing-consider Shiproom approval is required for the issue label Sep 30, 2019
@Tratcher Tratcher marked this pull request as ready for review October 2, 2019 21:06
@analogrelay analogrelay added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Oct 3, 2019
@analogrelay analogrelay merged commit f198e55 into release/2.1 Oct 3, 2019
@analogrelay analogrelay deleted the tratcher/2.1/samesite branch October 3, 2019 21:37
@vivmishra vivmishra modified the milestones: 2.1.x, 2.1.14 Oct 4, 2019
@ulrichb
Copy link

ulrichb commented Nov 6, 2019

Will this be ported to 2.2?

EDIT: Ah, found #13858.

@Tratcher
Copy link
Member Author

Tratcher commented Nov 6, 2019

@ulrichb yes, see #14996 (comment)

@ulrichb
Copy link

ulrichb commented Nov 7, 2019

@Tratcher thx!

@ulrichb
Copy link

ulrichb commented Nov 7, 2019

@Tratcher Somehow off-topic, but related: Will there be a VS 2017 SDK with .NET Core Runtime 2.2.8 (which will contain this fix)?

@Tratcher
Copy link
Member Author

Tratcher commented Nov 7, 2019

@shirhatti ?

@shirhatti
Copy link
Contributor

Yes, there's going to be a SDK that'll work with VS 2017 15.9 that contains this fix.
It won't be present in the box in VS 2017, you'll have to acquire it manually (no 2.2 SDK ever shipped as part of VS 2017).

@ulrichb
Copy link

ulrichb commented Nov 7, 2019

Okay, cool. That's perfect!

@amcasey amcasey added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions feature-http-abstractions Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants