Any user with "Staff access, allows viewing of catalogue in staff interface (catalogue)" and "Create SQL reports (create_reports)" permissions can create the SQl report that will dump API keys from the db via custom SQL report, like: "SELECT * FROM `api_keys`". Also reports allow to dumb and see hashed passwords, which I think is worth mentioning here but probably already known thing. To reproduce: 1) create/pick existing patron and give them these permissions: "Staff access, allows viewing of catalogue in staff interface (catalogue)", "Create SQL reports (create_reports)" and "Execute SQL reports (execute_reports)"; 2) go to the "Reports" -> "Create from SQL"; 3) create report using this SQL: "SELECT * FROM `api_keys`"; 4) run it, see how it dumps all API keys from the database.
Any user with "Staff access, allows viewing of catalogue in staff interface (catalogue)" and "Create SQL reports (create_reports)" permissions can create the SQl report that will dump API keys from the db via custom SQL report, like: "SELECT * FROM `api_keys`". Also reports allow to dump and see hashed passwords, which I think is worth mentioning here but probably already known thing. To reproduce: 1) create/pick existing patron and give them these permissions: "Staff access, allows viewing of catalogue in staff interface (catalogue)", "Create SQL reports (create_reports)" and "Execute SQL reports (execute_reports)"; 2) go to the "Reports" -> "Create from SQL"; 3) create report using this SQL: "SELECT * FROM `api_keys`"; 4) run it, see how it dumps all API keys from the database.
I don't see any good solutions to fix that. It's a known issue, users with the report permission have access to the whole DB! We could use temporary views, deny list (columns and/or tables), but you could always bypass it IMO.
One "partial" solution can be: to store hashed API keys like passwords (Joonas concluded in our brainstorm). This of course requires more complicated changes to the interface and also backwards compatibility and so on, so let's develop this idea and see.
(P.S. how to reset assignee to "not assigned"?)
And also another part is documentation + even maybe "in-site interface text" on the "permissions assignment page" to explicitly warn admin-operator that adding "create reports" rights has consequences for that user to be able to dump API keys and hashed passwords, and then it's advised to assign "create reports" only to "superlibrarian level" trusty persons/accounts. I.e. to give extra warning there for our users.
They should be hashed, for sure. Good catch!
Created attachment 124195 [details] [review] Bug 28772: Make Koha::ApiKey->store encrypt the secret This patch refactors the Koha::ApiKey class so: - It encrypts the generated secret - Allows accessing the plain text secret only immediately after the key creation (this implies that it won't be accessible if the key is fetched from the DB). - It implements an allow list for attributes, that are not read only. Changing any other of them will make ->store throw an exception. - A method for validating plain text secrets against the encrypted one is added. - A method for accessing the plain text secret is added. Returns undef if the object is not 'fresh'. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/ApiKey.t => SUCCESS: Tests pass! Expected behavior is confirmed 3. Sign off :-D
Created attachment 124247 [details] [review] Bug 28772: Make Koha::ApiKey->store encrypt the secret This patch refactors the Koha::ApiKey class so: - It encrypts the generated secret - Allows accessing the plain text secret only immediately after the key creation (this implies that it won't be accessible if the key is fetched from the DB). - It implements an allow list for attributes, that are not read only. Changing any other of them will make ->store throw an exception. - A method for validating plain text secrets against the encrypted one is added. - A method for accessing the plain text secret is added. Returns undef if the object is not 'fresh'. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/ApiKey.t => SUCCESS: Tests pass! Expected behavior is confirmed 3. Sign off :-D
Created attachment 124248 [details] [review] Bug 28772: Update existing keys Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 124249 [details] [review] Bug 28772: Add a warning about hash_password usage in updatedatabase.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 124250 [details] [review] Bug 28772: Display API secret once This patch makes the apikeys.pl display the generated API secret once, when generated. After that, it won't be displayed by the UI. To test: 1. Generate a new API key => FAIL: The secret is displayed in the API keys table 2. Visit some other page, and go back to the API keys page => FAIL: The API key secret is there 3. Apply this patch 4. Go to More > Manage API keys => SUCCESS: It no longer displays the secret 5. Generate a new API key => SUCCESS: The API key details (including the secret) are displayed. => SUCCESS: A message telling to copy the secret because it won't be displayed again is shown. 6. Repeat 4 => SUCCESS: The secret is no longer displayed 7. Sign off :-D
Is this really a security bug fix? It was the current behavior and we now improve it, but does it deserve a backport into all stables (and so coordinate releases)?
The previous one (Bug 28759) for sure security fix: and it's simple, removes visibility for keys for other users except for owner and superlibrarian, And the current one is more like "Security improvement", but :) - I was concerned if to create this one publicly then it's revealing 28759 issue partially. Jonathan: what to do with Bug 28759 then?
Besides that discussion, please test the patches promptly so we move forward.
Tomás: I thought, but you added more :P. I'll do it tomorrow
Created attachment 124251 [details] [review] Bug 28772: Update existing keys [STABLE] Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 124487 [details] [review] Bug 28772: Make Koha::ApiKey->store encrypt the secret This patch refactors the Koha::ApiKey class so: - It encrypts the generated secret - Allows accessing the plain text secret only immediately after the key creation (this implies that it won't be accessible if the key is fetched from the DB). - It implements an allow list for attributes, that are not read only. Changing any other of them will make ->store throw an exception. - A method for validating plain text secrets against the encrypted one is added. - A method for accessing the plain text secret is added. Returns undef if the object is not 'fresh'. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/ApiKey.t => SUCCESS: Tests pass! Expected behavior is confirmed 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124488 [details] [review] Bug 28772: Update existing keys Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124489 [details] [review] Bug 28772: Add a warning about hash_password usage in updatedatabase.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124490 [details] [review] Bug 28772: Display API secret once This patch makes the apikeys.pl display the generated API secret once, when generated. After that, it won't be displayed by the UI. To test: 1. Generate a new API key => FAIL: The secret is displayed in the API keys table 2. Visit some other page, and go back to the API keys page => FAIL: The API key secret is there 3. Apply this patch 4. Go to More > Manage API keys => SUCCESS: It no longer displays the secret 5. Generate a new API key => SUCCESS: The API key details (including the secret) are displayed. => SUCCESS: A message telling to copy the secret because it won't be displayed again is shown. 6. Repeat 4 => SUCCESS: The secret is no longer displayed 7. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124491 [details] [review] Bug 28772: Update existing keys [STABLE] Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Good catch, all works as expected. Signing off
QAing
ok( !$api_key->validate_secret( 'Wrong secret' ), 'Invalid secret returns true' ); Really true?
(In reply to Martin Renvoize from comment #22) > Good catch, all works as expected. Signing off Are you sure? Did you test a converted API key in an API call?
When we generate a CSRF token in Koha/Token.pm, we use Bytes::Random::Secure. Why not here? No blocker for now, but could these modules be moved closer to each other? The sentence "Make sure to copy your API secret now. You won’t be able to see it again!" should be more prominently displayed. It should stand out much more. No blocker for now. Needs further attention imo.
This is the most serious issue: Update existing keys in dbrev does: + my $digest = Koha::AuthUtils::hash_password( $api_key->{secret} ); + $sth->execute( $digest, $api_key->{client_id} ); Validate does: my $digest = Koha::AuthUtils::hash_password( $secret, $self->secret ); return $self->secret eq $digest; This does not seem to work. When you hashed, you used a new salt. You dont save it. When you check, you use the current secret as salt. Looks to me that your eq test will almost never return true?
Btw what about: return (0, 'access_denied') unless $api_key->secret eq $client_secret; In Koha/OAuth
(In reply to Marcel de Rooy from comment #27) > This does not seem to work. When you hashed, you used a new salt. You dont > save it. When you check, you use the current secret as salt. > Looks to me that your eq test will almost never return true? It works. Look at C4::Auth::checkpw_hash, which is broadly used in Koha: sub checkpw_hash { my ( $password, $stored_hash ) = @_; return if $stored_hash eq '!'; # check what encryption algorithm was implemented: Bcrypt - if the hash starts with '$2' it is Bcrypt else md5 my $hash; if ( substr( $stored_hash, 0, 2 ) eq '$2' ) { $hash = hash_password( $password, $stored_hash ); } else { $hash = md5_base64($password); } return $hash eq $stored_hash; }
(In reply to Marcel de Rooy from comment #24) > ok( !$api_key->validate_secret( 'Wrong secret' ), 'Invalid secret returns > true' ); > > Really true? Oops, copy and paste error on the test description, fixing.
(In reply to Marcel de Rooy from comment #28) > Btw what about: > > return (0, 'access_denied') unless $api_key->secret eq $client_secret; > > In Koha/OAuth This is the wrong bit. I forgot to tweak the oauth.t tests so it is passing the hash directly, which is wrong! Follow-up coming
(In reply to Tomás Cohen Arazi from comment #29) > (In reply to Marcel de Rooy from comment #27) > > This does not seem to work. When you hashed, you used a new salt. You dont > > save it. When you check, you use the current secret as salt. > > Looks to me that your eq test will almost never return true? > > It works. Look at C4::Auth::checkpw_hash, which is broadly used in Koha: > > sub checkpw_hash { > my ( $password, $stored_hash ) = @_; > > return if $stored_hash eq '!'; > > # check what encryption algorithm was implemented: Bcrypt - if the hash > starts with '$2' it is Bcrypt else md5 > my $hash; > if ( substr( $stored_hash, 0, 2 ) eq '$2' ) { > $hash = hash_password( $password, $stored_hash ); > } else { > $hash = md5_base64($password); > } > return $hash eq $stored_hash; > } You dont convince me. Look at the call in Auth: $hash = hash_password( $password, $stored_hash ); But you do not use a stored hash in the db rev! You get a random one. This really is different. Please read the dbrev carefully :)
(In reply to Marcel de Rooy from comment #32) > (In reply to Tomás Cohen Arazi from comment #29) > > (In reply to Marcel de Rooy from comment #27) > > > This does not seem to work. When you hashed, you used a new salt. You dont > > > save it. When you check, you use the current secret as salt. > > > Looks to me that your eq test will almost never return true? > > > > It works. Look at C4::Auth::checkpw_hash, which is broadly used in Koha: > > > > sub checkpw_hash { > > my ( $password, $stored_hash ) = @_; > > > > return if $stored_hash eq '!'; > > > > # check what encryption algorithm was implemented: Bcrypt - if the hash > > starts with '$2' it is Bcrypt else md5 > > my $hash; > > if ( substr( $stored_hash, 0, 2 ) eq '$2' ) { > > $hash = hash_password( $password, $stored_hash ); > > } else { > > $hash = md5_base64($password); > > } > > return $hash eq $stored_hash; > > } > > You dont convince me. > Look at the call in Auth: > $hash = hash_password( $password, $stored_hash ); > But you do not use a stored hash in the db rev! You get a random one. > This really is different. > > Please read the dbrev carefully :) Haven't reached the DBRev in my fixes. Almost there.
(In reply to Tomás Cohen Arazi from comment #33) > (In reply to Marcel de Rooy from comment #32) > > (In reply to Tomás Cohen Arazi from comment #29) > > > (In reply to Marcel de Rooy from comment #27) > > > > This does not seem to work. When you hashed, you used a new salt. You dont > > > > save it. When you check, you use the current secret as salt. > > > > Looks to me that your eq test will almost never return true? > > > > > > It works. Look at C4::Auth::checkpw_hash, which is broadly used in Koha: > > > > > > sub checkpw_hash { > > > my ( $password, $stored_hash ) = @_; > > > > > > return if $stored_hash eq '!'; > > > > > > # check what encryption algorithm was implemented: Bcrypt - if the hash > > > starts with '$2' it is Bcrypt else md5 > > > my $hash; > > > if ( substr( $stored_hash, 0, 2 ) eq '$2' ) { > > > $hash = hash_password( $password, $stored_hash ); > > > } else { > > > $hash = md5_base64($password); > > > } > > > return $hash eq $stored_hash; > > > } > > > > You dont convince me. > > Look at the call in Auth: > > $hash = hash_password( $password, $stored_hash ); > > But you do not use a stored hash in the db rev! You get a random one. > > This really is different. > > > > Please read the dbrev carefully :) > > Haven't reached the DBRev in my fixes. Almost there. Haha. If you didnt, how could you say that it works?
(In reply to Marcel de Rooy from comment #34) > > Haven't reached the DBRev in my fixes. Almost there. > > Haha. If you didnt, how could you say that it works? Because, empirically, I saw it working. I'm submitting my follow-up with a full test plan.
(In reply to Tomás Cohen Arazi from comment #35) > (In reply to Marcel de Rooy from comment #34) > > > Haven't reached the DBRev in my fixes. Almost there. > > > > Haha. If you didnt, how could you say that it works? > > Because, empirically, I saw it working. I'm submitting my follow-up with a > full test plan. Include a test with the converted API keys. Not only new keys.. I have the emperical experience that compact QA comments are not read very carefully. The emotion of seeing FQA is just too much :)
Created attachment 124691 [details] [review] Bug 28722: Regression tests To test: 1. Apply this patch 2. Run: $ kshell k$ prove tt/db_dependent/api/v1/oauth.t => FAIL: Tests fail! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 124692 [details] [review] Bug 28772: Make secret validation use the new method This patch makes the Koha::OAuth library use the new validation method To test: 1. In master, enable RESTOAuth2ClientCredentials and have your superlibrarian patron a client_id/secret pair generated 2. Use Postman to gain an access token with the client_id/secret pair => SUCCESS: This works in Koha 3. Use the access token to GET /api/v1/patrons => SUCCESS: It works 4. Apply this patchset up to the regression tests 5. Run: $ updatedatabase $ koha-plack --restart kohadev => SUCCESS: All good 6. Repeat 2 => FAIL: You get an error trying to acquire an access token. Boo 7. Run: $ kshell k$ prove t/db_dependent/api/v1/oauth.t => FAIL: Tests fail! 8. Apply this patch 9. Run: $ koha-plack --restart kohadev $ kshell k$ prove t/db_dependent/api/v1/oauth.t => SUCCESS: Tests pass! 10. Repeat 2 => SUCCESS: Your original client_id/secret pair works! 11. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 124693 [details] [review] Bug 28772: Do not hash secrets twice Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 124694 [details] [review] Bug 28772: (QA follow-up) Fix wrong message Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 124695 [details] [review] Bug 28772: Update existing keys [STABLE] Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
QA: Blocking this one in order to carefully examine the suggested improvements ;)
(In reply to Marcel de Rooy from comment #27) > This is the most serious issue: > > Update existing keys in dbrev does: > + my $digest = Koha::AuthUtils::hash_password( $api_key->{secret} > ); > + $sth->execute( $digest, $api_key->{client_id} ); > > Validate does: > my $digest = Koha::AuthUtils::hash_password( $secret, $self->secret ); > return $self->secret eq $digest; > > This does not seem to work. When you hashed, you used a new salt. You dont > save it. When you check, you use the current secret as salt. > Looks to me that your eq test will almost never return true? Found the answer: The first call without a salt returns a hash that includes the used salt. In the second call we include $self->secret, so in other words we provide bcrypt with the information to extract the salt from the first call. Hope to remember that now ;)
Created attachment 124696 [details] [review] Bug 28772: Make Koha::ApiKey->store encrypt the secret This patch refactors the Koha::ApiKey class so: - It encrypts the generated secret - Allows accessing the plain text secret only immediately after the key creation (this implies that it won't be accessible if the key is fetched from the DB). - It implements an allow list for attributes, that are not read only. Changing any other of them will make ->store throw an exception. - A method for validating plain text secrets against the encrypted one is added. - A method for accessing the plain text secret is added. Returns undef if the object is not 'fresh'. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/ApiKey.t => SUCCESS: Tests pass! Expected behavior is confirmed 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124697 [details] [review] Bug 28772: Update existing keys Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124698 [details] [review] Bug 28772: Add a warning about hash_password usage in updatedatabase.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124699 [details] [review] Bug 28772: Display API secret once This patch makes the apikeys.pl display the generated API secret once, when generated. After that, it won't be displayed by the UI. To test: 1. Generate a new API key => FAIL: The secret is displayed in the API keys table 2. Visit some other page, and go back to the API keys page => FAIL: The API key secret is there 3. Apply this patch 4. Go to More > Manage API keys => SUCCESS: It no longer displays the secret 5. Generate a new API key => SUCCESS: The API key details (including the secret) are displayed. => SUCCESS: A message telling to copy the secret because it won't be displayed again is shown. 6. Repeat 4 => SUCCESS: The secret is no longer displayed 7. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124700 [details] [review] Bug 28722: Regression tests To test: 1. Apply this patch 2. Run: $ kshell k$ prove tt/db_dependent/api/v1/oauth.t => FAIL: Tests fail! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> https://bugs.koha-community.org/show_bug.cgi?id=28772 Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124701 [details] [review] Bug 28772: Make secret validation use the new method This patch makes the Koha::OAuth library use the new validation method To test: 1. In master, enable RESTOAuth2ClientCredentials and have your superlibrarian patron a client_id/secret pair generated 2. Use Postman to gain an access token with the client_id/secret pair => SUCCESS: This works in Koha 3. Use the access token to GET /api/v1/patrons => SUCCESS: It works 4. Apply this patchset up to the regression tests 5. Run: $ updatedatabase $ koha-plack --restart kohadev => SUCCESS: All good 6. Repeat 2 => FAIL: You get an error trying to acquire an access token. Boo 7. Run: $ kshell k$ prove t/db_dependent/api/v1/oauth.t => FAIL: Tests fail! 8. Apply this patch 9. Run: $ koha-plack --restart kohadev $ kshell k$ prove t/db_dependent/api/v1/oauth.t => SUCCESS: Tests pass! 10. Repeat 2 => SUCCESS: Your original client_id/secret pair works! 11. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124702 [details] [review] Bug 28772: Do not hash secrets twice Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124703 [details] [review] Bug 28772: (QA follow-up) Fix wrong message Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124704 [details] [review] Bug 28772: Update existing keys [STABLE] Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124705 [details] [review] Bug 28772: (QA follow-up) Apply change to other dbrev too [STABLE] Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Should we keep this one in security? What about this report: select * from oauth_access_tokens
(In reply to Marcel de Rooy from comment #54) > Should we keep this one in security? > What about this report: > select * from oauth_access_tokens Can we add oauth_access_tokens to some deny-list?
Created attachment 124727 [details] [review] Bug 28772: Make validate_secret return 1|0 Not an empty string
Hi, I'm having trouble backporting to 20.05.x due to a warning: > root@e2231e847cad:koha(20.05.x-security)$ prove t/db_dependent/Koha/ApiKey.t > t/db_dependent/Koha/ApiKey.t .. 1/4 DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`api_keys`, CONSTRAINT `api_keys_fk_patron_id` FOREIGN KEY (`patron_id`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `api_keys` ( `client_id`, `description`, `patron_id`, `secret`) VALUES ( ?, ?, ?, ? )" with ParamValues: 0='48cbea53-9713-4316-829a-1ab9d435f31c', 1='a description', 2=62, 3='$2a$08$VRC3tfMUyQC.P.iuuVCax.pdfBmSY1gIKYMZI13gDK6iWOYlpX.FO'] at /kohadevbox/koha/Koha/Object.pm line 166 > t/db_dependent/Koha/ApiKey.t .. ok > All tests successful. > Files=1, Tests=4, 3 wallclock secs ( 0.03 usr 0.00 sys + 2.29 cusr 0.31 csys = 2.63 CPU) > Result: PASS After more testing, it doesn't happen on the security branches for 19.11.x and 20.11.x And it happens on 20.05.x whether I cherry-pick the patches from 19.11.x or 20.11.x The atomic update is being applied in all cases. Any hints? I can still generate, remove, unrevoke and delete keys in the UI. (no validity testing done though)
(In reply to Victor Grousset/tuxayo from comment #57) > Hi, I'm having trouble backporting to 20.05.x due to a warning: > > > root@e2231e847cad:koha(20.05.x-security)$ prove t/db_dependent/Koha/ApiKey.t > > t/db_dependent/Koha/ApiKey.t .. 1/4 DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`api_keys`, CONSTRAINT `api_keys_fk_patron_id` FOREIGN KEY (`patron_id`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `api_keys` ( `client_id`, `description`, `patron_id`, `secret`) VALUES ( ?, ?, ?, ? )" with ParamValues: 0='48cbea53-9713-4316-829a-1ab9d435f31c', 1='a description', 2=62, 3='$2a$08$VRC3tfMUyQC.P.iuuVCax.pdfBmSY1gIKYMZI13gDK6iWOYlpX.FO'] at /kohadevbox/koha/Koha/Object.pm line 166 > > t/db_dependent/Koha/ApiKey.t .. ok > > All tests successful. > > Files=1, Tests=4, 3 wallclock secs ( 0.03 usr 0.00 sys + 2.29 cusr 0.31 csys = 2.63 CPU) > > Result: PASS > > After more testing, it doesn't happen on the security branches for 19.11.x > and 20.11.x > And it happens on 20.05.x whether I cherry-pick the patches from 19.11.x or > 20.11.x > > The atomic update is being applied in all cases. > > Any hints? > > > I can still generate, remove, unrevoke and delete keys in the UI. > (no validity testing done though) This warning is hidden since commit 82776d5a1202dec0a1eb13a93dceb9f01d79df7e Bug 26157: Hide expected DBI warnings from tests
Great, no worries then. Backported: Pushed to 20.05.x security branch for 20.05.16
Bad news (or good, the tests are catching bugs), running the tests locally showed that these commits cause the following tests to fail: (there are DBI Exceptions even without this patch but some are not the same) root@b281b6624d26:koha(20.05.x-security)$ prove t/db_dependent/Koha/Object.t t/db_dependent/api/v1/auth_authenticate_api_request.t t/db_dependent/Koha/Object.t ........................... 10/20 DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`api_keys`, CONSTRAINT `api_keys_fk_patron_id` FOREIGN KEY (`patron_id`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `api_keys` ( `client_id`, `description`, `patron_id`, `secret`) VALUES ( ?, ?, ?, ? )" with ParamValues: 0='7b3051e0-02dc-42f7-8ac8-469d7108feec', 1='a description', 2=65, 3='$2a$08$ccvpjEufccH7g4aPY456OOdppMsEP9axhgJqI1vthjjGsy.bLWTWC'] at /kohadevbox/koha/Koha/Object.pm line 166 DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`api_keys`, CONSTRAINT `api_keys_fk_patron_id` FOREIGN KEY (`patron_id`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `api_keys` ( `client_id`, `description`, `patron_id`, `secret`) VALUES ( ?, ?, ?, ? )" with ParamValues: 0='c565267c-4b6f-437f-956b-b610184d34e9', 1='a description', 2=65, 3='$2a$08$7.mn.j69xU/W0obkhx5FLOxY80PneJZKy2BvK3rJflhDfpZ2Cd.mS'] at /kohadevbox/koha/Koha/Object.pm line 166 # Failed test 'Exception is thrown correctly' # at t/db_dependent/Koha/Object.t line 651. # expecting: Koha::Exceptions::Object::DuplicateID # found: Koha::Exceptions::Object::FKConstraint (Broken FK constraint) # Failed test 'Exception message is correct' # at t/db_dependent/Koha/Object.t line 654. # got: 'Broken FK constraint' # expected: 'Duplicate ID' # Looks like you planned 16 tests but ran 5. # Looks like you failed 2 tests of 5 run. # Failed test 'store() tests' # at t/db_dependent/Koha/Object.t line 729. Can't locate object method "duplicate_id" via package "Koha::Exceptions::Object::FKConstraint" at t/db_dependent/Koha/Object.t line 660. # Looks like your test exited with 255 just after 16. t/db_dependent/Koha/Object.t ........................... Dubious, test returned 255 (wstat 65280, 0xff00) Failed 5/20 subtests t/db_dependent/api/v1/auth_authenticate_api_request.t .. # Failed test '200 OK' # at t/db_dependent/api/v1/auth_authenticate_api_request.t line 69. # got: '403' # expected: '200' # Failed test 'exact match for JSON Pointer "/expires_in"' # at t/db_dependent/api/v1/auth_authenticate_api_request.t line 69. # got: undef # expected: '3600' # Failed test 'exact match for JSON Pointer "/token_type"' # at t/db_dependent/api/v1/auth_authenticate_api_request.t line 69. # got: undef # expected: 'Bearer' # Failed test 'has value for JSON Pointer "/access_token"' # at t/db_dependent/api/v1/auth_authenticate_api_request.t line 69. Use of uninitialized value $access_token in concatenation (.) or string at t/db_dependent/api/v1/auth_authenticate_api_request.t line 82. # Failed test '200 OK' # at t/db_dependent/api/v1/auth_authenticate_api_request.t line 93. # got: '401' # expected: '200' # Failed test 'The 'koha.user' object is defined in the stash' # at t/db_dependent/api/v1/auth_authenticate_api_request.t line 96. # Failed test 'userenv set correctly' # at t/db_dependent/api/v1/auth_authenticate_api_request.t line 99. # got: undef # expected: '68' # Failed test 'Interface correctly set to 'api'' # at t/db_dependent/api/v1/auth_authenticate_api_request.t line 100. # got: 'opac' # expected: 'api' # Looks like you planned 14 tests but ran 12. # Looks like you failed 8 tests of 12 run. t/db_dependent/api/v1/auth_authenticate_api_request.t .. 1/3 # Failed test 'token-based tests' # at t/db_dependent/api/v1/auth_authenticate_api_request.t line 107. t/db_dependent/api/v1/auth_authenticate_api_request.t .. 3/3 # Looks like you failed 1 test of 3. t/db_dependent/api/v1/auth_authenticate_api_request.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/3 subtests
The two tests fail also on security/21.05.04
same failures for security/new/security-release-20.11.10 security/19.11.22security
same failures on master
Created attachment 125088 [details] [review] Bug 28772: Fix Koha/Object.T Koha::ApiKeys is no longer the simple object we need to test Koha::Object->store, let use Koha::Cash::Register
Tomas, can you have a look at the other failure please?
Thanks for the patch, unfortunately seems like there is a missing dependency for 20.05.x root@c2ad25fd843a:koha(20.05.x-security)$ prove t/db_dependent/Koha/Object.t t/db_dependent/Koha/Object.t .. 10/20 # No tests run! # Failed test 'No tests run for subtest "store() tests"' # at t/db_dependent/Koha/Object.t line 736. Can't locate object method "new" via package "Koha::Cash::Register" (perhaps you forgot to load "Koha::Cash::Register"?) at t/db_dependent/Koha/Object.t line 619.
Created attachment 125112 [details] [review] Bug 28772: Fix Koha/Object.t Koha::ApiKeys is no longer the simple object we need to test Koha::Object->store, let use Koha::Cash::Register
(In reply to Victor Grousset/tuxayo from comment #66) > Thanks for the patch, unfortunately seems like there is a missing dependency > for 20.05.x Should pass with the amended patch.
(In reply to Jonathan Druart from comment #12) > Is this really a security bug fix? It was the current behavior and we now > improve it, but does it deserve a backport into all stables (and so > coordinate releases)? I'm not sure. It is a security bug because someone with reports permissions can impersonate other users through the API. I'm not sure it requires a coordinated release.
Created attachment 125116 [details] [review] Bug 28772: Fix auth_authenticate_api_request.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Tomás Cohen Arazi from comment #70) > Created attachment 125116 [details] [review] [review] > Bug 28772: Fix auth_authenticate_api_request.t > > Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> The new behaviour is that the 'fresh' Koha::ApiKey object carries the generated secret that is accessible through the ->plain_text_secret method. The tests weren't updated to use it, so they were passing the encrypted secret, which was plain wrong.
Created attachment 125118 [details] [review] Bug 28772: Fix Koha/Object.t Koha::ApiKeys is no longer the simple object we need to test Koha::Object->store, let use Koha::Library::Groups
With the last two patches those patches are ready to backported, please run the test suite locally. Tell us if you have conflicts when applying them on your respective branches.
Thanks Jonathan and Tomás :) confirmed fix on 20.05.x Will rerun the whole test suite and report in case of failures.
I got conflicts when applying the last patch and then got this: kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Koha/Object.t t/db_dependent/Koha/Object.t .. 1/18 DBD::mysql::st execute failed: Unknown column 'branchprinter' in 'field list' [for Statement "INSERT INTO `branches` ( `branchaddress1`, `branchaddress2`, `branchaddress3`, `branchcity`, `branchcode`, `branchcountry`, `branchemail`, `branchfax`, `branchip`, `branchname`, `branchnotes`, `branchphone`, `branchprinter`, `branchreplyto`, `branchreturnpath`, `branchstate`, `branchurl`, `branchzip`, `geolocation`, `issuing`, `marcorgcode`, `opac_info`, `pickup_location`) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? )" with ParamValues: 0='QADMdMlG', 1='VJKnXt9A', 2='HBsLJeAXM', 3='MYTdwc', 4='VaSzekoa5m', 5='Lq8VRIzrV', 6='OTliw_b', 7='h9uAsl8Ri', 8='MDfzjVnewk', 9='gDRQ0ocfvY', 10='GAVjikP', 11='AY4OtS2P', 12='MNo1lu_HgT6XpzrCdf_zC93daLsy_85itYY81l7GrK7lMglKS_B3Ui7LGlQtIG56JAZtCj', 13='zXfKBMZ0B7', 14='OWZluvG', 15='LbLhKT', 16='mXc_ngd', 17='RAdh0RXYzFtdHyZ5', 18='FO3p_UXnUe8QMRhN90T8_FxOYS46WTSRgBoYPSs0xjcpcUxGq5Wwg7uJKp_RaN82Zye0tLGp7rWkEEVikWvChBJT0iecYPrheOxTUWJ316jbBv0XJtZFFOWkwJC07XT6giITnCzhjXOYhbqSUm7zcCN6wXebIubdxqav5cs2ipWXrkfIJNqeUrvMDeSk3Iqk3yNPSJLHp2UI5FcQ_5jVRnzD5HZmRMvYxc8QAauXaanycEQ_qWmDkVwT7xvwN', 19=119, 20='NYSLw5qzUwMWbT4G', 21='oW_Pvl', 22=50] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836. # No tests run! # Failed test 'No tests run for subtest "is_changed / make_column_dirty"' # at t/db_dependent/Koha/Object.t line 87. DBIx::Class::Storage::DBI::_dbh_execute(): Unknown column 'branchprinter' in 'field list' at /home/vagrant/kohaclone/t/lib/TestBuilder.pm line 358 # Looks like your test exited with 255 just after 3. t/db_dependent/Koha/Object.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 16/18 subtests Test Summary Report ------------------- t/db_dependent/Koha/Object.t (Wstat: 65280 Tests: 3 Failed: 1) Failed test: 3 Non-zero exit status: 255 Parse errors: Bad plan. You planned 18 tests but ran 3. Files=1, Tests=3, 2 wallclock secs ( 0.02 usr 0.01 sys + 1.15 cusr 0.13 csys = 1.31 CPU) Result: FAIL
Created attachment 125128 [details] [review] Bug 28772: [19.11.x] Fix Koha/Object.t Koha::ApiKeys is no longer the simple object we need to test Koha::Object->store, let use Koha::Library::Groups
@Wainui are the relevant tests passing without the security patches? Because unfortunately it's hard to have an environment where all the tests pass. Like me and Kyle in koha-testing-docker have some tests failing in out branches (w/ and w/o security patches) and even master t/db_dependent/www/auth_values_input_www.t t/db_dependent/www/batch.t t/db_dependent/www/history.t t/Logger.t And Fridolin have some other failing tests on their LXD based manual Koha installation.
On my env when running the current security/19.11.22security branch, Koha/Object.t is passing. It uses the comment 72 patch. And it also works when running «[19.11.x] Fix Koha/Object.t»
The Object.t tests seem to still fail with or without the security patches If it works on your end though can I assume it is okay and carry on do you think?
(In reply to wainuiwitikapark from comment #79) > The Object.t tests seem to still fail with or without the security patches > > If it works on your end though can I assume it is okay and carry on do you > think? Yes, the tests are passing for me. It would be good to investigate the issue you are facing however. Could you try and catch one of us on IRC? Or you can drop me an email with the output of the prove command if you prefer.
Backported to 21.05.04, 20.11.10, 20.05.16, 19.11.22
Pushed to master for 21.11, thanks to everybody involved!
> Created attachment 124700 [details] [review] [review] > Bug 28722: Regression tests Wrong bug number, caught it too late..