Server: verify the pubkey signature before emitting Userauth - #87
Conversation
Since 35dece0 the Userauth event carried an unverified signature, and each effectful layer was expected to call Server.verify_pubkeyauth itself. A layer that only checked the offered public key against its authorized list -- the part that looks like authentication -- would accept any client claiming to own that key. Verify in input_userauth_request instead, and reply with a userauth failure otherwise. The effectful layer still decides whether the key is authorized, so the database lookups and TOFU that 35dece0 enables keep working. Added a test.
Awa.Server verifies the signature itself, so Awa_mirage.Auth.verify and the test server are left with the authorization question alone: is this public key on the user's list? The unknown user branch of Awa_mirage.Auth.verify used to evaluate "verify_pubkeyauth ~user pubkeyauth && false" so that a missing username could not be spotted by timing the reply. The server now verifies for existing and non-existing users alike, so that branch collapses into the other unknown user case.
Nothing outside server.ml calls it, and lib/server.ml has no .mli, so it is public only by virtue of being a toplevel binding. Leaving it there suggests a caller is still expected to verify signatures itself. This breaks the public API, hence a separate commit: drop it if the helper should stay.
|
I think this is fine to merge & release -- any opinion @reynir (does it fit well with banawa-chat)? |
reynir
left a comment
reynir
left a comment
There was a problem hiding this comment.
This breaks banawa-chat in a minor way similar to the changes to awa_mirage.ml. That's okay.
I wonder if it's always desirable to spend CPU cycles to verify signatures? I think I introduced that following the logic from password authentication that you should hash passwords always to avoid user enumeration. I'm not sure this logic makes sense for public keys.
I wonder if this could be solved with documentation (of which there currently is none! Not even .mli files :'))
I think this could be done in a separate PR also implementing prober key probing (right now we just go "sure why not" as long as the key type looks like something we can handle). |
3 participants
Since #74 the server emits
Userauth (user, Pubkey _)without checking the signature. Each effectful layer is expected to callServer.verify_pubkeyauthitself. A layer that only checks the offered key against its authorized list accepts any client claiming to own that key.awa-mirageand the test server do call it, so neither is affected; a hand-written layer may not.This PR moves the verification in
input_userauth_requestinstead. The effectful layer keeps the authorization decision, so the lookups and TOFU that #74 enables still work.The last commit removes
Server.verify_pubkeyauth, the only breaking change in the series -- drop it if you'd rather keep the helper exported.