Bug 6296 - Allow authentication to Koha via PKI / x.509 / SSL certificates
Summary: Allow authentication to Koha via PKI / x.509 / SSL certificates
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: 3.8
Hardware: All All
: P4 enhancement (vote)
Target Milestone: ---
Assignee: Robin Sheat
QA Contact: Ian Walls
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-03 22:50 UTC by Chris Cormack
Modified: 2013-05-23 06:23 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 6296: allow users to be authenticated by SSL client certs (37.41 KB, patch)
2011-06-14 07:36 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 6296: allow users to be authenticated by SSL client certs (35.99 KB, patch)
2011-07-28 22:45 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 6296: allow users to be authenticated by SSL client certs (26.67 KB, patch)
2011-10-19 02:23 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 6296: allow users to be authenticated by SSL client certs (27.08 KB, patch)
2011-11-24 02:39 UTC, Robin Sheat
Details | Diff | Splinter Review
Rebased but not signed off. (26.95 KB, patch)
2012-03-10 16:10 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6296: allow users to be authenticated by SSL client certs (27.19 KB, patch)
2012-03-17 01:52 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 6296: allow users to be authenticated by SSL client certs (29.21 KB, patch)
2012-03-17 09:05 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
[PASSED QA] Bug 6296: allow users to be authenticated by SSL client certs (29.44 KB, patch)
2012-03-17 19:58 UTC, Ian Walls
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2011-05-03 22:50:17 UTC
This enhancement will allow authentication to Koha via client side certificates.

This can be achieved with apache2 + mod_ssl
And the                 SSLVerifyClient Require directive
(Plus a lot more configuration)

Configuration for this as part of the install may fall outside the scope of this enhancement, this will be more concerned with dealing with a user that has been authenticated by apache in Koha
Comment 1 Robin Sheat 2011-06-14 07:36:53 UTC Comment hidden (obsolete)
Comment 2 Robin Sheat 2011-06-14 07:43:31 UTC
Some caveats: 
* When this is first set up, it pays to clear all session cookies relating to the koha host, especially on the OPAC, or it may look like it's not working.
* When using the email address verification, it is still required that users have a username defined, as the login process expects it. Don't know what'll happen if they don't, most likely it'll explode.
Comment 3 Chris Cormack 2011-07-28 22:45:38 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2011-07-28 22:46:00 UTC
Working in fine in production, signing off
Comment 5 Paul Poulain 2011-08-10 15:18:43 UTC
QA comment

I won't do anything on this patch, as I don't know how SSL works, I can't have a usefull comment. Ian, hoping you'll do better...
Comment 6 Ian Walls 2011-10-18 15:54:52 UTC
Patch no longer applies after followup to 5995 (return values of checkpw have been altered).  Also, system preferences have been condensed from their many language files.

Some additional comments:

Uses "common name" instead of 'userid' in the system preferences editor; would be much clearer to the library if userid was somehow noted or referenced, so they know how the login works.

The sample Apache configs should be added to etc/koha-httpd.conf, as comments.

I'm not clear why the return values of checkauth are no longer being captured in many of touched scripts.  Could you explain why this is a desirable global change (as it happens in this patch regardless of AllowPKIAuth's settings.
Comment 7 Robin Sheat 2011-10-18 20:26:45 UTC
Just addressing the last point, the value was never checked. It was stuck into a variable and that variable was never again referenced. I assume it was copy-paste code, and it was fairly misleading. Because I changed how it behaved, I had to check every instance checkauth was called and make sure it was doing the new right thing. For ones that never used the return value, I cleaned them up.)
Comment 8 Robin Sheat 2011-10-19 02:23:16 UTC Comment hidden (obsolete)
Comment 9 Robin Sheat 2011-10-19 02:26:57 UTC
New version that applies against master and has the suggested changes applied.

I accidentally dropped the sign-off from Chris though, hope that doesn't matter.
Comment 10 Paul Poulain 2011-10-24 11:40:16 UTC
Updating version : This ENH could be in Koha 3.8
Comment 11 Paul Poulain 2011-10-25 15:05:52 UTC
Bug versionned for master. entries will be made against rel_3_8 once the patch has been applied (see thread about that on koha-devel yesterday)
Comment 12 Marcel de Rooy 2011-11-14 12:59:13 UTC
Please resolve whitespace issues and rebase (syspref and updatedatabase).

Applying: Bug 6296: allow users to be authenticated by SSL client certs
/usr/share/koha/testclone/.git/rebase-apply/patch:29: space before tab in indent.
                my $borrower = GetMember(borrowernumber => $user);
/usr/share/koha/testclone/.git/rebase-apply/patch:30: space before tab in indent.
                if ($borrower) {
/usr/share/koha/testclone/.git/rebase-apply/patch:31: space before tab in indent.
                $borrowernumber = $user;
/usr/share/koha/testclone/.git/rebase-apply/patch:136: trailing whitespace.
                    SELECT borrowernumber, firstname, surname, flags, borrowers.branchcode,
/usr/share/koha/testclone/.git/rebase-apply/patch:137: trailing whitespace.
                    branches.branchname    as branchname,
warning: C4/Auth.pm has type 100755, expected 100644
error: patch failed: installer/data/mysql/sysprefs.sql:327
error: installer/data/mysql/sysprefs.sql: patch does not apply
error: patch failed: installer/data/mysql/updatedatabase.pl:4523
error: installer/data/mysql/updatedatabase.pl: patch does not apply
Using index info to reconstruct a base tree...
<stdin>:29: space before tab in indent.
                my $borrower = GetMember(borrowernumber => $user);
<stdin>:30: space before tab in indent.
                if ($borrower) {
<stdin>:31: space before tab in indent.
                $borrowernumber = $user;
<stdin>:136: trailing whitespace.
                    SELECT borrowernumber, firstname, surname, flags, borrowers.branchcode,
<stdin>:137: trailing whitespace.
                    branches.branchname    as branchname,
warning: squelched 3 whitespace errors
warning: 8 lines add whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merged C4/Auth.pm
Auto-merged C4/Members.pm
Auto-merged installer/data/mysql/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql
Auto-merged installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Failed to merge in the changes.
Comment 13 Robin Sheat 2011-11-24 02:39:23 UTC Comment hidden (obsolete)
Comment 14 Robin Sheat 2011-11-24 02:41:06 UTC
This cleans up the whitespace errors, and fixes the conflicts in updatedatabase and sysprefs.
Comment 15 Marcel de Rooy 2011-12-19 08:57:54 UTC
QA Comment:
Code looks good to me (just going through it), but I am inclined to think that this patch needs a new signoff, preferably from someone outside the company..
Changing status to reflect that.
Please respond if I am mistaken..
Comment 16 Jared Camins-Esakov 2012-03-10 16:10:46 UTC Comment hidden (obsolete)
Comment 17 Jared Camins-Esakov 2012-03-11 14:55:35 UTC
Well, an obscene number of hours worth of testing later, I have determined that this patch doesn't work. I suspect copy-and-paste errors. The AllowPKIAuth syspref is never checked, and even if it were checked where I believe it should be in C4::Auth::checkauth, it doesn't look like the code would ever be reached.
Comment 18 Robin Sheat 2012-03-17 01:02:54 UTC
There's definitely a patch missing. Looking at it now.
Comment 19 Robin Sheat 2012-03-17 01:52:07 UTC Comment hidden (obsolete)
Comment 20 Robin Sheat 2012-03-17 01:55:00 UTC
Hopefully this has everything needed. In particular, it does check the state of the syspref.
Comment 21 Jared Camins-Esakov 2012-03-17 09:05:54 UTC Comment hidden (obsolete)
Comment 22 Ian Walls 2012-03-17 19:58:27 UTC
Created attachment 8252 [details] [review]
[PASSED QA] Bug 6296: allow users to be authenticated by SSL client certs

This adds a new syspref: AllowPKIAuth. It can have one of three states:
* None
* Common Name
* emailAddress

If a) this is set to something that's not "None", and b) the webserver
is passing SSL client cert details on to Koha, then the relevant field
in the user's certificate will be matched up against the field in the
database and they will be automatically logged in. This is used as a
secure form of single sign-on in some organisations.

The "Common Name" field is matched up against the userid, while
"emailAddress" is matched against the primary email.

This is an example of what might go in the Apache configuration for the
virtual host:

    #SSLVerifyClient require # only allow PKI authentication
    SSLVerifyClient optional
    SSLVerifyDepth 2
    SSLCACertificateFile /etc/apache2/ssl/test/ca.crt
    SSLOptions +StdEnvVars

The last line ensures that the required details are
passed to Koha.

To test the PKI authentication, use the following curl command:
    curl -k --cert client.crt --key client.key  https://URL/
(look through the output to find the "Welcome," line to indicate that a user
has been authenticated or the "Log in to Your Account" to indicate that a
user has not been authenticated)

To create the certificates needed for the above command, the following series
of commands will work:
    # Create the CA Key and Certificate for signing Client Certs
    openssl genrsa -des3 -out ca.key 4096
    openssl req -new -x509 -days 365 -key ca.key -out ca.crt
    # This is the ca.crt file that the Apache config needs to know about,
    # so put the file at /etc/apache2/ssl/test/ca.crt

    # Create the Server Key, CSR, and Certificate
    openssl genrsa -des3 -out server.key 1024
    openssl req -new -key server.key -out server.csr

    # We're self signing our own server cert here.  This is a no-no in
    # production.
    openssl x509 -req -days 365 -in server.csr -CA ca.crt -CAkey ca.key \
        -set_serial 01 -out server.crt

    # Create the Client Key and CSR
    openssl genrsa -des3 -out client.key 1024
    openssl req -new -key client.key -out client.csr

    # Sign the client certificate with our CA cert. Unlike signing our own
    # server cert, this is what we want to do.
    openssl x509 -req -days 365 -in client.csr -CA ca.crt -CAkey ca.key \
        -set_serial 02 -out client.crt
    openssl pkcs12 -export -in client.crt -inkey client.key -out client.p12
    # In theory we can install this client.p12 file in Firefox or Chrome, but
    # the exact steps for doing so are unclear, and outside the scope of this
    # patch

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Tested with Common Name and E-mail authentication, as well as with PKI
authentication disabled. Regular logins continue to work in all cases when
SSL authentication is set to optional on the server.

Signed-off-by: Ian Walls <koha.sekjal@gmail.com>
QA comment: synchronized updatedatabase.pl version of syspref with sysprefs.sql
version, to avoid divergent databases between new and upgrading users.
Comment 23 Ian Walls 2012-03-17 19:59:17 UTC
Lots of whitespace cleanup, thank you.

Removes the capture of checkauth return values in a lot of scripts... but in all cases, these variables were not used later.  This is unnecessary, but not harmful, and does reduce the number of variables stored on those scripts.

Implementation of PKI / x.509 certs is safe for other login types, so this will not impact anyone not using the feature.

The line added to sysprefs.sql does not match the line in updatedatabase.pl.  The sysprefs.sql line is more thorough, giving a proper default value and a controlled list of values, where the updatedatabase version omits these things.  I've corrected this.

Marking Passed QA
Comment 24 Paul Poulain 2012-03-19 16:48:20 UTC
QA comment : it's very hard to see the difference between reindenting & really new code. For the next reindenting time, please provide 2 patches, that will be much more clear.

I've tested that it changes nothing when SSL not enabled, not tested SSL work well.

Patch pushed