Add OIDC remote sign-out to the sample. #613
Conversation
|
Hi @Tratcher, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
| { | ||
| var session = authContext.Properties[OpenIdConnectSessionProperties.SessionState]; | ||
| var sid = context.Request.Query["sid"]; | ||
| if (string.Equals(sid, session, System.StringComparison.Ordinal)) |
There was a problem hiding this 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
There was a problem hiding this comment.
IIRC the spec says what those are supposed to be. Maybe AAD does it differently then the spec?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agreed. The weird thing is that it works with AAD (but maybe it's not that weird? 😄)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 
There was a problem hiding this comment.
@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.
|
HttpLogoutSupported = false. Closing this PR for now as we're lacking an implantation for interop testing. |
|
Maybe you should use an openid connect spec compliant and certified server to test with - you can find a selection here: http://openid.net/certification/ just sayin' ;p |
#423 @brentschmaltz @tushargupta51 @PinpointTownes @muratg @HaoK @Eilon
This is one way to implement http://openid.net/specs/openid-connect-logout-1_0.html as an application endpoint. Incorporating this into the middleware has a number of problems:
A) it's another endpoint to listen on.
B) the spec is still pretty new, it's unclear how much it may change.
C) to avoid xsrf issues you need to flow and verify the session id. The middleware could only do this for the simple OIDC + cookies scenario. If you used Identity you'd have to manually flow the session id and implement your own endpoint anyways.
D) an app probably wants to do other things when the user logs out like clear session state, etc..
I recommend leaving it as a sample for now.