Skip to content
This repository was archived by the owner on Dec 13, 2018. It is now read-only.

Add OIDC remote sign-out to the sample. #613

Closed
wants to merge 1 commit into from
Closed

Add OIDC remote sign-out to the sample. #613

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion samples/OpenIdConnectSample/Startup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using Microsoft.AspNet.Builder;
using Microsoft.AspNet.Http;
using Microsoft.AspNet.Http.Authentication;
using Microsoft.AspNet.Http.Features.Authentication;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Microsoft.IdentityModel.Protocols.OpenIdConnect;
Expand Down Expand Up @@ -39,6 +40,23 @@ public void Configure(IApplicationBuilder app, ILoggerFactory loggerfactory)

app.Run(async context =>
{
var authContext = new AuthenticateContext(CookieAuthenticationDefaults.AuthenticationScheme);
await context.Authentication.AuthenticateAsync(authContext);
if (context.Request.Path == new PathString("/remote-sign-out"))
{
var session = authContext.Properties[OpenIdConnectSessionProperties.SessionState];
var sid = context.Request.Query["sid"];
if (string.Equals(sid, session, System.StringComparison.Ordinal))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#423 (comment)

IMHO, this comparison will unlikely work everywhere, since there's absolutely no obligation for an OP to use the same values for sid and session_state.

Identity Server, for instance, derives the session state from the session id (IIRC, it concatenates the session id and the client id and adds some salt) so you can't compare both values.

I guess it's probably not a big deal since it's just an AAD sample, but if you wanted it to be a general guidance, it would be something to fix.

/cc @leastprivilege

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC the spec says what those are supposed to be. Maybe AAD does it differently then the spec?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The specs are rather permissive as the only requirement is to include the client identifier, the origin URL and the initial state in the session state 😄

sid (session ID)

The sid (session ID) Claim used in ID Tokens and as a logout_uri parameter has the following definition:

OPTIONAL. String identifier for a Session. This represents a Session of an OP at an RP to a User Agent or device for a logged-in End-User. Its contents are unique to the OP and opaque to the RP. It MUST have sufficient entropy to prevent collisions between Session IDs generated by different OPs and to prevent it from being guessed by potential attackers. Its syntax is the same as an OAuth 2.0 Client Identifier.

http://openid.net/specs/openid-connect-logout-1_0.html

session_state

Session State. JSON [RFC7159] string that represents the End-User's login state at the OP. It MUST NOT contain the space (" ") character. This value is opaque to the RP. This is REQUIRED if session management is supported.

The Session State value is initially calculated on the server. The same Session State value is also recalculated by the OP iframe in the browser client. The generation of suitable Session State values is specified in Section 4.2, and is based on a salted cryptographic hash of Client ID, origin URL, and OP browser state. For the origin URL, the server can use the origin URL of the Authentication Response, following the algorithm specified in Section 4 of RFC 6454 [RFC6454].

// Here, the session_state is calculated in this particular way,
// but it is entirely up to the OP how to do it under the
// requirements defined in this specification.

http://openid.net/specs/openid-connect-session-1_0.html

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but as you point out they're not the same thing (except in AAD I guess where the spec is a more of a mild suggestion on how to do things). Point being, the check above shouldn't really work. You need to keep the sid from the original id_token and not the session state from the authorization response.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. The weird thing is that it works with AAD (but maybe it's not that weird? 😄)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so where does the sid claim come from? It's not necessarily the same thing as the session_state in the authorization response. Claims, from the client's perspective, are obtained in the id_token, no? Is ADD expecting a client to use the session_state value for this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the contract is: if discover.logout_session_supported==true, then a 'sid' claim should show up in the id_token. I agree that session_state is a different thing, however I suspect the that it may be the same value, since the idp will probably do similar things with it. It's confusing that the two 'logouts' have a different model for the session identifier, found in two different places.
My take is that AAD is not supporting the 'sid' yet. We added a property to support this in OIDCConfiguration - HttpLogoutSupported, I think RC1. Not sure, but it is there now. You can just check the metadata.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, session_state can be the same as the sid, but a client should not have to know the particularities of the implementation and do things that aren't described in the spec (such as using the session_state instead of the [absent] sid for this type of check).

i guess what's confusing to me is why doesn't AAD just do an AAD-specific MW and leave the protocol focused MW in the ASP.NET team's hands? AAD has already created AAD-specific libraries with ADAL, so why not this too? or perhaps that would expose too many of these lax protocol compliance issues in AAD. these are the sorts of issues OIDC was meant to fix with OAuth2. or maybe it's the fault of the spec authors all being too lax given all the vendors' influence.

i know this isn't your your fault @brentschmaltz, but i'm just expressing frustration that these AAD specifics seem to crop up frequently in what's purported to be a protocol library. and i apologize to @Tratcher for hijacking his PR with my off topic comments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we're all reviewing an AAD sample that relies on a feature that AAD doesn't support? #YOLO

@brockallen nah, let's integrate ADAL directly in the OIDC middleware :trollface:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brockallen I think you are correct, the 'session_state' belongs in a AAD-specific MW. We should just rely on the 'sid' claim. We shouldn't muddy the waters.

{
await context.Authentication.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme);
await context.Response.WriteAsync("Signed out remotely.");
return;
}
context.Response.StatusCode = 400;
await context.Response.WriteAsync("Missing or mismatched sid.");
return;
}

if (!context.User.Identities.Any(identity => identity.IsAuthenticated))
{
await context.Authentication.ChallengeAsync(OpenIdConnectDefaults.AuthenticationScheme, new AuthenticationProperties { RedirectUri = "/" });
Expand All @@ -49,7 +67,17 @@ public void Configure(IApplicationBuilder app, ILoggerFactory loggerfactory)
}

context.Response.ContentType = "text/plain";
await context.Response.WriteAsync("Hello Authenticated User");
await context.Response.WriteAsync("Hello Authenticated User\r\n");
foreach (var claim in context.User.Claims)
{
await context.Response.WriteAsync($"{claim.Type}, {claim.Value}\r\n");
}
foreach (var property in authContext.Properties)
{
await context.Response.WriteAsync($"{property.Key}, {property.Value}\r\n");
}

await context.Response.WriteAsync($"Logout: /remote-sign-out?sid={authContext.Properties[OpenIdConnectSessionProperties.SessionState]}\r\n");
});
}
}
Expand Down