-
Notifications
You must be signed in to change notification settings - Fork 477
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
feat(transport): add websocket transport #290
Conversation
1e5e8ab
to
e60c463
Compare
I really appreciate your effort to implement this and test it! Do you have any idea why it doesn't compile with a newer rust version? |
i'm not sure why, i think i have very little experience in rust (i've just started learning rust last week) to solve that, but probably something related to upgrade in alpine image? these are the error i got when re-building 0.4.8
What about building with debian based image instead? can we still got statically linked binary? |
I think it's due to alpine removing TLS related library. Actually, TLS in the docker image never works because we didn't ship the TLS in linking. On the other hand, statically linking against TLS is not something commonly approved and have troublesome nuance. So I think we can disable TLS related features(including tls, and your ws) in the docker image build, given that it never worked |
re-building 0.4.8 with debian based image seems to be successful
|
BTW, in the long term, we still would like to seek a cross-platform, statically linking TLS solution. rustls is the best bet but it still needs some time to be practical |
@rucciva I suspect it's this line that caused the difference, instead of the base image. tls is not used at all |
ah thats unfortunate, can we build multiple docker image tag instead? one that include all features (including related tls dependency, maybe using debian-slim flavor) and the existing one?
that confuses me. as far as what i understand, those features doesn't include tls right? but the build fails due too tls related error. |
For example like this one FROM rust:bookworm as builder
RUN apt update && apt install -y libssl-dev
WORKDIR /home/rust/src
COPY . .
ARG FEATURES
RUN cargo build --locked --release --features ${FEATURES:-client,server,noise,hot-reload}
RUN mkdir -p build-out/
RUN cp target/release/rathole build-out/
FROM gcr.io/distroless/cc-debian12
WORKDIR /app
COPY --from=builder /home/rust/src/build-out/rathole .
USER 1000:1000
ENTRYPOINT ["./rathole"] The github action could look like this. you can pull it from here. Although it is slightly bigger at 27mb (due to libc, libssl, and libgcc) compared to 4mb when using alpine. But the plus is it can contains all features including tls and websocket. |
Yes the dockerfile looks good to me. Go ahead! |
hi @rapiz1 , on second thought, should we make the dockerfile by default contains all features or maybe just produce docker image withh all features only? My reasoning is as follow:
|
We can go with one docker image containing all features. I see you change the base image to debian so tls should work. |
great, so i think i made the last change on this PR. you can see the docker job here. I've also made sure it runs fine |
Thanks for your contribution very much! |
Is it possible to use websocket for transport and noise for encryption? |
Hi @fernvenue , in theory it is possible to encrypt websocket frame using noise, i've done that with go. The existing noise transport is also a wrapper to tcp transport, so it might be possible to wrap the websocket transport instead. But why would you want that? IMHO The reason websocket is needed here is mainly to share an existing https port or where the only allowed protocol is http(s). If you can allocate dedicated port to rathole than it is best to go with the existing noise transport. |
@rucciva thanks for your reply.
Yea, I can allocate dedicated port, I thought websocket will be a better option for performance, especially for latency? |
I'm not sure about that, since websocket is a wrapper to tcp transport (when not using tls), the same with noise transport. If we add noise on top of websocket then it has more processing (noise + websocket + tcp) compared to existing noise transport (noise + tcp). Maybe you can test this first by comparing latency of rathole using plain tcp transport vs using websocket transport (without tls). If websocket transport is better than it might be worth to consider adding noise on top of websocket. |
Hi @rucciva, I just did benchmark test, here's the result:
Now we have answer about this question, if we want best performance, maybe keep using TCP as protocol instead of WebSocket will be great, yea, for both bandwidth and latency. |
Its awesome @fernvenue , thanks for the benchmark. Agreed with your result. For maximal performance without compromising security, the existing noise transport should be prefered. |
As per #134 , this would add support for using websocket transport. Especially useful in environment with strict policy that only allow incoming/outgoing connection via http(s) protocol.