Skip to content

feat!: Use more-trusted IP in rate-limiting (#241)#30285

Merged
timmc-edx merged 1 commit intoopen-release/nutmeg.masterfrom
timmc/ratelimit-backport-nutmeg
Apr 20, 2022
Merged

feat!: Use more-trusted IP in rate-limiting (#241)#30285
timmc-edx merged 1 commit intoopen-release/nutmeg.masterfrom
timmc/ratelimit-backport-nutmeg

Conversation

@timmc-edx
Copy link
Contributor

Previously, our rate-limiting code trusted the entire X-Forwarded-For
header, allowing a malicious client to spoof that header and evade
rate-limiting. This commit introduces a new module and setting
allowing us to make a more conservative choice of IPs.

  • Create new openedx.core.djangoapps.util.ip module for producing
    the IP "external chain" for requests based on the XFF header and the
    REMOTE_ADDR.
  • Include a function that gives the safest choice of IPs.
  • Add new setting CLOSEST_CLIENT_IP_FROM_HEADERS for configuring how
    the external chain is derived (i.e. setting the trust
    boundary). Currently has a default, but we may want to make it
    mandatory in the future.
  • Change django-ratelimit code to use the proximate IP in the external
    chain -- the one just outside the trust boundary.

Also:

  • Change XForwardedForMiddleware to use more conservative choice for
    its REMOTE_ADDR override
  • Other adjustments to XForwardedForMiddleware as needed in order to
    initialize new module and support code that needs the real
    REMOTE_ADDR value
  • Metrics for observability into the change (and XFF composition)
  • Feature switch to restore legacy mode if needed

This also gives us a path forward to removing use of the django-ipware
package, which is no longer maintained and has a handful of bugs that make it
difficult to use safely.

Internal ticket: ARCHBOM-2056

Backported from a251d18

Previously, our rate-limiting code trusted the entire `X-Forwarded-For`
header, allowing a malicious client to spoof that header and evade
rate-limiting. This commit introduces a new module and setting
allowing us to make a more conservative choice of IPs.

- Create new `openedx.core.djangoapps.util.ip` module for producing
  the IP "external chain" for requests based on the XFF header and the
  REMOTE_ADDR.
- Include a function that gives the safest choice of IPs.
- Add new setting `CLOSEST_CLIENT_IP_FROM_HEADERS` for configuring how
  the external chain is derived (i.e. setting the trust
  boundary). Currently has a default, but we may want to make it
  mandatory in the future.
- Change `django-ratelimit` code to use the proximate IP in the external
  chain -- the one just outside the trust boundary.

Also:

- Change `XForwardedForMiddleware` to use more conservative choice for
  its `REMOTE_ADDR` override
- Other adjustments to `XForwardedForMiddleware` as needed in order to
  initialize new module and support code that needs the real
  `REMOTE_ADDR` value
- Metrics for observability into the change (and XFF composition)
- Feature switch to restore legacy mode if needed

This also gives us a path forward to removing use of the django-ipware
package, which is no longer maintained and has a handful of bugs that make it
difficult to use safely.

Internal ticket: ARCHBOM-2056

Backported from a251d18
Copy link
Contributor

@rgraber rgraber left a comment

Choose a reason for hiding this comment

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

LGTM

@timmc-edx timmc-edx merged commit 894ed27 into open-release/nutmeg.master Apr 20, 2022
@timmc-edx timmc-edx deleted the timmc/ratelimit-backport-nutmeg branch April 20, 2022 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants