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

RESOLVED FIXED in Bugzilla 3.6

Status

()

Bugzilla
Administration
--
enhancement
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: Max Kanat-Alexander, Assigned: Max Kanat-Alexander)

Tracking

unspecified
Bugzilla 3.6
Bug Flags:
approval +

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 years ago
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.
(Assignee)

Updated

9 years ago
Target Milestone: --- → Bugzilla 3.6
Should support multiple proxies, not just one.
(Assignee)

Comment 2

9 years ago
Created attachment 411317 [details] [diff] [review]
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.
Assignee: administration → mkanat
Status: NEW → ASSIGNED
Attachment #411317 - Flags: review?(dkl)

Comment 3

9 years ago
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.
(Assignee)

Comment 4

9 years ago
Created attachment 411323 [details] [diff] [review]
v2

Thanks, that was a mistake.
Attachment #411317 - Attachment is obsolete: true
Attachment #411323 - Flags: review?(dkl)
Attachment #411317 - Flags: review?(dkl)
(Assignee)

Updated

9 years ago
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-
(Assignee)

Comment 6

9 years ago
Created attachment 411451 [details] [diff] [review]
v3

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+
(Assignee)

Comment 8

9 years ago
Yay! :-)
Flags: approval+
(Assignee)

Comment 9

9 years ago
Holding approval until the blockers are ready.
Flags: approval+ → approval?
Whiteboard: [es-ita] → [es-ita][wanted-bmo]
(Assignee)

Updated

8 years ago
Flags: approval? → approval+
(Assignee)

Comment 10

8 years ago
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
Last Resolved: 8 years ago
Keywords: relnote
Resolution: --- → FIXED
(Assignee)

Comment 11

8 years ago
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.