openid-certification / oidctest

THE CERTIFICATION TEST SUITE HAS BEEN MIGRATED TO A NEW SERVICE https://www.certificatinon.openid.net
Other
49 stars 15 forks source link

rp-backchannel-rpinitlogout: should not callback the initiating RP #205

Closed zandbelt closed 4 years ago

zandbelt commented 4 years ago

When my RP initiates the backchannel logout towards the OP, it also kills its own session before doing so. It does not expect the OP to callback to itself, the session is already killed by then. The test suite seems to assume that the RP needs a callback to kill the session, and the initial request is merely a redirect to trigger that.

The logout spec points to the session management spec which says in https://openid.net/specs/openid-connect-session-1_0.html#RPLogout:

An RP can notify the OP that the End-User has logged out of the site and might want to log out of the OP as well. In this case, the RP, after having logged the End-User out of the RP, redirects the End-User's User Agent to the OP's logout endpoint URL.

so though it does not say that the OP should not callback the initiating RP, I believe that follows out of the current text nevertheless: calling back is pointless because the RP should have already killed its own session, therefore I suggest to adapt the test suite for that.

rohe commented 4 years ago

This is about the post_logout_redirect_uri again, right ? https://openid.net/specs/openid-connect-session-1_0.html#RedirectionAfterLogout If not present then no call back to the RP is expected (or wanted). If present then the RP do expect a callback.

zandbelt commented 4 years ago

no, it is about the logout request itself from OP to RP

rohe commented 4 years ago

OK, so about the backchannel_logout_uri then ?

zandbelt commented 4 years ago

what about it?

rohe commented 4 years ago

Since we're dealing with Single Logout here the OP will try to log you out from all the RPs that have active sessions for the user. If you have specified a backchannel_logout_uri (or frontchannel_logout_uri for that matter) then the OP will contact the RP otherwise not.

zandbelt commented 4 years ago

yes, and my point is that the initiating RP already killed the session, as the spec suggests, so does not need the callback

rohe commented 4 years ago

So, you can leave out the backchannel_logout_uri but then I would say that you don't support backchannel logout. There is nothing in the spec that says that the OP should refrain from doing a callback to the RP that initiated the end session. Seems like this is something for the mailing list to discuss.

panva commented 4 years ago

An RP should still expect to get both front and back-channel notifications regardless of being the RP that initiated that logout. Think about a setup where the initiation is just a frontend webpage but session information is stored on the backend as well. We should not be exempting those scenarios.

zandbelt commented 4 years ago

Well the spec says that whatever the RP would have to do on the incoming logout request, it should have already done that before initiating the request towards the OP...

Anyway, I'm thinking: a) we should make OIDC certification require the RP to deal with incoming backchannel requests that do not have a session associated with them gracefully (for both RP and OP), so return the equivalent of "OK" b) if we do not do a) we'd have to come up with another test that actually simulates a different RP initiating the request, so the backchannel logout request to the RP being tested finds an existing session

we may not want to go b) anyhow...

panva commented 4 years ago

Shouldn't we just allow any of 200, 501 or 504 as the specs indicate then? No honestly, since the OP isn't likely going to do anything if it gets anything else than a 200 OK if i was writing an RP that accepts backchannel logout requests if i didn't find the session i'd still respond with 200.

zandbelt commented 4 years ago

agreed, that's what I'm proposing as well: I also think it should be made explicit as part of the certification requirements, to cater for interoperability

selfissued commented 4 years ago

The OP is required to send logout messages to all RPs that it believes are logged in with it, including the RP sending an RP-Initiated Logout message to it. That's true for all three OP-Initiated logout mechansims (Session, Front-Channel, Back-Channel). Nowhere does the spec say that the RP should try to log itself out before sending the RP-Initiated Logout message. (It can always clear its login state at any time, so doing so isn't a spec violation.)

I do agree that RPs should gracefully handle logout messages if they're already logged out - effectively making them idempotent.

This is a valuable discussion. I'll file a spec issue making sure that these things are completely clear in the logout specs. Thanks all!

I believe that this issue should be closed on this basis.

zandbelt commented 4 years ago

Nowhere does the spec say that the RP should try to log itself out before sending the RP-Initiated Logout message. (

As mentioned earlier, it does:

The logout spec points to the session management spec which says in https://openid.net/specs/openid-connect-session-1_0.html#RPLogout:

An RP can notify the OP that the End-User has logged out of the site and might want to log out of the OP as well. In this case, the RP, after having logged the End-User out of the RP, redirects the End-User's User Agent to the OP's logout endpoint URL.

and exactly because of that, it does not make sense to callback the initiating RP, because: assuming the OP believes the RP implements the spec in the way above, it can assume that the RP does not have a session any longer.

panva commented 4 years ago

Let's see, if that was ALWAYS the case what's the point of the state parameter if not to tie back a post_logout_redirect_uri redirect form the OP to make RP session state clear AFTER OP logout safe?

I feel the RP should be able to choose when it wants to clear its session, not to be forced to drop it before. Doing so would make the state absolutely moot.

zandbelt commented 4 years ago

then you suggest to change the spec, because the wording right now suggests something else

also: state is optional can can be used by the RP literally for anything, including keeping track of language or other prefs, not necessarily login session related

panva commented 4 years ago

Absolely, it's a draft for a reason and this ain't a big change, for one i don't agree we shouldn't be sending notifications to the RP that initiated the logout. I recall a discussion with Vladimir on the list about a similar point, if an RP is registered for notifications and initiates the logout but the OP renders the prompt and the end-user ONLY clears the RP that initiated's session, both oidc-provider and Vladimir's project will trigger notifications for ONLY the RP that triggered.

zandbelt commented 4 years ago

I also think it is more robust to send to all RPs, since robust RPs will have to deal with non-existing session logout requests anyway, but RPs need to cater for it and the spec now suggest they don't have to. So, either we make it explicit in the certification requirements, or we change the spec. That's all.

selfissued commented 4 years ago

As a practical matter, the OP has no way of knowing for any of the three methods that the RP sending the RP-Initiated Logout message is already logged out, therefore it must attempt to log it out if it believes that it was logged in. This action should be idempotent, and therefore always safe.

panva commented 4 years ago

I also think it is more robust to send to all RPs

issue title: rp-backchannel-rpinitlogout: should not callback the initiating RP

I'm confused... even under the assumption that the RP must clear before (which I believe we'll change), the spec does not say NOT to notify all RPs. So why wouldn't the test suite always send it?

zandbelt commented 4 years ago

at least it will make RPs (like mine) implement more robust behaviour...

but I also think there should be more clear guidance on how and when to kill sessions for RPs

also: the spec also does not say that the OP should not notify all RPs twice... ;-)

panva commented 4 years ago

So the left TODO?

or my prefered, much simpler to parse and to understand the intention of the notification:

selfissued commented 4 years ago

I'm mostly with @panva's suggestions. That said, I believe that we should update the language at https://openid.net/specs/openid-connect-backchannel-1_0.html#BCResponse to state that if the RP is already logged out, that it should return a 200, since the logout already succeeded.

I'll plan to describe that RPs should treat logout requests as being idempotent in all three specs.

I'll file Connect issues later today and link to them in this issue.

Thanks again for these discussions. This is exactly the kind of feedback we need to tighten up the specs before they're final!

zandbelt commented 4 years ago

technically I still believe the suite should verify that the RP actually deleted the session before the backchannel call comes in - since that is what the spec will still dictate - but I also think it is probably too much trouble right now; as said, it would also require another test that includes the RP in a logout sequence that it did not initiate itself; oh well, you can't win 'm all; I did verify that the 200 now makes the suite happy

travisspencer commented 4 years ago

As a practical matter, the OP has no way of knowing for any of the three methods that the RP sending the RP-Initiated Logout message is already logged out, therefore it must attempt to log it out if it believes that it was logged in. This action should be idempotent, and therefore always safe.

Agree but wanted point out that the request to the RP is idempotent by definition since it's a GET request. HTTP already says that it must not change state on the server.