Mergeback PR from private to public. - #30284
Merged
Merged
Conversation
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
Contributor
Author
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
Contributor
Author
|
EdX Release Notice: This PR has been deployed to the production environment. |
1 similar comment
Contributor
Author
|
EdX Release Notice: This PR has been deployed to the production environment. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge private changes back to the public repo post-PR-merge.
Please review and tag appropriate parties.