[Enhancement]Lock account for timeout duration after failed login attempts #51948
Conversation
|
|
@mergify rebase |
…empts Signed-off-by: Rohit Satardekar <[email protected]>
✅ Branch has been successfully rebased |
There was a problem hiding this comment.
@rohitrs1983 what if the user account get locked and then never attempted for a long time. The failed record is not cleaned. May need to add periodic task to clean expired locks.
…empts Signed-off-by: Rohit Satardekar <[email protected]>
@kevincai have fix the issue. i have added the cleanup mechanism as part of password validation process with a default cleanup timeout is 24h. |
|
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
| return checkPasswordForNative(remoteUser, remoteHost, remotePasswd, randomString); | ||
| UserIdentity authenticatedUser = null; | ||
| String userAndHost = remoteUser.concat("@").concat(remoteHost); | ||
| for (Map.Entry<String, Long> attempt : FAIL_TIME.entrySet()) { |
There was a problem hiding this comment.
This is not actually related to the checkPassword to the given remoteUser/remoteHost. Can extract it to a separate method to explicit what it is doing.
Also can reduce the check interval of the cleanup, no need to check for every checkPassowrd, but probably every 30 seconds or every minute is good enough.
| if (!isUserLocked(attempt.getKey())) { | ||
| clearFailedAttemptRecords(attempt.getKey()); | ||
| } else { | ||
| if (getRemainingLockedTime(attempt.getKey()) <= 0) { | ||
| clearFailedAttemptRecords(attempt.getKey()); | ||
| } | ||
| } |
There was a problem hiding this comment.
| if (!isUserLocked(attempt.getKey())) { | |
| clearFailedAttemptRecords(attempt.getKey()); | |
| } else { | |
| if (getRemainingLockedTime(attempt.getKey()) <= 0) { | |
| clearFailedAttemptRecords(attempt.getKey()); | |
| } | |
| } | |
| if (!isUserLocked(attempt.getKey()) || getRemainingLockedTime(attempt.getKey()) <= 0) { | |
| clearFailedAttemptRecords(attempt.getKey()); | |
| } |
| clearFailedAttemptRecords(userAndHost); | ||
| } | ||
| } | ||
| resetFailedAttemptRecords(); |
There was a problem hiding this comment.
ditto, shall move this to the periodic cleanup routine.
| String userAndHost = authInfo.fullUserName.concat("@").concat(authInfo.remoteIp); | ||
| if (globalStateMgr.getAuthenticationMgr().isUserLocked(userAndHost)) { | ||
| throw new AccessDeniedException("Access denied for " + userAndHost + ". Locked for " + | ||
| globalStateMgr.getAuthenticationMgr().getRemainingLockedTime(userAndHost) + " seconds."); |
There was a problem hiding this comment.
not sure it is good or not to show end user the seconds remaining for next attempt.
| return null; | ||
| } | ||
| authenticatedUser = checkPasswordForNative(remoteUser, remoteHost, remotePasswd, randomString); | ||
| if (Config.max_failed_login_attempts >= 0) { |
There was a problem hiding this comment.
is it possible to prompt to user, the account will be locked for next xx fail attempts
Lock account for timeout duration after failed login attempts
Why I'm doing:
What I'm doing:
Fixes #51917
$ mysql -h127.0.0.1 -P9030 -u jack -p
Enter password:
ERROR 1045 (28000): Access denied for user 'jack' (using password: YES)
$ mysql -h127.0.0.1 -P9030 -u jack -p
Enter password:
ERROR 1045 (28000): Access denied for user 'jack' (using password: YES)
$ mysql -h127.0.0.1 -P9030 -u jack -p
Enter password:
ERROR 5204 (HY000): Access denied for user 'jack'@'127.0.0.1'. Account is blocked for 30 seconds due to 3 consecutive failed logins.
$ mysql -h127.0.0.1 -P9030 -u jack -p
Enter password:
ERROR 5204 (HY000): Access denied for user 'jack'@'127.0.0.1'. Account is blocked for 4 seconds due to 3 consecutive failed logins.
$ mysql -h127.0.0.1 -P9030 -u jack -p
Enter password:
mysql>
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: