893195 - Allow token based authentication for webservices

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[David Lawrence [:dkl]]

Similar to how we allow users to pass in Bugzilla_login/Bugzilla_password (username/password with REST) with any WebService method call, we should also allow for passing of a unique token as a parameter. This could be provided by User.login in the data returned and the client can use the token for subsequent requests. We could use the same userid/token stored in logincookies as the token which would negate the need for a new table/column and they could be expired in the same way cookies can be expired.

Thoughs? Will create a patch illustrating how this could work. 

dkl

Comment 1

[David Lawrence [:dkl]]

Attachment: Patch to add new Bugzilla_token that can be used instead of cookies and username/password pair (v1)

Working concept to see if this is possible from a functionality standpoint as well as retaining proper security. Basically I modified User.login to allow for
returning a new key/value called Bugzilla_token that is the userid and cookie token combined. This 'token' can then be passed to any webservice calls as a parameter instead of requiring cookies or passing in the user login and password each time. A third party client could then call GET /login or User.login to get the token and then store that token for each future webservice call instead of storing the username/password. The token, since it is part of the normal logincookies table, would expire as normal and also could use the IP address to give slightly higher protection. Please look over and let me know if my idea has merit and we can refine as needed or come up with a better implementation.

dkl

Comment 2

[:glob ✱]

Comment on attachment 775377 [details] [diff] [review]
Patch to add new Bugzilla_token that can be used instead of cookies and username/password pair (v1)

Review of attachment 775377 [details] [diff] [review]:
-----------------------------------------------------------------

> Working concept to see if this is possible from a functionality standpoint as well as retaining proper security..

i'm not sure if you want a full review, or just feedback.  since you used the review flag, i'm going to do that :)


there's a few places where Bugzilla_token needs to be removed from canonicalised queries or $cgi (look for where Bugzilla_login is being removed).

::: Bugzilla/Auth/Login/CGI.pm
@@ +20,5 @@
>  
> +# If the Bugzilla_token is valid, then verification is not
> +# required, otherwise we require it for username/passwords.
> +our $_requires_verification = 1;
> +sub requires_verification { return $_requires_verification };

using 'our' means it will be per-process under mod_perl, which isn't what you want.
use the request_cache instead.

@@ +59,5 @@
> +                       WHERE cookie = ?", undef, $login_token);
> +            $_requires_verification = 0;
> +            return { user_id => $user_id };
> +        }
> +    }

this approach looks sane, however i don't like that the logincookies table is updated in multiple locations.
i would rather see this code extracted into its own method in Bugzilla/Auth/Login/Cookie.pm, and B::A::L::CGI.pm call that method.

::: Bugzilla/WebService/Server/REST.pm
@@ +536,4 @@
>  
>  Pass in as query parameters of any request:
>  
> +login=[e-mail]&password=ilovewilma

bedrock.com is a real domain; use example.com instead.  sorry i missed this in my initial review.

::: Bugzilla/WebService/User.pm
@@ +84,5 @@
> +    my $cgi = Bugzilla->cgi;
> +    my $cookie = first { $_->name eq 'Bugzilla_logincookie' }
> +        @{$cgi->{'Bugzilla_cookie_list'}};
> +    if ($cookie) {
> +        $result->{'Bugzilla_token'} = $user->id . "-" . $cookie->value;

i think just "token" would be a better field name.

when i call the login method via rest, this field isn't included in the response (however it is present when returning via xmlrpc).

i don't think using CGI's cookie_list is the right way to get to this token; it should be available from an Auth object.

@@ +454,5 @@
> +On success, a hash containing two items, C<id>, the numeric id of the
> +user that was logged in, and a "Bugzilla_token" which can be passed in
> +the parameters as authentication in other calls. A set of http cookies
> +is also sent with the response. These cookies can be sent along with
> +any future requests to the webservice, for the duration of the session.

that wording doesn't make it clear that you can provide a token *or* a cookie; it sounds like you need to provide both to authenticate.

Comment 3

[David Lawrence [:dkl]]

Attachment: Patch to add new Bugzilla_token that can be used instead of cookies and username/password pair (v2)

Thanks for the review. Hopefully better patch attached.

dkl

Comment 4

[David Lawrence [:dkl]]

Attachment: Patch to add new Bugzilla_token that can be used instead of cookies and username/password pair (v3)

Forgot about canonicalize_query in the last patch and also some hidden field exclusion places in the templates.

dkl

Comment 5

[:glob ✱]

Comment on attachment 775952 [details] [diff] [review]
Patch to add new Bugzilla_token that can be used instead of cookies and username/password pair (v3)

Review of attachment 775952 [details] [diff] [review]:
-----------------------------------------------------------------

you also need to delete the credentials in userprefs when doing sudo.

passing in an invalid token needs to throw an error instead of silently ignoring the token (this mirrors the behaviour exhibited by passing in an invalid login+password).

calling logout without passing a token should also result in an error (currently it returns "ok").

::: Bugzilla/Auth.pm
@@ +55,5 @@
>  
>      # Now verify his username and password against the DB, LDAP, etc.
> +    if ($self->{_info_getter}->{successful}->requires_verification
> +        and !Bugzilla->request_cache->{auth_skip_verification})
> +    {

requires_verification() should be the only means of returning a value here -- getting Auth.pm to check the request_cache is a spooky action at a distance.

as Auth/Login/CGI.pm needs to change something on a per-request basis, it should be responsible for setting state in the request_cache and returning the correct value via requires_verification.

::: Bugzilla/Auth/Login/CGI.pm
@@ +34,5 @@
> +        # Since the login token is basically the same as the cookies
> +        # normally stored for a user session, we use the same call to
> +        # verify the validity of the login token. Also if token is valid
> +        # skip DB verification.
> +        if (Bugzilla::Auth::Login::Cookie::valid_login_cookie($user_id, $login_token, remote_ip())) {

you may as well get Bugzilla::Auth::Login::Cookie to optionally export that method

::: Bugzilla/Auth/Login/Cookie.pm
@@ +58,5 @@
>  
> +# Check to see if the provided login cookie and userid
> +# match what is in the database.
> +sub valid_login_cookie {
> +    my ($user_id, $login_cookie, $ip_addr) = @_;

$ip_addr is always remote_ip(), so there's no need to pass it as an argument.

::: Bugzilla/WebService/Server/REST.pm
@@ +421,5 @@
>  
>  sub _fix_credentials {
>      my ($self, $params) = @_;
> +    # Allow user to pass in login=foo&password=bar as a convenience
> +    # even if not calling GET /login. We also do not delete them as 

trailing whitespace

::: Bugzilla/WebService/User.pm
@@ +81,5 @@
> +    # cookies value should have cached away as well. We will
> +    # use that value combined with the user id to create a the
> +    # token that can be used with future requests in the query
> +    # parameters.
> +    if ($user->authorizer->{'_persister'}->login_cookie) {

as login_cookie isn't supported by all persisters, you need to use ->can('login_cookie') here.

@@ +83,5 @@
> +    # token that can be used with future requests in the query
> +    # parameters.
> +    if ($user->authorizer->{'_persister'}->login_cookie) {
> +        $result->{'Bugzilla_token'} =
> +            $user->id . "-" . $user->authorizer->{'_persister'}->login_cookie;

this should be called 'token', not 'Bugzilla_token'.

@@ +92,5 @@
>  
>  sub logout {
>      my $self = shift;
>      Bugzilla->logout;
> +    return { ok => 1 };

you're changing the returned value for a stable method.
i'm not sure this change is worth it.

if you think it is, the documentation needs to be updated, as well as ensuring a big notice about this change is added to the 5.0 release notes.

note - this change can't be backported to earlier versions as breaking a stable interface can only happen during major version changes.

Comment 6

[David Lawrence [:dkl]]

Attachment: Patch to add new Bugzilla_token that can be used instead of cookies and username/password pair (v4)

Comment 7

[:glob ✱]

Comment on attachment 776680 [details] [diff] [review]
Patch to add new Bugzilla_token that can be used instead of cookies and username/password pair (v4)

Review of attachment 776680 [details] [diff] [review]:
-----------------------------------------------------------------

all looks good except for the why you're overriding the requires_* subs needs to be changed.

::: Bugzilla/Auth/Login.pm
@@ +33,5 @@
> +    my ($self) = @_;
> +    return exists $self->{'requires_verification'}
> +           ? $self->{'requires_verification'}
> +           : requires_verification_default;
> +}

the constants are subs that you can subclass in Bugzilla/Auth/Login/CGI.pm to get this behaviour; there's no need to make the way to override requires_* different from the other constants.

revert the _default changes and these two requires_* subs, then add the following to Bugzilla/Auth/Login/CGI.pm:

sub requires_verification {
    my ($self) = @_;
    return exists $self->{'requires_verification'} ? $self->{'requires_verification'} : 1;
}

Comment 8

[David Lawrence [:dkl]]

Attachment: Patch to add new Bugzilla_token that can be used instead of cookies and username/password pair (v5)

Thanks. New patch.

dkl

Comment 9

[:glob ✱]

Comment on attachment 778500 [details] [diff] [review]
Patch to add new Bugzilla_token that can be used instead of cookies and username/password pair (v5)

>+    # protection in place.) We do allow this for GET /login as we need to
>+    # for Bugzilla::Auth::Persist::Cookie to create a login cookie that we
>+    # can also use for Bugzilla_token support. This is OK as it requires
>+    # a login and password to be supplied and will fail if they are not
>+    # valid for the user.
>+#    if (!grep($_ eq $self->request->method, ('POST', 'PUT'))
>+#        && !($self->bz_class_name eq 'Bugzilla::WebService::User'
>+#            && $self->bz_method_name eq 'login'))
>+#    {
>         # XXX There's no particularly good way for us to get a parameter
>         # to Bugzilla->login at this point, so we pass this information
>         # around using request_cache, which is a bit of a hack. The
>         # implementation of it is in Bugzilla::Auth::Login::Stack.
>         Bugzilla->request_cache->{'auth_no_automatic_login'} = 1;
>-    }
>+#    }

did you mean to comment out these lines?

Comment 10

[David Lawrence [:dkl]]

Attachment: 893195_6.patch

Commented lines fixed.

Comment 11

[David Lawrence [:dkl]]

Attachment: Patch to add new Bugzilla_token that can be used instead of cookies and username/password pair (v7)

Removed leftover code change from devel environment.

Comment 12

[:glob ✱]

Comment on attachment 779760 [details] [diff] [review]
Patch to add new Bugzilla_token that can be used instead of cookies and username/password pair (v7)

r=glob

Comment 13

[:glob ✱]

while i'm happy with this patch, given the security aspects to this change i'd like to get another set of eyes on this for approval.

Comment 14

[Frédéric Buclin]

Comment on attachment 779760 [details] [diff] [review]
Patch to add new Bugzilla_token that can be used instead of cookies and username/password pair (v7)

I don't like the way the authentication code is hacked to work with Bugzilla_token. A token is stored in the DB and behaves the same way as a cookie. From a security point of view, a cookie and a token are some information which you pretend to exist in the logincookies table, and if that bit of information exists in the logincookies table and point to the expected user ID, then we declare the user to be correctly authenticated. So you must write your Bugzilla_token code at the same place as for cookies, i.e. in Auth/Login/Cookie.pm itself. Other Auth::* modules must not be altered. In particular, this means not hacking the highly sensitive requires_verification bit.



>=== modified file 'Bugzilla/Auth/Login/Cookie.pm'

>+    if (valid_login_cookie($user_id, $login_cookie, $ip_addr)) {

3 arguments passed...


>+sub valid_login_cookie {
>+    my ($user_id, $login_cookie) = @_;

... only 2 taken into account?! With what I said above. This method becomes useless anyway as the code will be self-contained.



>=== modified file 'Bugzilla/Auth/Persist/Cookie.pm'

>+    # Store the cookie value for later use as a login token
>+    # if we are using webservices
>+    $self->{'login_cookie'} = $login_cookie;

I'm not sure what you try to do here. This Auth object is not persistent (but I didn't test your patch).


>+    # If we are a webservice causing a token instead of cookies we
>+    # can grab the cookie value from the token.
>+    my $token  = trim(Bugzilla->input_params->{'Bugzilla_token'});

Why does $token come from Bugzilla->input_params instead of $self->login_token? $self contains validated data. Bugzilla->input_params contains user-defined data, which must be considered untrusted by default.


>+    if (defined $token) {
>+        my ($user_id, $login_token) = split('-', $token, 2);
>+        $login_cookie = $login_token if $login_token;
>+    }

With this code, you are overriding the login cookie, if it exists, which cannot be deleted. If both exist, they are supposed to match, but one could attempt to abuse that. Correctness will depend on what takes precedence when calling Bugzilla.login with both a login cookie and Bugzilla_token. So it will depend on the updated patch. IMO, to avoid potential duplicated login cookies in the DB and to enforce security, I would say: ignore/delete Bugzilla_token if there is already a valid login cookie available, else you could end with two login cookies in the logincookies DB table.



>=== modified file 'Bugzilla/WebService/User.pm'

>-    # Username and password params are required 
>+    # Username and password params are required
>     foreach my $param ("login", "password") {
>-        defined $params->{$param} 
>+        defined $params->{$param}

Removing trailing whitespaces is a noble idea, but you break the history of the file when one wants to know which patch implemented such or such part of the code. This is a bit painful. I would be all for it if you were altering this code anyway, but that's not the case here.


>+    # When the cookie was set for persisting the login, the
>+    # cookies value should have cached away as well. We will

s/cookies/cookie/. Not sure what "cache away" means. :)


>+    # use that value combined with the user id to create a the

s/a the/a/


>+    # token that can be used with future requests in the query
>+    # parameters.
>+    if ($user->authorizer->{'_persister'}->can('login_cookie')
>+        && $user->authorizer->{'_persister'}->login_cookie)
>+    {
>+        $result->{'token'} = $user->id . "-" .
>+                             $user->authorizer->{'_persister'}->login_cookie;
>+    }

Couldn't you simply get the token from $cgi->{Bugzilla_cookie_list} and drop this login_cookie() method entirely?



>=== modified file 'Bugzilla/WebService/Util.pm'

>+Allows for certain parameters related to authentication such as Bugzilla_login,
>+Bugzilla_password, and Bugzilla_token to have shorter named equivalients passed in.

s/equivalients/equivalents/ ?



>=== modified file 'attachment.cgi'

>         my $cgi_params = $cgi->canonicalise_query(@field_names, 't',
>-            'Bugzilla_login', 'Bugzilla_password');
>+            'Bugzilla_login', 'Bugzilla_password', 'Bugzilla_token');

Why is Bugzilla_token removed from here? REST accesses rest.cgi only, not other CGI scripts. If Bugzilla_token reaches attachment.cgi, that's a bug.

Same question for remaining files.

Comment 15

[David Lawrence [:dkl]]

Attachment: 893195_8.patch

Comment 17

[David Lawrence [:dkl]]

Comment on attachment 786600 [details] [diff] [review]
893195_8.patch

Changing to glob as Lpsolit mentioned he did not have much time to look at this one.

dkl

Comment 18

[:glob ✱]

Comment on attachment 786600 [details] [diff] [review]
893195_8.patch

Review of attachment 786600 [details] [diff] [review]:
-----------------------------------------------------------------

this mostly looks good, however if you provide an invalid token, we should throw an error instead of silently ignoring the token.

Comment 19

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #18)
> this mostly looks good, however if you provide an invalid token, we should
> throw an error instead of silently ignoring the token.

I think it should behave the same way as login cookies. If you pass an invalid login cookie, it intentionally returns { failure => AUTH_NODATA }.

Comment 20

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #19)
> (In reply to Byron Jones ‹:glob› from comment #18)
> > this mostly looks good, however if you provide an invalid token, we should
> > throw an error instead of silently ignoring the token.
> 
> I think it should behave the same way as login cookies. If you pass an
> invalid login cookie, it intentionally returns { failure => AUTH_NODATA }.

I tend to agree with LpSolit. If you pass an invalid token and you are performing an operation that requires you to be logged in, or have specific permissions, you will get an error anyway. I agree that the difference between a token and a cookie could be interpreted differently, but it should be harmless to just treat a token as a cookie.

dkl

Comment 21

[David Lawrence [:dkl]]

Attachment: 893195_9.patch

New patch that throws error when invalid token is used and also locks out the user if too many failed attempts.

dkl

Comment 22

[:glob ✱]

after chatting with dkl about this bug, he convinced me that returning AUTH_NODATA would be the better approach.

bug 908338 has been filed to deal with the use-case i had in mind when r-'ing attachment 786600 [details] [diff] [review] -- consumers should be able to tell if their credentials are still valid.

Comment 23

[:glob ✱]

Comment on attachment 786600 [details] [diff] [review]
893195_8.patch

reverting my prior r-

r=glob

Comment 24

[David Lawrence [:dkl]]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk  
modified Bugzilla/Auth.pm
modified Bugzilla/Auth/Login.pm
modified Bugzilla/Auth/Login/Cookie.pm                                               
modified Bugzilla/Auth/Persist/Cookie.pm
modified Bugzilla/WebService/User.pm
modified Bugzilla/WebService/Util.pm
modified Bugzilla/WebService/Server/JSONRPC.pm
modified Bugzilla/WebService/Server/REST.pm
modified Bugzilla/WebService/Server/REST/Resources/User.pm
Committed revision 8718.

Comment 25

[Erik Bryn]

I just stumbled upon the invalid token handling behavior (filed as bug 916955).

I don't agree with the current behavior of not throwing an error if an invalid token is provided, especially since an error is thrown for invalid username and password combinations. The initial motivation for adding token-based authentication was to be a more secure alternative to having to keep a user's username and password around in memory, but now, because of this behavior, using token-based authentication is less desirable.

I'm building a client that consumes the Bugzilla REST API. When I make authenticated requests, I expect to receive authenticated responses. If I can't expect that, I need to have a mechanism for determining whether or not the response data is in fact correct for the current user. For example: Product.get. How is a client supposed to know whether it is getting the expected authenticated response data or not? It's terribly inefficient to expect me to be hitting the User.valid_login API before making the request.

Comment 27

[Frédéric Buclin]

Comment on attachment 786600 [details] [diff] [review]
893195_8.patch

>=== modified file 'Bugzilla/Auth/Persist/Cookie.pm'

>     if ($cookie) {
>+        push(@login_cookies, $cookie->value);
>     }
>     else {
>+        push(@login_cookies, $cgi->cookie("Bugzilla_logincookie"));
>+    }
>+
>+    # If we are a webservice using a token instead of cookie
>+    # then add that as well to the login cookies to delete
>+    if (my $login_token = $user->authorizer->login_token) {
>+        push(@login_cookies, $login_token->{'login_token'});
>+    }
>+
>+    return if !@login_cookies;

This last return statement will never be triggered as you always push something into @login_cookies, either $cookie->value or $cgi->cookie("Bugzilla_logincookie"), even if both are undefined. And so !@login_cookies is always false.

You should only push $cgi->cookie("Bugzilla_logincookie") if it's defined.

Comment 28

[Frédéric Buclin]

There is another problem if you return early: when called with $type = LOGOUT_KEEP_CURRENT, all other sessions should be closed, even if you currently have no login cookies, but in this case, the early return would prevent that, keeping all sessions active. This is a regression.

Comment 29

[Dave Miller [:justdave]]

Since there's no schema changes involved, we're going to go ahead and backport this to 4.4 so we can use it as part of the fix for bug 713926.

Comment 30

[David Lawrence [:dkl]]

Backporting this to 4.4 as well. Patch coming.

dkl

Comment 31

[David Lawrence [:dkl]]

Attachment: 893195_44_1.patch

Patch for 4.4

Comment 32

[:glob ✱]

Comment on attachment 8379180 [details] [diff] [review]
893195_44_1.patch

r=glob

Comment 33

[Frédéric Buclin]

Comment on attachment 8379180 [details] [diff] [review]
893195_44_1.patch

>=== modified file 'Bugzilla/Auth/Persist/Cookie.pm'

>     else {
>-        $login_cookie = $cgi->cookie("Bugzilla_logincookie") || '';
>+        push(@login_cookies, $cgi->cookie("Bugzilla_logincookie"));

You re-introduce the bug I fixed in bug 748095. Please backport my fix here.

Comment 34

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #33)
> Comment on attachment 8379180 [details] [diff] [review]
> 893195_44_1.patch
> 
> >=== modified file 'Bugzilla/Auth/Persist/Cookie.pm'
> 
> >     else {
> >-        $login_cookie = $cgi->cookie("Bugzilla_logincookie") || '';
> >+        push(@login_cookies, $cgi->cookie("Bugzilla_logincookie"));
> 
> You re-introduce the bug I fixed in bug 748095. Please backport my fix here.

Patch backed out. Working on new patch now.

dkl

Comment 35

[David Lawrence [:dkl]]

Attachment: 893195_44_2.patch

New patch including fix from bug 748095. Moving forward r+ and a+.

dkl

Comment 36

[David Lawrence [:dkl]]

Committing to: bzr+ssh://[e-mail]/bugzilla/4.4
modified Bugzilla/Auth.pm
modified Bugzilla/Auth/Login.pm
modified Bugzilla/Auth/Login/Cookie.pm
modified Bugzilla/Auth/Persist/Cookie.pm
modified Bugzilla/WebService/User.pm
modified Bugzilla/WebService/Util.pm
modified Bugzilla/WebService/Server/JSONRPC.pm
modified Bugzilla/WebService/Server/XMLRPC.pm
Committed revision 8664.

Comment 37

[David Lawrence [:dkl]]

Reopening for the 4.4 port:

1) Need to add proper History section for the token changes in POD
2) Need to also backport User.valid_login for token checking.

Will ask for new review for the new patch

dkl

Comment 38

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #37)
> 2) Need to also backport User.valid_login for token checking.

That's not the right place to backport User.valid_login. If you need it for 4.4 (but do we really need it in 4.4?), then you should reopen bug 908338, not this bug. Doing everything in this bug is going to confuse a lot of people.

So AFAICS, this bug should remain closed (it's already closed, but with approval/review flags removed?? dkl, don't mess everything, please :)).

Comment 39

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #38)
> (In reply to David Lawrence [:dkl] from comment #37)
> > 2) Need to also backport User.valid_login for token checking.
> 
> That's not the right place to backport User.valid_login. If you need it for
> 4.4 (but do we really need it in 4.4?), then you should reopen bug 908338,
> not this bug. Doing everything in this bug is going to confuse a lot of
> people.
> 
> So AFAICS, this bug should remain closed (it's already closed, but with
> approval/review flags removed?? dkl, don't mess everything, please :)).

Ok will leave out User.valid_login for the 4.4 backport. I will fix the POD and commit without reopening this bug. 

Justdave, sorry to bother but can you re-approve this bug as I am not able to a+.

dkl

Comment 40

[David Lawrence [:dkl]]

Comment on attachment 8382274 [details] [diff] [review]
893195_44_2.patch

Moving the r+ back to this patch as no change is needed.

Comment 41

[Frédéric Buclin]

Added to relnotes for 5.0rc1.