Add authentication revalidation logic to IndividualLocalAuth server-side Blazor template. Implements #10698 #11548
Conversation
|
Looks good to me, the security stamp is exactly that, its meant to be used to sign out everywhere, that's what cookies does, and by checking it directly, you are getting all the same invalidations that would cause cookies to be invalidated. Lockouts don't cause security stamps to roll I don't believe, but other than that yes. |
|
Thanks for the feedback, @HaoK!
That means the logic here (calling Is there anything else we should check besides this? |
|
I don't think you should deal with lockouts, just like lockouts don't invalidate all cookies (that would be a denial of service attack since you could just lock out a user and that would prevent all existing logged in users from ever accessing the site as well). The normal login flow should fail if the user is locked out, but if the security stamp hasn't changed, that isn't something you should worry about. @blowdart can confirm my understanding is correct |
|
Sorry Hao, I don't understand your points.
Why would locking out one user affect other users?
The normal login flow isn't applicable - our goal is for the long-running Blazor circuit state to get updated periodically. If a user is marked as locked out, we'd expect them to stop being allowed to use the app (at least as of the next time we revalidate them). So I think we do need to account for this. If you still think I'm wrong please let me know and perhaps we could chat verbally. Besides lock-out, is there anything else that would be a basis for thinking a user should no longer be allowed to interact with the app? I'm guessing no, but just checking for completeness. |
|
What I meant was, lets say I know your email, but I don't know your password, if I just spam bad login attempts with your email which resulted in login attempts being blocked for a certain amount of time, today that doesn't automatically cause existing cookies to be invalidated. I'm not clear exactly what the analogy is for blazor/circuits. But I'd imagine most people are permanently logged into their email accounts, someone being able to kick you out by attempting bad logins seems bad. Its only when you go and change your password, or update your authenticator key, or any other security sensitive action that causes the security stamp to change, that should result in your current sessions to become invalid. That's what I was trying to explain, does that make sense? |
|
OK, thanks for clarifying. I was guessing that the purpose of the "lock out" feature was something more like "site owner decides they don't like a given user and wants to ban them" or "this employee just got fired, so make sure they can't access company data". From your comment I guess the real purpose is rate-limiting password guess attempts, in which case I agree it should not be something that triggers automatic logout for existing authenticated sessions. Based on this I will regard the existing logic in this PR as correct. Thanks for the info! |
|
Right, so if you want to revoke all existing sign ins like that permanently, you can disable/lockout the user and then call |
|
@SteveSandersonMS Sorry for the delay, I've been busy putting out other fires. I'll try and take a look at it today. |
.../content/RazorComponentsWeb-CSharp/Areas/Identity/RevalidatingAuthenticationStateProvider.cs
Show resolved
Hide resolved
| /// authentication state at regular intervals. If a signed-in user's security | ||
| /// stamp changes, this revalidation mechanism will sign the user out. | ||
| /// </summary> | ||
| /// <typeparam name="TUser">The type encapsulating a user.</typeparam> |
There was a problem hiding this comment.
Should we have a comment here that says something like: "this is an important security feature, do not modify it unless you have a good reason"?
There was a problem hiding this comment.
I don't know, that seems pretty strange. Certain modifications would be totally reasonable (e.g., changing RevalidationInterval to a shorter or longer duration).
If you feel strongly, and want to suggest a particular phrasing, please let me know. Otherwise it seems more normal in our templates not to add scare-comments. For example we don't have similar comments about app.UseHsts or app.UseHttpsRedirection.
.../content/RazorComponentsWeb-CSharp/Areas/Identity/RevalidatingAuthenticationStateProvider.cs
Outdated
Show resolved
Hide resolved
| try | ||
| { | ||
| // Get the sign-in manager from a new scope to ensure it fetches fresh data | ||
| using (var scope = _scopeFactory.CreateScope()) |
There was a problem hiding this comment.
It looks like there's a race condition here that I'm not sure how to mitigate. The _scopeFactory could be disposed in any order with Dispose being called on this class. (I'm assuming that _scopeFactory) will become electrified when the app services are disposed.
If this happens the ObjectDisposedException is going be handled by the exception handler at L88, so we're probably fine.
Just thought I'd mention.
There was a problem hiding this comment.
You're right, but I also don't see any clear way of improving on this. We don't have a way to know for sure whether _scopeFactory was already disposed, or even if we did we couldn't lock it to prevent it being disposed in between that check and the usage of it or any services it supplies.
Like you've pointed out, the worst case is that it will log the exception, but the behavior will still be correct. Given this cycle only occurs every 30 mins by default, this should be extremely rare, if app developers ever see it at all.
If this becomes an issue in the future, I suspect we might need to introduce some event like "circuit will shut down soon" that things like this can listen to and proactively stop themselves.
| } | ||
|
|
||
| void IDisposable.Dispose() | ||
| => _loopCancellationTokenSource.Cancel(); |
There was a problem hiding this comment.
I think it's safe to call .Cancel() multiple times, but I wanted to mention because it will definitely happen. If we see an error or a change in security stamp we cancel the token, then the user disconnects, then the circuit is disposed and calls cancel again.
There was a problem hiding this comment.
My research also agrees with the conclusion that you can cancel multiple times. I think it's preferable to keep the code here simpler by leaving this as-is instead of introducing some locking or whatever we'd do to guarantee single cancel.
SteveSandersonMS
force-pushed
the
stevesa/revalidate-authentication-state
branch
from
ca60c12 to
7524725
Compare
Implements #10698
Following discussions with security and SignalR folks, we've decided we need to have some form of periodic revalidation for authentication in server-side Blazor. This is meaningful only in the Identity case, because that's what has the concept of security stamps and can re-query the DB to get updated state.
Before broader code review, I'd first like to check with authentication/Identity experts that the logic here looks right. Ping @HaoK. And @blowdart, is there anyone else you'd like to look at this?
The specific bit that authentication/Identity experts might most be interested in is the contents of
CheckIfAuthenticationStateIsValidAsync. This gets called periodically (every 30 minutes by default) and is responsible for deciding whether to sign the user out. The rule is very simple: if your security stamp changed, we log you out. Then the user is forced to go through the login flow again, which refreshes everything about theirClaimsPrincipal(if they are still even able to log in).So, @HaoK, can you let me know if you think the logic in
CheckIfAuthenticationStateIsValidAsyncaccounts for everything necessary? For example, if a user got locked out or had their roles changed, then their security stamp will have changed, right?If you need any further information, I'd be happy to have a brief meeting to explain any finer points of the mechanism. Let me know.