This endpoint would check that the POSTed password matches the Koha database password. In theory, you could have other validations here as well (e.g. check if patron is expired, restricted, etc.). But maybe those should be their own endpoints. Adding this to support a Keycloak User Storage SPI as per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17004#c39
+1
How will we prevent abuse for this password-guessing service? Hehe
(In reply to Tomás Cohen Arazi from comment #2) > How will we prevent abuse for this password-guessing service? Hehe It's not a public/anonymous endpoint. Only an authenticated and authorized user could use it.
(In reply to David Cook from comment #3) > (In reply to Tomás Cohen Arazi from comment #2) > > How will we prevent abuse for this password-guessing service? Hehe > > It's not a public/anonymous endpoint. Only an authenticated and authorized > user could use it. But happy to add more security to lock accounts on too many bad password checks.
(In reply to David Cook from comment #4) > (In reply to David Cook from comment #3) > > (In reply to Tomás Cohen Arazi from comment #2) > > > How will we prevent abuse for this password-guessing service? Hehe > > > > It's not a public/anonymous endpoint. Only an authenticated and authorized > > user could use it. > > But happy to add more security to lock accounts on too many bad password > checks. I think it would make sense to use the existing lock feature here, also consistent. ILS-DI locks too.
(In reply to Katrin Fischer from comment #5) > (In reply to David Cook from comment #4) > > (In reply to David Cook from comment #3) > > > (In reply to Tomás Cohen Arazi from comment #2) > > > > How will we prevent abuse for this password-guessing service? Hehe > > > > > > It's not a public/anonymous endpoint. Only an authenticated and authorized > > > user could use it. > > > > But happy to add more security to lock accounts on too many bad password > > checks. > > I think it would make sense to use the existing lock feature here, also > consistent. > > ILS-DI locks too. Done!
Created attachment 136215 [details] [review] Bug 30962: REST API: Add endpoint /patrons/:patron_id/check_password This patch adds an endpoint for /patrons/:patron_id/check_password This allows a third-party, using an authenticated and authorized Koha API user, to check if the username and password given by a user is correct in Koha. For example, a Keycloak extension can be created using its User Storage SPI to use Koha as the user database for Keycloak. This API allows us to authenticate the user as a particular Koha user - without creating a Koha user session for them. Test plan: 0. Apply patch and koha-plack --restart kohadev 1. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=RESTBasicAuth 2. Enable "RESTBasicAuth" 3. Run the following commands while substituting correct values for <koha_user> and <koha_password> 3. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/patrons/51/check_password -d '{ "password": "<koha_password>" }' -v 4. Note "200 OK" response 5. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/patrons/51/check_password -d '{ "password": "this is definitely not the password" }' -v 6. Note "400 Bad Request" response and error message {"error":"Invalid password"}
Created attachment 136300 [details] [review] Bug 30962: REST API: Add endpoint /patrons/:patron_id/check_password This patch adds an endpoint for /patrons/:patron_id/check_password This allows a third-party, using an authenticated and authorized Koha API user, to check if the username and password given by a user is correct in Koha. For example, a Keycloak extension can be created using its User Storage SPI to use Koha as the user database for Keycloak. This API allows us to authenticate the user as a particular Koha user - without creating a Koha user session for them. Test plan: 0. Apply patch and koha-plack --restart kohadev 1. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=RESTBasicAuth 2. Enable "RESTBasicAuth" 3. Run the following commands while substituting correct values for <koha_user> and <koha_password> 3. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/patrons/51/check_password -d '{ "password": "<koha_password>" }' -v 4. Note "200 OK" response 5. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/patrons/51/check_password -d '{ "password": "this is definitely not the password" }' -v 6. Note "400 Bad Request" response and error message {"error":"Invalid password"} Signed-off-by: David Nind <david@davidnind.com>
Regarding security, I've been thinking more about this, and in theory you could lock down all non-public API routes by IP address, if your organisation has static IP addresses and requires VPNs for working from home (WFH). We do this on other systems that have admin APIs. It's possible that you might need to provide a third-party access to an admin API (like this one in bug 30962), but then you can add their IP address to the allow list. It just adds another layer of security over top of the existing security measures. We could promote the idea by adding some configuration directives to Apache that allow all IP addresses for both public and non-public API routes and include some comments about how they can lock down the non-public API routes by doing X, Y, and Z.
I was thinking about this again, and really it's very similar to an LDAP lookup. It requires staff-level authorization, and with the account lockout functionality, it wouldn't be any different from someone trying to brute force the OPAC login.
1. Missing tests (you must provide tons of tests to cover the different situations) 2. Route's name should not be a verb (/password/validation maybe?) 3. Routes that returns empty should return 204 4. It's always returning "Invalid password" even for other failures (like too many attempts) 5. It allows you to check for pwd validation for a user you don't know their userid (you can force brute only by knowing the patron's id). I don't think it's a security concern as userid could be guessed anyway (?) 6. following 5, you can lock any accounts if FailedLoginAttempts is set, no need to know the userid list. How bad is that?
(In reply to Jonathan Druart from comment #11) > 1. Missing tests (you must provide tons of tests to cover the different > situations) Are all the existing API tests in "t/db_dependent/api/v1/"? > 2. Route's name should not be a verb (/password/validation maybe?) I did struggle to come up with a noun for this one so I take the point. I think "validation" or "authentication" instead of "check_password" makes sense. It also makes things more flexible for the future. I suppose the concept would be that we're creating the patron validation or authentication by supplying these details... > 4. It's always returning "Invalid password" even for other failures (like > too many attempts) I suppose a generic error like "Authentication failed" or "Validation failed" would be better. > 5. It allows you to check for pwd validation for a user you don't know their > userid (you can force brute only by knowing the patron's id). I don't think > it's a security concern as userid could be guessed anyway (?) > 6. following 5, you can lock any accounts if FailedLoginAttempts is set, no > need to know the userid list. How bad is that? Remember that this isn't a public endpoint. Depending on an attacker's goal, it would be silly to try to brute force /patrons/:patron_id/check_password when you could just use /patrons/:patron_id/password to set the password to whatever you want, and then use /patrons/:patron_id to get the userid in order to login. Both password API endpoints require the same level of staff-level authorization from an authenticated API user. That said, I do take the point. I'm not opposed to changing this to "/patrons/authentication" and sending userid and password (like http://localhost:8080/cgi-bin/koha/ilsdi.pl/ilsdi.pl?service=Describe&verb=AuthenticatePatron)
While I originally created this to support a Keycloak User Storage SPI extension, I've had libraries interested in it as an alternative to the ILS-DI API "AuthenticatePatron" verb. It's essentially the 1 thing that the ILS-DI API can do that the REST API can't do, which means that libraries use the ILS-DI API (or SIP2) to authenticate patrons against the Koha database for third-parties software. So it would be great to get this into Koha core, so that we can continue to move away from the ILS-DI API. -- We've actually had more and more libraries using Keycloak with the Koha user storage extension, because it means they get to-purpose software for SSO, while still getting to manage all their patrons in Koha. It's super useful. (I also have a goal of Koha using Keycloak Oauth2 access tokens to bring SSO to Koha's REST API, so that third-parties could call public API endpoints on behalf of Koha users without third-parties needing dedicated API users with staff-level permissions, which would be a massive improvement on security/trust relationships, but one step at a time...)
I'm keen to keep working on this but without any additional feedback I don't really have any work to do...
I'm planning on making a plugin for this, and it's got me thinking more about what people would accept in the longer term... I'm thinking /api/v1/auth/ will be the route, and it'll take "username" and "password" JSON POST parameters. I've noticed FortiAuthenticator works this way; it's the way that the ILS-DI API works; it would be difficult to abuse. I suppose we could argue about the route name. FortiAuthenticator uses that exact same route, but I suppose it could be /api/v1/patrons/auth, so it makes it more obvious that it's a route not for authenticating to use the API but rather just for authenticating patrons.
(In reply to David Cook from comment #12) > (In reply to Jonathan Druart from comment #11) > > 1. Missing tests (you must provide tons of tests to cover the different > > situations) > > Are all the existing API tests in "t/db_dependent/api/v1/"? Not sure what you mean here, but we try to enforce 100% code coverage on the tests. > > 2. Route's name should not be a verb (/password/validation maybe?) > > I did struggle to come up with a noun for this one so I take the point. I'd go with 'validation'. > > 4. It's always returning "Invalid password" even for other failures (like > > too many attempts) > > I suppose a generic error like "Authentication failed" or "Validation > failed" would be better. If you think generic is fine, go for it. If you think there are use cases in which the caller could take advantage on the information, please catch those exceptions and return something meaningful. > > 5. It allows you to check for pwd validation for a user you don't know their > > userid (you can force brute only by knowing the patron's id). I don't think > > it's a security concern as userid could be guessed anyway (?) > > 6. following 5, you can lock any accounts if FailedLoginAttempts is set, no > > need to know the userid list. How bad is that? Maybe not the right bug to talk about how to prevent brute force attacks, as you said: this is not a public route.
(In reply to Tomás Cohen Arazi from comment #16) > (In reply to David Cook from comment #12) > > Are all the existing API tests in "t/db_dependent/api/v1/"? > > Not sure what you mean here, but we try to enforce 100% code coverage on the > tests. Are good examples of API tests found in that location? > > > 2. Route's name should not be a verb (/password/validation maybe?) > > > > I did struggle to come up with a noun for this one so I take the point. > > I'd go with 'validation'. In light of the /api/v1/auth/* routes that we have these days... /api/v1/auth/validation? Or do we want another path prefix in there like "password" for /api/v1/auth/password/validation? And we'd be posting userid and password to that endpoint. -- I'm currently using /patrons/:patron_id/check_password and when I implemented bug 31982 locally it did cause a problem because I was using the OPAC interface. I switched to the Staff Interface API and then it was all good. In my case, the consumer of this API is Keycloak, which also runs locally. If it was a third-party system, I think I'd need to create an security exception to allow it through to that endpoint. I keep thinking about public vs non-public APIs... and I figure public APIs are things that every day people and front-end systems should be able to use. They offer the same functionality to the public as the OPAC but they do so in a machine-friendly way. If an action is only allowed by staff, it's part of the non-public admin API. And if there's a third-party system that needs to access that, because we trust it, then we make an explicit exception for it, and ideally keep the scope narrowed to just what it needs.
Created attachment 144810 [details] [review] Bug 30962: REST API: Add endpoint /patrons/:patron_id/check_password This patch adds an endpoint for /patrons/:patron_id/check_password This allows a third-party, using an authenticated and authorized Koha API user, to check if the username and password given by a user is correct in Koha. For example, a Keycloak extension can be created using its User Storage SPI to use Koha as the user database for Keycloak. This API allows us to authenticate the user as a particular Koha user - without creating a Koha user session for them. Test plan: 0. Apply patch and koha-plack --restart kohadev 1. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=RESTBasicAuth 2. Enable "RESTBasicAuth" 3. Run the following commands while substituting correct values for <koha_user> and <koha_password> 3. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/auth/password/validation -d '{ "username": "<koha_username">, "password": "<koha_password>" }' -v 4. Note "204 No Content" response 5. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/auth/password/validation -d '{ "username": "<koha_username">, "password": "this is definitely not the password" }' -v 6. Note "400 Bad Request" response and error message {"error":"Validation failed"}
I think the updated patch includes everything except the unit tests. I've just run out of time for that today, as holidays start in 5 minutes... In the new year, I'll try to model some tests using t/db_dependent/api/v1/idp.t as a template. I think that should be what's needed...
Created attachment 144964 [details] [review] Bug 30962: REST API: Add endpoint /auth/password/validation This patch adds an endpoint for /auth/password/validation This allows a third-party, using an authenticated and authorized Koha API user, to check if the username and password given by a user is correct in Koha. For example, a Keycloak extension can be created using its User Storage SPI to use Koha as the user database for Keycloak. This API allows us to authenticate the user as a particular Koha user - without creating a Koha user session for them. Test plan: 0. Apply patch and koha-plack --restart kohadev 1. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=RESTBasicAuth 2. Enable "RESTBasicAuth" 3. Run the following commands while substituting correct values for <koha_user> and <koha_password> 3. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/auth/password/validation -d '{ "username": "<koha_username">, "password": "<koha_password>" }' -v 4. Note "204 No Content" response 5. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/auth/password/validation -d '{ "username": "<koha_username">, "password": "this is definitely not the password" }' -v 6. Note "400 Bad Request" response and error message {"error":"Validation failed"}
Created attachment 144965 [details] [review] Bug 30962: Add unit tests for /auth/password/validation endpoint Test plan: 0. Apply patch 1. prove -v t/db_dependent/api/v1/password_validation.t
(In reply to Jonathan Druart from comment #11) > 1. Missing tests (you must provide tons of tests to cover the different > situations) > 2. Route's name should not be a verb (/password/validation maybe?) > 3. Routes that returns empty should return 204 > 4. It's always returning "Invalid password" even for other failures (like > too many attempts) > 5. It allows you to check for pwd validation for a user you don't know their > userid (you can force brute only by knowing the patron's id). I don't think > it's a security concern as userid could be guessed anyway (?) > 6. following 5, you can lock any accounts if FailedLoginAttempts is set, no > need to know the userid list. How bad is that? I think that I've addressed all these points now :)
Created attachment 145350 [details] [review] Bug 30962: REST API: Add endpoint /auth/password/validation This patch adds an endpoint for /auth/password/validation This allows a third-party, using an authenticated and authorized Koha API user, to check if the username and password given by a user is correct in Koha. For example, a Keycloak extension can be created using its User Storage SPI to use Koha as the user database for Keycloak. This API allows us to authenticate the user as a particular Koha user - without creating a Koha user session for them. Test plan: 0. Apply patch and koha-plack --restart kohadev 1. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=RESTBasicAuth 2. Enable "RESTBasicAuth" 3. Run the following commands while substituting correct values for <koha_user> and <koha_password> 3. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/auth/password/validation -d '{ "username": "<koha_username">, "password": "<koha_password>" }' -v 4. Note "204 No Content" response 5. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/auth/password/validation -d '{ "username": "<koha_username">, "password": "this is definitely not the password" }' -v 6. Note "400 Bad Request" response and error message {"error":"Validation failed"} Signed-off-by: David Nind <david@davidnind.com>
Created attachment 145351 [details] [review] Bug 30962: Add unit tests for /auth/password/validation endpoint Test plan: 0. Apply patch 1. prove -v t/db_dependent/api/v1/password_validation.t Signed-off-by: David Nind <david@davidnind.com>
Giving this one a try...
1) Unit tests pass 2) QA test tools pass 3) Test plan I followed the test plan and while I have the replies 204 and 400, I don't see the error as described: error message {"error":"Validation failed"} 4) Postman I also tried to test this with Postman. * Verified that BasicAuth worked as expected by listing patrons * Tried: localhost:8081/api/v1/auth/password/validation * Params: username, password * Verb: POST * Body: { "username": "...", "password": "..." } a) Matching username + password * cardnumber + correct password = 400 - Bad request * username + correct password = 204 - No Content (that's a success?) * username + incorrect password = 400 - Bad Request - error: Validation failed * username + incorrect password so many times to make the account lock: 400 - Bad Request - error: Validation failed Notes: * The login page in Koha allows for cardnumber + password AND userid + password at the same time. I think we should extend this route in a separate bug to also support cardnumber/both to make this easier to use and also mimick what ILS-DI and SIP do as well. I've filed: Bug 32739 - REST API: Extend endpoint /auth/password/validation for cardnumber * We do want the account to lock with too many attempts, which it does. The error stays the same, but I think that's good too and matches what we do on the OPAC, we don't want to give away too much information. *thumbs up* QA fail: * The route users username, but the patrons api uses user_id. We should make things match and use user_id here as well. Almost ready to PQA, please fix!
Created attachment 145765 [details] [review] Bug 30962: REST API: Add endpoint /auth/password/validation This patch adds an endpoint for /auth/password/validation This allows a third-party, using an authenticated and authorized Koha API user, to check if the username and password given by a user is correct in Koha. For example, a Keycloak extension can be created using its User Storage SPI to use Koha as the user database for Keycloak. This API allows us to authenticate the user as a particular Koha user - without creating a Koha user session for them. Test plan: 0. Apply patch and koha-plack --restart kohadev 1. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=RESTBasicAuth 2. Enable "RESTBasicAuth" 3. Run the following commands while substituting correct values for <koha_user> and <koha_password> 3. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/auth/password/validation -d '{ "username": "<koha_username">, "password": "<koha_password>" }' -v 4. Note "204 No Content" response 5. curl -XPOST -H "Content-Type: application/json" -u <koha_user>:<koha_password> http://localhost:8081/api/v1/auth/password/validation -d '{ "username": "<koha_username">, "password": "this is definitely not the password" }' -v 6. Note "400 Bad Request" response and error message {"error":"Validation failed"} Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 145766 [details] [review] Bug 30962: Add unit tests for /auth/password/validation endpoint Test plan: 0. Apply patch 1. prove -v t/db_dependent/api/v1/password_validation.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 145767 [details] [review] Bug 30962: (QA follow-up) Rename attribute and simplify tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Katrin Fischer from comment #26) > * The login page in Koha allows for cardnumber + password AND userid + > password at the same time. I think we should extend this route in a separate > bug to also support cardnumber/both to make this easier to use and also > mimick what ILS-DI and SIP do as well. > I've filed: Bug 32739 - REST API: Extend endpoint /auth/password/validation > for cardnumber Nice idea! > QA fail: > > * The route users username, but the patrons api uses user_id. We should make > things match and use user_id here as well. I submitted a follow-up to help this bug get pushed soon! Hope it covers your concern. dcook: hope you're fine with my tests simplification, it is much easier to maintain and similar to what we do everywhere (with two exceptions I think).
(In reply to Tomás Cohen Arazi from comment #30) > dcook: hope you're fine with my tests simplification, it is much easier to > maintain and similar to what we do everywhere (with two exceptions I think). I've taken a quick look and if you're happy with it, I'm happy with it.
Created attachment 145883 [details] [review] Bug 30962: Add unit tests for /auth/password/validation endpoint Test plan: 0. Apply patch 1. prove -v t/db_dependent/api/v1/password_validation.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 145884 [details] [review] Bug 30962: (QA follow-up) Rename attribute and simplify tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Pushed to master for 23.05. Nice work everyone, thanks!
Nice work, thanks everyone! Pushed to 22.11.x for the next release.
Enhancement will not be backported to 22.05.x
Back end stuff, doesn't affect the end user so there's nothing to add/amend in the manual. I'm told the API docs is automated, so there's no need to put those bugs as Needs documenting.
Hi there, I need to backport these patches to 22.05 as this bug is a dependency of Bug 36575. However, the patches don't apply cleanly, could I please get some patches for 22.05?
(In reply to wainuiwitikapark from comment #38) > Hi there, > > I need to backport these patches to 22.05 as this bug is a dependency of Bug > 36575. > > However, the patches don't apply cleanly, could I please get some patches > for 22.05? Hmm... are you sure that you need this dependency? My suggestion for bug 36575 would be to just not backport the changes to t/db_dependent/api/v1/password_validation.t Nothing else in bug 36575 should touch any code here.