-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Limit unsuccessful login attempts on the console #878
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
b045f39
to
0962517
Compare
@@ -81,10 +86,20 @@ func (s *ConsoleServer) Authenticate(ctx context.Context, in *console.Authentica | |||
role = console.UserRole_USER_ROLE_ADMIN | |||
uname = in.Username | |||
id = uuid.Nil | |||
} else { | |||
if lockout, until := s.loginAttemptCache.Add(s.config.GetConsole().Username, ip); lockout != LockoutTypeNone { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a bug in extractClientAddressFromContext
allowing IP address to be spoofed, making it easy to bypass this limiter. Bug is that IP address extracted as a first element of x-forwarded-for header, not rightmost (N - 1), where N is number of known proxies, as a result IP address as nakama knows it is entirely controlled by user.
Even without IP spoofing, with enough IP addresses available it looks like it is quite possible to exhaust server memory by authenticating with sufficiently log usernames. If we truncate username to something like 20 chars before storing in the cache it will be much harder to pull it off
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per this the left-most address is supposed to indicate the client address, assuming trustworthy proxies across the chain. Either way we'll leave IP-based restrictions for a later stage.
Account lockouts are only tracked for valid accounts that actually exist, a bad actor cannot fill up the list with arbitrary usernames.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assuming trustworthy proxies across the chain.
But we dont have trustworthy chain. User can make request with populated X-Forwarded-For and it will be the IP address nakama uses, because each intermediate hop appends to the list, so selecting ips[0] picks one provided by the user.
Account lockouts are only tracked for valid accounts that actually exist, a bad actor cannot fill up the list with arbitrary usernames.
I see, I misread the code.
Adds a cache of failed login attempts, providing both account-based and IP-based lockouts after a number of failed login attempts have been received in a determined amount of time.
The user is locked from making any further login attempts until it automatically unlocks after a period of time.
A successful login clears the failed attempts for that account, but cached IP failed attempts only clear after the specific timespan.