dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.21k stars 9.95k forks source link

IAntiforgery is limited and doesn't support multiple instances/options #22217

Closed Shazwazza closed 3 years ago

Shazwazza commented 4 years ago

The IAntiforgery interface and the publicly available APIs is very limiting. It allows you to register one and only one IAntiforgery implementation with one set of options.

Our application is meant to be non-intrusive and thereby we cannot and do not want to set global options/services. But even in other scenarios the IAntiforgery implementation isn't flexible enough. Say for example I wanted to have a front-end and back-end setup where the front-end uses different cookie/header names than the back-end. This isn't really possible and also because everything is internal it's not possible to create an additional service with different options.

I have looked into a work around which i think could work but it's super ugly. Have a custom interface like IBackOfficeAntiforgery inheriting from IAntiforgery and the implementation get's the default IAntiforgery and it's options injected into it and it wraps all methods. For each method we can then check if the request is for the back office and if so then we need to 'trick' the underlying implementation by manually changing the request headers and request cookies to match the cookie/header names that we want. This is also difficult because there are no public implementations of IRequestCookieCollection so that needs to be manually made to. Once it's tricked, then we revert the request to original values. This is pretty nasty.

Is there any known work arounds for this or could there be some way of creating a 2nd (or more) IAntiforgery service with differing options?

Shazwazza commented 4 years ago

side note - it seems like IRequestCookieCollection.Keys should have been an immutable collection? it throws if you try to manipulate it.

javiercn commented 4 years ago

@Shazwazza thanks for contacting us.

We are not sure we understand what your requirement is.

Our application is meant to be non-intrusive and thereby we cannot and do not want to set global options/services.

Not sure what this means, but Antiforgery in ASP.NET Core is meant to be global for the app and it is designed to be consistent with other parts of the frameworks, so it'll use services and options as the rest of the framework does.

But even in other scenarios the IAntiforgery implementation isn't flexible enough. Say for example I wanted to have a front-end and back-end setup where the front-end uses different cookie/header names than the back-end.

Can you help us understand what are you trying to achieve here with a more concrete example so that we can help you? The browser and the server need to agree for Antiforgery to work. The browser can't decide to send the request token in a way the server doesn't expect.

Shazwazza commented 4 years ago

Yep sure i can explain, we are porting our CMS (Umbraco) over to netcore from netframework. In our back office app it's an angular application. The antiforgery implementation there uses a cookie and a header of custom names, in our case UMB-XSRF-TOKEN and X-UMB-XSRF-TOKEN respectively. In the past we used to validate these with the ASP.NET Framework anti-forgery implementation like:

try
{
    AntiForgery.Validate(cookieToken, headerToken);
}
catch (Exception ex)
{
   // ... failed
}

and we could create tokens like:

AntiForgery.GetTokens(null, out cookieToken, out headerToken);

and then we could use those value to assign to custom cookies we have. One of those cookies is readable in JS and one is not. The readable one is used in JS to set the header in each request to our controllers.

This worked great but with the new antiforgery implementation:

We basically need to use our own header/cookie names and we can't without changing it for everyone. With the old antiforgery implementation you could validate specific values and now that is not possible which might be ok if we could have a separate service implementation with custom options. But that's also not possible.

javiercn commented 4 years ago

@Shazwazza thanks for the additional details.

What you say makes sense in your case. I think providing your own IAntiforgery implementation is the way to go here. That said, I would check what other CMS implementations do for this.

Maybe @sebastienros has some insights?

The antiforgery implementation in ASP.NET Core is meant to be global, as is optimized for the most common case of having 1 app on the process, so this scenario is not something we really contemplated I think.

sebastienros commented 4 years ago

You want the app to be isolated, but not really ;)

The issue is that you are having a problem with AFT but next it will be with Razor options, identity services, or anything that the user can configure and might impact you.

So yes a mitigation is to have your own services registered in DI, as I assume you can't force a specific DI that provides named services resolution, or it would also collide with the user app (one more example).

And I am sure you can't use a separate process because you want devs to be able to access in-process APIs to access content APIs directly for perf reasons. But that really is the only long term solution if you want to prevent interferences. There will always be something that collides with the user app. I assume that you can solve these issues one at a time, and can tell your users to just fallback to a separate site and use REST calls if there is something that can't be solved.

In Orchard, since we support multi-tenancy and each tenant has its own DI container, we can also create two tenants and have the user site resolve services explicitely from the "backend" tenant. Though I don't know anyone doing this, unless when we want to share an OpenId Server tenant with all websites, all running in the same process.

Shazwazza commented 4 years ago

Hi @sebastienros !

You want the app to be isolated, but not really ;)

Well yes :) and it should be possible. For the most part it's been fine up until this point. There are some work around needed to make this work but it's still more than possible to isolate our own controllers/filters/mvc options from the global ones. The Application Model allows us to do all of that nicely. Identity is also fine because we can have our own interfaces and also inherit from base classes and register/inject our own services. Identity also relies on the generic user type which also allows for easier segregation. We we have had to jump through a few hoops to fully segregate since there are a few 'global' only things like IdentityOptions, ILookupNormalizer, IdentityErrorDescriber, but all of those can be sub-classed and injected as separate services and this works well. We will solve these things one at a time and we're ok with that. The goal for Umbraco will be the same as it is today in that it will not get in the way of a developer's own site implementation.

For antiforgery however things are different because we cannot sub-class anything because everything is 100% internal. If we could sub-class DefaultAntiforgery then the problem would be solved since we could register our own service/interface and use our own AntiforgeryOptions.

I think providing your own IAntiforgery implementation is the way to go here.

essentially that is what we want to do but we don't want to rewrite how antiforgery works since it's all already done and would rather not re-invent security related code.

As of now we have 2 options which are both hacks:

public async Task<bool> ValidateTokensAsync(HttpContext httpContext, string cookieToken, string headerToken)
{
    // We need to do some tricks here, save the initial cookie/header vals, then reset later
    var originalCookies = httpContext.Request.Cookies;
    var originalCookiesHeader = httpContext.Request.Headers[HeaderNames.Cookie];
    var originalHeader = httpContext.Request.Headers[_antiforgeryOptions.Value.HeaderName];

    try
    {
        // swap the cookie anti-forgery cookie value for the one we want to validate
        // (this is how you modify request cookies, it's the only way)
        var cookieHeaderVals = CookieHeaderValue.ParseList(originalCookiesHeader);
        cookieHeaderVals.Add(new CookieHeaderValue(_antiforgeryOptions.Value.Cookie.Name, cookieToken));
        httpContext.Request.Headers[HeaderNames.Cookie] = cookieHeaderVals.Select(c => c.ToString()).ToArray();

        // swap the anti-forgery header value for the one we want to validate
        httpContext.Request.Headers[_antiforgeryOptions.Value.HeaderName] = headerToken;

        // now validate
        return await _defaultAntiforgery.IsRequestValidAsync(httpContext);
    }
    finally
    {
        // reset

        // change request cookies back to original
        var cookieHeaderVals = CookieHeaderValue.ParseList(originalCookiesHeader);
        httpContext.Request.Headers[HeaderNames.Cookie] = cookieHeaderVals.Select(c => c.ToString()).ToArray();

        // change the header back to normal
        httpContext.Request.Headers[_antiforgeryOptions.Value.HeaderName] = originalHeader;
    }
}

and creating the tokens is still at least easy and similar to how it worked in .Net Framework:

public void GetTokens(HttpContext httpContext, out string cookieToken, out string headerToken)
{
    var set = _defaultAntiforgery.GetTokens(httpContext);

    cookieToken = set.CookieToken;
    headerToken = set.RequestToken;
}

If DefaultAntiforgery was made public with a public constructor (everything else could remain internal if you wanted), this would solve the problem.

slaneyrw commented 4 years ago

Another situation that has arisen for us is that the internal DefaultAntiforgeryTokenGenerator uses the current request's HttpContext.User property to extract a UID, via DefaultClaimUidExtractor.

That, in turn, finds the first UID claim ( from a list of hardcoded claim types ) for the first identity from the principal.

The issue is that the HttpContext.User is mutated by the Authorization PolicyEvaluator so if request that generates the token doesn't run the same ( or equivalent ) policies with the same Authentication schemes ( and in the same order ), the UID found can change on the subsequent post, therefore validation fails.

ghost commented 3 years ago

Thank you for contacting us. Due to a lack of activity on this discussion issue we're closing it in an effort to keep our backlog clean. If you believe there is a concern related to the ASP.NET Core framework, which hasn't been addressed yet, please file a new issue.

This issue will be locked after 30 more days of inactivity. If you still wish to discuss this subject after then, please create a new issue!