session server ssh FEATURE lock an account out after repeated failed password authentication - #640
niklas-moser wants to merge 1 commit into
Conversation
20c3063 to
dccb1e7
Compare
Roytak
left a comment
Roytak
left a comment
There was a problem hiding this comment.
Thanks for the contribution and sorry for the delay! I think this feature is very useful, but will need a bit more work - mainly making this optional (off by default) and configurable. Also could add at least one test e.g. for locking an account out after N attempts.
|
Thank you @Roytak . I added two commits for you to review regarding the requested changes. If you want I can squash them after re-review, or you can do it. |
Roytak
left a comment
Roytak
left a comment
There was a problem hiding this comment.
Needs some minor changes + 1 bigger one to nc_authlock_get. Also needs a rebase on top of devel due to a conflict. Otherwise okay.
5e38d57 to
3be86c3
Compare
|
Hey @Roytak . Thank you for the re-review. I forced-pushed an updated commit. Let me know what you think now! |
Roytak
left a comment
Roytak
left a comment
There was a problem hiding this comment.
Please remove the persistence part, the standard does not require it and it makes the code needlessly complicated. The review is then mostly centered around the new YANG, where I suggested some rewordings.
3be86c3 to
01e2121
Compare
|
Thanks a lot @Roytak . Removing the file mirror simplifies the code a lot. I pushed the requested changes, let me know if there is something else to improve. |
Roytak
left a comment
Roytak
left a comment
There was a problem hiding this comment.
Thanks, the in-memory tally keyed on the username looks good now. The main remaining point is making the lockout a single server-wide policy (see the comment on the password-lockout container), the rest are smaller fixes to the YANG descriptions. After this it should be an okay from me.
01e2121 to
c690a3f
Compare
|
Thank you @Roytak . I moved the container to make the ssh-lockout a single server-side policy. As a result other parts of the code had to be changed as well:
Does that seem okay to you? |
Roytak
left a comment
Roytak
left a comment
There was a problem hiding this comment.
Thanks, the server-wide lockout looks good. Remaining are small things.
…word authentication Nothing reads ssh_auth_attempts, so password guessing is unlimited both within a connection and across them. auth-timeout bounds how long one authentication may take, not how many may be tried, and pam_faillock only ever sees the keyboard-interactive method. Add max-auth-attempts to the SSH client-authentication of libnetconf2-netconf-server, which disconnects a session after that many failed authentication attempts, and an opt-in, server-wide ssh-password-lockout container under ln2-netconf-server. When present, consecutive failed password-based authentications are counted per user across connections and all SSH endpoints, listening and Call Home, and the user is refused password-based authentication for duration seconds after max-consecutive-failures. The hooks sit on the three credential checks in session_server_ssh.c that both auth backends funnel through, so the password, system keyboard-interactive and PAM methods share one tally. A custom keyboard-interactive callback is not covered. The tally is kept in memory and only ever holds users known to the server. Public key auth is deliberately not counted, which keeps a locked out deployment recoverable. Motivated by 3GPP TS 33.117 4.2.3.4.5 (consecutive failed login attempts) and 4.2.3.4.3.3 (brute force and dictionary attacks). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
c690a3f to
0b80601
Compare
|
Thanks @Roytak . Updated |
Roytak
left a comment
Roytak
left a comment
There was a problem hiding this comment.
Looking good, thanks. I think that's all from me, but will need a final review from @michalvasko .
michalvasko
left a comment
michalvasko
left a comment
There was a problem hiding this comment.
Looks okay, thanks, just one minor issue.
| time_t now = time(NULL); | ||
| char *name; | ||
|
|
||
| if (!opts->max_fails || !username) { |
There was a problem hiding this comment.
I do not think username can be NULL.
3 participants
Nothing reads ssh_auth_attempts, so password guessing is unlimited both within a connection and across them. auth-timeout bounds how long one authentication may take, not how many may be tried, and pam_faillock only ever sees the
keyboard-interactive method — accounts using hashed-password are verified by libnetconf2 itself with crypt(3).
This counts consecutive password failures per account across connections and refuses the account for NC_AUTHLOCK_TIME after NC_AUTHLOCK_MAX_FAILS. The hooks sit on the three credential checks in session_server_ssh.c that both the message-and callback-based backends funnel through (auth_password_check, kbdint_verify_passwd, pam_authenticate), so all three methods share one tally and neither dispatch file is touched. The tally is mirrored to a state file and re-read when it changes, so a lockout survives a restart and can be cleared on a running server. A session hitting NC_AUTHLOCK_SESSION_MAX_FAILS is disconnected — ssh_auth_attempts finally gets a reader.
Public key auth is deliberately not counted, which keeps a locked out deployment recoverable. TLS is unaffected.
Open question: the policy is compiled in (5 failures, 300 s, 900 s window, 6 per session, 64 accounts). ietf-netconf-server has no leaves for it and your augment carries only auth-timeout, so making it configurable means extending
libnetconf2-netconf-server.yang. If you want to make it configurable, we can change the approach there
Motivated by O-RAN WG11 R004 / 3GPP TS 33.117 4.2.3.4.3.1.