[Koha-bugs] [Bug 10988] Allow login via Google OAuth2

[bugzilla-daemon at bugs.koha-community.org]

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10988

--- Comment #58 from David Cook <dcook at prosentient.com.au> ---
Comment on attachment 46016
  --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=46016
Bug 10988 - Allow for Google OAuth2 logins Combined all of the patches above
into one, making them apply to master again.

Review of attachment 46016:
 --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=10988&attachment=46016)
-----------------------------------------------------------------

I hadn't realized that this was OpenID Connect until Martin pointed it out and
until I saw the "openid" value in the scope of the Authorization Request. 

I actually wrote an OpenID Connect feature for Koha for a client in 2014, but
due to a lack of time and an incorrectly implemented third party OpenID Connect
server, I never got around to upstreaming it to the community codebase. Perhaps
I should try and remedy that one day. In any case, I've included some comments
in the code review below based on my experience.

Nicholas, what documentation did you use for this patch? When I look at
https://developers.google.com/identity/protocols/OpenIDConnect, it specifies
different endpoints than you've used here. I'd recommend consulting that
webpage and http://openid.net/specs/openid-connect-core-1_0.html. The latter is
what I used for making an OpenID Connect compliant server app.

::: opac/svc/auth/googleoauth2
@@ +53,5 @@
> +# protocol is assumed in OPACBaseURL see bug 5010.
> +my $redirecturl  = $host . '/cgi-bin/koha/svc/auth/googleoauth2';
> +my $issuer       = 'accounts.google.com';
> +my $clientid     = C4::Context->preference('GoogleOAuth2ClientID');
> +my $clientsecret = C4::Context->preference('GoogleOAuth2ClientSecret');

Alternatively, you could put the ClientID and the ClientSecret in
koha-conf.xml. While it would give less control to libraries, it would keep
this information on a more "need to know" basis.

@@ +86,5 @@
> +elsif ( defined $query->param('code') ) {
> +    my $code    = $query->param('code');
> +    my $ua      = LWP::UserAgent->new();
> +    my $request = POST(
> +        'https://accounts.google.com/o/oauth2/token',

How did you choose this endpoint? Both
https://developers.google.com/identity/protocols/OpenIDConnect?hl=en#discovery
and https://accounts.google.com/.well-known/openid-configuration provide
versioned endpoints.

@@ +99,5 @@
> +    );
> +    my $response = $ua->request($request)->decoded_content;
> +    my $json     = decode_json($response);
> +    if ( exists( $json->{'id_token'} ) ) {
> +        $request = POST( 'https://www.googleapis.com/oauth2/v1/tokeninfo',

tokeninfo isn't a standard OpenID Connect endpoint. It's a debugging tool that
Google has available, but they discourage its use in production:
https://developers.google.com/identity/protocols/OpenIDConnect?hl=en#validatinganidtoken

Use of tokeninfo also makes it harder for the rest of us to read the code since
a decrypted token has a standard layout, but I haven't found what a tokeninfo
response contains.

@@ +105,5 @@
> +        $response = $ua->request($request)->decoded_content;
> +        $json     = decode_json($response);
> +
> +# Confirm (as google suggests) that the issuer and audience are what we expect them to be
> +        if (   ( $json->{'issuer'} eq $issuer )

As noted above, a standard response should be $json->{'iss'}, but otherwise
this is good as per #2 at
http://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation.

@@ +106,5 @@
> +        $json     = decode_json($response);
> +
> +# Confirm (as google suggests) that the issuer and audience are what we expect them to be
> +        if (   ( $json->{'issuer'} eq $issuer )
> +            && ( $json->{'audience'} eq $clientid )

As noted in #3 at
http://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation,
$json->{'aud'} (the standard claim), "may" be an array. I'm not familiar with
Google's responses... it might always return a string, but the spec points out
that you should check for either a string or an array (and to reject the token
if it doesn't contain the expected audience or if it includes untrusted
audiences).

@@ +107,5 @@
> +
> +# Confirm (as google suggests) that the issuer and audience are what we expect them to be
> +        if (   ( $json->{'issuer'} eq $issuer )
> +            && ( $json->{'audience'} eq $clientid )
> +            && exists( $json->{'email'} ) )

This is bizarre... the token shouldn't contain the email. That must be for
debugging purposes in tokeninfo. You should have to get it from
https://www.googleapis.com/oauth2/v3/userinfo or a similar endpoint, which also
need its "sub" claim to be validated.

@@ +108,5 @@
> +# Confirm (as google suggests) that the issuer and audience are what we expect them to be
> +        if (   ( $json->{'issuer'} eq $issuer )
> +            && ( $json->{'audience'} eq $clientid )
> +            && exists( $json->{'email'} ) )
> +        {

At this point, you must also verify that the token type is "Bearer" (case
insensitive as per RFC 6749 Section 5.1).

At this point, you must also be verifying that the token isn't expired. 

You must also validate the "azp" claim if it's present in the response. 

For more on validation, consult
http://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation and RFC
6749 (https://tools.ietf.org/html/rfc6749)

@@ +148,5 @@
> +
> +}
> +else {
> +    my $prompt = $query->param('reauthenticate') // q{};
> +

I agree with Martin. Even on Google's page, it states "You must protect the
security of your users by preventing request forgery attacks."
(https://developers.google.com/identity/protocols/OpenIDConnect?hl=en#createxsrftoken).

As developers, we have a responsibility to the users of Koha to ensure their
security and their privacy.

-- 
You are receiving this mail because:
You are watching all bug changes.