openbmc / phosphor-rest-server

REST server that transposes dbus interfaces to REST
Apache License 2.0
4 stars 11 forks source link

Login errors return OK status code in HTTP and in response body #25

Closed KennethWilke closed 8 years ago

KennethWilke commented 8 years ago

When attempting to login, if using the wrong password the HTTP response gives a 200 OK in the HTTP headers as well as in the return body. It does give an error only in the data bit of the response

$ curl -v -c cjar -k -X POST -H "Content-Type: application/json" -d '{"data": [ "root", "notmypassword" ] }' https://obmc.example.com/login
Note: Unnecessary use of -X or --request, POST is already inferred.
*   Trying obmc.example.com...
* Connected to obmc.example.com (obmc.example.com) port 443 (#0)
* found 173 certificates in /etc/ssl/certs/ca-certificates.crt
* found 697 certificates in /etc/ssl/certs
* ALPN, offering http/1.1
* SSL connection using TLS1.2 / ECDHE_RSA_AES_128_GCM_SHA256
*    server certificate verification SKIPPED
*    server certificate status verification SKIPPED
*    common name: localhost (does not match 'obmc.example.com')
*    server certificate expiration date OK
*    server certificate activation date OK
*    certificate public key: RSA
*    certificate version: #3
*    subject: C=XX,L=Default City,O=openbmc.org,CN=localhost
*    start date: Fri, 06 Nov 2015 20:35:06 GMT
*    expire date: Sat, 05 Nov 2016 20:35:06 GMT
*    issuer: C=XX,L=Default City,O=openbmc.org,CN=localhost
*    compression: NULL
* ALPN, server did not agree to a protocol
> POST /login HTTP/1.1
> Host: obmc.example.com
> User-Agent: curl/7.47.0
> Accept: */*
> Content-Type: application/json
> Content-Length: 38
> 
* upload completely sent off: 38 out of 38 bytes
< HTTP/1.1 200 OK
< Content-Length: 87
< Content-Type: application/json
< Date: Thu, 01 Sep 2016 21:48:46 GMT
< Server: Rocket 1.2.4 Python/2.7.9
< Connection: keep-alive
< 
{
  "data": "Invalid username or password", 
  "message": "200 OK", 
  "status": "ok"
* Connection #0 to host obmc.example.com left intact
KennethWilke commented 8 years ago

It would be best if this returned an HTTP 401 error for invalid credentials

bradbishop commented 8 years ago

What would firefox ( or any browser ) do in response to the 401?

The RFC seems to say that clients should "fix" a 401 response with a filled out WWW-Authenticate header, so I'm guessing firefox would give a login dialog, wait for the credentials and redo the post with the header. Which will still not work work since we aren't doing http authentication.

Is 401 really appropriate with non-http auth?

KennethWilke commented 8 years ago

I'm not sure what a browser would do in response to the 401, as I usually use this interface via curl or python. Whichever 4xx error code you prefer is fine, but I think a response code that indicates there was a client error is more appropriate than returning a success when an error occurred.

I'm not aware of any APIs that don't use 401 as the authentication error response code

agangidi53 commented 8 years ago

Quoting some examples:

OPENSTACK API:

http://developer.openstack.org/api-ref/identity/v3/ Return Unauthorized (401) If one of the following errors occurred: Authentication was not performed / The specified X-Auth-Token header is not valid / The authentication credentials are not valid.

TWITTER API:

https://dev.twitter.com/overview/api/response-codes 401 Unauthorized : Authentication credentials were missing or incorrect. Also returned in other circumstances, for example all calls to API v1 endpoints now return 401 (use API v1.1 instead).

REDFISH SCALABLE PLATFORMS MANAGEMENT API SPECIFICATION:

https://www.dmtf.org/sites/default/files/standards/documents/DSP0266_0.96.0a.pdf 401 Unauthorized The authentication credentials included with this request are missing or invalid.

bradbishop commented 8 years ago

fair enough. this kind of pedantry probably only matters if you are making something super generic anyway (like a browser) so I'll cave here.

bradbishop commented 8 years ago

FWIW...

from RFC 7235:

3.1.  401 Unauthorized

   The 401 (Unauthorized) status code indicates that the request has not
   been applied because it lacks valid authentication credentials for
   the target resource.  The server generating a 401 response MUST send
   a WWW-Authenticate header field (Section 4.1) containing at least one
   challenge applicable to the target resource.

And a sample 401 from twitter:

[toshiba:openbmc]$ curl -v -H'Authorization: Bearer foo' https://api.twitter.com/1.1/friends/ids.json?cursor=-1\&screen_name=twitterapi\&count=5000
* About to connect() to api.twitter.com port 443 (#0)
*   Trying 199.16.156.40...
* Connected to api.twitter.com (199.16.156.40) port 443 (#0)
* Initializing NSS with certpath: sql:/etc/pki/nssdb
* skipping SSL peer certificate verification
* SSL connection using TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
* Server certificate:
*       subject: CN=api.twitter.com,OU=Twitter Security,O="Twitter, Inc.",L=San Francisco,ST=California,C=US
*       start date: Jun 29 00:00:00 2016 GMT
*       expire date: Sep 19 12:00:00 2019 GMT
*       common name: api.twitter.com
*       issuer: CN=DigiCert SHA2 High Assurance Server CA,OU=www.digicert.com,O=DigiCert Inc,C=US
> GET /1.1/friends/ids.json?cursor=-1&screen_name=twitterapi&count=5000 HTTP/1.1
> User-Agent: curl/7.29.0
> Host: api.twitter.com
> Accept: */*
> Cookie: guest_id=v1%3A147334394411813376
> Authorization: Bearer foo
>
< HTTP/1.1 401 Authorization Required
< content-length: 62
< content-type: application/json; charset=utf-8
< date: Thu, 08 Sep 2016 14:24:36 GMT
< server: tsa_b
< strict-transport-security: max-age=631138519
< x-connection-hash: 8b420c260fa252f733fb080123c8b55a
< x-response-time: 3
<
* Connection #0 to host api.twitter.com left intact
{"errors":[{"code":89,"message":"Invalid or expired token."}]}[toshiba:openbmc]$

Note the lack of the required WWW-Authenticate header.

So the twitter api seems to violate the RFC. I guess if the big guys don't care...we don't need to either.

My plan is to just return 401 without any headers beyond the content headers. That seem reasonable?

KennethWilke commented 8 years ago

Seems reasonable to me, the body has enough detail on what the issue was, and the status code will tip off developers that the login didn't succeed.

agangidi53 commented 8 years ago

Yup . This sounds good!!!