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.
Comment | File | Size | Author |
---|---|---|---|
#13 | remote_proxy_documentation.patch | 4.03 KB | joelcollinsdc |
#11 | remote_proxy_documentation.patch | 4.3 KB | joelcollinsdc |
#10 | remote_proxy_documentation.patch | 4.3 KB | joelcollinsdc |
#6 | remote_proxy_documentation.patch | 4.32 KB | joelcollinsdc |
remote_proxy_documentation.patch | 1.54 KB | joelcollinsdc | |
Comments
Comment #1
jhodgdonImproving 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)
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!
Comment #2
grendzy CreditAttribution: grendzy commentedAdding 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?
Comment #3
joelcollinsdc CreditAttribution: joelcollinsdc commentedThanks 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!
Comment #4
grendzy CreditAttribution: grendzy commentedAFAIK 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.
Comment #5
jhodgdonSorry, 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.
Comment #6
joelcollinsdc CreditAttribution: joelcollinsdc commentedAttempt #2, this time against d8.
Comment #7
joelcollinsdc CreditAttribution: joelcollinsdc commentedComment #8
joelcollinsdc CreditAttribution: joelcollinsdc commented@ #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
Comment #9
jhodgdonThere 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)
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. :)
Comment #10
joelcollinsdc CreditAttribution: joelcollinsdc commentedAttempt #3. you guys are fast.
Comment #11
joelcollinsdc CreditAttribution: joelcollinsdc commentedFound some more stuff
Comment #12
joelcollinsdc CreditAttribution: joelcollinsdc commentedComment #13
joelcollinsdc CreditAttribution: joelcollinsdc commentedRealized i'm being way too wordy. cut out some cruft.
Comment #14
jhodgdonI 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!
Comment #15
grendzy CreditAttribution: grendzy commentedworks for me.
Comment #16
webchickNice work on cleaning this up!
Committed and pushed to 8.x and 7.x.