Bug 23068 - Add ability for Koha to handle X-Forwarded-For headers so REMOTE_ADDR features work behind a proxy
Summary: Add ability for Koha to handle X-Forwarded-For headers so REMOTE_ADDR feature...
Status: CLOSED 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: Kyle M Hall
QA Contact: Martin Renvoize
URL:
Keywords: dependency
: 21477 (view as bug list)
Depends on:
Blocks: 24538 25950 33967
  Show dependency treegraph
 
Reported: 2019-06-06 19:12 UTC by Kyle M Hall
Modified: 2023-06-12 23:55 UTC (History)
8 users (show)

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


Attachments
Bug 23068: [DO NOT PUSH] Add debugging to C4::Context (728 bytes, patch)
2019-06-06 19:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23068 - Add ability for Koha to handle X-Forwarded-For headers so REMOTE_ADDR features work behind a proxy (2.19 KB, patch)
2019-06-06 19:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23068: Allow the list of proxy IPs to be set in the koha-conf.xml file (2.02 KB, patch)
2019-06-13 17:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23068: Add ability for Koha to handle X-Forwarded-For headers so REMOTE_ADDR features work behind a proxy (2.24 KB, patch)
2019-07-02 06:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23068: Allow the list of proxy IPs to be set in the koha-conf.xml file (2.07 KB, patch)
2019-07-02 06:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23068: Allow the list of proxy IPs to be set in the koha-conf.xml file (2.14 KB, patch)
2019-07-10 15:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23068: Add ability for Koha to handle X-Forwarded-For headers so REMOTE_ADDR features work behind a proxy (11.32 KB, patch)
2019-08-23 18:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23068: [DO NOT PUSH] Debugging code for test plan (646 bytes, patch)
2019-08-23 18:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23068: Add ability for Koha to handle X-Forwarded-For headers so REMOTE_ADDR features work behind a proxy (11.16 KB, patch)
2019-08-23 18:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23068: [DO NOT PUSH] Debugging code for test plan (646 bytes, patch)
2019-08-23 18:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23068: Add ability for Koha to handle X-Forwarded-For headers so REMOTE_ADDR features work behind a proxy (8.15 KB, patch)
2019-10-28 13:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23068: [DO NOT PUSH] Debugging code for test plan (695 bytes, patch)
2019-10-28 13:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23068: Add ability for Koha to handle X-Forwarded-For headers so REMOTE_ADDR features work behind a proxy (8.20 KB, patch)
2019-10-29 14:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23068: Add ability for Koha to handle X-Forwarded-For headers so REMOTE_ADDR features work behind a proxy (8.76 KB, patch)
2019-10-29 16:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23068: Add ability for Koha to handle X-Forwarded-For headers so REMOTE_ADDR features work behind a proxy (8.81 KB, patch)
2019-10-31 16:07 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2019-06-06 19:12:17 UTC
Koha has a number of features that rely on knowing the IP address of the connecting client. If that server is behind a proxy these features do not work. We can use Plack::Middleware::RealIP to automatically convert the X-Forwarded-For header into the REMOTE_ADDR environment variable so no Koha code need be modified. 

A future enhancement could be to take advantage of the module's trusted_proxy setting to allow Koha to use these feature even after going through *multiple* proxies. This would require being able to read configuration from the plack.psgi file and is beyond the scope of this enhancement.
Comment 1 Kyle M Hall 2019-06-06 19:35:52 UTC
Actually, we can use an environment variable so I'm increasing the scope to include setting the list of proxies!
Comment 2 Kyle M Hall 2019-06-06 19:42:31 UTC
Created attachment 90395 [details] [review]
Bug 23068: [DO NOT PUSH] Add debugging to C4::Context
Comment 3 Kyle M Hall 2019-06-06 19:42:47 UTC
Created attachment 90396 [details] [review]
Bug 23068 - Add ability for Koha to handle X-Forwarded-For headers so REMOTE_ADDR features work behind a proxy

Koha has a number of features that rely on knowing the IP address of the connecting client. If that server is behind a proxy these features do not work. We can use Plack::Middleware::RealIP to automatically convert the X-Forwarded-For header into the REMOTE_ADDR environment variable so no Koha code need be modified.

We can take advantage of the module's trusted_proxy setting to allow Koha to use these feature even after going through *multiple* proxies.

TEST PLAN:
1) Apply this patch set
2) Install Plack::Middleware::RealIP via cpanm or your favorite utility
3) Update your plack.psgi with the changes you find in this patch set ( this process differs based on your testing environment )
4) Restart plack
5) Tail the plack error log for your instance
6) Use curl to access the OPAC, adding an X-Forwarded-For header: curl --header "X-Forwarded-For: 32.32.32.32" http://127.0.0.1:8080
7) Note the logs output this address if you are unproxied
8) If you are proxied, restart plack using a command like below, where the ip you see in the logs is what you put in the environment variable:
    KOHA_TRUSTED_PROXIES="172.22.0.1 1.1.1.1" koha-plack --restart kohadev
9) Repeat step 6
7) You should now see 32.32.32.32 as the remote address in your logs!
Comment 4 Kyle M Hall 2019-06-13 17:21:52 UTC
*** Bug 21477 has been marked as a duplicate of this bug. ***
Comment 5 Kyle M Hall 2019-06-13 17:46:40 UTC
Created attachment 90582 [details] [review]
Bug 23068: Allow the list of proxy IPs to be set in the koha-conf.xml file
Comment 6 Martin Renvoize 2019-07-02 06:13:56 UTC
QA Questions
1) Does this work with the Mojo derived API's too?
2) What if Koha is running under CGI and not plack - yes.. there are still some out there :P
Comment 7 Martin Renvoize 2019-07-02 06:16:12 UTC
Created attachment 91173 [details] [review]
Bug 23068: Add ability for Koha to handle X-Forwarded-For headers so REMOTE_ADDR features work behind a proxy

Koha has a number of features that rely on knowing the IP address of the connecting client. If that server is behind a proxy these features do not work. We can use Plack::Middleware::RealIP to automatically convert the X-Forwarded-For header into the REMOTE_ADDR environment variable so no Koha code need be modified.

We can take advantage of the module's trusted_proxy setting to allow Koha to use these feature even after going through *multiple* proxies.

TEST PLAN:
1) Apply this patch set
2) Install Plack::Middleware::RealIP via cpanm or your favorite utility
3) Update your plack.psgi with the changes you find in this patch set ( this process differs based on your testing environment )
4) Restart plack
5) Tail the plack error log for your instance
6) Use curl to access the OPAC, adding an X-Forwarded-For header: curl --header "X-Forwarded-For: 32.32.32.32" http://127.0.0.1:8080
7) Note the logs output this address if you are unproxied
8) If you are proxied, restart plack using a command like below, where the ip you see in the logs is what you put in the environment variable:
    KOHA_TRUSTED_PROXIES="172.22.0.1 1.1.1.1" koha-plack --restart kohadev
9) Repeat step 6
7) You should now see 32.32.32.32 as the remote address in your logs!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2019-07-02 06:16:15 UTC
Created attachment 91174 [details] [review]
Bug 23068: Allow the list of proxy IPs to be set in the koha-conf.xml file

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2019-07-02 06:17:55 UTC
Fixed up the commit line to get the QA scripts happy and signing off.. I'll leave my two QA questions for a QA person to follow up but I didn't see any point in holding up SO.
Comment 10 Kyle M Hall 2019-07-10 15:24:24 UTC
Created attachment 91447 [details] [review]
Bug 23068: Allow the list of proxy IPs to be set in the koha-conf.xml file

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Kyle M Hall 2019-07-10 18:05:39 UTC
Found a problem. The middleware is working via plack so we can the real ip address when we are on a script that uses plack. When we visit a script that doesn't use plack, this code doesn't get executed and so Koha sees the proxy ip instead.
And Koha does what it always does when a session changes ip addresses, it forces a re-login
Comment 12 Kyle M Hall 2019-08-23 18:23:15 UTC
Created attachment 92461 [details] [review]
Bug 23068: Add ability for Koha to handle X-Forwarded-For headers so REMOTE_ADDR features work behind a proxy

Koha has a number of features that rely on knowing the IP address of the connecting client. If that server is behind a proxy these features do not work.
This patch adds a module to automatically convert the X-Forwarded-For header into the REMOTE_ADDR environment variable for both CGI and Plack processes.

TEST PLAN:
1) Apply this patch set
2) Install Plack::Middleware::RealIP via cpanm or your favorite utility
3) Update your plack.psgi with the changes you find in this patch set ( this process differs based on your testing environment )
4) Restart plack
5) Tail the plack error log for your instance
6) Use curl to access the OPAC, adding an X-Forwarded-For header: curl --header "X-Forwarded-For: 32.32.32.32" http://127.0.0.1:8080
7) Note the logs output this address if you are unproxied
8) If you are proxied, restart plack using a command like below, where the ip you see in the logs ("REAL IP) is what you put in the koha conf:
    <koha_trusted_proxies>172.22.0.1 1.1.1.1</koha_trusted_proxies>
9) Restart all the things!
10) Repeat step 6
11) You should now see "REAL IP: 32.32.32.32" in the plack logs as the remote address in your plack-error.log logs!
12) Disable plack so you are running in cgi mode, repeat step 6 again
13) You should see "REAL IP: 32.32.32.32" as the remove address in your opac-error.log logs!
Comment 13 Kyle M Hall 2019-08-23 18:23:27 UTC
Created attachment 92462 [details] [review]
Bug 23068: [DO NOT PUSH] Debugging code for test plan
Comment 14 Kyle M Hall 2019-08-23 18:24:30 UTC
Created attachment 92463 [details] [review]
Bug 23068: Add ability for Koha to handle X-Forwarded-For headers so REMOTE_ADDR features work behind a proxy

Koha has a number of features that rely on knowing the IP address of the connecting client. If that server is behind a proxy these features do not work.
This patch adds a module to automatically convert the X-Forwarded-For header into the REMOTE_ADDR environment variable for both CGI and Plack processes.

TEST PLAN:
1) Apply this patch set
2) Install Plack::Middleware::RealIP via cpanm or your favorite utility
3) Update your plack.psgi with the changes you find in this patch set ( this process differs based on your testing environment )
4) Restart plack
5) Tail the plack error log for your instance
6) Use curl to access the OPAC, adding an X-Forwarded-For header: curl --header "X-Forwarded-For: 32.32.32.32" http://127.0.0.1:8080
7) Note the logs output this address if you are unproxied
8) If you are proxied, restart plack using a command like below, where the ip you see in the logs ("REAL IP) is what you put in the koha conf:
    <koha_trusted_proxies>172.22.0.1 1.1.1.1</koha_trusted_proxies>
9) Restart all the things!
10) Repeat step 6
11) You should now see "REAL IP: 32.32.32.32" in the plack logs as the remote address in your plack-error.log logs!
12) Disable plack so you are running in cgi mode, repeat step 6 again
13) You should see "REAL IP: 32.32.32.32" as the remove address in your opac-error.log logs!
Comment 15 Kyle M Hall 2019-08-23 18:24:50 UTC
Created attachment 92464 [details] [review]
Bug 23068: [DO NOT PUSH] Debugging code for test plan
Comment 16 Kyle M Hall 2019-10-28 13:37:35 UTC
Created attachment 94807 [details] [review]
Bug 23068: Add ability for Koha to handle X-Forwarded-For headers so REMOTE_ADDR features work behind a proxy

Koha has a number of features that rely on knowing the IP address of the connecting client. If that server is behind a proxy these features do not work.
This patch adds a module to automatically convert the X-Forwarded-For header into the REMOTE_ADDR environment variable for both CGI and Plack processes.

TEST PLAN:
1) Apply this patch set
2) Install Plack::Middleware::RealIP via cpanm or your favorite utility
3) Update your plack.psgi with the changes you find in this patch set ( this process differs based on your testing environment )
4) Restart plack
5) Tail the plack error log for your instance
6) Use curl to access the OPAC, adding an X-Forwarded-For header: curl --header "X-Forwarded-For: 32.32.32.32" http://127.0.0.1:8080
7) Note the logs output this address if you are unproxied
8) If you are proxied, restart plack using a command like below, where the ip you see in the logs ("REAL IP) is what you put in the koha conf:
    <koha_trusted_proxies>172.22.0.1 1.1.1.1</koha_trusted_proxies>
9) Restart all the things!
10) Repeat step 6
11) You should now see "REAL IP: 32.32.32.32" in the plack logs as the remote address in your plack-error.log logs!
12) Disable plack so you are running in cgi mode, repeat step 6 again
13) You should see "REAL IP: 32.32.32.32" as the remove address in your opac-error.log logs!

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Comment 17 Kyle M Hall 2019-10-28 13:37:48 UTC
Created attachment 94808 [details] [review]
Bug 23068: [DO NOT PUSH] Debugging code for test plan

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Comment 18 David Cook 2019-10-29 08:07:28 UTC
(In reply to Martin Renvoize from comment #6)
> QA Questions
> 1) Does this work with the Mojo derived API's too?
> 2) What if Koha is running under CGI and not plack - yes.. there are still
> some out there :P

I have these questions too, although I've been using CGI behind (sometimes multiple) reverse proxies for a long time, so I've already hacked around where necessary to support X-Forwarded-For. It's not really shareable code though as it's very specific.
Comment 19 David Cook 2019-10-29 08:14:56 UTC
Comment on attachment 94807 [details] [review]
Bug 23068: Add ability for Koha to handle X-Forwarded-For headers so REMOTE_ADDR features work behind a proxy

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

::: C4/Context.pm
@@ +1107,5 @@
> +
> +=cut
> +
> +sub set_remote_address {
> +    if ( C4::Context->config('koha_trusted_proxies') ) {

Wouldn't this still be called under Plack?

Couldn't we just look to see if it's Plack via environmental variables and only do this for CGI?
Comment 20 Kyle M Hall 2019-10-29 12:52:23 UTC
(In reply to David Cook from comment #19)
> Comment on attachment 94807 [details] [review] [review]
> Bug 23068: Add ability for Koha to handle X-Forwarded-For headers so
> REMOTE_ADDR features work behind a proxy
> 
> Review of attachment 94807 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Context.pm
> @@ +1107,5 @@
> > +
> > +=cut
> > +
> > +sub set_remote_address {
> > +    if ( C4::Context->config('koha_trusted_proxies') ) {
> 
> Wouldn't this still be called under Plack?
> 
> Couldn't we just look to see if it's Plack via environmental variables and
> only do this for CGI?

That's a good thought. Is there an environment variable that is set for scripts running under plack?
Comment 21 Kyle M Hall 2019-10-29 13:22:06 UTC
(In reply to David Cook from comment #19)
> Comment on attachment 94807 [details] [review] [review]
> Bug 23068: Add ability for Koha to handle X-Forwarded-For headers so
> REMOTE_ADDR features work behind a proxy
> 
> Review of attachment 94807 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Context.pm
> @@ +1107,5 @@
> > +
> > +=cut
> > +
> > +sub set_remote_address {
> > +    if ( C4::Context->config('koha_trusted_proxies') ) {
> 
> Wouldn't this still be called under Plack?
> 
> Couldn't we just look to see if it's Plack via environmental variables and
> only do this for CGI?

Actually, it only gets called under plack at startup, since C4::Auth is loaded at that time. So, yes, we could do that, but it doesn't actually hurt performance by any meaningful metric!
Comment 22 Kyle M Hall 2019-10-29 14:36:41 UTC
(In reply to Martin Renvoize from comment #6)
> QA Questions
> 1) Does this work with the Mojo derived API's too?

Yes it does!

> 2) What if Koha is running under CGI and not plack - yes.. there are still
> some out there :P

It does indeed! I had to engineer it specifically to work under CGI as well as we still have a handful of scripts that we still run under CGI instead of plack.
Comment 23 Martin Renvoize 2019-10-29 14:58:36 UTC
Created attachment 94838 [details] [review]
Bug 23068: Add ability for Koha to handle X-Forwarded-For headers so REMOTE_ADDR features work behind a proxy

Koha has a number of features that rely on knowing the IP address of the connecting client. If that server is behind a proxy these features do not work.
This patch adds a module to automatically convert the X-Forwarded-For header into the REMOTE_ADDR environment variable for both CGI and Plack processes.

TEST PLAN:
1) Apply this patch set
2) Install Plack::Middleware::RealIP via cpanm or your favorite utility
3) Update your plack.psgi with the changes you find in this patch set ( this process differs based on your testing environment )
4) Restart plack
5) Tail the plack error log for your instance
6) Use curl to access the OPAC, adding an X-Forwarded-For header: curl --header "X-Forwarded-For: 32.32.32.32" http://127.0.0.1:8080
7) Note the logs output this address if you are unproxied
8) If you are proxied, restart plack using a command like below, where the ip you see in the logs ("REAL IP) is what you put in the koha conf:
    <koha_trusted_proxies>172.22.0.1 1.1.1.1</koha_trusted_proxies>
9) Restart all the things!
10) Repeat step 6
11) You should now see "REAL IP: 32.32.32.32" in the plack logs as the remote address in your plack-error.log logs!
12) Disable plack so you are running in cgi mode, repeat step 6 again
13) You should see "REAL IP: 32.32.32.32" as the remove address in your opac-error.log logs!

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2019-10-29 15:00:23 UTC
Sorry dude.. lots of QA script failures.. and I think this also introduces a new dependancy that will need to be listed: 

 FAIL   C4/Auth.pm
   FAIL   valid
                Can't locate object method "set_remote_address" via package "C4::Context"

 OK     C4/Context.pm
 FAIL   Koha/Middleware/RealIP.pm
   FAIL   critic
                Variable declared in conditional statement at line 53, column 9. Declare variables outside of the condition.
   FAIL   pod
                *** WARNING:
                =head3 without preceding higher level

                 in file Koha/Middleware/RealIP.pm

                *** WARNING:
                =head3 without preceding higher level

                 in file Koha/Middleware/RealIP.pm

                *** WARNING:
                =head3 without preceding higher level

                 in file Koha/Middleware/RealIP.pm

                *** WARNING:
                =head3 without preceding higher level

                 in file Koha/Middleware/RealIP.pm

   FAIL   pod coverage
                POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666
   FAIL   valid
                Can't locate Net/Netmask.pm in @INC (you may need to install the Net::Netmask module) (@INC contains: /home/martin/kohaclone /home/martin/kohaclone/C4/SIP /home/martin/qa-test-tools /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/local/share/perl/5.24.1 /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base)

 OK     etc/koha-conf.xml

Processing additional checks OK!
Comment 25 Kyle M Hall 2019-10-29 16:26:06 UTC
Created attachment 94839 [details] [review]
Bug 23068: Add ability for Koha to handle X-Forwarded-For headers so REMOTE_ADDR features work behind a proxy

Koha has a number of features that rely on knowing the IP address of the connecting client. If that server is behind a proxy these features do not work.
This patch adds a module to automatically convert the X-Forwarded-For header into the REMOTE_ADDR environment variable for both CGI and Plack processes.

TEST PLAN:
1) Apply this patch set
2) Install Plack::Middleware::RealIP via cpanm or your favorite utility
3) Update your plack.psgi with the changes you find in this patch set ( this process differs based on your testing environment )
4) Restart plack
5) Tail the plack error log for your instance
6) Use curl to access the OPAC, adding an X-Forwarded-For header: curl --header "X-Forwarded-For: 32.32.32.32" http://127.0.0.1:8080
7) Note the logs output this address if you are unproxied
8) If you are proxied, restart plack using a command like below, where the ip you see in the logs ("REAL IP) is what you put in the koha conf:
    <koha_trusted_proxies>172.22.0.1 1.1.1.1</koha_trusted_proxies>
9) Restart all the things!
10) Repeat step 6
11) You should now see "REAL IP: 32.32.32.32" in the plack logs as the remote address in your plack-error.log logs!
12) Disable plack so you are running in cgi mode, repeat step 6 again
13) You should see "REAL IP: 32.32.32.32" as the remove address in your opac-error.log logs!

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Comment 26 David Cook 2019-10-29 23:44:45 UTC
(In reply to Kyle M Hall from comment #21)
> Actually, it only gets called under plack at startup, since C4::Auth is
> loaded at that time. So, yes, we could do that, but it doesn't actually hurt
> performance by any meaningful metric!

I was wondering about that a bit. That's a good point hehe.

In any case, there should be a number of "psgi.*" environmental variables: https://metacpan.org/pod/PSGI#The-Environment.
Comment 27 Martin Renvoize 2019-10-31 16:07:02 UTC
Created attachment 94919 [details] [review]
Bug 23068: Add ability for Koha to handle X-Forwarded-For headers so REMOTE_ADDR features work behind a proxy

Koha has a number of features that rely on knowing the IP address of the connecting client. If that server is behind a proxy these features do not work.
This patch adds a module to automatically convert the X-Forwarded-For header into the REMOTE_ADDR environment variable for both CGI and Plack processes.

TEST PLAN:
1) Apply this patch set
2) Install Plack::Middleware::RealIP via cpanm or your favorite utility
3) Update your plack.psgi with the changes you find in this patch set ( this process differs based on your testing environment )
4) Restart plack
5) Tail the plack error log for your instance
6) Use curl to access the OPAC, adding an X-Forwarded-For header: curl --header "X-Forwarded-For: 32.32.32.32" http://127.0.0.1:8080
7) Note the logs output this address if you are unproxied
8) If you are proxied, restart plack using a command like below, where the ip you see in the logs ("REAL IP) is what you put in the koha conf:
    <koha_trusted_proxies>172.22.0.1 1.1.1.1</koha_trusted_proxies>
9) Restart all the things!
10) Repeat step 6
11) You should now see "REAL IP: 32.32.32.32" in the plack logs as the remote address in your plack-error.log logs!
12) Disable plack so you are running in cgi mode, repeat step 6 again
13) You should see "REAL IP: 32.32.32.32" as the remove address in your opac-error.log logs!

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Martin Renvoize 2019-10-31 16:09:21 UTC
Nice work Kyle, all QA issues fixed.

Works well, dependancies is available in D8 by default so no need to worry about packaging.

Passing QA :)
Comment 29 Martin Renvoize 2019-10-31 16:10:43 UTC
Nice work!

Pushed to master for 19.11.00
Comment 30 David Cook 2019-11-04 23:58:46 UTC
Yay! Nice one, Kyle!
Comment 31 Ed Veal 2019-11-13 23:47:55 UTC
Great job Kyle.
Comment 32 Victor Grousset/tuxayo 2019-11-18 18:37:28 UTC
KohaDevBoxes (even new ones) and other development environments based on Debian or Ubuntu need to install the package "libnet-netmask-perl"
Comment 33 David Cook 2019-11-19 07:51:20 UTC
(In reply to Victor Grousset/tuxayo from comment #32)
> KohaDevBoxes (even new ones) and other development environments based on
> Debian or Ubuntu need to install the package "libnet-netmask-perl"

I've often wondered when the debian control files get updated with the dependencies...
Comment 34 Martin Renvoize 2019-11-19 08:12:34 UTC
(In reply to Victor Grousset/tuxayo from comment #32)
> KohaDevBoxes (even new ones) and other development environments based on
> Debian or Ubuntu need to install the package "libnet-netmask-perl"

Correct, it has been escalated to a 'required' dependancy. This is because for plack we need to load it before we load C4::Context and as such we can't use it optionally as initially thought.
Comment 35 Martin Renvoize 2019-11-19 08:17:51 UTC
(In reply to David Cook from comment #33)
> (In reply to Victor Grousset/tuxayo from comment #32)
> > KohaDevBoxes (even new ones) and other development environments based on
> > Debian or Ubuntu need to install the package "libnet-netmask-perl"
> 
> I've often wondered when the debian control files get updated with the
> dependencies...

The control files were updated almost immediately.. The process is to add a dependency to C4/Installer/PerlDependancies.pm and then push that up to master.

The evening the nightly build bot will spot the new dependency and add it to the control file and send the patch to bug 17084 which the release manager should be watching.

The next day, the release manager will do a quick QA on the resultant file and push the patch as necessary.

So, in this case this took a sum total of a day to get the new dependency into the debian control files (there was indeed a day of Jenkins getting upset about the missing dependency too as the packages were failing to build).

I can't entirely remember how the biblibre sandboxes work (koha-testing-docker, which koha-sandboxes-docker depends upon for it's base koha image, has already been updated).  Do you now re-run apt install koha-perldeps or anything regularly?  That should have caught the debian control change.. also, I presume if you are running such a command then you're also tracking the nightly repository to pickup the very latest changes.

Hope that helps, 

Martin
Comment 36 David Cook 2019-11-20 00:28:23 UTC
(In reply to Martin Renvoize from comment #34)
> Correct, it has been escalated to a 'required' dependancy. This is because
> for plack we need to load it before we load C4::Context and as such we can't
> use it optionally as initially thought.

I was thinking we could actually make good use of Net::Netmask in other parts of Koha as well. I'd love to use it for processing ILS-DI:AuthorizedIPs (could also use it for OpacSuppressionByIPRange, RestrictedPageLocalIPs, and surely other places that don't come to mind yet...).

So it being a required dependency could be a motivator for updating those too. *puts it on his infinite todo list*.
Comment 37 Victor Grousset/tuxayo 2019-11-20 14:01:54 UTC
> Do you now re-run apt install koha-perldeps or anything regularly?  That should have caught the debian control change.. also, I presume if you are running such a command then you're also tracking the nightly repository to pickup the very latest changes.

It was on a KohaDevBox, I guess the nightly repo wasn't up to date yet.
Comment 38 David Cook 2020-01-30 01:35:39 UTC
I just want to say that this is working beautifully in 19.11. Thanks a lot, Kyle!
Comment 39 David Cook 2020-01-30 03:05:45 UTC
I've been thinking about this one more, and I just want to say again well done, Kyle! 

I like how it can handle multiple proxies, and the trust model so that end users can't hack REMOTE_USER using their own user-supplied headers. 

It's too bad there are no unit tests for Koha::Middleware::RealIP. I ended up writing a very basic one here just to test that it worked for multiple proxies.
Comment 40 David Cook 2020-01-30 03:11:42 UTC
One last note for people interested in this feature, Net::Netmask lets you define IP addresses in a number of ways. If you wanted to trust a whole local subnet, you could do 192.168.0.0/16, or something like that. 

(If you're using a proxy like an AWS Elastic Load Balancer, which is dynamically assigned an IP address chosen by AWS, you could specify the subnets for your ELBs, and then wouldn't have to worry about IP addresses changing and keeping your koha-conf.xml up to date.)
Comment 41 David Cook 2020-01-30 04:04:53 UTC
Ok found a little problem and I'll open up a separate bug report for it.
Comment 42 Martin Renvoize 2020-01-30 07:42:17 UTC
(In reply to David Cook from comment #39)
> It's too bad there are no unit tests for Koha::Middleware::RealIP. I ended
> up writing a very basic one here just to test that it worked for multiple
> proxies.

Agreed, but I didn't want to hold up this vital development and we weren't at all sure how to write such a test.. as it sounds like you've got the foundations for a test written, please submit it upstream ;)
Comment 43 David Cook 2020-01-30 23:44:40 UTC
(In reply to Martin Renvoize from comment #42)
> (In reply to David Cook from comment #39)
> > It's too bad there are no unit tests for Koha::Middleware::RealIP. I ended
> > up writing a very basic one here just to test that it worked for multiple
> > proxies.
> 
> Agreed, but I didn't want to hold up this vital development and we weren't
> at all sure how to write such a test.. as it sounds like you've got the
> foundations for a test written, please submit it upstream ;)

That makes sense. I really am loving this development overall!

I'll do a patch for https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24538 and write a test for it. It won't be total coverage I suppose, but it should be enough to test the change.
Comment 44 Martin Renvoize 2020-02-01 08:42:36 UTC
Thanks David, that's great news.
Comment 45 David Cook 2020-02-18 09:26:50 UTC
(In reply to Martin Renvoize from comment #44)
> Thanks David, that's great news.

Finally getting around to this...

Have to leave for the night but the following isn't working for some reason:

t::lib::Mocks::mock_preference('koha_trusted_proxies', '1.1.1.1');

Will have to look at this more tomorrow...
Comment 46 Kyle M Hall 2020-02-18 11:53:45 UTC
koha_trusted_proxies is a config setting from the Koha conf xml file, so mock_preference won't work on that. You'll want to use mock_config instead.

(In reply to David Cook from comment #45)
> (In reply to Martin Renvoize from comment #44)
> > Thanks David, that's great news.
> 
> Finally getting around to this...
> 
> Have to leave for the night but the following isn't working for some reason:
> 
> t::lib::Mocks::mock_preference('koha_trusted_proxies', '1.1.1.1');
> 
> Will have to look at this more tomorrow...
Comment 47 David Cook 2020-02-18 23:05:33 UTC
(In reply to Kyle M Hall from comment #46)
> koha_trusted_proxies is a config setting from the Koha conf xml file, so
> mock_preference won't work on that. You'll want to use mock_config instead.
> 

So obvious in the light of day hehe. Thanks, Kyle. Yesterday was a loooong day.
Comment 48 David Cook 2020-02-19 02:18:40 UTC
(In reply to Martin Renvoize from comment #44)
> Thanks David, that's great news.

It only took me nearly 3 weeks but I finally got there heh.

Unit test and bug fix available at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24538.
Comment 49 David Cook 2020-02-25 02:18:17 UTC
Unfortunately, it looks like this feature breaks file upload for CGI Koha. I haven't checked Plack Koha yet but might still be relevant.

Please see https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 for more details. I'll include a first go at a patch there, which I've shown to work in CGI Koha.
Comment 50 David Cook 2023-06-07 07:21:07 UTC
Trying to remember why we needed this since in theory Plack::Middleware::ReverseProxy should've handled this...
Comment 51 David Cook 2023-06-07 07:23:01 UTC
Also wondering how we could update REMOTE_USER so that it appears in plack.log...
Comment 52 David Cook 2023-06-09 01:36:11 UTC
(In reply to David Cook from comment #50)
> Trying to remember why we needed this since in theory
> Plack::Middleware::ReverseProxy should've handled this...

Ahh I see now. Plack::Middleware::ReverseProxy chooses the last IP in the list (rather than the first), so if you're using a reverse proxy in front of Koha's Apache reverse proxy, then you'll get the IP address for that reverse proxy instead of the client IP address. 

I guess I can understand that... since it is the most recent IP address to connect... and then we use Koha::Middleware::RealIP (inspired by Plack::Middleware::RealIP) to validate the proxy addresses and get the real client IP...

--

I can get the client IP via $ENV{REMOTE_ADDR} in Koha as set by Koha::Middleware::RealIP, but plack.log is still using the proxy address as set by Plack::Middleware::ReverseProxy. (I've confirmed that by hacking on Plack::Middleware::ReverseProxy.pm in k-t-d.)
Comment 53 David Cook 2023-06-09 03:01:38 UTC
(In reply to David Cook from comment #52)
> I can get the client IP via $ENV{REMOTE_ADDR} in Koha as set by
> Koha::Middleware::RealIP, but plack.log is still using the proxy address as
> set by Plack::Middleware::ReverseProxy. (I've confirmed that by hacking on
> Plack::Middleware::ReverseProxy.pm in k-t-d.)

This one is driving me a little crazy...

It looks like /usr/share/perl5/Plack/Runner.pm applies the Plack::Middleware::AccessLog middleware and that's wrapped around the app last...

Yet it appears to be getting the REMOTE_ADDR from Plack::Middleware::ReverseProxy and not Koha::Middleware::RealIP. (If you remove Plack::Middleware::ReverseProxy then it'll get it from Koha::Middleware::RealIP.)

But it seems more complicated than that because adding Koha::Middleware::RealIP *after* Plack::Middleware::ReverseProxy doesn't seem to make a difference which seems illogical...
Comment 54 David Cook 2023-06-09 03:36:43 UTC
The problem is Koha::Middleware::SetEnv. It's a fundamentally flawed middleware.

The wrapping of middleware works by passing around a hash reference (ie $env), and Koha::Middleware::SetEnv breaks the referential train.

It passes a new hashref to Koha::Middleware::RealIP which then passes that new reference to the Koha app. 

But when the post-processing aspect of the Plack middleware runs, after leaving the Koha::Middleware::SetEnv wrapper, it goes back to Plack::Middleware::Static which has the *original* reference, which undoes the changes of Koha::Middleware::SetEnv wrapper and Koha::Middleware::RealIP.

We don't notice this because a lot of our middleware only "pre-process". But Plack::Middleware::AccessLog" is a "post-process" middleware.

This should actually be a fairly easy fix, and it'll fix the IP address in plack.log when running behind a reverse proxy.
Comment 55 David Cook 2023-06-09 03:54:35 UTC
Boy that was crazy-making but feels good to figure out a tough problem 😅
Comment 56 Kyle M Hall 2023-06-09 10:30:42 UTC
(In reply to David Cook from comment #55)
> Boy that was crazy-making but feels good to figure out a tough problem 😅

Thanks not only for figuring this out, but writing it up for the rest of us!
Comment 57 David Cook 2023-06-12 23:55:48 UTC
(In reply to Kyle M Hall from comment #56)
> Thanks not only for figuring this out, but writing it up for the rest of us!

Thanks, Kyle! That really means a lot to me :)