Bug 1993

*** Bug 1554 has been marked as a duplicate of this bug. ***

This patch doesn't seem to address the security concerns raised by Atz in the last imported comment... could someone speak to this before it is processed through QA?  What precautions are taken?  What vulnerabilities are possible?

This patch seems to have a little error:

my $base = C4::Context->config('intranetdir');
...
$command .= $base . "/misc/cronjobs/runreport.pl";

So the the $command will be ".../koha/intranet/misc/cronjobs/runreport.pl" which doesn't exist.

I think this is the correct code:

$command .= $base . "../bin/cronjobs/runreport.pl";

So the the $command will be ".../koha/intranet/../bin/cronjobs/runreport.pl", pointing exactly to existent run report script.

Right?

(In reply to comment #6)
> This patch doesn't seem to address the security concerns raised by Atz in
> the last imported comment... could someone speak to this before it is
> processed through QA?  What precautions are taken?  What vulnerabilities are
> possible?

Surely that shouldn't block the improvement patch?

The last comment from Atz was "Please note that there are big security issues for using "at" (or in this case "atq") in a multiple-client hosting environment, since each Koha will be running as the same "apache" user.  There is no differentiation on what jobs are visible: everybody will see everything."

That has never been best practice and I'd be surprised and disappointed if liblime did it.

The current packages for debian use apache-mpm-itk to constrain each individual vhost to a particular system user, so the above vulnerability doesn't occur in that way. Other installers could use the same MPM, or they could use other apache modules like mod_ruid to achieve a similar isolation.

If all virtualhosts were running as one user, the vulnerability would have meant that libraries could mess with each others' jobs - deleting, amending, adding jobs for other libraries - but they've probably also got other security vulnerabilities that would arise from the lack of isolation.  I suspect most people who install it themselves and don't follow best practice are only hosting one library, in which case the vulnerability wouldn't occur because there are simply no other libraries to mess with.

That's probably a QA or RM decision that boils down to: do we design Koha to work well under best practice, or not implement a feature because someone could cause a security flaw with it?

But I'm not sure the security concern applies if installed in as suggested by any INSTALL* files, does it?  I think the only installation method that addresses multiple Koha hostings are the packages for debian and they seem safe.

The later comment about using cron is sound, but I think C4::Scheduler uses cron when appropriate now anyway.  It may be better to implement C4::Scheduler with a cron job that uses koha-foreach or multiple user crontabs to review the scheduled tasks every 10 minutes or something, but the current system-level method doesn't seem awful.

Please provide a test plan, problem description and some notes on how this patch is solving the problem. 

Also, I would like to see Vitor's question addressed (comment 7).

Hi Sophie, I missed the notes on the patch yesterday and I think I have some idea now what the patch does, but test plans are a requirement.

Test plan (as illustrated by Chris screenshots)
- Tools > Task scheduler
  or
  Reports > Use saved > Schedule (on the line of the report you want tu run)
- Select time, date, report to run (if not already chosen), output format, mail to send the result.
- Click on Save

Scheduled report is added to the table of existing scheduled reports under the form.

Without the patch, after planned time, if you refresh the page, scheduled report is not displaied anymore, but report is not run and no mail is sent.

With the patch, after planned time, recipient receive a mail containing the result of the report in plain text or attached as csv file dépending of choosen option.

I tried to rebase, but I am unsure of some of the conflicts.

I see a pacth signed off (in 2012??), and status does not apply. Kyle, did you sign it off after your last comment?

Mathieu Saby

Sophie, is there time to rebase this? If the scheduler is broken in master currently this could classify as a bug fix.

Patch rebased

I confirm scheduler is broken in master, and probably in 3.8. We never could use it in Rennes 2!

M. Saby

The QA script reports problems:

 * misc/cronjobs/runreport.pl                                               FAIL
	pod                         OK
	forbidden patterns          OK
	valid                       FAIL
		"my" variable $sql masks earlier declaration in same scope 
		"my" variable $type masks earlier declaration in same scope 
		"my" variable $report_name masks earlier declaration in same scope 

Continuing testing for now.

When trying to schedule an error message is shown:
Failed to add scheduled task

And this is in the logs:
[Sun Apr 07 19:06:15 2013] [error] [client 127.0.0.1] You do not have permission to use atq., referer: http://localhost:8080/cgi-bin/koha/tools/scheduler.pl
[Sun Apr 07 19:06:15 2013] [error] [client 127.0.0.1] You do not have permission to use atq., referer: http://localhost:8080/cgi-bin/koha/tools/scheduler.pl

Katrin, you need to give permission to your apache user to use at

On your server, you should have a file /etc/at.allow containing the name of this user (www-data most of the time) and/or a file /etc/at.deny *not* containing the name of you user

I'll take care to provide a new patch for QA issues

Hi Sophie, switch back to signed off when the follow-up patch is ready for testing. Thx!

I have squashed the previously signed off patch and my followup
This latter modifies get_saved_report function call whose API has changed

Hi Sophie,
please don't squash signed off patches together with new changes. This way we don't know what was texted by the sign offer and the sign offer also might not be happy with it :)

That said - can you maybe provide the original patches? The signed off patch could be unobsoleted, but not sure about how to divide out the newer changes.

Created attachment 18063 [details] [review]
Bug 1993: Improving task scheduler

Remove "url" as format as it is not implemented
    Add csv as output format
    Use MIME:Lite to send email
    Call runreport.pl with right parameters

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Finally, patch only needed to be rebased.
But now I don't know iln which status I should put it

Hi Sophie,
signed off is ok if it was signed off before and only needed a rebase.

Hi Sophie,

I am at a loss here... I don't know how to fix the problem with permissions on running at. I have added the users to a newly created at.allow file, but it's just not working. :(

Created attachment 18183 [details]
screenshot

Actually, the logs show no new permission errors now, but I still get the same message in the interface about the job not being added (see screenshot)

(In reply to comment #29)
> I am at a loss here... I don't know how to fix the problem with permissions
> on running at. I have added the users to a newly created at.allow file, but
> it's just not working. :(

If you have Linux distro with SELinux enforced, that could be a reason too. In that case you need to tell SELinux too that this is okay..

I am using Ubuntu and didn't activate anything. Maybe it's realted to the form that has changed between the screenshots and the current version in master?
The error from the logs is gone, so I think I must have solved the permission problem, but I still can't schedule anything, which makes me think that there is something wrong with my input or something else inside Koha?

Katrin:
Just another observation. I set my SELinux temporarily to Permissive. I can add a job, but there is another security issue.
At job run time, I receive the mail: This account is currently not available.
Why? Because my apache user is not allowed to login. So the job cannot be run, although it is allowed via at.allow etc.

This brings me to a more important question: Should we encourage Koha users to allow login for apache user to make use of the Task Scheduler? Note that is a general Apache hardening measure to not allow interactive login for the apache user. IMO we (as Koha developers) should not stimulate Koha users to lower security barriers to enable Koha functionality.

Another approach to the Task Scheduler could be to have a specific cronjob, look for Koha reports to run at specified times without allowing apache to add generic jobs with all security risks attached..

Moving this report to In Discussion. Will send a mail to the dev list.

(In reply to comment #33)
> This brings me to a more important question: Should we encourage Koha users
> to allow login for apache user to make use of the Task Scheduler? Note that
> is a general Apache hardening measure to not allow interactive login for the
> apache user. IMO we (as Koha developers) should not stimulate Koha users to
> lower security barriers to enable Koha functionality.

I agree: we should not be asking users to grant special privileges to the user that Apache runs as,

> Another approach to the Task Scheduler could be to have a specific cronjob,
> look for Koha reports to run at specified times without allowing apache to
> add generic jobs with all security risks attached..

I think writing a cronjob (and abandoning Schedule::At) is the way to go.  In particular, I suggest the following implementation:

- create a database table to store a queue of reports to run (or maybe generalize it a bit and call it a job queue table)
- the current report scheduling functionality would just add rows to the queue with a run start time set by the user
- the cronjob would periodically check the queue and run any pending reports whose run start time has passed

This would be pretty light-weight, but would allow us to get fancier in the future.  For example, adding functionality to schedule reports to run periodically could be done by writing code to add entries into the job queue.

I am not sure how we continue best here. I think in the past it was at least possible to make the scheduler work by changing the permissions on sysadmin level. I know that this has not been recommended for security reasons.

I wonder that now, that this seems not to be working at all, we should hide (not delete) the options from the user interface until we can find someone who sponsors the rewrite.

Do you know if this feature really work in the past?
I find strange to pay again to fix a feature broken by somebody's code 2 or 3 years ago. Is not it really possible to fix this feature, and to make an other bug for improving safety ?

M. Saby

"I find strange to pay again to fix a feature broken by somebody's code 2 or 3 years ago." - ah well, this happens with most software, where a feature is added without a good-enough test set for it, or where some other design change is seen as desirable enough to be breaking other features, which is maybe what happened here (security is good).  Customers are paying for this sort of maintenance in other software - some developers joke "plan to throw one away - you will anyway" - and it's just more visible in open software like Koha.

So if this feature is readded (maybe with a task-runner script something like drupal's cron.php, maybe with a proper cronjob), it should be added with some test to help avoid breaking it again.

Hopefully this is the right bug to write about this in. Just noticed that runreport.pl (which I'm calling manually from a cron job), whilst allowing for a format of html, doesn't set the "Content-Type" on such emails to "text/html".

A simple patch replaces:

    if ($email){
        my %mail = (
            To      => $to,
            From    => $from,
            Subject => encode('utf8', $subject ),
            Message => encode('utf8', $message )
        );
        sendmail(%mail) or carp 'mail not sent:' . $Mail::Sendmail::error;
    } else {
        print $message;
    }

with
    if ($email){
        my %mail;
        if ($format eq 'html') {
                $message = "<html><head><style>tr:nth-child(n+1) { background-color: #ccc;}</style></head><body>$message</body></html>";
           %mail = (
              To      => $to,
              From    => $from,
              'Content-Type' => 'text/html',
              Subject => encode('utf8', $subject ),
              Message => encode('utf8', $message )
          );
        } else {
          %mail = (
              To      => $to,
              From    => $from,
              Subject => encode('utf8', $subject ),
              Message => encode('utf8', $message )
          );
        }
        sendmail(%mail) or carp 'mail not sent:' . $Mail::Sendmail::error;
    } else {
        print $message;
    }

(In reply to Nicholas van Oudtshoorn from comment #38)
> Hopefully this is the right bug to write about this in. Just noticed that
> runreport.pl (which I'm calling manually from a cron job), whilst allowing
> for a format of html, doesn't set the "Content-Type" on such emails to
> "text/html".
> 

I am not sure about that, Nicholas.
This bug is about scheduling running reports, not about mailing in html format.
If you can't find a more specific bug, please open a new report for it.
And your patch is welcome too. Preferably as formatted by git. (Look for instructions on using git-bz; that will make it easier for you.)
Thanks.

Sorry about the noise. Have created the new bug report (10777), and will upload patch there shortly!

(In reply to Nicholas van Oudtshoorn from comment #40)
> Sorry about the noise. Have created the new bug report (10777), and will
> upload patch there shortly!

Thx

(In reply to Nicholas van Oudtshoorn from comment #40)
> Sorry about the noise. Have created the new bug report (10777), and will
> upload patch there shortly!

A patch more than makes up for it, people can make as much noise as they want, if they also send patches ;)

Dear Sophie,

I have made changes in the koha files according to the patch you had attached in this thread.  But emails are still not generating and recipient does not receive any emails. I have also checked the at.allow and at.deny file. my at.deny file is empty that , according to my understanding , means all the users have the permissions to run task scheduler. Kindly help me with the solution ,as i am stuck on this from last few days.

When i schedule a task using task scheduler. It showed me the following command under the action tab.

export KOHA_CONF="/etc/koha/koha-conf.xml"; /usr/share/koha/intranet/cgi-bin/misc/cronjobs/runreport.pl --format csv --to ghulam.sarwar@techlogix.com 6


Looking forward to your reply.

(In reply to Sophie MEYNIEUX from comment #11)
> Test plan (as illustrated by Chris screenshots)
> - Tools > Task scheduler
>   or
>   Reports > Use saved > Schedule (on the line of the report you want tu run)
> - Select time, date, report to run (if not already chosen), output format,
> mail to send the result.
> - Click on Save
> 
> Scheduled report is added to the table of existing scheduled reports under
> the form.
> 
> Without the patch, after planned time, if you refresh the page, scheduled
> report is not displaied anymore, but report is not run and no mail is sent.
> 
> With the patch, after planned time, recipient receive a mail containing the
> result of the report in plain text or attached as csv file dépending of
> choosen option.

I got it to work by changing the command definition in tools/scheduler.pl

    my $command =
        "export KOHA_CONF=\"$CONFIG_NAME\"; export PERL5LIB=" . $base . "; " . $base
      . "/misc/cronjobs/runreport.pl ";
    $command = $command . "--format=$format " if $format;
    $command = $command . "--to='$email' " if $email;
    $command = $command . $report if $report;

AND also added www-data, which was defined in /etc/apache2/envvars (APACHE_RUN_USER) to the /etc/at.allow and no more permission problem

I had created an sql report, and did not test email, though without email the report output was seen in the mail box for the www-data user.
(su - www-data     and then 'mail'   and  p for print. )

I am just documenting my cursory findings.

I like Galen's idea of using a database table to store reports, and then using a cronjob to execute those reports on a periodic basis.

I'm not sure if we even need to allow them to specify an exact time though. One of our libraries mentioned wanting to have daily/weekly/monthly reports. It would probably be enough to set up three cronjobs (daily, weekly, monthly), and then run the reports (or jobs) that have been designated as such in the DB table.

After all, if the report is run before the library opens or after it closes, it should provide all the functionality desired. I would have my doubts about needing a report run at 1:35pm each day.

I suppose it is possible though that you might want "morning" and "afternoon" reports, so maybe it is worth using times in the DB table. Or perhaps that's where you just add another cronjob daily cronjob for a different time. 

That would give the sysadmin ultimate say while also giving librarians the overall functionality that they desire.

Robin also suggested using the 'batch' command which "executes commands when system load levels permit" (source: man batch (1) ).

I was able to make it work using Wajasu's method. But the html is still not working. Requires re-write of scheduler.pl CGI.

This bug might be related: 

Bug 14712 - Feature for controlling cron jobs/cmd line scripts from staff client
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14712

There are people in Sweden working on it.

I'm also working on a related feature via my work on bug 10662.

My work is also sponsored by folks in Sweden, so hurray Sweden!

I've been thinking about opening a new bug, or looking up one of these bugs to post patches.

Basically, I've written a task scheduler, which accepts a few variables (e.g. start_time, repeat_interval, task_type, params). It then uses plugin modules to run the actual task. I've already written plugins to download records via OAI-PMH, and to upload those records to a new Koha API for OAI-PMH records specifically. It would be fairly trivial to create a "Report" plugin. 

The web user interface relies on a template include which is matched to the plugin.

The whole thing needs some work, but I'm hoping folks will test it and I'm looking at polishing it up...

At the moment, it's all at bug 10662, but I'm thinking of breaking into different bug reports...

This is a minor issue, but trivial to implement once the gory work on the scheduler itself is complete -- 

When scheduling reports the list of reports in the drop-down menu should be alphabetized.

Most libraries have a large inventory of reports, so having them in alphabetical order makes a lot of sense.

*** Bug 20673 has been marked as a duplicate of this bug. ***

After seeing a comment on #3935, I was thinking that maybe this is worth another revisit sometime soon.

There are lots of pre-existing job queues and task schedulers out there in the world, but - considering our embracing of the Mojolicious framework - maybe we should opt for Minion (https://mojolicious.org/perldoc/Minion). Martin Renvoize has advocated this in the past and has some experience with it, I think.

For #10662, I wrote a job queue / task scheduler using https://metacpan.org/pod/POE::Component::JobQueue and https://metacpan.org/pod/POE::Wheel::Run. I had looked at Minion a bit, but I think we hadn't accepted Mojolicious yet, and I had some requirements (like deleting already enqueued jobs before they had a chance to run) which it didn't seem to have.

I think the hardest thing for this issue is not the work itself... but making a choice in a sea of possibilities.

On that note, maybe this is an issue that would best be addressed using plugins. We could create a uniform task scheduler user interface and internal API, and then the "hard work" of interfacing with a particular scheduling system could be left up to a plugin.

That also means that we could experiment a bit before forging ahead with "The One True Way"?

I've been thinking more about this recently.

How to be all things to all people?

For instance, the original Koha task scheduler for Bug 1993 was meant to schedule a report to run once at an absolute time and email the result to someone. 

Bug 25245 wants plugins to be able to run every night. That is, a periodic task that runs at the same time every day/night (while not being too fussed on exact start/end times).

Bug 10662 wants to schedule tasks to run every X seconds. That is, a periodic task that runs X seconds after it last ran. 

Those are arguably 3 different types of schedules. 

The Celery project would handle that using ETA and Periodic Tasks:
https://docs.celeryproject.org/en/stable/userguide/calling.html#eta-and-countdown
https://docs.celeryproject.org/en/latest/userguide/periodic-tasks.html

Looking at cron, we can see it just loops infinitely and sleeps for 1 minute at a time then checks what jobs need to run:
https://github.com/cronie-crond/cronie/blob/master/src/cron.c#L358

In the past, I've used timers using Perl's POE or Golang's Timer to alert me when a job needs to start. That's fine so long as your process runs forever. You do need to persist the job data, so that if you restart you can re-create your timers. 

In terms of the actual implementation... lately I'm thinking a daemon with a HTTP API so you can create, retrieve, update, delete, start timer, stop timer. When a timer does fire... the daemon should add the task to a queue. (This is also what cron does https://github.com/cronie-crond/cronie/blob/master/src/cron.c#L589). 

Now that queue could be a RabbitMQ message queue. It could be an in-memory queue in the task scheduler daemon and it could run tasks itself by forking a child process, using a thread (if not Perl), or using a connected worker. It could be a database table. 

On that note, it becomes apparent to me that we don't need to depend on Bug 22417 to move forward here. The task scheduler can be a black box with an API for the front end, and then pluggable backends for different queue systems. 

Although as I say that I haven't thought it through enough.

With a FIFO queue, we run into cron's limitation of not tracking execution of a task. As is, Bug 10662 scenario is impossible, because we don't have the result of the task to update the task with new data (ie new "from" date) and new time to schedule.

With RabbitMQ, we can use message acknowledgements to determine when the task is completed. Celery also has a result store. 

But Bug 10662 would still be unique in that it's not really a periodic task but rather an absolute task that gets rescheduled. It looks like Celery does actually have callbacks that can be run on the result at the end of a task. That's interesting. That would be a good thing to develop. Arguably the Report Email functionality could make good use of that. The task would be to generate the report and store the result. The callback would then email it. In the case of Bug 10662, the callback could perhaps update the original task, enqueue a new one. I'd have to think more about that mechanism...

At the moment, I'm writing a new task scheduler in Golang for a plugin version of Bug 10662. Like the existing Perl version attached to Bug 10662, it closely couples the task scheduler and OAI-PMH harvester. In the Golang case, when the timer fires, a new thread is created to do a HTTP download of XML (which actually pipes the XML stream into another thread with a XML stream reader in it which concurrently parses and writes away the downloaded records). I am planning to give each task a thread to take those downloaded records and import them into Koha (via a Plugin API). But if I were to use a more generalized task scheduler... I'm not sure how I'd do it. If we did have a message queue, perhaps the download task would send the records to an API which would record them and then enqueue a RabbitMQ message to an import worker.

I'm actually feeling pretty inspired by this project at the moment: https://github.com/walmartlabs/bigben

It's way beyond anything we'd need with Koha, but it has some interesting ideas. I especially like the "Event Inflow diagram".

In essence, it would be useful if the input/output of the task scheduler was via RabbitMQ.

I have some experimenting to do on optimal use of message queues for different models of communication.

Since Koha has embraced Mojolicious, I'm thinking that I could write a "task_scheduler.pl" (koha-scheduler) using Mojo::EventEmitter and Mojo::IOLoop to get a reasonably good scheduler with a minimum amount of code.

Ideally, its output would be sending messages to RabbitMQ.

The real question is input and task storage. Bug 22417 gave us task storage. We could extend that to include scheduler information. As for input... I'd say the best solution would be communicating over a socket. Where all services are running locally, Unix socket is the easiest. For more distributed setups, we could make sure to use a TCP socket (we could even make it a HTTP API). 

(Looking at Minion, you can run it as a standalone job queue or leverage the event loop in a Mojolicious application. The latter option might not work when using Mojolicious as a PSGI application though. We do have Bug 20582 for turning Koha into a Mojolicious application though.)

I can try to do a POC soon but can't make any guarantees...

(In reply to David Cook from comment #56)
> I can try to do a POC soon but can't make any guarantees...

I haven't had the time/energy to do any coding in my own time this year, so probably won't be working on this any time soon.

As Martin has noted elsewhere:

(In reply to Martin Renvoize from comment #6)
> I've long felt our use of 'at' in scheduler was wrong, but we've not had a
> reasonable alternative.  We now have BackgroundJobs and I could see us using
> that system as an alternative to using 'at' and calling a script.

I agree about using BackgroundJobs for the execution. 

We'll still need a scheduler, and I think the shortest path forward is a cronjob-based scheduler script, which polls the database every X interval looking for jobs to run.

(In reply to David Cook from comment #58)
> As Martin has noted elsewhere:
> 
> (In reply to Martin Renvoize from comment #6)
> > I've long felt our use of 'at' in scheduler was wrong, but we've not had a
> > reasonable alternative.  We now have BackgroundJobs and I could see us using
> > that system as an alternative to using 'at' and calling a script.
> 
> I agree about using BackgroundJobs for the execution. 
> 
> We'll still need a scheduler, and I think the shortest path forward is a
> cronjob-based scheduler script, which polls the database every X interval
> looking for jobs to run.

If we do this, could we make it work for scheduled repeated sending?

I am thinking of something like daily, on a specific day of the week, first of the month, etc. and also a time for each of those. 

A specific time might not work I guess or at least not exactly. Maybe something like "after 8:00" and then the first cron run after that time would run it and  set a 'last run on' date?

(In reply to Katrin Fischer from comment #59)
> If we do this, could we make it work for scheduled repeated sending?
> 
> I am thinking of something like daily, on a specific day of the week, first
> of the month, etc. and also a time for each of those. 
> 
> A specific time might not work I guess or at least not exactly. Maybe
> something like "after 8:00" and then the first cron run after that time
> would run it and  set a 'last run on' date?

It would probably be wise to factor in recurring scheduled tasks.

We do have some local backend stuff for this where we have "hourly", "daily", "weekly", and "monthly" granularity. Plus you can set hours and days... 

Needs more thought but probably a good idea to think about it.

[ Just noting that there is a RabbitMQ plugin to send delayed messages, but that's not what we want here:
https://github.com/rabbitmq/rabbitmq-delayed-message-exchange ]