Bug 33967 - REMOTE_ADDR incorrect in plack.log when run behind a proxy
Summary: REMOTE_ADDR incorrect in plack.log when run behind a proxy
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 23068
Blocks:
  Show dependency treegraph
 
Reported: 2023-06-09 03:41 UTC by David Cook
Modified: 2023-12-28 20:47 UTC (History)
1 user (show)

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


Attachments
Bug 33967: Fix SetEnv middleware to preserve $env integrity (2.03 KB, patch)
2023-06-09 04:16 UTC, David Cook
Details | Diff | Splinter Review
Bug 33967: Fix SetEnv middleware to preserve $env integrity (2.08 KB, patch)
2023-06-09 05:22 UTC, David Nind
Details | Diff | Splinter Review
Bug 33967: Add unit test to ensure SetEnv doesn't compromise $env (3.18 KB, patch)
2023-06-09 06:48 UTC, David Cook
Details | Diff | Splinter Review
Bug 33967: Add unit test to ensure SetEnv doesn't compromise $env (3.22 KB, patch)
2023-06-09 06:49 UTC, David Cook
Details | Diff | Splinter Review
Bug 33967: Fix SetEnv middleware to preserve $env integrity (2.13 KB, patch)
2023-06-09 11:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33967: Add unit test to ensure SetEnv doesn't compromise $env (3.27 KB, patch)
2023-06-09 11:39 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2023-06-09 03:41:01 UTC
As I note in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23068#c54 the Koha::Middleware::SetEnv breaks the flow of $env through the Plack middleware wrapper chain.

This means that post-processing middleware that happen after Koha::Middleware::SetEnv (e.g. Plack::Middleware::AccessLog) will use the wrong $env variable (ie the $env variable that was being used before Koha::Middleware::SetEnv was run in the pre-process middleware phase).

By retaining the original $env hashref in Koha::Middleware::SetEnv, we should be able to preserve the referential integrity. 

Going to give a patch a go now...
Comment 1 David Cook 2023-06-09 04:16:59 UTC
Created attachment 152227 [details] [review]
Bug 33967: Fix SetEnv middleware to preserve $env integrity

This patch fixes the Koha::Middleware::SetEnv, so that it
preserves the referential integrity of the $env hash
reference as it passes through the pre-process and post-process
stages of middleware wrapped around the core Koha Plack application.

Test plan:
0a. Apply the patch
0b. vi /etc/koha/sites/kohadev/koha-conf.xml
0c. Uncomment "koha_trusted_proxies"
0d. Add "172.0.0.0/8" to the end of the "koha_trusted_proxies"
0e. koha-plack --restart kohadev
1. vi /etc/apache2/sites-enabled/kohadev.conf
2. Add the following to the OPAC virtualhost:
SetEnv OVERRIDE_SYSPREF_LibraryName "The Best, Truly the Best, Koha Library"
RequestHeader add X-Koha-SetEnv "OVERRIDE_SYSPREF_LibraryName The Best\, Truly the Best\, Koha Library"
RequestHeader add X-Forwarded-For "192.168.100.100"
3. service apache2 restart
4. tail -f /var/log/koha/kohadev/plack.log
5. Go to http://localhost:8080/
6. Note that the plack.log lists 192.168.100.100 as the client IP address
7. Note that the title of the OPAC webpage is
"The Best, Truly the Best, Koha Library Library catalog"
Comment 2 David Nind 2023-06-09 05:22:36 UTC
Created attachment 152228 [details] [review]
Bug 33967: Fix SetEnv middleware to preserve $env integrity

This patch fixes the Koha::Middleware::SetEnv, so that it
preserves the referential integrity of the $env hash
reference as it passes through the pre-process and post-process
stages of middleware wrapped around the core Koha Plack application.

Test plan:
0a. Apply the patch
0b. vi /etc/koha/sites/kohadev/koha-conf.xml
0c. Uncomment "koha_trusted_proxies"
0d. Add "172.0.0.0/8" to the end of the "koha_trusted_proxies"
0e. koha-plack --restart kohadev
1. vi /etc/apache2/sites-enabled/kohadev.conf
2. Add the following to the OPAC virtualhost:
SetEnv OVERRIDE_SYSPREF_LibraryName "The Best, Truly the Best, Koha Library"
RequestHeader add X-Koha-SetEnv "OVERRIDE_SYSPREF_LibraryName The Best\, Truly the Best\, Koha Library"
RequestHeader add X-Forwarded-For "192.168.100.100"
3. service apache2 restart
4. tail -f /var/log/koha/kohadev/plack.log
5. Go to http://localhost:8080/
6. Note that the plack.log lists 192.168.100.100 as the client IP address
7. Note that the title of the OPAC webpage is
"The Best, Truly the Best, Koha Library Library catalog"

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 David Cook 2023-06-09 06:48:19 UTC
Created attachment 152229 [details] [review]
Bug 33967: Add unit test to ensure SetEnv doesn't compromise $env

This patch includes a unit test which checks the memory address
of the $env hashref in middlewares before and after
Koha::Middleware::Env is applied.

It succeeds when the same $env is maintained before and after
this middleware. It fails if the $env has been overwritten with
a new hashref (as evidenced by the new memory address for the
hash).
Comment 4 David Cook 2023-06-09 06:49:44 UTC
Created attachment 152230 [details] [review]
Bug 33967: Add unit test to ensure SetEnv doesn't compromise $env

This patch includes a unit test which checks the memory address
of the $env hashref in middlewares before and after
Koha::Middleware::Env is applied.

It succeeds when the same $env is maintained before and after
this middleware. It fails if the $env has been overwritten with
a new hashref (as evidenced by the new memory address for the
hash).
Comment 5 David Cook 2023-06-09 06:51:22 UTC
If you want to verify the unit test works, you can either run it before applying the first patch, or you can compromise Koha/Middleware/SetEnv.pm by adding the following:
$env = {}; 

right before the final return in the call sub.
Comment 6 Kyle M Hall 2023-06-09 11:39:15 UTC
Created attachment 152241 [details] [review]
Bug 33967: Fix SetEnv middleware to preserve $env integrity

This patch fixes the Koha::Middleware::SetEnv, so that it
preserves the referential integrity of the $env hash
reference as it passes through the pre-process and post-process
stages of middleware wrapped around the core Koha Plack application.

Test plan:
0a. Apply the patch
0b. vi /etc/koha/sites/kohadev/koha-conf.xml
0c. Uncomment "koha_trusted_proxies"
0d. Add "172.0.0.0/8" to the end of the "koha_trusted_proxies"
0e. koha-plack --restart kohadev
1. vi /etc/apache2/sites-enabled/kohadev.conf
2. Add the following to the OPAC virtualhost:
SetEnv OVERRIDE_SYSPREF_LibraryName "The Best, Truly the Best, Koha Library"
RequestHeader add X-Koha-SetEnv "OVERRIDE_SYSPREF_LibraryName The Best\, Truly the Best\, Koha Library"
RequestHeader add X-Forwarded-For "192.168.100.100"
3. service apache2 restart
4. tail -f /var/log/koha/kohadev/plack.log
5. Go to http://localhost:8080/
6. Note that the plack.log lists 192.168.100.100 as the client IP address
7. Note that the title of the OPAC webpage is
"The Best, Truly the Best, Koha Library Library catalog"

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Kyle M Hall 2023-06-09 11:39:33 UTC
Created attachment 152242 [details] [review]
Bug 33967: Add unit test to ensure SetEnv doesn't compromise $env

This patch includes a unit test which checks the memory address
of the $env hashref in middlewares before and after
Koha::Middleware::Env is applied.

It succeeds when the same $env is maintained before and after
this middleware. It fails if the $env has been overwritten with
a new hashref (as evidenced by the new memory address for the
hash).

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Tomás Cohen Arazi 2023-06-09 11:48:27 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 9 Martin Renvoize 2023-07-12 08:24:34 UTC
Thanks for all the hard work!

Pushed to 23.05.x for the next release
Comment 10 Matt Blenkinsop 2023-07-12 09:41:53 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x