Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add config to read the last IP address in the list of proxies rather than the first #17765

Merged
merged 4 commits into from Jul 16, 2021

Conversation

diosmosis
Copy link
Member

Description:

Fixes L3-123.

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@diosmosis diosmosis added the Needs Review PRs that need a code review label Jul 13, 2021
@diosmosis diosmosis added this to the 4.4.0 milestone Jul 13, 2021
@tsteur
Copy link
Member

tsteur commented Jul 13, 2021

Before merging once the config is final be good to document this security feature in https://matomo.org/faq/how-to-install/faq_98/ and maybe in https://matomo.org/docs/security-how-to/ we could as well mention that they might want to enable this config if they are using a load balancer.

@mattab you will want to document this in the enterprise guide probably as well.

@@ -641,6 +641,9 @@
; By enabling this flag the header HTTP_X_FORWARDED_URI will be considered for the current script name.
proxy_uri_header = 0

; If set to 1 we use the last IP in the list of proxy IPs when determining the client IP.
proxy_ip_read_last_in_list = 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we make 0 as default to keep bc. Or at least set this option automatically to 0 on update, to ensure it won't break any instances. Could maybe only do that if proxy_client_headers is set.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was specified to set it to 1 in the issue (I assume it adds security). cc @tsteur

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default we want to keep existing behaviour and follow the RFC so we'd set it to 0 as otherwise it would be a breaking change.

We probably wouldn't leave the default at 1 and change it for existing installs as there are often random issues with it and it would require an additional step when setting up proxy headers. I reckon we could create an issue though to do this in Matomo 5 and put it directly into the Matomo 5 milestone.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi could tweak the description a bit.Like

; If set to 1, the last IP in the list of proxy IPs will be used when determining the client IP. Using the last IP can increase the security when you are using proxy headers in combination with a load balancer. By default, the first IP is read according to RFC7239 which may be required in some cases when the client sends the IP through a proxy header as well as the load balancer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI put #17202 in Matomo 5 milestone

@diosmosis
Copy link
Member Author

@sgiehl / @tsteur applied review feedback

Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me now

@diosmosis diosmosis changed the title by default read the last IP address in the list of proxies rather than the first add config to read the last IP address in the list of proxies rather than the first Jul 15, 2021
@mattab
Copy link
Member

mattab commented Jul 15, 2021

@mattab you will want to document this in the enterprise guide probably as well.

Sounds good, let me know when the user guide is updated and i'll copy the information in the PDF enterprise guide.

@diosmosis diosmosis merged commit dc29fc3 into 4.x-dev Jul 16, 2021
@diosmosis diosmosis deleted the l3-123-rightmost-proxy-id branch July 16, 2021 03:02
@tsteur
Copy link
Member

tsteur commented Jul 16, 2021

@mattab added a bit to the docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants