Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proxy Support #48

Merged
merged 24 commits into from Oct 22, 2022
Merged

Proxy Support #48

merged 24 commits into from Oct 22, 2022

Conversation

Tert0
Copy link
Contributor

@Tert0 Tert0 commented Sep 10, 2022

This PR adds Proxy Support.

TODO

  • Velocity Support
  • Bungeecord Support
  • Add Plugin Message API (Client Event)
  • VELOCITY_MODERN_FORWARDING_WITH_KEY_V2

Closes #39

Copy link
Member

@rj00a rj00a left a comment

Choose a reason for hiding this comment

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

Looking good so far!

src/client.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/protocol/packets/c2s.rs Outdated Show resolved Hide resolved
src/proxy/velocity.rs Outdated Show resolved Hide resolved
…dress length check, changed send_plugin_message parameters, moved velocity secret and moved ConnectionMode Docs
src/server.rs Outdated Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
@Tert0 Tert0 marked this pull request as ready for review October 9, 2022 06:54
@Tert0 Tert0 requested a review from rj00a October 9, 2022 06:55
@rj00a
Copy link
Member

rj00a commented Oct 19, 2022

Where did you get your information on how the proxies work?

@Tert0
Copy link
Contributor Author

Tert0 commented Oct 19, 2022

I've read the source code of Velocity and the PaperMC Implementation

@rj00a
Copy link
Member

rj00a commented Oct 20, 2022

Reviewing this now. Will be pushing changes tomorrow.

Copy link
Member

@rj00a rj00a left a comment

Choose a reason for hiding this comment

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

Changes I've Made

  • Cleaned up documentation in config.rs.
  • The handle_login function was quite large so I moved all that logic into a new submodule.
  • Removed the proxy module since there wasn't much there.
  • Removed incorrect uses of unwrap and expect. (If we panic it would bring down the whole server).
  • Used existing types to parse where applicable.
  • Set the IP address of the client in the BungeeCord login procedure.
  • Removed Velocity version check. Let me know if it was wrong to remove this.
  • More descriptive error messages.
  • Many stylistic adjustments and optimizations.

Issues to Resolve Before Merging

  • The second element of the NUL-delimited list in the BungeeCord procedure appears to be the client's IP address, but I'm not certain. Can you confirm?
  • VELOCITY_MODERN_FORWARDING_WITH_KEY_V2 is unimplemented. Probably not a blocker for merging. Not sure how important it is.
  • After logging in with ConnectionMode::BungeeCord or ConnectionMode::Velocity, the client fails to fully join and is disconnected with the following error. This problem exists before and after my changes.

last-screenshot

@Tert0
Copy link
Contributor Author

Tert0 commented Oct 21, 2022

  • The second element of the NUL-delimited list in the BungeeCord procedure appears to be the client's IP address, but I'm not certain. Can you confirm?

I think it's the IP you have used to conntect to the server. (e.g. 127.0.0.1 or localhost)

  • VELOCITY_MODERN_FORWARDING_WITH_KEY_V2 is unimplemented. Probably not a blocker for merging. Not sure how important it is.

I think i forget to Push These changes, so i'll push them later.

@rj00a
Copy link
Member

rj00a commented Oct 21, 2022

I think it's the IP you have used to conntect to the server. (e.g. 127.0.0.1 or localhost)

When I tried it, the first element was localhost while the second was 127.0.0.1. So I think the first is the address the player used to connect (possibly requiring DNS lookup) while the second is the address of the connecting player. Correct me if I'm wrong.

@rj00a
Copy link
Member

rj00a commented Oct 22, 2022

I'm going to go ahead and merge what we have here to avoid conflicts.

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.

Proxy Support
2 participants