charmbracelet / wish

Make SSH apps, just like that! 💫
MIT License
3.5k stars 69 forks source link

Auth is handled before the ratelimiter middleware #325

Open shaunco opened 4 weeks ago

shaunco commented 4 weeks ago

Describe the bug Rate limiting (fail2ban) is usually used to prevent brute force auth attacks against a server. Wish offers both authentication and ratelimiting, but seems to call the auth handler first and then only calls the ratelimiter middleware if auth succeeds. Calling the rate limiter first produces the desired effect of preventing brute force auth attacks.

To Reproduce

s, err := wish.NewServer(
    wish.WithAddress(":2222"),
    wish.WithHostKeyPath(myHostKeyPath),
    wish.WithPasswordAuth(func(ctx ssh.Context, password string) bool {
        return password == "password"
    }),
    wish.WithMiddleware(
        ratelimiter.Middleware(ratelimiter.NewRateLimiter(1, 2, 1000)),
    ),
)

In the above server, auth is always called - regardless of rate limits, and the ratelimiter Middleware is only called if auth succeeds (returns true).

Expected behavior Rate limiting should happen before auth, perhaps via s.ConnCallback instead of middleware.

pushand commented 2 days ago

does the order matter? move middleware before WithPasswardAuth?

shaunco commented 2 days ago

The order doesn't matter. The WithMiddleware writes to an array, but WithPasswordAuth (and the other auth methods) each write to a different struct variable inside the Server struct, which are processed before the Middleware.