Project

General

Profile

Actions

Feature #14718

closed

[API] Option to issue salted token in login procedure

Added by Peter Amstutz about 5 years ago. Updated about 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
Story points:
2.0
Release relationship:
Auto

Description

When redirecting a user agent to the Arvados login endpoint (https://aaaaa.arvadosapi.com/login), an application may specify a remote cluster ID in the query string (.../login?remote=bbbbb).

In that case, assuming the login is successful and a new token is issued, the new token will be salted for the specified remote.

This story does not cover prompting the user differently depending on the requested scope, although we will want to do so in the future.


Subtasks 1 (0 open1 closed)

Task #14751: Review 14718-api-login-salted-tokenResolvedLucas Di Pentima01/26/2019Actions

Related issues

Blocks Arvados - Feature #12958: [Federation] Workbench login chooserClosedActions
Actions #1

Updated by Peter Amstutz about 5 years ago

  • Status changed from New to In Progress
Actions #2

Updated by Peter Amstutz about 5 years ago

  • Subject changed from [API] untrusted client login receives salted token to [API] login flow for remote clusters that receives salted token
Actions #4

Updated by Peter Amstutz about 5 years ago

  • Related to Feature #12958: [Federation] Workbench login chooser added
Actions #5

Updated by Tom Clegg about 5 years ago

  • Subject changed from [API] login flow for remote clusters that receives salted token to [API] Option to issue salted token in login procedure
  • Description updated (diff)
  • Category set to API
  • Status changed from In Progress to New
Actions #6

Updated by Tom Clegg about 5 years ago

  • Related to deleted (Feature #12958: [Federation] Workbench login chooser)
Actions #7

Updated by Tom Clegg about 5 years ago

Actions #10

Updated by Tom Clegg about 5 years ago

  • Tracker changed from Bug to Feature
Actions #11

Updated by Tom Clegg about 5 years ago

  • Story points set to 2.0
Actions #12

Updated by Peter Amstutz about 5 years ago

  • Target version changed from To Be Groomed to Arvados Future Sprints
Actions #13

Updated by Tom Morris about 5 years ago

  • Target version changed from Arvados Future Sprints to 2019-01-30 Sprint
Actions #14

Updated by Lucas Di Pentima about 5 years ago

  • Assigned To set to Lucas Di Pentima
Actions #15

Updated by Lucas Di Pentima about 5 years ago

  • Status changed from New to In Progress
Actions #16

Updated by Lucas Di Pentima about 5 years ago

Updates at 4bb449eb5 - branch 14718-api-login-salted-token
Test run: https://ci.curoverse.com/job/developer-run-tests/1041/

Took advantage of return_to being passed around between API server and SSO to propagate the remote parameter.

Actions #17

Updated by Tom Clegg about 5 years ago

In user_sessions_controller.rb → login, it looks like params[:remote] is missing a CGI.escape() in case it's an unexpectedly non-alphanum/malicious string -- it gets appended to another string provided by the same client so I don't see how it would be exploitable, but I'm inclined to be defensive about it anyway.

        remote_param += "remote=#{params[:remote]}" 
Stashing the remote param in the return_to URL seems sensible enough, but
  • it's sneaky/non-obvious enough that it should be explained in comments
  • it should be removed from return_to before create() redirects there. If I'm following correctly, when Workbench sends the user to https://api/login?return_to=https://wb/home&remote=bbbbb, the user will eventually land on either https://wb/home?remote=bbbbb or https://wb/home depending on whether they needed to go through the joshid→sso→sessions#create flow or took the "already logged in" short cut. It would be better if we could ensure they always land on https://wb/home as requested. (Am I following correctly?)

I wonder whether it might even be less confusing to use a format like return_to="bbbbb,https://wb/home" (or ",https://wb/home" for the unsalted case) to avoid all the corner cases involved in stashing the remote id in the client-provided URL. For example, I'd rather not even try to figure out whether we're doing the right thing here when the client-provided return_to is something like "https://wb/home?foo=bar#foo/bar?baz".

Actions #18

Updated by Lucas Di Pentima about 5 years ago

Updates at 0d1836a8d
Test run: https://ci.curoverse.com/job/developer-run-tests/1043/

Addressed above suggestions.

Actions #19

Updated by Tom Clegg about 5 years ago

I think we should
  • ignore the remote param if it contains a comma (or perhaps even if it doesn't match /^[0-9a-z]{5}$/)
  • escape the whole combined param including the comma (I notice CGI.escape(',') == '%2c' -- and we will unescape the whole thing before splitting, so we might as well escape the whole thing after joining)

the rest LGTM, thanks!

Actions #20

Updated by Tom Clegg about 5 years ago

Tom Clegg wrote:

  • ignore the remote param if it contains a comma (or perhaps even if it doesn't match /^[0-9a-z]{5}$/)

...only on the "login" handler, not the "callback" handler.

Actions #21

Updated by Tom Clegg about 5 years ago

On second thought, I think we should error out (rather than ignoring the remote param) in both callback and login handlers if the "remote" value doesn't look like a cluster ID.

Actions #22

Updated by Lucas Di Pentima about 5 years ago

Updates at 245e4cafd
Test run: https://ci.curoverse.com/job/developer-run-tests/1044/

  • Validates the remote cluster id parameter both on login endpoint & omniauth callback.
  • Added tests.
  • Updated lib/controller handler test to support the new format.
Actions #23

Updated by Tom Clegg about 5 years ago

LGTM, thanks!

Actions #24

Updated by Lucas Di Pentima about 5 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 100
Actions #25

Updated by Tom Morris about 5 years ago

  • Release set to 15
Actions

Also available in: Atom PDF