527586 - Use X-Forwarded-For instead of REMOTE_ADDR for trusted proxies

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Max Kanat-Alexander]

It's a somewhat-common situation that people have their Bugzilla behind some sort of proxy that does something (caching, SSL, etc.) with incoming traffic. Unfortunately, this breaks $ENV{REMOTE_ADDR}, because all traffic seems to be coming from the proxy.

We need a parameter (for the Advanced section) that allows admins to specify the IP of their proxy, and then if traffic comes from that proxy, we trust the X-Forwarded-For header instead of the REMOTE_ADDR.

Comment 1

[Reed Loden [:reed]]

Should support multiple proxies, not just one.

Comment 2

[Max Kanat-Alexander]

Attachment: v1

Here we go! Fairly straightforward.

I didn't use $cgi->remote_addr or $cgi->http because there could be times where we want to get the remote IP but we don't want to create a CGI object.

Comment 3

[Frédéric Buclin]

Comment on attachment 411317 [details] [diff] [review]
v1

>+sub remote_ip {
>+    my $ip = $ENV{'REMOTE_ADDR'} || '127.0.0.1';
>+    return $ip if !$ip;

I don't understand this line. $ip is always defined (127.0.0.1 if nothing else), so you never return here.

Comment 4

[Max Kanat-Alexander]

Attachment: v2

Thanks, that was a mistake.

Comment 5

[David Lawrence [:dkl]]

Comment on attachment 411323 [details] [diff] [review]
v2

This is not applying cleanly for me. I have the dependencies installed as well. 

[dkl@localhost new-trunk]$ patch -p0 < /tmp/proxies.diff 
patching file Bugzilla/Auth/Login/Cookie.pm
patching file Bugzilla/Auth/Persist/Cookie.pm
Hunk #1 FAILED at 51.
1 out of 1 hunk FAILED -- saving rejects to file Bugzilla/Auth/Persist/Cookie.pm.rej
patching file Bugzilla/Config/Advanced.pm
patching file Bugzilla/Error.pm
patching file Bugzilla/Token.pm
patching file Bugzilla/Util.pm
patching file template/en/default/admin/params/advanced.html.tmpl

Seems that to conflict with the new input_params patch committed recently.

Dave

Comment 6

[Max Kanat-Alexander]

Attachment: v3

Ah, thanks! Bitrot fixed. bzr-loom for the win.

Comment 7

[David Lawrence [:dkl]]

Comment on attachment 411451 [details] [diff] [review]
v3

Looks good and works well in my testing. r=dkl

Comment 8

[Max Kanat-Alexander]

Yay! :-)

Comment 9

[Max Kanat-Alexander]

Holding approval until the blockers are ready.

Comment 10

[Max Kanat-Alexander]

On checkin I had to also fix Bugzilla::User, which used remote_addr in three places from the lockout code (which was checked in since this patch was written).

Checking in Bugzilla/Error.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Error.pm,v  <--  Error.pm
new revision: 1.27; previous revision: 1.26
done
Checking in Bugzilla/Token.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Token.pm,v  <--  Token.pm
new revision: 1.59; previous revision: 1.58
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.201; previous revision: 1.200
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.99; previous revision: 1.98
done
Checking in Bugzilla/Auth/Login/Cookie.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/Cookie.pm,v  <--  Cookie.pm
new revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/Auth/Persist/Cookie.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Persist/Cookie.pm,v  <--  Cookie.pm
new revision: 1.13; previous revision: 1.12
done
Checking in Bugzilla/Config/Advanced.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Advanced.pm,v  <--  Advanced.pm
new revision: 1.2; previous revision: 1.1
done
Checking in template/en/default/admin/params/advanced.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/advanced.html.tmpl,v  <--  advanced.html.tmpl
new revision: 1.2; previous revision: 1.1
done

Comment 11

[Max Kanat-Alexander]

Added to the release notes in bug 547466.

Comment 12

[Dave Miller [:justdave]]

Comment on attachment 411451 [details] [diff] [review]
v3

For the record, the remote_ip() function in this patch is totally broken, but it's already been committed, so we'll need a new bug to fix it.