Refactor authentication, delay to effectful layer #74
Conversation
Userauth is now an event. This means the effectful layer can do e.g. database lookups or LDAP requests when authenticating a user. As an added bonus an effectful layer that implements trust on first use (TOFU) user authentication is now possible.
lib/auth.ml
Outdated
| | Some { password = Some password; _ }, Password password' -> | ||
| let open Digestif.SHA256 in | ||
| let a = digest_string password | ||
| and b = digest_string password' in |
There was a problem hiding this comment.
seeing it like this, I was wondering whether we should store the password as a hash or somehow derived in the memory of the server -- but this can obviously be another PR since AFAICT this PR doesn't change it, only makes it more obvious
| else | ||
| Awa.Server.reject_userauth t.server userauth | ||
| in | ||
| send_msg { t with server } reply |
There was a problem hiding this comment.
should this logic be done inside of awa -- i.e. provide a function:
let auth ? success =
if success then
Awa.Server.accept_userauth t.server userauth
else
Awa.Server.reject_userauth t.server userauththis way we wouldn't repeat that in both the test and the mirage server!?
There was a problem hiding this comment.
or, maybe to reduce the error conditions, we could provide the Userauth event with "yay" and "nay" (which are accept_userauth / reject_userauth - which wouldn't need to check the state - i.e. there shouldn't be the error case).
There was a problem hiding this comment.
(Sorry I pressed the "resolve conversation" button instead of the "comment" button and at the same time lost my comment...)
This is interesting. I think we then need to make sure that the Server.t doesn't progress through reading and processing incoming packets?
I think it makes sense to combine accept_userauth / reject_userauth to a variant that takes a bool
There was a problem hiding this comment.
make sure that the
Server.tdoesn't progress
being functional and value-passing, isn't it the case that the effectful layer only has that information -- a mutable Server.t -- and from within the core protocol (server.ml) we're not able to verify that "Server.t has not progressed".
lib/auth.ml
Outdated
| type pubkeyauth = { | ||
| pubkey : Hostkey.pub ; | ||
| session_id : string ; | ||
| service : string ; | ||
| sig_alg : Hostkey.alg ; | ||
| signed : string ; | ||
| } | ||
|
|
||
| let pubkey_of_pubkeyauth { pubkey; _ } = pubkey | ||
|
|
||
| type userauth = | ||
| | Password of string | ||
| | Pubkey of pubkeyauth | ||
|
|
There was a problem hiding this comment.
Moving this into Auth made sense to me as the related verification code lives there. But it makes it a bit more annoying to make pubkeyauth opaque as we need to construct it in Server.
Of course, we can create a awa.mli and better control what is exposed to library users.
There was a problem hiding this comment.
After some thought: Maybe this is moved back to Server.ml, and the Auth.user/Auth.db related code is moved into Awa_mirage?
There was a problem hiding this comment.
I'd be happy to have a awa.mli :) about code moving, I'm not sure, if you think it is worth, please do it :) though moving purely functional stuff into the effectful mirage layer is something I'd avoid.
There was a problem hiding this comment.
Another motivation for me of moving that code to Awa_mirage is I may eventually want to get rid of that code tbh. After all, a (static) list of users in memory is maybe not the best fit for all. We may want to use various password hashing algorithms. Or we may accept public keys based on their fingerprint (this may be a very bad idea; I don't know).
| @@ -266,6 +278,22 @@ let rec input_userauth_request t username service auth_method = | |||
| else | |||
| handle_auth t | |||
|
|
|||
| let reject_userauth t _userauth = | |||
There was a problem hiding this comment.
The reason for the unused userauth argument is that in a .mli we can better ensure proper protocol flow as you can only call it if you've got a userauth in your hand - which, if made opaque, you can only get from a Userauth _ event.
| let t = { t with auth_state = Auth.Inprogress (u, s, succ nfailed) } in | ||
| Ok (t, Ssh.Msg_userauth_failure ([ "publickey"; "password" ], false)) | ||
| | Auth.Done | Auth.Preauth -> | ||
| Error "userauth in unexpected state" |
There was a problem hiding this comment.
I don't know if we should raise invalid argument here instead?!
| fun user userauth -> | ||
| match user, userauth with | ||
| | "foo", Awa.Server.Password "bar" -> | ||
| true | ||
| | "awa", Awa.Server.Pubkey pubkeyauth -> | ||
| Awa.Server.verify_pubkeyauth ~user:"awa" pubkeyauth && | ||
| Awa.Server.pubkey_of_pubkeyauth pubkeyauth = key | ||
| | _ -> false |
There was a problem hiding this comment.
This shows a different way to do authentication - we could even read the public key on request.
Move bits of Auth into Server and Awa_mirage.
Userauth is now an event. This means the effectful layer can do e.g. database lookups or LDAP requests when authenticating a user.
As an added bonus an effectful layer that implements trust on first use (TOFU) user authentication is now possible (like banawa-ssh used in banawa-chat).
I also generated .mli files auth.mli and server.mli that I can commit. There I make
Awa.Server.userauths constructors private, andAwa.Server.pubkeyauthis opaque.TODO is updating the tests / awa_test_server.