Bug 36084 - Pass CSRF token to SVC scripts (1/2)
Summary: Pass CSRF token to SVC scripts (1/2)
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 36102
Blocks: 36194 36211 36234 36235 36426
  Show dependency treegraph
 
Reported: 2024-02-13 14:18 UTC by Jonathan Druart
Modified: 2024-04-17 09:14 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00


Attachments
Bug 36084: C4::Auth+plack.psgi for svc? (5.65 KB, patch)
2024-02-13 14:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36084: svc - article_request - POC (17.01 KB, patch)
2024-02-13 14:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36084: Bring fetch for everywhere (6.18 KB, patch)
2024-02-13 14:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36084: Add a Dialog class (1.33 KB, patch)
2024-02-13 14:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36084: Add a global #messages div (6.38 KB, patch)
2024-02-13 14:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36084: Do not allow absence of token (2.32 KB, patch)
2024-02-14 08:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36084: Add cud-login to the login form (4.55 KB, patch)
2024-02-14 13:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36084: svc - authorised_values - APIClient now global (5.07 KB, patch)
2024-02-14 15:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36084: Fix cookie domain for www/ tests (1.31 KB, patch)
2024-03-04 13:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2024-02-13 14:18:53 UTC
On top of bug 35560 we need to pass the CSRF token to the ajax/svc scripts.
Comment 1 Jonathan Druart 2024-02-13 14:25:04 UTC
Created attachment 162108 [details] [review]
Bug 36084: C4::Auth+plack.psgi for svc?

Suggestion to move the CSRF check to CGI->new so that we will check it
for every request, and it will cover svc scripts as well (they are not
using get_template_and_user).

The token will be retrieve from the param list *or the csrf_token
header* (do we want to name it x-koha-csrf-token instead?).

This will be done for *every* request that are not GET: CSRF token is now
required everywhere CGI is used (side-effects possible?).
Comment 2 Jonathan Druart 2024-02-13 14:25:07 UTC
Created attachment 162109 [details] [review]
Bug 36084: svc - article_request - POC

This is a proof of concept
Comment 3 Jonathan Druart 2024-02-13 14:25:10 UTC
Created attachment 162110 [details] [review]
Bug 36084: Bring fetch for everywhere

We are retrieving the awesome fetch modules from Vue, so that it can be
used in other areas. Here we will use it to inject the CSRF token to the
header of every POST request.
Comment 4 Jonathan Druart 2024-02-13 14:25:13 UTC
Created attachment 162111 [details] [review]
Bug 36084: Add a Dialog class

To display potential errors.
Comment 5 Jonathan Druart 2024-02-13 14:25:16 UTC
Created attachment 162112 [details] [review]
Bug 36084: Add a global #messages div
Comment 6 Jonathan Druart 2024-02-13 14:26:31 UTC
We can do it this way, with a strong base.

Or you are doing it the naive way and add a new csrf_token param to the body and check it in every svc scripts (without me).

In discussion, waiting for feedback.
Comment 7 Marcel de Rooy 2024-02-13 14:38:53 UTC
+        } elsif ( $request_method ne 'GET' ) {
+            # Get the CSRF token from the param list or the header
+            my $csrf_token = $q->param('csrf_token') || $q->http('HTTP_CSRF_TOKEN');
+

Doesnt this include REST API already via plack.psgi ?
Comment 8 Marcel de Rooy 2024-02-13 14:39:13 UTC
(In reply to Jonathan Druart from comment #6)
> We can do it this way, with a strong base.
> 
> Or you are doing it the naive way and add a new csrf_token param to the body
> and check it in every svc scripts (without me).
> 
> In discussion, waiting for feedback.

All for a smarter central approach.
Comment 9 Jonathan Druart 2024-02-13 15:25:17 UTC
(In reply to Marcel de Rooy from comment #7)
> +        } elsif ( $request_method ne 'GET' ) {
> +            # Get the CSRF token from the param list or the header
> +            my $csrf_token = $q->param('csrf_token') ||
> $q->http('HTTP_CSRF_TOKEN');
> +
> 
> Doesnt this include REST API already via plack.psgi ?

No, the REST API does not use CGI.
Comment 10 Jonathan Druart 2024-02-13 15:28:43 UTC
For discussion: note that we have a breach in plack.psgi. We do not die if the the token is not valid, we only undef $op. I think it is enough (at least for now), but it does not cover us from future bad usage (from devs). We don't really want to explode as we want to propagate the error to the end-user.
Comment 11 Jonathan Druart 2024-02-14 08:36:13 UTC
Created attachment 162133 [details] [review]
Bug 36084: Do not allow absence of token

Well, this test was silly, I was focussed on propagating an error to the
UI, but we really need to explode in this case.

Note that this requires more work as login is now broken.
Comment 12 Jonathan Druart 2024-02-14 13:56:55 UTC
Created attachment 162147 [details] [review]
Bug 36084: Add cud-login to the login form

TODO This needs to be covered by tests.
Comment 13 Jonathan Druart 2024-02-14 13:57:57 UTC
If this does not apply, please use the branch bug_34478_svc from the security repo.
Comment 14 Jonathan Druart 2024-02-14 13:58:43 UTC
(In reply to Jonathan Druart from comment #12)
> Created attachment 162147 [details] [review] [review]
> Bug 36084: Add cud-login to the login form
> 
> TODO This needs to be covered by tests.

This feels hacky, but I didn't find something better. Also not sure about the side-effects. Please test and read carefully!

I've added this patch here as it fixes a regression from the changes in plack.psgi.
Comment 15 Jonathan Druart 2024-02-14 15:34:58 UTC
Created attachment 162150 [details] [review]
Bug 36084: svc - authorised_values - APIClient now global

APIClient is not a global variable, which will make the next changes
much easier!
Comment 16 Jonathan Druart 2024-02-14 15:42:27 UTC
(In reply to Jonathan Druart from comment #15)
> Created attachment 162150 [details] [review] [review]
> Bug 36084: svc - authorised_values - APIClient now global
> 
> APIClient is not a global variable, which will make the next changes
> much easier!

FIXME here. We need to lazy import (in api-client.js) and use Asset for the import in doc-head-close (but keep the type="module"!)
Comment 17 David Cook 2024-02-14 23:03:23 UTC
(In reply to Jonathan Druart from comment #12)
> Created attachment 162147 [details] [review] [review]
> Bug 36084: Add cud-login to the login form
> 
> TODO This needs to be covered by tests.

Got your message on IRC. Shouldn't we be doing this separately to the svc/ work? 

I'd have to re-test, but last time I tried a patch like this the CSRF test was using the logged in user session against the CSRF token generated for the anonymous session. If things haven't changed since then, I don't know that this patch will work. 

But maybe bug 36092 fixes that...
Comment 18 David Cook 2024-02-14 23:09:12 UTC
If we're going to make changes at the plack.psgi level, why not just do a Koha::Middleware? 

Perhaps we should think about making it work similar to C4::Context->set_remote_address and Koha::Middleware::RealIP. 

For Plack scripts, the middleware comes into effect. If it's non-Plack (like the non-proxied scripts like export.pl), then it uses C4::Context->set_remote_address, which is invoked by C4::Auth at compile time (ie in the BEGIN block).
Comment 19 David Cook 2024-02-14 23:12:14 UTC
Comment on attachment 162108 [details] [review]
Bug 36084: C4::Auth+plack.psgi for svc?

Review of attachment 162108 [details] [review]:
-----------------------------------------------------------------

::: debian/templates/plack.psgi
@@ +55,3 @@
>              $q->param( 'op', '' );
>              $q->param( 'debug_programming_error', "'$original_op' must not start with 'cud-' for GET" );
> +        } elsif ( $request_method ne 'GET' ) {

I don't think this condition makes sense, because it means we require CSRF tokens for HEAD requests. (Also for OPTIONS and TRACE although those two are less common request methods.)

I know it's annoying, but I think we'd have to test specifically for POST, PUT, DELETE, PATCH. 

That said, we can just do $stateful_methods{ $request_method } to still make the elsif() pretty clean.
Comment 20 David Cook 2024-02-15 04:57:24 UTC
Bug 36094 is an interesting one. I don't think that we can get rid of "svc/authentication" yet, but it does expose a challenging problem. How do we handle CSRF and an API authentication endpoint?

DSpace appears to expose "/api/authn/status" and "/api/authn/login" endpoints. So you'd first GET "/api/authn/status" (which includes a CSRF token in the response headers), and then you post to "/api/authn/login" using the CSRF token.

If we want to continue supporting external use of "svc/authentication" then we probably want to add "svc/auth/status" or "svc/authn/status" and use a similar model. 

--

I suppose we could just say that we don't support external usage of the SVC API anymore...
Comment 21 David Cook 2024-02-15 05:00:57 UTC
misc/bin/connexion_import_daemon.pl will be broken if we require CSRF for SVC API.

It should probably be use "svc/authentication"
Comment 22 Jonathan Druart 2024-02-15 08:19:34 UTC
(In reply to David Cook from comment #17)
> (In reply to Jonathan Druart from comment #12)
> > Created attachment 162147 [details] [review] [review] [review]
> > Bug 36084: Add cud-login to the login form
> > 
> > TODO This needs to be covered by tests.
> 
> Got your message on IRC. Shouldn't we be doing this separately to the svc/
> work?

comment 14: "I've added this patch here as it fixes a regression from the changes in plack.psgi."
It could be moved somewhere else, but to be honest it's already a lot of bug to deal with and keep up-to-date...

> I'd have to re-test, but last time I tried a patch like this the CSRF test
> was using the logged in user session against the CSRF token generated for
> the anonymous session. If things haven't changed since then, I don't know
> that this patch will work. 

Please define "does not work".

(In reply to David Cook from comment #18)
> If we're going to make changes at the plack.psgi level, why not just do a
> Koha::Middleware? 

Changes to plack.psgi are already made from bug 34478.

Yes, why not? Why not refactor C4::Auth at the same time? :)

> Perhaps we should think about making it work similar to
> C4::Context->set_remote_address and Koha::Middleware::RealIP. 
> 
> For Plack scripts, the middleware comes into effect. If it's non-Plack (like
> the non-proxied scripts like export.pl), then it uses
> C4::Context->set_remote_address, which is invoked by C4::Auth at compile
> time (ie in the BEGIN block).

More changes, more questions. KISS for now please (it's already complex enough).

(In reply to David Cook from comment #19)
> Comment on attachment 162108 [details] [review] [review]
> Bug 36084: C4::Auth+plack.psgi for svc?
> 
> Review of attachment 162108 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: debian/templates/plack.psgi
> @@ +55,3 @@
> >              $q->param( 'op', '' );
> >              $q->param( 'debug_programming_error', "'$original_op' must not start with 'cud-' for GET" );
> > +        } elsif ( $request_method ne 'GET' ) {
> 
> I don't think this condition makes sense, because it means we require CSRF
> tokens for HEAD requests. (Also for OPTIONS and TRACE although those two are
> less common request methods.)
> 
> I know it's annoying, but I think we'd have to test specifically for POST,
> PUT, DELETE, PATCH. 

Are we using HEAD, OPTIONS or TRACE already?

> That said, we can just do $stateful_methods{ $request_method } to still make
> the elsif() pretty clean.

Please provide a patch.

(In reply to David Cook from comment #20)
> Bug 36094 is an interesting one. I don't think that we can get rid of
> "svc/authentication" yet, but it does expose a challenging problem. 

Answering there.
Comment 23 Marcel de Rooy 2024-02-15 10:17:12 UTC
TODO koha-tmpl/intranet-tmpl/prog/en/modules/admin/didyoumean.tt
SKIPPED (in remaining pad) This is part of the svc operation.
The form does not submit, there is still an ajax post with token.
Comment 24 Jonathan Druart 2024-02-15 13:17:25 UTC
See the remote branch for the update - security/bug_34478_svc
Comment 25 David Cook 2024-02-15 23:38:39 UTC
(In reply to Jonathan Druart from comment #22)
> > I'd have to re-test, but last time I tried a patch like this the CSRF test
> > was using the logged in user session against the CSRF token generated for
> > the anonymous session. If things haven't changed since then, I don't know
> > that this patch will work. 
> 
> Please define "does not work".

Don't worry about it. I'll look at it today with bug 36102.
 
> (In reply to David Cook from comment #18)
> > If we're going to make changes at the plack.psgi level, why not just do a
> > Koha::Middleware? 
> 
> Changes to plack.psgi are already made from bug 34478.
> 
> Yes, why not? Why not refactor C4::Auth at the same time? :)

Writing a new middleware and refactoring C4::Auth are completely different things. Writing a new middleware is quick and easy. Also easier to test. It's also been done before.

> More changes, more questions. KISS for now please (it's already complex
> enough).

What I'm talking about would be less complex. But it's fine. Teamwork is hard. Happy to follow your lead.
 
> Are we using HEAD, OPTIONS or TRACE already?

HEAD is very common and useful. I use it in plugins and monitoring. I imagine a lot of people use it in their monitoring. 

OPTIONS and TRACE are not so common. 
 
> > That said, we can just do $stateful_methods{ $request_method } to still make
> > the elsif() pretty clean.
> 
> Please provide a patch.

Will do.
Comment 26 David Cook 2024-02-15 23:42:15 UTC
(In reply to Jonathan Druart from comment #24)
> See the remote branch for the update - security/bug_34478_svc

I just noticed you've removed svc/authentication in that branch. I really don't think we should do that.
Comment 27 David Cook 2024-02-16 00:01:21 UTC
I just realized that the changes to debian/templates/plack.psgi aren't automatically reflected in /etc/koha/sites/kohadev/plack.psgi 

Has anyone been testing those changes?
Comment 28 Jonathan Druart 2024-02-16 07:20:07 UTC
(In reply to David Cook from comment #27)
> I just realized that the changes to debian/templates/plack.psgi aren't
> automatically reflected in /etc/koha/sites/kohadev/plack.psgi 
> 
> Has anyone been testing those changes?

Yes, I think people working on this are aware of that. The file must be copied.
Comment 29 Marcel de Rooy 2024-02-16 09:01:35 UTC
From remaining pad:

koha-tmpl/intranet-tmpl/prog/js/pages/preferences.js
MOVED: This is part of the svc stuff too. Comment on 36084

Please fix here
Comment 30 David Cook 2024-02-21 06:20:53 UTC
I've added support for CSRF tokens to svc/authentication, but there's more work to do on the svc/ API as a whole 

I'm happy to look at this
Comment 31 David Cook 2024-02-21 06:23:20 UTC
(In reply to David Cook from comment #30)
> I've added support for CSRF tokens to svc/authentication, but there's more
> work to do on the svc/ API as a whole 
> 
> I'm happy to look at this

I reckon that all svc/* endpoints should return a X-CSRF_TOKEN in their response headers, so that API consumers outside of the Koha web app can interact with the SVC API.

If they encounter errors, they can do a GET svc/authentication to get a new CSRF token in the X-CSRF_TOKEN response header, but that won't be as smooth as getting it in any SVC API response.

--

Happy to discuss this more, especially if I've made too many assumptions in my communications. It's been a long day.
Comment 32 Jonathan Druart 2024-02-21 08:53:45 UTC
(In reply to David Cook from comment #30)
> I've added support for CSRF tokens to svc/authentication, but there's more
> work to do on the svc/ API as a whole 
> 
> I'm happy to look at this

Not before we discussed it. This bug is about the other svc scripts. authentication and CSRF token for external consumer needs to be discussed somewhere else I think.
Comment 33 Marcel de Rooy 2024-02-22 10:15:51 UTC
Just a dumb question:

Was thinking if we could use ajaxSend event to include the token (from the meta tag) in a general header file for catching all cases where we do these ajax requests in Koha itself based on cookie auth?
And check the token in check_cookie_auth?

Other REST API use should probably use API tokens where there seems to be no need for an extra csrf token? Not talking about Basic etc. ;)
Comment 34 Jonathan Druart 2024-02-22 14:23:31 UTC
(In reply to Marcel de Rooy from comment #33)
> Just a dumb question:
> 
> Was thinking if we could use ajaxSend event to include the token (from the
> meta tag) in a general header file for catching all cases where we do these
> ajax requests in Koha itself based on cookie auth?
> And check the token in check_cookie_auth?
> 
> Other REST API use should probably use API tokens where there seems to be no
> need for an extra csrf token? Not talking about Basic etc. ;)

Please see what is done on the branch :)
Comment 35 Jonathan Druart 2024-02-22 14:23:53 UTC
koha-tmpl/intranet-tmpl/prog/js/fetch/http-client.js

24         this.csrf_token = $('meta[name="csrf-token"]').attr("content");
Comment 36 Jonathan Druart 2024-02-22 14:31:11 UTC
So basically the idea here (for svc scripts) is to have:
* An api-client.js file to have the same interface everywhere (that was not necessary but I liked the idea of forcing the use here..)
* OR do not have an api-client.js file if there is much interest in the changes (I did that for creator_batches). It is much easier, but less fun.

* Have 'op' or not! We could early exit in the script of request_method ne 'POST' (or DELETE, PUT, etc.). This is not done everywhere and not consistent (neither after my changes).
If the svc script rejects other methods we don't need an 'op' parameter (eg. the ones using C4::Service or testing $query->request_method eq "POST").
Comment 37 Jonathan Druart 2024-03-01 13:22:04 UTC
Pushed to master for 24.05.00.
Comment 38 Tomás Cohen Arazi 2024-03-04 13:27:13 UTC
Created attachment 162712 [details] [review]
Bug 36084: Fix cookie domain for www/ tests

This patch changes the hardcoded `koha.local` value on
t::lib::Mocks::Zebra so tests don't fail when the domain is not `koha`.

To test:
1. Run:
   $ ktd --shell
  k$ export KOHA_INTRANET_URL=http://kohadev-intra.myDNSname.org:8081
  k$ prove t/db_dependent/www/batch.t
=> FAIL: Tests fail!
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 39 Jonathan Druart 2024-03-04 13:33:33 UTC
Last patch pushed to master.
Comment 40 Fridolin Somers 2024-04-17 09:14:24 UTC
Depends on Bug 36102 not in 23.11.x