search

apereo / dotnet-cas-client

Apereo .NET CAS Client
Apache License 2.0
234 stars 172 forks source link

Bad practice to store per-request value in ThreadStatic variables in ASP.Net #63

Open lanorkin opened 7 years ago

lanorkin commented 7 years ago

In ASP.Net, there's no guarantee that a request will use the same thread for its entire duration, see here for example https://stackoverflow.com/a/4791234/2170171

As a result this logic with [ThreadStatic] works on small load, but can lead to security issues on highload:

https://github.com/apereo/dotnet-cas-client/blob/master/DotNetCasClient/CasAuthentication.cs#L107

        // Provide reliable way for arbitrary components in forms
        // authentication pipeline to access CAS principal
        [ThreadStatic]
        private static ICasPrincipal currentPrincipal;

One of possible solutions is to use HttpContext.Current.Items[...] instead of static variables, but in general providing static access to principal can be considered a bad design.

phantomtypist commented 7 years ago

@lanorkin

Just to confirm, this is a duplicate of issue #32?

There is a PR already out with a fix, but we'll have to review it. If you have time, please look at the PR and provide feedback in issue #32 so the original person how submitted the PR can see your comments.

Much appreciated!

lanorkin commented 7 years ago

@phantomtypist Yes, it demonstrates the result of sharing thread between requests. I would say that this #63 is not duplicate, as #32 is caused by #63, but sure it's up to you to decide. I don't agree with fix though, will comment in #32

phantomtypist commented 7 years ago

@lanorkin Thanks for taking the time you put into this. After a short review, I think I'm on the same page as @serac with a possible alternative solution (https://github.com/apereo/dotnet-cas-client/pull/33#issuecomment-155762594). Did you happen to read that comment? If so I'd like to hear your thoughts?

Maybe duplicate is not the right word. Boy do I wish that GitHub issues was as robust/enterprise-grade as Atlassian JIRA is... is there no way to link two issues together and identify the relationship as "related"? Maybe I'm just not seeing it.

lanorkin commented 7 years ago

@phantomtypist @tcalvert @serac Not sure what is best place to discuss this as we already have discussions here, in issue #32 and in PR #33. Let it be here. I completely agree with fix in PR in short term (hey, who am I kidding? two years is not short term, so I think PR should be just merged and released to nuget, as that's really huge security flaw, which is really not good for an authentication project). But in long term, Thread.CurrentPrincipal is not really good place to share Identity. What I mean, you of course can set it as it is needed for smooth work, but it cannot be master storage for this information. If library user wants to use own principal class, they should have a way to read authentication data. As for what to use for that master storage - currently it is [ThreadStatic], which is just wrong. I advise HttpContext.Current, which is better, and will work for all places where HttpContext is available. As @tcalvert mentions, there can be cases where HttpContext is not available. I agree with that, but from my perspective to choose better place some global redesign should be done for the library.