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

refactor(backend): replace TLS implementation with flexible-hyper-server-tls #606

Merged
merged 6 commits into from
Jul 26, 2023

Conversation

ravenclaw900
Copy link
Owner

@ravenclaw900 ravenclaw900 commented Jul 25, 2023

The old TLS implementation, aside from the fact that it was not taking full advantage of Rust's flexible type system, was horribly, horribly, flawed. The main reason is it could only handle attempting 1 TLS handshake at a time. This is fine if you're working with standard clients, but it also makes the dashboard incredibly easy to DDOS. There are 2 ways to fix this: allow handling multiple handshakes at once, or add a timeout for every handshake.

I separated out the TLS implementation into its own crate, flexible-hyper-server-tls, basically completely rewrote it to make it more robust, and implemented both of the above fixes.

Just to demonstrate the shortcomings of the old TLS system, I recorded some demos.

Old: https://asciinema.org/a/8nRKbdf7AkPLmP3QxFZUSmPwp
New: https://asciinema.org/a/RYtJX6zUoOC44lMujKDv3ooUH

I'll be publishing a hotfix release soon to address these issues.

…rver-tls`

The old TLS implementation was very naive, didn't take full advantage of Rust types, and could easily be DDOSed. I rewrote it in `flexible-hyper-server-tls`, which is what we're going to use now.
@ravenclaw900 ravenclaw900 added the bug Something isn't working label Jul 25, 2023
@ravenclaw900 ravenclaw900 requested a review from MichaIng July 25, 2023 22:15
@ravenclaw900 ravenclaw900 self-assigned this Jul 25, 2023
@ravenclaw900 ravenclaw900 changed the title refactor(backend): replace tls implementation with flexible-hyper-server-tls refactor(backend): replace TLS implementation with flexible-hyper-server-tls Jul 25, 2023
@ravenclaw900
Copy link
Owner Author

I should probably make a security advisory for this.

@ravenclaw900
Copy link
Owner Author

Slight increase in binary size, but that's a small price to pay for doing things correctly. Still, I wonder if there's anything that can be done about it.

@ravenclaw900 ravenclaw900 merged commit 2c810b3 into main Jul 26, 2023
@ravenclaw900 ravenclaw900 deleted the flexible-tls branch July 26, 2023 19:21
@MichaIng
Copy link
Collaborator

MichaIng commented Jul 26, 2023

I did not find time to have a closer look into the dedicated module, but based on your explanation it is pretty important indeed. Having it as dedicated crate, and published at crates.io as another step to increase trust and has a chance to get some more reviews, feedback and contributions independently of DietPi(-Dashboard). Well done, and many thanks for this heads up!

The overall code has increased, so not unexpected that the binary size increased as well a little. However, I see replacing higher level features with lower level features-util helped a little (I guess?). Anyway, as you said: Worth it!

A release would be great indeed, so I can enforce the update during next DietPi update this Saturday.

@ravenclaw900
Copy link
Owner Author

ravenclaw900 commented Jul 26, 2023

However, I see replacing higher level features with lower level features-util helped a little (I guess?).

$ ls -l
# --cut--
3127352 Jul 26  2023 dietpi-dashboard-futures
3127352 Jul 26  2023 dietpi-dashboard-futures-util

That was the hope, but it apparently made no difference at all, since everything that wasn't used was eliminated by the compiler. Honestly, that was more for cleaning up the dependency tree, because futures pulled in all of its sub-crates (including futures-util).

A release would be great indeed

Working on it, but since I'm just cherry-picking the TLS changes onto the old 0.6.1 release, I have to deal with the old build system, which isn't working correctly on CI. Probably once I get #594 merged, I'll make one more in the 0.6.x lineup before working on some actual breaking changes.

@ravenclaw900
Copy link
Owner Author

ravenclaw900 commented Jul 26, 2023

New plan: build all the release binaries using cross on my computer. Should get the same result, because cross builds are supposed to be reproducible; and I don't have to worry about breaking anything, because it's building from a different branch.

@MichaIng
Copy link
Collaborator

Next DietPi update enforces the dashboard reinstall/update: MichaIng/DietPi@611d775

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants