Bug 16929 - Prevent opac-memberentry waiting for random chars
Summary: Prevent opac-memberentry waiting for random chars
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Whiteboard:
Keywords: dependency
Depends on:
Blocks: 17096 17905 16992 16993 17097 17109
  Show dependency treegraph
 
Reported: 2016-07-15 12:13 UTC by Marcel de Rooy
Modified: 2017-01-31 09:24 UTC (History)
7 users (show)

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


Attachments
Bug 16929: Wrapper around CSRF call (6.10 KB, patch)
2016-07-15 12:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16929: Prevent opac-memberentry waiting for random chars (10.30 KB, patch)
2016-07-19 13:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16929: [QA Follow-up] Shortcut methods and use statements (6.98 KB, patch)
2016-07-21 06:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16929: [QA Follow-up] Add dependency for Bytes::Random::Secure (862 bytes, patch)
2016-07-29 10:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16929: Prevent opac-memberentry waiting for random chars (10.30 KB, patch)
2016-08-08 08:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16929: [QA Follow-up] Shortcut methods and use statements (6.98 KB, patch)
2016-08-08 08:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16929: [QA Follow-up] Add dependency for Bytes::Random::Secure (881 bytes, patch)
2016-08-08 08:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16929: Prevent opac-memberentry waiting for random chars (10.34 KB, patch)
2016-08-08 08:39 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16929: [QA Follow-up] Shortcut methods and use statements (6.96 KB, patch)
2016-08-08 08:40 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16929: [QA Follow-up] Add dependency for Bytes::Random::Secure (873 bytes, patch)
2016-08-08 08:41 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16929: [QA Follow-up] Add dependency for Bytes::Random::Secure (919 bytes, patch)
2016-08-08 11:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16929: Prevent opac-memberentry waiting for random chars (10.42 KB, patch)
2016-08-08 12:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16929: [QA Follow-up] Shortcut methods and use statements (7.04 KB, patch)
2016-08-08 12:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16929: [QA Follow-up] Add dependency for Bytes::Random::Secure (947 bytes, patch)
2016-08-08 12:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16929: [QA Follow-up] Add dependency for Bytes::Random::Secure (974 bytes, patch)
2016-08-09 14:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2016-07-15 12:13:53 UTC

    
Comment 1 Marcel de Rooy 2016-07-15 12:15:37 UTC
See bug 16591 latest comments.
Patches still concept.
Comment 2 Marcel de Rooy 2016-07-15 12:16:54 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2016-07-19 13:49:33 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2016-07-19 13:52:00 UTC
Latest patch always uses Bytes::Random::Secure and does not eval/alarm as in the concept. Includes a test.
Comment 5 Jonathan Druart 2016-07-20 14:43:40 UTC
I'd prefer to see a generate_csrf method than a CSRF flag.
It'd be better to use instead of require the 2 modules.
Comment 6 Marcel de Rooy 2016-07-20 15:05:53 UTC
(In reply to Jonathan Druart from comment #5)
> I'd prefer to see a generate_csrf method than a CSRF flag.
> It'd be better to use instead of require the 2 modules.

Well, we could add generate_csrf of course. The flag on itself is kind of a shortcut for type => $type.
The require is just performance: no need to load it for general use.
Comment 7 Jonathan Druart 2016-07-20 15:29:57 UTC
(In reply to Marcel de Rooy from comment #6)
> The require is just performance: no need to load it for general use.

It's not recommended. For instance if the module is missing of buggy, we want to know it at the compilation step.
Comment 8 Marcel de Rooy 2016-07-21 06:58:32 UTC Comment hidden (obsolete)
Comment 9 Mirko Tietgen 2016-07-29 10:26:52 UTC
Bytes::Random::Secure should be added to PerlDependencies.pm.

libbytes-random-secure-perl is not available for Debian Wheezy.
Comment 10 Marcel de Rooy 2016-07-29 10:39:33 UTC
(In reply to Mirko Tietgen from comment #9)
> Bytes::Random::Secure should be added to PerlDependencies.pm.
> 
> libbytes-random-secure-perl is not available for Debian Wheezy.

It actually came with WWW::CSRF. It loads Bytes::Random::Secure. Is there a package for that one?

Will add a follow-up for B::R::S
Comment 11 Marcel de Rooy 2016-07-29 10:43:35 UTC Comment hidden (obsolete)
Comment 12 Mirko Tietgen 2016-07-29 10:53:41 UTC
> It actually came with WWW::CSRF. It loads Bytes::Random::Secure. Is there a
> package for that one?

If it is used directly, I think it should be in PerlDependencies.

There is no package for WWW:CSRF in Wheezy. I created packages for it and dependencies for the unstable branch for bug 16591. https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16591#c7
I did not get any feedback regarding Wheezy so far and did not have time to try it myself, so I have not added them to oldstable yet. 


> Will add a follow-up for B::R::S

Thanks!
Comment 13 Mirko Tietgen 2016-07-29 15:22:12 UTC
libbytes-random-secure-perl is in the Koha stable and olstable repositories now.
Comment 14 Marcel de Rooy 2016-08-08 08:03:45 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2016-08-08 08:03:50 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2016-08-08 08:03:54 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2016-08-08 08:08:37 UTC
Rebased for opac-memberentry and Dependencies.pm
Comment 18 Marc Véron 2016-08-08 08:39:16 UTC Comment hidden (obsolete)
Comment 19 Marc Véron 2016-08-08 08:40:14 UTC Comment hidden (obsolete)
Comment 20 Marc Véron 2016-08-08 08:41:00 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2016-08-08 11:20:15 UTC
Marcel, why did you choose 0.29? 0.28 is packaged for jessie.

I also think that we need to fill debian/control for new deps.
Mirko, could you confirm that you expect from devs to update it?
Comment 22 Marcel de Rooy 2016-08-08 11:24:45 UTC
(In reply to Jonathan Druart from comment #21)
> Marcel, why did you choose 0.29? 0.28 is packaged for jessie.
> 
> I also think that we need to fill debian/control for new deps.
> Mirko, could you confirm that you expect from devs to update it?

I would say that the PM edits debian/control, not individual devs ..
Comment 23 Marcel de Rooy 2016-08-08 11:26:09 UTC Comment hidden (obsolete)
Comment 24 Mirko Tietgen 2016-08-08 11:29:29 UTC
(In reply to Jonathan Druart from comment #21)
> Marcel, why did you choose 0.29? 0.28 is packaged for jessie.
> 
> I also think that we need to fill debian/control for new deps.
> Mirko, could you confirm that you expect from devs to update it?


In theory, yes. but since the file is supposed to be generated, not edited manually, and it will clash with other bugs with dependencies, I think it's easier if I do it once something got pushed. I'm working on automating that.
Comment 25 Jonathan Druart 2016-08-08 12:09:20 UTC
Created attachment 54152 [details] [review]
Bug 16929: Prevent opac-memberentry waiting for random chars

Move calls to WWW::CSRF to Koha::Token.
Send a safe random string to WWW::CSRF instead of letting CSRF make a
blocking call to Bytes::Random::Secure. If your server has not enough
entropy, opac-memberentry will hang waiting for more characters in
dev/random. Koha::Token uses Bytes::Random::Secure with the NonBlocking
flag.

Test plan:
[1] Do not yet apply this patch.
[2] If your server has not enough entropy, calling opac-memberentry may
    take a while. But this not may be the case for you (no worries).
[3] Apply this patch.
[4] Verify that opac-memberentry still works as expected.
[5] Run t/Token.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Yes, my server had entropy trouble (reason for finding the problem).
This patch resolves the delay.

Tested all 3 patches together, works as expected.
Signed-off-by: Marc <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 26 Jonathan Druart 2016-08-08 12:09:24 UTC
Created attachment 54153 [details] [review]
Bug 16929: [QA Follow-up] Shortcut methods and use statements

Resolves the following comments:

I'd prefer to see a generate_csrf method than a CSRF flag.
It'd be better to use instead of require the 2 modules.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Marc <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 27 Jonathan Druart 2016-08-08 12:09:27 UTC Comment hidden (obsolete)
Comment 28 Marcel de Rooy 2016-08-09 14:39:05 UTC
Created attachment 54216 [details] [review]
Bug 16929: [QA Follow-up] Add dependency for Bytes::Random::Secure

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Marc <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 29 Marcel de Rooy 2016-08-09 14:39:39 UTC
Rebased last patch for Dependencies.pm (trivial)
Comment 30 Kyle M Hall 2016-08-10 13:29:37 UTC
Pushed to master for 16.11, thanks Marcel!
Comment 31 Mirko Tietgen 2016-08-11 09:33:58 UTC
The corresponding control file update is at bug 17084: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17084#c1
Comment 32 Mason James 2017-01-31 09:24:46 UTC
Pushed to 16.05.x, for 16.05.09 release