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

Aspnet External Cookie not set for Sign On with Microsoft Post 4.72 SameSite Changes on Asp.net MVC #331

Closed
AaronShermanSbt opened this issue Feb 3, 2020 · 24 comments

Comments

@AaronShermanSbt
Copy link

AaronShermanSbt commented Feb 3, 2020

Apologies for being a bit verbose, but want to try and be specific. I am having trouble with .AspNet.ExternalCookie after the same site changes of Nov 2019 and Azure update of Jan 2020

Setup:

  • .net Framework 4.72 Post samesite changes
  • Azure/IIS on windows 10
  • asp.net MVC

Issue:
Sign on with Microsoft was broken with the changes to support SameSite on Azure. We were able to bandaid fix by applying the “roll back” technique of aspnet:SuppressSameSiteNone.

Symptoms:
With the suppress same site flag post logging in with Microsoft the .AspNet.ExternalCookie is set but without samesite:none. Without the flag the cookie is never set (or at least in a way I can see it)

What I have tried

this sounds like the problem on #324 which was closed, without a real resolution - the requester said when he re-installed the the path the bug returned.

my code, apology for a lot of it, I tried to rip out as much as I could that was extraneous.

using Microsoft.AspNet.Identity;
using Microsoft.AspNet.Identity.Owin;
using Microsoft.IdentityModel.Tokens;
using Microsoft.Owin;
using Microsoft.Owin.Cors;
using Microsoft.Owin.Host.SystemWeb;
using Microsoft.Owin.Infrastructure;
using Microsoft.Owin.Security.Cookies;
using Microsoft.Owin.Security.OAuth;
using Microsoft.Owin.Security.OpenIdConnect;
using Owin;
using Abc.Auth.Identity.DBContext;
using Abc.Auth.Identity.Helpers;
using Abc.Auth.Identity.Models;
using Abc.Auth.Providers;
using Abc.Constants.AppSettings;
using System;
using System.Threading.Tasks;

namespace Abc.Web
{
    public class Startup
    {
        public static OAuthAuthorizationServerOptions OAuthOptions { get; private set; }


        //Copied from https://github.com/aspnet/AspNetKatana/blob/dev/src/Microsoft.Owin.Security.Cookies/Provider/DefaultBehavior.cs
        private static bool IsAjaxRequest(IOwinRequest request)
        {
            IReadableStringCollection query = request.Query;
            if (query != null)
            {
                if (query["X-Requested-With"] == "XMLHttpRequest")
                {
                    return true;
                }
            }

            IHeaderDictionary headers = request.Headers;
            if (headers != null)
            {
                if (headers["X-Requested-With"] == "XMLHttpRequest")
                {
                    return true;
                }
            }
            return false;
        }


        public virtual void Configuration(IAppBuilder app)
        {

            ConfigureAuth(app);
        }
        public void ConfigureAuth(IAppBuilder app)
        {

            // Configure the db context, user manager and role manager to use a single instance per request
            app.CreatePerOwinContext(MyIdentityDbContext.Create);
            app.CreatePerOwinContext<ApplicationUserManager>(ApplicationUserManager.Create);
            app.CreatePerOwinContext<ApplicationRoleManager>(ApplicationRoleManager.Create);

            // Enable the application to use a cookie to store information for the signed in user
            // and to use a cookie to temporarily store information about a user logging in with a third party login provider
            // Configure the sign in cookie

            var cao = new CookieAuthenticationOptions
            {
                AuthenticationType = DefaultAuthenticationTypes.ApplicationCookie,
                LoginPath = new PathString("/account/logon"),
                LogoutPath = new PathString("/account/logout"),
                Provider = new CookieAuthenticationProvider
                {
                    // Enables the application to validate the security stamp when the user logs in.
                    // This is a security feature which is used when you change a password or add an external login to your account.
                    OnValidateIdentity = SecurityStampValidator.OnValidateIdentity<ApplicationUserManager, ApplicationUser, int>(
                       validateInterval: TimeSpan.FromMinutes(20),
                       regenerateIdentityCallback: (manager, user) => user.GenerateUserIdentityAsync(manager, DefaultAuthenticationTypes.ApplicationCookie),
                       getUserIdCallback: (id) => (Int32.Parse(id.GetUserId()))),

                    // Same logic as https://github.com/aspnet/AspNetKatana/blob/dev/src/Microsoft.Owin.Security.Cookies/Provider/DefaultBehavior.cs
                    // from the "internal static readonly Action<CookieApplyRedirectContext> ApplyRedirect" method, except if its an ajax request we just return the code 
                    // instead of the "X-Responded-JSON with 200 response" nonsense
                    OnApplyRedirect = (ctx =>
                    {
                        if (!IsAjaxRequest(ctx.Request))
                        {
                            ctx.Response.Redirect(ctx.RedirectUri);
                        }
                    }),

                    /* I changed this part */
                    OnException = (context =>
                    {
                        throw context.Exception;
                    })
                },

                // Potential fix for OWIN authentication issues:
                //http://katanaproject.codeplex.com/wikipage?title=System.Web%20response%20cookie%20integration%20issues&referringTitle=Documentation
                //https://katanaproject.codeplex.com/workitem/197
                //https://stackoverflow.com/questions/20737578/asp-net-sessionid-owin-cookies-do-not-send-to-browser
                CookieManager = new SameSiteCookieManager(new SystemWebCookieManager())

            };

            app.UseCookieAuthentication(cao);
            app.UseExternalSignInCookie();

            Microsoft.Owin.Security.OpenIdConnect.OpenIdConnectAuthenticationOptions office365 = CreateOffice365Options();

            app.UseOpenIdConnectAuthentication(office365);

        }

        private OpenIdConnectAuthenticationOptions CreateOffice365Options()
        {
            var office365 = new Microsoft.Owin.Security.OpenIdConnect.OpenIdConnectAuthenticationOptions();
            office365.ClientId = Statics.Office365ClientId; ;
            office365.Caption = "Office 365";

            office365.Authority = Statics.Office365AuthorityBaseUri;

            office365.CookieManager = new SameSiteCookieManager(new SystemWebCookieManager());

            office365.TokenValidationParameters = new TokenValidationParameters
            {
                // instead of using the default validation (validating against a single issuer value, as we do in line of business apps (single tenant apps)),
                // we turn off validation
                //
                // NOTE:
                // * In a multitenant scenario you can never validate against a fixed issuer string, as every tenant will send a different one.
                // * If you don’t care about validating tenants, as is the case for apps giving access to 1st party resources, you just turn off validation.
                // * If you do care about validating tenants, think of the case in which your app sells access to premium content and you want to limit access only to the tenant that paid a fee,
                //       you still need to turn off the default validation but you do need to add logic that compares the incoming issuer to a list of tenants that paid you,
                //       and block access if that’s not the case.
                // * Refer to the following sample for a custom validation logic: https://github.com/AzureADSamples/WebApp-WebAPI-MultiTenant-OpenIdConnect-DotNet

                ValidateIssuer = false
            };
            office365.Notifications = new OpenIdConnectAuthenticationNotifications()
            {
                // If there is a code in the OpenID Connect response, redeem it for an access token and refresh token, and store those away.

                RedirectToIdentityProvider = (context) =>
                {
                    // This ensures that the address used for sign in and sign out is picked up dynamically from the request
                    // this allows you to deploy your app (to Azure Web Sites, for example)without having to change settings
                    // Remember that the base URL of the address used here must be provisioned in Azure AD beforehand.
                    string appBaseUrl = context.Request.Scheme + Uri.SchemeDelimiter + context.Request.Host + context.Request.PathBase;
                    context.ProtocolMessage.RedirectUri = appBaseUrl + "/account/office365";
                    context.ProtocolMessage.PostLogoutRedirectUri = appBaseUrl;

                    return Task.FromResult(0);
                },

                AuthenticationFailed = (context) =>
                {
                    // Suppress the exception if you don't want to see the error

                    //Elmah.ErrorSignal.FromCurrentContext().Raise(new Exception($"DEBUG: O365 auth failed: {context.Exception.Message}"));
                    Elmah.ErrorSignal.FromCurrentContext().Raise(context.Exception);
                    context.HandleResponse();
                    return Task.FromResult(0);
                }
            };

            return office365;
        }

    }



    public class SameSiteCookieManager : ICookieManager
    {
        private readonly ICookieManager _innerManager;

        public SameSiteCookieManager() : this(new CookieManager())
        {
        }

        public SameSiteCookieManager(ICookieManager innerManager)
        {
            _innerManager = innerManager;
        }

        public void AppendResponseCookie(IOwinContext context, string key, string value,
                                         CookieOptions options)
        {
            CheckSameSite(context, options);
            _innerManager.AppendResponseCookie(context, key, value, options);
        }

        public void DeleteCookie(IOwinContext context, string key, CookieOptions options)
        {
            CheckSameSite(context, options);
            _innerManager.DeleteCookie(context, key, options);
        }

        public string GetRequestCookie(IOwinContext context, string key)
        {
            return _innerManager.GetRequestCookie(context, key);
        }

        private void CheckSameSite(IOwinContext context, CookieOptions options)
        {
            if (options.SameSite == SameSiteMode.None && DisallowsSameSiteNone(context))
            {
                options.SameSite = null;
            }
        }

        public static bool DisallowsSameSiteNone(IOwinContext context)
        {
            return false;
        }
    }

}
@Tratcher
Copy link
Member

Tratcher commented Feb 3, 2020

Looks like you're on the right track. I have seen some reports that the .NET patches have exacerbated the existing System.Web cookie issue. All cases I've seen have been fixed by liberal use of the SystemWebCookieManager. In your example I think the piece you're missing is being able to apply that to the app.UseExternalSignInCookie(); cookie. While there isn't an overload of UseExternalSignInCookie that will let you configure it, you can replace it with this:
https://github.com/aspnet/AspNetIdentity/blob/a24b776676f12cf7f0e13944783cf8e379b3ef70/src/Microsoft.AspNet.Identity.Owin/Extensions/AppBuilderExtensions.cs#L118-L125

Also, did you intentionally leave out the implementation for this?

        public static bool DisallowsSameSiteNone(IOwinContext context)
        {
            return false;
        }

@AaronShermanSbt
Copy link
Author

The link to the source code doesn't work (404). Everywhere I could find to override the cookiemanager I have, but curious to see if there was another.

For the diallows I left it blank since the issue is the critical code never calling that method, and this was already 200 lines of code

thanks

@Tratcher
Copy link
Member

Tratcher commented Feb 3, 2020

Woops, fixed the link.

@AaronShermanSbt
Copy link
Author

It looks like the key line of code to change behavior is:

 app.UseCookieAuthentication(new CookieAuthenticationOptions
            {
                AuthenticationMode = AuthenticationMode.Passive,
          
            });

but if I do this, the rest of my login code breaks when cookie: .AspNet.ApplicationCookie is no longer set downstream. I have not looked into that issue as of yet, but is that what you would expect to happen?

If we upgrade to .net core do these issues just go away?

@Tratcher
Copy link
Member

Tratcher commented Feb 3, 2020

You shouldn't need to change the AuthenticationMode for the external cookie, you should only need to add the SystemWebCookieManager.

If we upgrade to .net core do these issues just go away?

Correct, the System.Web cookie issue is not present in ASP.NET Core because it doesn't rely on System.Web at all. Core does still need some of the SameSite mitigations like DisallowsSameSiteNone which is outlined in the docs.

@AaronShermanSbt
Copy link
Author

If you look at my initial code you can see both openId and cookies I am using a new SystemWebCookieManager.

With setting break points in the SameSiteCookieManager which should be called before the SystemWebCookieManager the code is neveral called for the extneral cookie.

Looks like my comment on my passive was wrong, the cookie name replaced the default of ".AspNet.ApplicationCookie". Passive still broke my code, but did not fix any of my other issues.

can you get openid to work on 4.72 using only the nuget packages? I am wondering if somehow the wrong code was built / deployed? (grasping at straws)... I really think something was broken deep in .net/owin with the november patch...

@Tratcher
Copy link
Member

Tratcher commented Feb 3, 2020

Let me clarify. You're missing one more place to plug in SystemWebCookieManager, inside UseExternalSignInCookie. To do that you're going to need to replace UseExternalSignInCookie with the following:

            app.UseCookieAuthentication(cao);
            // app.UseExternalSignInCookie();

            app.SetDefaultSignInAsAuthenticationType(DefaultAuthenticationTypes.ExternalCookie);
            app.UseCookieAuthentication(new CookieAuthenticationOptions
            {
                AuthenticationType = DefaultAuthenticationTypes.ExternalCookie,
                AuthenticationMode = AuthenticationMode.Passive,
                CookieName = CookiePrefix + externalAuthenticationType,
                ExpireTimeSpan = TimeSpan.FromMinutes(5),
                CookieManager = new SystemWebCookieManager();
            });

@AaronShermanSbt
Copy link
Author

Apologies, maybe I've been staring at this code for too long, but isn't that what I already have?

I have set the cookiemanager in 2 locations, see screenshot of code with colors. Is there a 3rd? The change I think you are suggesting we did a long time ago.

image

@Tratcher
Copy link
Member

Tratcher commented Feb 3, 2020

UseExternalSignInCookie adds an additional instance of UseCookieAuthentication for the external cookie. That instance also needs the SystemWebCookieManager.

@AaronShermanSbt
Copy link
Author

Sorry, I am still very confused. How do I override UseExternalSignInCookie to give it it's own cookie manager?

Here is the signature I can see. I feel like I am missing something really obvious...

image

@Tratcher
Copy link
Member

Tratcher commented Feb 3, 2020

You can't, you have to replace it as shown above: #331 (comment)

@AaronShermanSbt
Copy link
Author

Sorry for being so hard to help.

What does "replace mean" / Where do I replace it:

  1. make my own AppBuilderExtensions.cs?
  2. comment out the line of code that says app.UseExternalSignInCookie();?

I think replace can mean a few very different things. can you update the comment with more of the code before/after?

thanks
aaron

@Tratcher
Copy link
Member

Tratcher commented Feb 3, 2020

2. comment out the line of code that says app.UseExternalSignInCookie();?

Yes. There's no overload of UseExternalSignInCookie that does what you want, so you have to copy its code into Startup and then modify it.

@AaronShermanSbt
Copy link
Author

can you show an example of the full method?

@Tratcher
Copy link
Member

Tratcher commented Feb 3, 2020

Is this any clearer? #331 (comment)

@AaronShermanSbt
Copy link
Author

SO MUCH CLEARER I was not understanding that you wanted a second instance of UseCookieAuthentication, I thought you were trying to change the first one.

I'll be back online later tonight to test the code.

@AaronShermanSbt
Copy link
Author

I think that fixed it.

Thank you for your help, I really appreciate it. Will battle test the code a bit more tomorrow, but looks good.

Is there a reason you don't update the owin code to do that behavior by default? Is Owin for .net 4.72 going to still be maintained, or was this the last major update?

@Tratcher
Copy link
Member

Tratcher commented Feb 4, 2020

Is there a reason you don't update the owin code to do that behavior by default?

The Owin components were designed to work with multiple servers so they don't ship with any direct dependencies on SystemWeb components. That said, I'll file a template bug so we can at least enable SystemWebCookieManager by default in new projects.

Is Owin for .net 4.72 going to still be maintained, or was this the last major update?

The Microsoft.Owin components get critical updates but minimal new feature work. All feature development is happening over in ASP.NET Core.

@amitkub
Copy link

amitkub commented Feb 5, 2020

The above solution solved this problem for me (for openidconnect) - still testing

Does the LinkedInAuthenticationOptions allow changing CookieManager?
I can't see that option in LinkedInAuthenticationOptions

@Tratcher
Copy link
Member

Tratcher commented Feb 5, 2020

Does the LinkedInAuthenticationOptions allow changing CookieManager?
I can't see that option in LinkedInAuthenticationOptions

That's not one we maintain. Looks like it came from here? It hasn't been updated in a while, someone might have to go add that option.

@amitkub
Copy link

amitkub commented Feb 7, 2020

Now my windows live signin breaks

            var mao = new MicrosoftAccountAuthenticationOptions()
            {
                ClientId = microsoftKey,
                ClientSecret = microsoftSecret,
**new line**                CookieManager = new SystemWebCookieManager(),
            };
            mao.Scope.Add("wl.emails");
            app.UseMicrosoftAccountAuthentication(mao);

Any reason for this

============================================

Just to explain - this was because of updating owin from 3.0 to 4.1. For this update - MSFT have changed (I think) the ability to connect to live accounts. This meant that my existing apps registration didn't work. To solve:

  1. Create new apps in the Azure Active Directory | applications with the option "both AD accounts and live accounts" selected.

@Tratcher
Copy link
Member

Tratcher commented Feb 9, 2020

Breaks how?

@ghost
Copy link

ghost commented Feb 15, 2020

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment. If it is closed, feel free to comment when you are able to provide the additional information and we will re-investigate.

See our Issue Management Policies for more information.

@AaronShermanSbt
Copy link
Author

the changes above resolved my problem.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants