Project

General

Profile

Actions

Feature #21137

closed

When using OpenID authentication with SSO, redirect user to a logout URL after doing browser logout

Added by Peter Amstutz 7 months ago. Updated 6 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Story points:
-
Release relationship:
Auto

Description

https://openid.net/specs/openid-connect-rpinitiated-1_0.html

"An RP requests that the OP log out the End-User by redirecting the End-User's User Agent to the OP's Logout Endpoint. This URL is normally obtained via the end_session_endpoint element of the OP's Discovery response or may be learned via other mechanisms."

https://openid.net/specs/openid-connect-backchannel-1_0.html

"This specification defines a logout mechanism that uses direct back-channel communication between the OP and RPs being logged out; this differs from front-channel logout mechanisms, which communicate logout requests from the OP to RPs via the User Agent."

When the user visits the /logout endpoint and OpenID Connect authentication is in use:

  • if the token looks like an OpenID connect token, try to invalidate it using backchannel logout
  • return a redirect to end_session_endpoint with the post_logout_redirect_uri set to the original redirect_to and id_token_hint to the token

Subtasks 1 (0 open1 closed)

Task #21150: Review 21137-rp-initiated-logoutResolvedBrett Smith11/17/2023Actions
Actions #1

Updated by Peter Amstutz 7 months ago

  • Tracker changed from Bug to Feature
Actions #3

Updated by Peter Amstutz 7 months ago

  • Description updated (diff)
Actions #4

Updated by Peter Amstutz 7 months ago

  • Target version changed from Future to Development 2023-11-08 sprint
Actions #5

Updated by Peter Amstutz 7 months ago

  • Release set to 67
Actions #6

Updated by Peter Amstutz 7 months ago

  • Category changed from Workbench2 to API
  • Description updated (diff)
  • Subject changed from When using OpenID authentication with SSO, option to redirect user to a logout URL after doing browser logout to When using OpenID authentication with SSO, redirect user to a logout URL after doing browser logout
Actions #7

Updated by Peter Amstutz 7 months ago

  • Assigned To set to Brett Smith
Actions #8

Updated by Peter Amstutz 7 months ago

  • Description updated (diff)
Actions #9

Updated by Peter Amstutz 7 months ago

  • Description updated (diff)
Actions #10

Updated by Peter Amstutz 7 months ago

  • Target version changed from Development 2023-11-08 sprint to Development 2023-11-29 sprint
Actions #11

Updated by Brett Smith 6 months ago

  • Status changed from New to In Progress

Peter Amstutz wrote:

When the user visits the /logout endpoint and OpenID Connect authentication is in use:

  • if the token looks like an OpenID connect token, try to invalidate it using backchannel logout

I don't think we want this. The back-channel logout spec describes a mechanism by which OPs can tell RPs that a user is logged out. Note the summary in section 2.5:

The OP uses an HTTP POST to the registered back-channel logout URI to trigger the logout actions by the RP.

What does this do for us, given that the user is expected to hit our regular logout endpoint first?

If we do need this, we'll need to add an API endpoint. And the OIDC library we're currently using doesn't seem to support this spec, so we'll also need to find and integrate another library that does, or implement our own logout token validation code. Either way, it makes this a fairly significant story.

Actions #12

Updated by Peter Amstutz 6 months ago

Brett Smith wrote in #note-11:

Peter Amstutz wrote:

When the user visits the /logout endpoint and OpenID Connect authentication is in use:

  • if the token looks like an OpenID connect token, try to invalidate it using backchannel logout

I don't think we want this. The back-channel logout spec describes a mechanism by which OPs can tell RPs that a user is logged out. Note the summary in section 2.5:

The OP uses an HTTP POST to the registered back-channel logout URI to trigger the logout actions by the RP.

What does this do for us, given that the user is expected to hit our regular logout endpoint first?

If we do need this, we'll need to add an API endpoint. And the OIDC library we're currently using doesn't seem to support this spec, so we'll also need to find and integrate another library that does, or implement our own logout token validation code. Either way, it makes this a fairly significant story.

You're right, I misunderstood what the back channel spec is for. The direction of communication in that spec is for the OpenID Provider (OP) to tell the Relaying Party (RP) that the user is terminating the session. While this would have some security value, it isn't the main goal of the story.

It looks like what we want in the OpenID bearer token case is still RP-Initiated logout, with the bearer token sent along so it will be invalidated as part of the logout process.

Actions #13

Updated by Brett Smith 6 months ago

21137-rp-initiated-logout @ bd471a9eadaf564fb4beafd7db995b7762942c1d - developer-run-tests: #3907

doc-and-sdk-R retest developer-run-tests-doc-and-sdk-R: #2072 (that was a weird failure, it doesn't look transient but I can't figure it out)

  • All agreed upon points are implemented / addressed.
    • Yes. Note from earlier discussion that we decided that only RP-initiated logout is included in the story. We decided backchannel logout is not really relevant for what we're doing.
  • Anything not implemented (discovered or discussed during work) has a follow-up story.
    • N/A
  • Code is tested and passing, both automated and manual, what manual testing was done is described
    • See above
  • Documentation has been updated.
    • I don't believe there's anything to document for either users or administrators. This strictly adds support for an OIDC extension using discovery metadata already published by the OP.
  • Behaves appropriately at the intended scale (describe intended scale).
    • Yes. There's no major scale change, it just gives logout requests a different RedirectLocation when applicable.
  • Considered backwards and forwards compatibility issues between client and server.
    • Yes, there is no change to way users call the logout endpoint or parse the response.
  • Follows our coding standards and GUI style guidelines.
    • Coding standards yes, GUI style N/A
Actions #14

Updated by Tom Clegg 6 months ago

I don't think we're using the right id_token_hint here. The spec says "ID Token previously issued by the OP".
  • If the token in our request is an OIDC token (AcceptAccessToken is in play), it would be an access token, not an ID token.
  • If the token in our request is an Arvados token, we will have seen an OP-issued ID token during the login process, but (with the current code) it wasn't saved anywhere, so we still can't send it.

Given that the spec says the OP MUST ask the End-User [whether to log out of the OP as well] if an id_token_hint was not provided, and given the user might or might not expect the Arvados logout button to log out of their entire SSO session, perhaps an acceptable solution is to never send an id_token_hint?

Code style:
  • "Handle errors immediately" pattern would be clearer here:
            resp, err := logout(ctx, ctrl.Cluster, opts)
    -       creds, credsOK := auth.FromContext(ctx)
    -       if err == nil && ctrl.endSessionURL != nil && credsOK && len(creds.Tokens) > 0 {
    +       if err != nil {
    +               return arvados.LogoutResponse{}, err
    +       }
    +       creds, credsOK := auth.FromContext(ctx)
    +       if ctrl.endSessionURL != nil && credsOK && len(creds.Tokens) > 0 {
    
  • These can be just c.Assert:
    -       if !c.Check(err, check.IsNil) {
    -               return
    -       }
    +       c.Assert(err, check.IsNil)
    
  • TestEndSessionEndpointBadScheme checks for a non-nil error, but could easily check that it's the right error (I'm wary of "ensure this returns an error" tests that could keep passing due to some unrelated error). c.Check(err, check.ErrorMatches, `.*MUST use HTTPS.*`)
Actions #15

Updated by Brett Smith 6 months ago

Tom Clegg wrote in #note-14:

I don't think we're using the right id_token_hint here. The spec says "ID Token previously issued by the OP".
  • If the token in our request is an OIDC token (AcceptAccessToken is in play), it would be an access token, not an ID token.
  • If the token in our request is an Arvados token, we will have seen an OP-issued ID token during the login process, but (with the current code) it wasn't saved anywhere, so we still can't send it.

Given that the spec says the OP MUST ask the End-User [whether to log out of the OP as well] if an id_token_hint was not provided, and given the user might or might not expect the Arvados logout button to log out of their entire SSO session, perhaps an acceptable solution is to never send an id_token_hint?

I've gone over it myself again and your reading all seems right to me. I've made that change.

A general point, not about this branch or the review: I did what the story said to do. It seems like it would be nicer process-wise if we could've figured this out before I started writing code? "Arvados doesn't retain the ID token" seems like it was knowable at grooming time. And I don't know if any other reviewer would've caught this.

  • "Handle errors immediately" pattern would be clearer here: [...]

Done.

  • These can be just c.Assert: [...]

Not literally the same because this is inside a helper method. Assert would end test execution immediately; this lets the calling test run more checks if it wants. I don't know if that will ever matter, but I do feel like I see a pattern in our tests where we end up with odd duplication or setup code paths because someone wanted to use like 90% of a helper method with one tweak but changing that method was too much work or whatever. I was trying to prevent history repeating by assuming less about what the caller wants to do.

  • TestEndSessionEndpointBadScheme checks for a non-nil error, but could easily check that it's the right error (I'm wary of "ensure this returns an error" tests that could keep passing due to some unrelated error). c.Check(err, check.ErrorMatches, `.*MUST use HTTPS.*`)

Done.

21137-rp-initiated-logout @ 795fecd239bb7905c92576be8c6e1c3144c17fc3 - developer-run-tests: #3920

doc-and-sdk-R rerun - developer-run-tests-doc-and-sdk-R: #2086

Actions #16

Updated by Tom Clegg 6 months ago

  • These can be just c.Assert: [...]

Not literally the same because this is inside a helper method.

Ah yes, I missed that. You're right, Check is better.

All LGTM, thanks.

Actions #17

Updated by Brett Smith 6 months ago

  • Status changed from In Progress to Resolved
Actions

Also available in: Atom PDF