Closed Bug 527586 Opened 12 years ago Closed 12 years ago

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

Categories

(Bugzilla :: Administration, task)

task
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 3.6

People

(Reporter: mkanat, Assigned: mkanat)

References

Details

(Whiteboard: [es-ita][wanted-bmo])

Attachments

(1 file, 2 obsolete files)

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.
Target Milestone: --- → Bugzilla 3.6
Should support multiple proxies, not just one.
Attached patch v1 (obsolete) — Splinter Review
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.
Assignee: administration → mkanat
Status: NEW → ASSIGNED
Attachment #411317 - Flags: review?(dkl)
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.
Attached patch v2 (obsolete) — Splinter Review
Thanks, that was a mistake.
Attachment #411317 - Attachment is obsolete: true
Attachment #411323 - Flags: review?(dkl)
Attachment #411317 - Flags: review?(dkl)
Whiteboard: [es-ita]
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
Attachment #411323 - Flags: review?(dkl) → review-
Attached patch v3Splinter Review
Ah, thanks! Bitrot fixed. bzr-loom for the win.
Attachment #411323 - Attachment is obsolete: true
Attachment #411451 - Flags: review?(dkl)
Comment on attachment 411451 [details] [diff] [review]
v3

Looks good and works well in my testing. r=dkl
Attachment #411451 - Flags: review?(dkl) → review+
Yay! :-)
Flags: approval+
Holding approval until the blockers are ready.
Flags: approval+ → approval?
Whiteboard: [es-ita] → [es-ita][wanted-bmo]
Flags: approval? → approval+
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: relnote
Resolution: --- → FIXED
Added to the release notes in bug 547466.
Keywords: relnote
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.
You need to log in before you can comment on or make changes to this bug.