search

wtsi-hgi / irobot

iRODS data brokerage service
GNU General Public License v3.0
0 stars 0 forks source link

Include realm (etc.) in authentication handlers' challenge-response #8

Closed jrandall closed 7 years ago

jrandall commented 7 years ago

From https://tools.ietf.org/html/rfc2617#page-4:

The realm directive (case-insensitive) is required for all
   authentication schemes that issue a challenge.

Currently iRobot appears to serve headers that look like this: 

WWW-Authenticate: Basic, Arvados

According to my understanding of RFC2617, this is not permitted -- iRobot should instead send a WWW-Authenticate header that looks something like this (at a minimum): 

WWW-Authenticate: Basic realm="some realm", Arvados realm="some realm"

Note also that clients need to parse this carefully (From: https://tools.ietf.org/html/rfc2617#section-1.2): 

Note: User agents will need to take special care in parsing the WWW-
   Authenticate or Proxy-Authenticate header field value if it contains
   more than one challenge, or if more than one WWW-Authenticate header
   field is provided, since the contents of a challenge may itself
   contain a comma-separated list of authentication parameters.

Because the server would be in spec to serve something like this: 

WWW-Authenticate: Basic realm="some realm", numberwang=14, Arvados realm="some realm", wangernum=6
jrandall commented 7 years ago

FWIW, I'm not sure there is a good reason to have an Arvados authentication mechanism, as it's semantics are not different from Basic - the API token is basically a password - there is no challenge or nonce involved. Why not support Arvados API token auth by simply having a special username arvados for which you provide the API token as the password?

Xophmeister commented 7 years ago

I'm aware of the realm issue, at least as far as basic authentication is concerned. It's not a priority of mine, right now, to include it. Although I didn't realise it was common to all challenge-response type authentication schemes, so that may bump it up the priority list. Having said that, note that RFC7235 (which updates RFC2617) suggests that the realm is no longer required: https://tools.ietf.org/html/rfc7235#section-2.2

The Arvados authentication handler is already written, so I don't see any utility in refactoring it into the basic auth handler. I agree that they're basically the same -- i.e., a token string that can be decoded by some decoder...but then, OAuth is also like that -- but I would argue that having the authentication scheme alongside it points you to the correct decoder. (Also, what if your basic authenticator has a user called arvados, or whatever magic string you choose?)

Anyway, for example, my Arvados auth handler makes a request to the Arvados API with that token to get the Arvados username; whereas my basic auth handler just decodes the username straight out of the token. (Presuming they pass, obviously!) That seemed quite a satisfying approach to me.

jrandall commented 7 years ago

Ok - the clearest note of the change to not require realm is in appendix A: https://tools.ietf.org/html/rfc7235#appendix-A

Xophmeister commented 7 years ago

I'll leave this closed, but I may add an optional realm to at least the Basic authentication handler because this is actually used in production. It may be worth providing a general way to include handler parameters (e.g., Digest authentication uses a bunch of them, IIRC), but it's not a huge priority.