feat: Allow multiple options in request_ip_source #1985
No reviewers
Labels
No labels
Abandoned
Blocked
Bug
Changelog
Added
Changelog
Missing
Changelog
None
Cherry-picking
Database
Dependencies
Dependencies/Renovate
Difficulty
Easy
Difficulty
Hard
Difficulty
Medium
Documentation
Enhancement
Good first issue
Help wanted
Inherited
Matrix/Administration
Matrix/Appservices
Matrix/Auth
Matrix/Client
Matrix/Core
Matrix/E2EE
Matrix/Federation
Matrix/Hydra
Matrix/MSC
Matrix/Media
Matrix/T&S
Merge
Merge/Manual
Merge/Squash
Meta
Meta/CI
Meta/Packaging
Priority
Blocking
Priority
High
Priority
Low
Security
Status
Confirmed
Status
Duplicate
Status
Invalid
Status
Needs Investigation
Support
bob the builder
No project
No assignees
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
continuwuation/continuwuity!1985
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "Omar007/continuwuity:feature/ip-src-fallback"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The current
request_ip_sourcesetting only allows a single option.While it does fall back to the peer IP if the header is missing as of !2003,
which likely covers a lot of regular use, only allowing a single option limits
the deployment options available to more advanced deployments.
Setups where internal and external traffic use different reverse proxies will
end up with the wrong IP and the implicitness of the fallback allows for
situations where the used IP is not the IP expected.
By introducing a setting that allows multiple options to be set,
this limitation is resolved and it becomes possible to have client IP resolution
behind different reverse proxies and also making it possible to decide if and/or
what the fallback should be.
If set, options are evaluated in order. If all fail, the request fails.
Pull request checklist:
mainbranch, and the branch is named something other thanmain.myself, if applicable. This includes ensuring code compiles.
request_ip_source@ -21,0 +89,4 @@.iter().filter_map(|s| ClientIp::for_source(s)).map(|f| f(&parts)).reduce(|a, b| a.or_else(|_| b))I thought about maybe doing the mapping early and recording it as an extension, then only grab the extension and execute it here but placing the type in the top-level src/router crate would not allow it to be used in here and I don't know if some other common location would make sense 🤔
@ -117,3 +117,2 @@[workspace.dependencies.axum-client-ip]version = "1.3"[workspace.dependencies.client-ip]where is this dependency used?
In
src/api/router. It's the actual functionality that was behindaxum-client-ip. I guess it can be moved to a lower level maybe?Can't seem to link to the line within the PR, it's in the
for_sourcefunction starting on line 62.24b90164ef594f5ea32a@Omar007 this fails clippy right now
Weird, I'm fairly sure I ran it earlier. I'll do the rebase and run it again 🤔
594f5ea32aa12805e0c9Ok turns out after my last set of changes I only ended up doing the
+nightlyformat before I committed, I forgot the clippy run at the very end. Fixed, apologies 🙈@ -391,2 +395,3 @@/// default: ["direct"]#[serde(default)]pub request_ip_source: Option<String>,pub request_ip_source: Vec<String>,This is a breaking change for people that have already set this to something
Seems to have overlapped with !2003 (cc @nex)
I can probably rebase things on top of the changes made by !2003 no probs, at first glance that does not look to be problematic 🤔
In regards to the config change, I could introduce something like
allowed_ip_sourcesinstead (feel free to suggest a better name!) to match the explicitness of this as opposed to the implicitness of !2003, then change the behaviour based on the config option used:allowed_ip_sourcesis set, extract from the explicitly supplied options, don't allow anything else.allowed_ip_sourcesis NOT set butrequest_ip_source(the current) is set, extract from the chosen option and always fallback to direct (the behaviour as implemented by !2003).Setting neither or setting them to 'direct' would be the same and ignores any headers.
EDIT: maybe
accepted_ip_sourceswould be better for the new option?a12805e0c9a655e85dc6Rebased against the changes in !2003 and !1973. Also did the config property split for now.
There is apparently a difference between the clippy run documented in CONTRIBUTING.md and what is being run in the Forgejo workflow. I got nothing locally, picking up the ones returned from the workflow now.
EDIT: looks like the workflow runs with
--profile test. Should the CONTRIBUTING.md be updated to use that as well?a655e85dc6e0737beb16Ok w/e that latest failure cause is, is beyond my influence haha. Looks like the server was unreachable so it needs a retry.
allowed_ip_sourcesmakes sense to me. We might deprecaterequest_ip_sourcein the future to avoid having the almost duplicate behaviour, but for now it's fine to avoid breaking anything