Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Improve X-Forwarded-For handling#495

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
joseluisq merged 1 commit intostatic-web-server:masterfromJeidnx:trusted_proxies
Nov 11, 2024

Conversation

Jeidnx
Copy link
Contributor

@JeidnxJeidnx commentedNov 2, 2024•
edited
Loading

Description

Adds two new options to configure logging of X-Forwarded-IP addresses.

  • log-forward-for: is a boolean to enable logging of the header in the first place. Logging is done in the same manner as log-remote-address, meaning it will only show with log-level info or higher
  • trusted-proxies: is an optional list of IP addresses from which to accept the x-forwarded-for header. If unspecified or an empty string, all IP addresses are allowed
    Both of the options are optional and log-forward-for defaults to false. This means that existing uses of log_remote_address will silently stop logging from the x-forwarded-for header.

Related Issue

Resolves#494

Motivation and Context

See#494

How Has This Been Tested?

I have tested all combinations oflog-remote-addr,log-forwarded-for andtrusted-proxies with both CLI options and a config file. I confirmed that they output the expected logs.

Screenshots (if appropriate):

Some config snippets and what they output. The requests are all sent from::1 withX-Forwarded-For: 1.1.1.1

Example 1:

log-remote-address =truelog-forwarded-for =false
INFO static_web_server::info: log requests with remote IP addresses: enabled=trueINFO static_web_server::info: log X-Forwarded-For real remote IP addresses: enabled=falseINFO static_web_server::info: trusted IPs for X-Forwarded-For: allINFO static_web_server::log_addr: incoming request: method=GET uri=/ remote_addr=[::1]:56232

Example 2 (this would reflect the old behavior):

log-remote-address =truelog-forwarded-for =true
INFO static_web_server::info: log requests with remote IP addresses: enabled=trueINFO static_web_server::info: log X-Forwarded-For real remote IP addresses: enabled=trueINFO static_web_server::info: trusted IPs for X-Forwarded-For: allINFO static_web_server::log_addr: incoming request: method=GET uri=/ remote_addr=[::1]:48046 real_remote_ip=1.1.1.1

Example 3:

log-remote-address =falselog-forwarded-for =truetrusted-proxies = ["::1"]
INFO static_web_server::info: log requests with remote IP addresses: enabled=falseINFO static_web_server::info: log X-Forwarded-For real remote IP addresses: enabled=trueINFO static_web_server::info: trusted IPs for X-Forwarded-For: [::1]INFO static_web_server::log_addr: incoming request: method=GET uri=/ real_remote_ip=1.1.1.1

Example 4:

log-remote-address =truelog-forwarded-for =truetrusted-proxies = ["1.1.1.1","127.0.0.1"]
INFO static_web_server::info: log requests with remote IP addresses: enabled=trueINFO static_web_server::info: log X-Forwarded-For real remote IP addresses: enabled=trueINFO static_web_server::info: trusted IPs for X-Forwarded-For: [1.1.1.1, 127.0.0.1]INFO static_web_server::log_addr: incoming request: method=GET uri=/ remote_addr=[::1]:57270

@semanticdiff-comSemanticDiff.com
Copy link

semanticdiff-combot commentedNov 2, 2024•
edited
Loading

Review changes with  SemanticDiff

Changed Files
FileStatus
  src/handler.rs  52% smaller
  src/log_addr.rs  23% smaller
  src/settings/cli.rs  1% smaller
  docs/content/configuration/command-line-arguments.mdUnsupported file format
  docs/content/configuration/config-file.mdUnsupported file format
  docs/content/configuration/environment-variables.mdUnsupported file format
  docs/content/features/logging.mdUnsupported file format
  src/server.rs  0% smaller
  src/settings/file.rs  0% smaller
  src/settings/mod.rs  0% smaller
  src/testing.rs  0% smaller

@joseluisqjoseluisq added enhancementNew feature or request v2v2 release labelsNov 3, 2024
@joseluisq
Copy link
Collaborator

It looks fine to me. You need to add some log examples in the PR description of your tests and updatethe documentation page, you can find the doc pagehere and check outthis if you want to build the docs locally.

Let me know when the PR is ready to review it.

--

Additionally, I wonder if we also need to support atrust all proxies option for reverse proxies with non-static IPs like AWS ELB or similar.

@JeidnxJeidnx marked this pull request as ready for reviewNovember 5, 2024 11:23
@Jeidnx
Copy link
ContributorAuthor

Yeah i took some time to think it over and to me it seemed the most sensible thing was to seperate the x-forwarded-for logging from the remote address logging completely. I have introduced two new options, one to toggle logging of the x-forwarded-for header and one to optionally allowlist certain IPs, where the default is to trust all IPs.

I also ran into some issues when trying to build the docs, the commandhere doesn't seem to mount the docs in the correct directory. I also found some other small issues with the documentation, i will open a seperate issue for this.

@joseluisq
Copy link
Collaborator

I also ran into some issues when trying to build the docs, the commandhere doesn't seem to mount the docs in the correct directory. I also found some other small issues with the documentation, i will open a seperate issue for this.

The docs instructions are kind of outdated but feel free to update them.

For running the Docs dev server via Docker, just trydocker-compose -f docs/docker-compose.yml up --build. This is what I use when testing changes across pages locally.

@JeidnxJeidnx changed the titleImplement trusted_proxiesImprove X-Forwarded-For handlingNov 5, 2024
@Jeidnx
Copy link
ContributorAuthor

This is ready to be reviewed btw. Sorry i forgot to mention that.

@joseluisq
Copy link
Collaborator

This is ready to be reviewed btw. Sorry i forgot to mention that.

Great, I will give it a check today.

Copy link
Collaborator

@joseluisqjoseluisq left a comment

Choose a reason for hiding this comment

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

It looks fine to me.
I just left an update on the logging feature page to do. After that, we should merge the PR.

Adds a `log-forwarded-for` option to control if the X-Forwarded-Forheader information should be logged. Also includes a `trusted-proxies`option to optionally specify from which IPs to accept this header.Existing uses of log-remote-address will stop logging forwarded-forIPs after this change.
Copy link
Collaborator

@joseluisqjoseluisq left a comment

Choose a reason for hiding this comment

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

Ready for the next release, Thanks!

@joseluisqjoseluisq merged commit13e3f38 intostatic-web-server:masterNov 11, 2024
34 checks passed
@joseluisqjoseluisq added this to thev2.34.0 milestoneNov 11, 2024
@JeidnxJeidnx deleted the trusted_proxies branchNovember 11, 2024 14:58
@joseluisqjoseluisq added the breaking-featureThis feature contain breaking changes labelNov 28, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@joseluisqjoseluisqjoseluisq approved these changes

Assignees

@JeidnxJeidnx

Labels
breaking-featureThis feature contain breaking changesenhancementNew feature or requestv2v2 release
Projects
None yet
Milestone
v2.34.0
Development

Successfully merging this pull request may close these issues.

Add trusted_proxies option
2 participants
@Jeidnx@joseluisq
[8]ページ先頭
©2009-2025 Movatter.jp