IdentityServer / IdentityServer4

OpenID Connect and OAuth 2.0 Framework for ASP.NET Core
https://identityserver.io
Apache License 2.0
9.23k stars 4.02k forks source link

New user session generated when transitioning from anonymous to authenticated user #1815

Closed tillig closed 4 years ago

tillig commented 6 years ago

We have a use case where we're challenging a user with security questions prior to issuing a token. The user is "sort of anonymous" but sort of not - we know who they say they are, we just aren't totally sure yet.

Over the course of the process we need to track the user's session (e.g., the JWT session ID) and they need to keep that session ID from the time they start the process to the time they log out.

In DefaultUserSession.CreateSessionIdAsync I see that there is logic to not only generate a session ID if the auth properties dictionary is missing it, but also if the previous subject ID isn't the same as the new subject ID.

From the DefaultUserSessionTests.CreateSessionId_when_user_is_authenticated_but_different_sub_should_create_new_sid test I gather the idea is that if the authenticated user changes then a new session ID should be created.

I'm curious if the logic should be more like "If the user was previously authenticated and is now a different authenticated user then create a new sid."

That is, instead of this:

var currentSubjectId = (await GetUserAsync())?.GetSubjectId();
var newSubjectId = principal.GetSubjectId();

if (!properties.Items.ContainsKey(SessionIdKey) || currentSubjectId != newSubjectId)
{
  properties.Items[SessionIdKey] = CryptoRandom.CreateUniqueId(16);
}

Something like this:

var currentSubjectId = (await GetUserAsync())?.GetSubjectId();
var newSubjectId = principal.GetSubjectId();

if (!properties.Items.ContainsKey(SessionIdKey) ||
    (currentSubjectId != null && currentSubjectId != newSubjectId))
{
  properties.Items[SessionIdKey] = CryptoRandom.CreateUniqueId(16);
}

That would allow an anonymous user to transition to an authenticated user without getting a new session ID if the property is available.

Is there risk in that?

Granted, if #1791 gets resolved I can create my own derived IUserSession implementation but I'm also interested to see if this might be valuable in the default service.

TomCJones commented 6 years ago

this is a very real case that I am wrestling with myself. In my case I want to create the session even before the user tries to identify themselves with things like ip address. My thought is that there needs to be some other middle ware that processes the inbound messages until it is happy and then recovers/issues the sid. It also possible that the user has multiple logons, in the Microsoft case the multiple logons' can even have the same mailto: name.

brockallen commented 6 years ago

We can discuss.

kernelcoredump commented 6 years ago

In the Samples Quickstarts ExternalAuthentication AccountController.cs, a session ID was being generated and returned by the external provider, but this same issue is causing that session ID to be overridden with a newly-generated ID from the IdSrv4 DefaultUserSession service.

The sample might still be trying to interface with the pre-2.x usage of an ISessionIdService by directly setting the sid property item. This property item is not checked by the new IUserSession session creation mechanism.

brockallen commented 6 years ago

@kernelcoredump that sid claim is from the upstream provider and necessary to maintain for SLO purposes. it's different than the sid that IdentityServer creates and issues in its own tokens.

brockallen commented 6 years ago

Sessions are established when credentials are presented and the user has authenticated. New sessions should be established when we have new credentials for a new user or if the current user ends their session.

That would allow an anonymous user to transition to an authenticated user without getting a new session ID if the property is available.

If the user is anonymous, what establishes the end of one anonymous session and the start of a new one? Presumably something in your custom code or perhaps that's not a scenario for you?

That would allow an anonymous user to transition to an authenticated user without getting a new session ID if the property is available.

IIRC we had code like that in the past, but there was some use case where that was failing. I would have to go dig up the change/bug fix. IIRC, it was just before our 2.0 release.

tillig commented 6 years ago

The start of a logical session (for us) is when the user has entered credentials and started the authentication flow. It's more of a conversation than just username + password = token; it's username + password, then challenge with one or more authentication questions, then you get a token. From an issuance perspective, we don't trust that you are you until you've finished the whole conversation, but we need a sort of key that remains consistent across that conversation and into the point at which the authenticated user is truly trusted and gets a token.

The session ID seemed like a logical solution for that given that, too, needs to remain consistent across token refreshes and such.

The establishment of a new session happens in the UI authentication controller at the point the user has entered credentials that need to be validated - basically the POST of the initial set of creds. That's effectively when the "authentication state machine" starts.

brockallen commented 6 years ago

And then how do your clients and APIs consume this custom session id concept? Or is it just something you're only tracking within your login UI workflow?

tillig commented 6 years ago

The sid is used mostly as a cache key in clients and APIs for things that, say, get eagerly loaded into cache for serving future API requests. Some of that caching happens during credential validation and the rest of that login conversation and the cached values loaded there need to continue existing into the authenticated session.

brockallen commented 6 years ago

So you're caching things in IdentityServer based on sid and the clients and APIs hit APIs in IdentityServer passing that sid? Is that the gist?

tillig commented 6 years ago

Caching things in Identity Server, in protected APIs, and in clients, all based on sid. sid is acting as the coordinating claim that ties everything together for a single logical session.

brockallen commented 6 years ago

I guess what I'm getting at is that a client won't know a sid until the user has fully authenticated, and they would never gain any knowledge of one of these sids before the user had fully completed the login workflow in IdentityServer.

tillig commented 6 years ago

The client may not, but our APIs, which include the APIs to generate challenge questions, do. (Those get called with client credentials tokens, but we pass the sid to them that will be issued for the user if they pass through the authentication conversation.)

Later in the authenticated user session, the user's token may be used to challenge them with additional questions mid-session. sid needs to be the same there as it was during the original auth conversation.

tillig commented 6 years ago

Another of my teammates, @yelob, may be taking over for me to answer additional questions. If you see him chime in, it's the same setup / use case.

TomCJones commented 6 years ago

this is becoming a common user case, where there is no point at which the user if "fully authenticated" as any resource (api) access could require an additional authentication event to occur. So there are two approaches that id svr could assume:

  1. that the id svr is one of many authentication events undertaken by the client on behalf of a user
  2. that the id svr participates in the many authentication events associated with that user. I suspect that the first case is likely to be the one of most utility. The result of that is that multiple id svr instances are involved in a single user-client interaction session (sid). Or that the sid is not useful to the client at all.

    From: Travis Illig notifications@github.com Sent: Wednesday, December 6, 2017 11:12 AM To: IdentityServer/IdentityServer4 Cc: tom jones; Comment Subject: Re: [IdentityServer/IdentityServer4] New user session generated when transitioning from anonymous to authenticated user (#1815)

Another of my teammates, @yelobhttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fyelob&data=02%7C01%7C%7C33ba1efdf3084952062b08d53cdd59ba%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636481843740556865&sdata=3GZLncWqkGHq2SKWMS%2BJXAOftD5nfhexyezNc44u2eI%3D&reserved=0, may be taking over for me to answer additional questions. If you see him chime in, it's the same setup / use case.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FIdentityServer%2FIdentityServer4%2Fissues%2F1815%23issuecomment-349743626&data=02%7C01%7C%7C33ba1efdf3084952062b08d53cdd59ba%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636481843740556865&sdata=AzZsTme2JUJUf4xn5h4nHqw%2FJqyaDpoEBSr0ljv2EKc%3D&reserved=0, or mute the threadhttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAKxq1upXTXDJY5WtBU3IV1WqbrMdd4sqks5s9uc0gaJpZM4QvRFg&data=02%7C01%7C%7C33ba1efdf3084952062b08d53cdd59ba%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636481843740556865&sdata=irlZyke4eQc48ypwwE6S1Y59hFn8yq%2Fjtu9k9PmKLX4%3D&reserved=0.

TomCJones commented 6 years ago

There is, of course, one other approach. The id svr does what it does, and that is an end to authentication. Every other operation is assumed to be authorization (by fiat) and is treated as assigning a role to the authenticated user.

brockallen commented 6 years ago

For now why don't you just use a custom claim?

tillig commented 6 years ago

All of the plumbing to make sure the session ID was there, as a base claim in the token, and persists the same across token refreshes and all that - it's already there. Adding all that in as a custom claim will likely mean additional / duplicate work to get a custom claim to happen. Plus the general understanding of what JWT session ID is supposed to mean is understand across all the disparate microservice development teams in the enterprise as well as the customers of our APIs. Making it a custom claim throws a wrench in that.

brockallen commented 6 years ago

Making it a custom claim throws a wrench in that.

Except for the fact that you're asking IdentityServer to extend its notion of a session beyond what it understands. I was just offering a suggestion for the time being before we have an opportunity to add something like this as a feature (if it even makes sense without a lot of hurdles on our end).

brockallen commented 6 years ago

It sounds like my suggestion above is a reasonable workaround, so I don't think we plan to add any features around this soon.

tillig commented 6 years ago

We will likely keep our forked implementations of things, then, as a custom claim won't work for us. Even some of the internal things being public with some virtuals would help, but it is what it is. I don't think the concept of "logical user session" is really that far from the intent of token session, so we may have to agree to disagree about the level of reasonableness of the workaround and the idea that it's a vastly different concept of session than what Identity Server should have. It's definitely a wider view than what's there, and maybe that's all that matters at the immediate moment.

I'm on vacation and only have phone access through the beginning of the year so it's pretty hard to get in here and articulate long-form. However, given its not just me or my team interested in such control over issuance of the session ID, it may be worth considering such a feature valuable in more than a nebulous "future" context.

I understand the point, that this may not be one of those 80% use cases... But, again, even some of the internals opened up with virtual in place could make customizations here easier. Even if it's not a formally supported "feature."

leastprivilege commented 4 years ago

happening here https://github.com/IdentityServer/IdentityServer4/pull/3756

lock[bot] commented 4 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.