We use drupal behind a CDN and a local reverse proxy, and trying to get the real client IP address was very confusing. I would prefer that drupal allow you to say that you trust a particular header to contain an unspoofed client IP, but I realize that in the past that argument has been made and lost. Instead of complaining about this, at least we need to improve documentation to make it more obvious what steps are needed to be taken in a proxy environment to get a real IP address.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Status: Active » Needs work

Improving the doc sounds like a good idea, and this is a good start!

But there are some problems in your patch:

a) After reading it (note: I am not a proxy expert or an expert in how to set them up in Drupal) I was very confused about $conf['reverse_proxy'] vs. $conf['reverse_proxy_addresses'] vs. $conf['reverse_proxy_header'] -- could that be made clearer -- do you need all of them, just some, or what?

b) X-Forarded-For -- I think this is a typo and should be X-Forwarded-For ?

c) " If $conf['reverse_proxy'] = TRUE, ..." This should be written with the word "is" in place of "=" (we try to use English rather than code in docs, and besides = is assignment, not equality test).

d)

+ * If $conf['reverse_proxy'] = TRUE, you must specify every reverse proxy IP
+ * in your environment in this array.  The following logic is used to

I think "this" here is referring to $conf['reverse_proxy_addresses'] , correct? If so, please say "the $conf['reverse_proxy_addresses'] array" instead, because $conf['reverse_proxy_addresses'] hasn't been referred to yet, so "this array" doesn't make sense?

e) The list will not format well on the API site, and doesn't follow our list guidelines (each item should be a sentence, for instance). I guess I'm not so concerned about the formatting, but the items should definitely end in ".".

f) "IP's" ==> "IPs". Apostrophe is not correct here.

g) "The rightmost IP remaining"... what is "right" here, the last one in the array, and if so, which array?

h) Generally, I think this list of the process would be clearer if it used words rather than (1) (2) etc.

i) The last sentence does not end with a .

Also, next time you attach a patch to an issue, please set the status to "needs review", which will trigger the test bot, and also alert human readers that there is a patch to review. Thanks!

grendzy’s picture

Adding info about other proxy options makes sense - and I'd like to keep the cautionary info about header forgery (spoofing). For D8, an improved solution might be to support CIDR network notation.

Do any CDN providers offer a way to verify the headers? Or are we forced to live the possibility of spoofing?

joelcollinsdc’s picture

Thanks guys, I will incorporate the changes and try again.

Quick question though, over the weekend I was thinking this over. Is there a limitation in drupal about changes to default.settings.php? Doesn't drupal compare the default.settings.php in sites/default with that in other folders to ensure nothing has changed? Are there any complications getting changes in this file committed?

I'm just trying to make sure that i'm not wasting time recommending changes to this file in the 7.x branch.

Thanks!

grendzy’s picture

AFAIK the string freeze doesn't apply to default.settings.php, since it isn't translated. The change would have to be made in D8 first, though.

I didn't see a CDN page in the on-line handbook - this could be another useful option as there'd be more space to go into detail, even including info about specific vendors.

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

Sorry, didn't notice that before. Changing to Drupal 8, with backport tag. This can most likely get put into Drupal 7 as well as Drupal 8.

joelcollinsdc’s picture

Status: Needs work » Needs review
FileSize
4.32 KB

Attempt #2, this time against d8.

joelcollinsdc’s picture

Title: Reverse proxy IP address handling confusing, at minimum needs better documentation » Improve reverse proxy ip address handing comment and documentation
Priority: Normal » Minor
joelcollinsdc’s picture

@ #2: We use akamai, and I asked them. Its not so much a problem of a feature of CDNs but more of their configuration. Akamai does allow you to prevent any traffic that bypasses akamai. (its an additional service they offer). If you were using this service, you could in theory trust their 'True-Client-IP' header.

@ #4: is this the right page in the handbook? http://drupal.org/node/425990

jhodgdon’s picture

Status: Needs review » Needs work

There are quite a few style/grammar/etc. things that need fixing in this patch (I cannot comment on the accuracy):

a) Drupal should always be capitalized.
b) Our style guide requires a comma before and/or in a list:
apples, bananas, or oranges
site caching, security, or encryption benefits
c) The patch seems to introduce a new style for default.settings.php sections (with the "Reverse proxy settings:" header at the top). It should follow the style of other sections.
d) One space, not two, between sentences.
e) I think HTTP needs to be all-caps in " http headers"
f) Spelling error: restricitons
g) In " client could bypass restricitons by IP by settings the" should be "by setting the..."
h)

+ * environment, this setting should remain commented out.
+
+ * If this setting is TRUE, you must know every possible reverse proxy IP

This blank line needs a * at the beginning of it
i) There are spaces at the end of some lines that need to be removed
j) Spelling error: X-Forarded-For

OK, that is as far as I got. Please proofread the patch. :)

joelcollinsdc’s picture

Title: Improve reverse proxy ip address handing comment and documentation » Improve reverse proxy ip address handing commenting and documentation
FileSize
4.3 KB

Attempt #3. you guys are fast.

joelcollinsdc’s picture

Found some more stuff

joelcollinsdc’s picture

Status: Needs work » Needs review
joelcollinsdc’s picture

Realized i'm being way too wordy. cut out some cruft.

jhodgdon’s picture

I think the formatting, grammar, spelling, and style of this patch is fine. I will leave it to someone else to review the patch for accuracy, since I don't know anything about these settings in Drupal, or proxies for that matter. Thanks for the nice cleanup!

grendzy’s picture

Status: Needs review » Reviewed & tested by the community

works for me.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Nice work on cleaning this up!

Committed and pushed to 8.x and 7.x.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.