Skip to content

Add ClientIp extractor and migrate handlers#428

Merged
jevolk merged 1 commit into
matrix-construct:mainfrom
theredspoon:feat/client-ip-extractor
Apr 26, 2026
Merged

Add ClientIp extractor and migrate handlers#428
jevolk merged 1 commit into
matrix-construct:mainfrom
theredspoon:feat/client-ip-extractor

Conversation

@theredspoon
Copy link
Copy Markdown
Contributor

@theredspoon theredspoon commented Apr 21, 2026

Summary

Introduces a tuwunel-internal ClientIp extractor in src/api/client_ip.rs and migrates all 22 API handler call sites away from axum_client_ip::InsecureClientIp.

This change is plumbing only. It is designed to be a no-op for every existing deployment:

  • The extractor default branch delegates to InsecureClientIp, preserving today's header scan chain and ConnectInfo fallback exactly.
  • A new marker, ConfiguredIpSource, gates the SecureClientIp branch. Nothing in this change installs that marker; Add configurable ip_source for client IP resolution. #429 installs it only when ip_source is configured.
  • The existing SecureClientIpSource::ConnectInfo extension installed by src/router/layers.rs is intentionally untouched and ignored by this extractor, so Unix-socket deployments (Unix listener can't read client ip #310) remain unaffected.

Two-PR series

This is the first PR in a two-PR effort for configurable, spoofing-resistant client IP resolution:

Keeping this split lets reviewers verify the mechanical extractor migration independently from the user-facing configuration and documentation work. #429 should land after this PR.

Test plan

  • cargo check -p tuwunel_api
  • cargo clippy -p tuwunel_api --no-deps -- -D warnings
  • rustup run nightly rustfmt --check on touched files
  • cargo test -p tuwunel_api client_ip
  • Unit tests in src/api/client_ip.rs cover fallback and configured paths, malformed headers, IPv6, XFF priority, configured-header absence, and the auto-installed SecureClientIpSource regression.
  • Manual smoke: start tuwunel, hit /_matrix/client/versions, confirm tracing emits the connecting IP as today.

@theredspoon theredspoon force-pushed the feat/client-ip-extractor branch from ee7d3b2 to 4da9771 Compare April 21, 2026 19:23
@theredspoon theredspoon marked this pull request as ready for review April 21, 2026 19:27
@theredspoon theredspoon changed the title Add ClientIp extractor and migrate handlers. (processes #427) Add ClientIp extractor and migrate handlers Apr 21, 2026
@theredspoon theredspoon force-pushed the feat/client-ip-extractor branch from 4da9771 to 3b8df39 Compare April 21, 2026 19:28
@theredspoon theredspoon force-pushed the feat/client-ip-extractor branch 2 times, most recently from 0976c05 to edc7bd9 Compare April 22, 2026 11:54
@theredspoon theredspoon force-pushed the feat/client-ip-extractor branch from edc7bd9 to b0b15dc Compare April 22, 2026 11:55
@theredspoon
Copy link
Copy Markdown
Contributor Author

Update after the force-push:

  • The failing upstream job was Audit, caused by RUSTSEC-2026-0104 against rustls-webpki 0.103.12.
  • I amended this PR's single commit to update Cargo.lock to rustls-webpki 0.103.13.
  • Current Add ClientIp extractor and migrate handlers #428 head: b0b15dca4e3e5fb2b0e323bab15e98e643ac2207.

Local validation performed:

  • Ran the Docker clippy bake target against the PR stack with the same relevant CI matrix shape: cargo_profiles=["test"], feat_sets=["all"], rust_toolchains=["nightly"], Debian testing-slim, Rust target x86_64-unknown-linux-gnu, sys target x86_64-v1-linux-gnu. It completed successfully with the CI -D warnings Clippy path. Local note: I set cargo_installs=cargo-chef for that Clippy run so the build skipped unrelated cargo helper installs and reached the lint step.
  • Ran cargo audit --stale --deny yanked --deny unsound --deny unmaintained --deny warnings --color never locally with cargo-audit 0.22.1; it exited successfully on the restacked branch containing this lockfile update.

Constraints / caveats:

  • The full Docker audit bake target did not complete locally because the Docker/Colima environment exhausted host disk space and then rustup hit Input/output error (os error 5) before the advisory scan. I used the host cargo-audit command above to verify the actual advisory condition instead.
  • Fresh GitHub Actions runs after the force-push are currently action_required with jobs: [], so upstream CI has not actually rerun yet and likely needs maintainer approval/re-run for the forked PR.

@theredspoon
Copy link
Copy Markdown
Contributor Author

Quick maintainer nudge: after the force-push/restack, the current workflow runs for #428 and #429 look like they need maintainer approval again.

@theredspoon
Copy link
Copy Markdown
Contributor Author

theredspoon commented Apr 23, 2026

Quick update on the rerun for this PR (#428): the current red is rust-sdk-integ bench failing in tests::room::test_event_with_context, where the SDK received a RoomEncrypted event instead of the expected decrypted RoomMessage.

That is a different failure from the earlier room_privacy timeout, and #429 passed the same rust-sdk-integ jobs, so this still looks more like unstable integration-test behavior than a clear regression in #428's touched code.

I’m not planning to make further changes to #428 unless a failing test points to a concrete issue in the PR itself, so from my side the remaining options are either rerunning to see whether the failure reproduces or merging #429 and closing out/superseding this PR.

@jevolk
Copy link
Copy Markdown
Member

jevolk commented Apr 23, 2026

I'm sorry for not addressing this sooner. These flakes are not your fault and no further action has been required. I sincerely appreciate your effort here 🙏🏻 and I'll be attending to this very shortly. If in any case there is a delay and (considering the broad scope of the diff) a conflict forms, I will take responsibility for resolving that. Thank you! 🫡

@jevolk jevolk merged commit ef082ef into matrix-construct:main Apr 26, 2026
153 of 160 checks passed
@theredspoon theredspoon deleted the feat/client-ip-extractor branch May 3, 2026 23:35
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