add max-auth-attempts - #621
Conversation
Limit the number of failed SSH authentication attempts before the server disconnects the client, default 3.
Roytak
left a comment
Roytak
left a comment
There was a problem hiding this comment.
Hi, thank you for the contribution. This feature was part of the code a couple years ago, but we decided to remove it. Is there any specific reason why you find it useful to have? I found a couple things that will need to be adjusted and also you should add a new test e.g. to test_ssh.c that tests this.
| leaf max-auth-attempts { | ||
| type uint16 { | ||
| range "1..max"; | ||
| } | ||
| default 3; | ||
| description | ||
| "Maximum number of failed SSH authentication attempts before disconnecting the client."; | ||
| } |
There was a problem hiding this comment.
Mention in the description that it is per connection. I think that the RFC4252 should be referenced here.
Furthermore, this description should probably mention something along the lines "each failed authentication request, including rejected public keys, counts as one attempt", because some SSH clients try all their configured public keys and hence hitting the 3 might be easy, so just so the users are aware.
| /* user not known, set his authentication methods to public key only so that | ||
| * there is no interaction and it will simply be denied */ | ||
| ERR(NULL, "User \"%s\" not known by the server.", username); | ||
| ssh_set_auth_methods(session->ti.libssh.session, SSH_AUTH_METHOD_PUBLICKEY); | ||
| ssh_message_reply_default(msg); | ||
| return 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
ssh_auth_attempts need to be increased here, otherwise it would always remain 0 and that would lead to unlimited authenticaiton attempts (e.g. 3 would never be reached by the calling function) + we need to either reach the code checking if we hit max attempts from here or copy it here.
| ssh_message_reply_default(msg); | ||
| if (session->opts.server.ssh_max_auth_attempts && | ||
| (session->opts.server.ssh_auth_attempts >= session->opts.server.ssh_max_auth_attempts)) { | ||
| /* disconnect immediately so client does not prompt again */ |
There was a problem hiding this comment.
I think that both ssh_message_reply_default and ssh_disconnect should be called here, the abrupt disconnect is unnecessary.
| ++session->opts.server.ssh_auth_attempts; | ||
| VRB(session, "Authentication method \"%s\" not supported.", str_method); | ||
| ssh_message_reply_default(msg); | ||
| return 0; |
There was a problem hiding this comment.
We could make this more consistent, by just setting ret = 1 and then down below the max attempts check would happen + the possible disconnect. Mention this in a comment above the VRB.
2 participants
Limit the number of failed SSH authentication attempts before the server disconnects the client, default 3.