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

New explicit and extensible server API #59

Merged
merged 23 commits into from
Feb 6, 2025
Merged

Conversation

valpackett
Copy link
Contributor

@valpackett valpackett commented Jan 27, 2025

(Fixes #15 #19 #31 & more)

This is my take on the server API redesign. Rather than introducing new "executor" traits to customize the behavior through more inversion-of-control, we go in the opposite direction and expose the match over the commands directly to the caller, providing easy obvious one-call default handlers:

    let (proto, cmd, mut target_addr) = proto.read_command().await?;

    target_addr = target_addr.resolve_dns().await?;

    match cmd {
        Socks5Command::TCPConnect => {
            run_tcp_proxy(proto, &target_addr, request_timeout, nodelay).await?;
        }
        Socks5Command::UDPAssociate => {
            run_udp_proxy(proto, &target_addr, reply_ip).await?;
        }
        _ => {
            proto.reply_error(&ReplyError::CommandNotSupported).await?;
        }
    };

When the user code looks like this:

  • it's obvious how to access the raw target_addr before DNS resolution
  • it's obvious how to skip DNS resolution
  • it's obvious how to disable UDP
  • it's obvious how to swap out a custom implementation of the proxying process itself, be it for a router/gateway type thing that would carry the request through other protocols to an exit node, or to use an accelerated proxying method
  • it's possible to implement TCPBind

And because this API is based on typestates, when you go to swap out run_tcp_proxy for something custom (or try to implement TCPBind), you won't forget to reply to the client – the type of proto at that point only has reply_success and reply_error as methods, and you need to call reply_success to get access to the raw socket and begin proxying.


Now, while here… authentication also needed to be rewritten. The Authentication trait from the previous API was… somewhat strange IMO, forcing the use of Arc/dyn while representing sort of "the checking business logic as a Thing" but not a method in terms of the SOCKS5 protocol.

The new API makes it easy to support only password auth:

    let (proto, _) = Socks5ServerProtocol::accept_password_auth(socket, |user, pass| {
        user == "admin" && pass == "correct horse battery staple"
    }).await?;
    // here goes the code from the above

Or no auth:

    let proto = Socks5ServerProtocol::accept_no_auth(socket).await?; // same…

Or skip the whole auth handshake (now with a clear reminder that it's not real compliant SOCKS5 anymore):

    let proto = Socks5ServerProtocol::skip_auth_this_is_not_rfc_compliant(socket); // same…

But below those helpers, if we unwrap that one layer, we encounter infinite extensibility, with custom methods that actually can be negotiated via custom method ID numbers.

If we inline accept_password_auth

    let (user, pass, auth) = Socks5ServerProtocol::start(inner)
        .negotiate_auth(&[PasswordAuthentication])
        .await?
        .read_username_password()
        .await?;
    if user == "user" && pass == "correct horse battery staple" {
        Ok(auth.accept().await?.finish_auth())
    } else {
        auth.reject().await?;
        Err(SocksError::AuthenticationRejected(
            "Wrong username/password".to_owned(),
        ))
    }

We get to see the entire flow of the protocol. (This is still all typestate-based: after we've negotiated with only PasswordAuthentication there's nowhere to go but to read_username_password() , and then we must accept() or reject() the password.)

Now, &[PasswordAuthentication] looks kinda silly, doesn't it? But that's because we only have one method. If we'd like to use method negotiation, that's where that slice comes in: we make our method type an enum containing various methods, and we get generic method negotiation with fully static dispatch, without any dyn overhead anywhere ever.

This is how we can use that to permit allowlisted source IP addresses to use no-authentication:

    let proto = match Socks5ServerProtocol::start(socket)
        .negotiate_auth(&[
            StandardAuthentication::PasswordAuthentication(PasswordAuthentication),
            StandardAuthentication::NoAuthentication(NoAuthentication),
        ])
        .await?
    {
        StandardAuthenticationStarted::PasswordAuthentication(auth) => {
            let (user, pass, auth) = auth.read_username_password().await?;
            if user == "user" && pass == "correct horse battery staple" {
                auth.accept().await?.finish_auth()
            } else {
                auth.reject().await?;
                return Err(SocksError::AuthenticationRejected(
                    "Wrong username/password".to_owned(),
                ));
            }
        }
        StandardAuthenticationStarted::NoAuthentication(auth) => {
            if check_allowed(socket_addr) {
                auth.finish_auth()
            } else {
                return Err(SocksError::AuthenticationRejected(
                    "You need username/password, you specifically".to_owned(),
                ));
            }
        }
    };

StandardAuthentication there is an enum defined by the library that includes those two methods, but it's possible to define custom enums that include fully custom methods! An example is included.


Little tiny TODOs left:

  • revise error types, would make sense now to split that giant SocksError
  • maybe let accept_password_auth return a value, would be less awkward to get a username out of the closure than by mutating a local
  • add a router example because why not

Start working on a new, safer, more extensible, lower-level-ish API.
First step, extract the methods handling the authentication, for now
without touching the actual code inside of them.

Don't make the methods public yet as this API isn't great yet.
Extend the type-level state machine further.
Missing the success reply for now as that requires refactoring
the actual proxying functions, which will be done next.
…uccess

Now almost the entire process goes through the type-level state machine,
with only a break for the DNS resolution left, which will be dealt with next.
Now that almost everything in the middle belongs to Socks5ServerProtocol,
reunite what's left in the main Socks5Socket impl into one syntactic block.
This is the new typestate-based auth method API that provides open-ended
extensibilty (i.e. a library user can add their own methods, using for
example the 80h-FEh private ID range specified by the RFC).

It provides a lot of safety, except for the possibility for two methods
to declare the same ID. Sadly, functions in traits cannot be const,
or I would've written a static assert at least in the enum macro..
Now with run_tcp_proxy and run_udp_proxy it's very clear that
upgrade_to_socks5 only translates from old API to new.
Now we can finally see it in all its glory!
Simple, yet explicit and therefore extensible.

Helpers have been added for auth to make it look nicer.
Not relevant anymore; with the new API, we don't do the Stream thing that
wasn't really adding much at all.
@dizda
Copy link
Owner

dizda commented Jan 30, 2025

Thanks @valpackett I like this new design, I'll try to implement it on my side.

@yuguorui I'd like your view on this, wdyt?

@dizda
Copy link
Owner

dizda commented Jan 30, 2025

@valpackett After switching my project to this PR and simply running it again, the authentication with password is broken.

2025-01-30T03:48:38.616436Z DEBUG conn{id=2server=1}:SOCKS5{ip=*********}: fast_socks5::server: Handshake headers: [version: 5, methods len: 3]    
2025-01-30T03:48:38.616457Z DEBUG conn{id=2server=1}:SOCKS5{ip=*********}: fast_socks5::server: methods supported sent by the client: [0, 1, 2]    
2025-01-30T03:48:38.616473Z DEBUG conn{id=2server=1}:SOCKS5{ip=*********}: fast_socks5::server: Reply with method 0

while on master I'd get:

2025-01-30T03:44:22.481104Z DEBUG conn{id=1server=1}:SOCKS5{ip=*********}: fast_socks5::server: Handshake headers: [version: 5, methods len: 3]    
2025-01-30T03:44:22.481122Z DEBUG conn{id=1server=1}:SOCKS5{ip=*********}: fast_socks5::server: methods supported sent by the client: [0, 1, 2]    
2025-01-30T03:44:22.481135Z DEBUG conn{id=1server=1}:SOCKS5{ip=*********}: fast_socks5::server: Reply with method AuthenticationMethod::Password (2)    
2025-01-30T03:44:22.481376Z DEBUG conn{id=1server=1}:SOCKS5{ip=*********}: fast_socks5::server: Auth: [version: 1, user len: 11]    
2025-01-30T03:44:22.481392Z DEBUG conn{id=1server=1}:SOCKS5{ip=*********}: fast_socks5::server: username bytes: [117, 1, 101, 114, 95, 52, 102, 101, 101, 1, 1]    
2025-01-30T03:44:22.481404Z DEBUG conn{id=1server=1}:SOCKS5{ip=*********}: fast_socks5::server: Auth: [pass len: 13]    
2025-01-30T03:44:22.481415Z DEBUG conn{id=1server=1}:SOCKS5{ip=*********}: fast_socks5::server: password bytes: [115, 117, 112, 1, 114, 98, 1, 109, 98, 111, 117, 55, 55]    

Any thought?

It might be necessary to return some password check result for
further usage. Allow doing it in a convenient way.
@dizda
Copy link
Owner

dizda commented Jan 31, 2025

added a small fix valpackett#1

valpackett and others added 7 commits February 1, 2025 03:02
Just a fun demo showing how a router / frontend proxy could be built.
Use the declared AddrError type as the actual return type in this module.
anyhow should not be used in library code at all.
Define a thiserror type for errors that occur when TCP connecting to targets.
Prepare a ReplyError conversion for these, as the proper error replying
was lost in the refactoring before.
New small thiserror based type for UDP header related errors.
Not sure why ReplyErrors were used here before: all of this happens way
after we've had a chance to reply with either success or error in the
TCP SOCKS5 flow.
Liberate the new server API from anyhow; restore the reporting of
errors to the client as reply_error.
that reports errors to the client.
@yuguorui
Copy link
Contributor

yuguorui commented Feb 3, 2025

@yuguorui I'd like your view on this, wdyt?

The Authentication trait from the previous API was… somewhat strange IMO.

  1. To be honest, I also think that the design of Authentication trait is not so reasonable, and my implementation of executor is largely to match the current Authentication trait style, and try to introduce as few major changes as possible.
  2. I also like the design of reply_success, which ensures that users will not make mistakes。
  3. I will try to migrate to the new interface in my projects to confirm its generalizability.

I think it's good overall, sorry for not replying quickly due to the Spring Festival.

@dizda
Copy link
Owner

dizda commented Feb 5, 2025

Thank you @yuguorui , waiting for your call before merging this

@yuguorui
Copy link
Contributor

yuguorui commented Feb 6, 2025

Thank you @yuguorui , waiting for your call before merging this

Yes, I have done the migration in my project and I did not find any issues while migrating to the new API. Both TCPConnect and UDPAssociate are working fine.

https://github.com/yuguorui/rfor/commits/typestate/

@dizda
Copy link
Owner

dizda commented Feb 6, 2025

Thanks for your feedback @yuguorui much appreciated, and thanks @valpackett for your work.

I'll merge this and release a new version.

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.

question: how can one forward to another tcp proxy? As in chain proxies?
3 participants