Arvados

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.

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

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.*`)

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

• 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.