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

Add brute force protection to default password authentication #862

Closed
nynymike opened this Issue Aug 1, 2018 · 14 comments

Comments

Projects
None yet
4 participants
@nynymike
Contributor

nynymike commented Aug 1, 2018

We should cache authentication attempts for a given username, and add a delay of 5 seconds for before every fourth attempt. It would nice if we could expose this feature for use in interception scripts too.

@yurem

This comment has been minimized.

Contributor

yurem commented Aug 22, 2018

Implemented and updated default oxAuth config. It's turn off by default to support backward compatibility with 3.1.x.

Here is new config with values description:

	"authenticationProtectionConfiguration": {
		"attemptExpiration": 15, // How long store in cache information about particular login attempt. It's needed to count login attempts withing specified period of time
		"maximumAllowedAttempts": 10, // How many attempts application can allow. It's reserved to use in custom script. For example script can lock user account after specified number of attempts.
		"maximumAllowedAttemptsWithoutDelay": 4, // How many attempts application allow without delay
		"delayTime": 2, // Delay time in seconds after reaching maximumAllowedAttemptsWithoutDelay limit.
		"bruteForceProtectionEnabled": false // Enable or disable service, This functionality can be enabled dynamically. All other values requires restart
	}

oxAuth uses new service when there is no person script or when script call basic authentication API.

To get current list of non expired attempts from script we can use next code:

from org.xdi.service.cdi.util import CdiUtil
from org.xdi.oxauth.service import AuthenticationProtectionService

...
            authenticationProtectionService = CdiUtil.bean(AuthenticationProtectionService)
            nonExpiredAttempts = authenticationProtectionService.getNonExpiredAttempts(user_name)
...
            isReachAttemptCountLimit = authenticationProtectionService.isReachAttemptCountLimit(user_name)

@yurem yurem closed this Aug 22, 2018

@yurem

This comment has been minimized.

Contributor

yurem commented Aug 22, 2018

Let me know if we need to change properties names or enable it by default

@natt-tester

This comment has been minimized.

natt-tester commented Sep 11, 2018

@yurem, I've tested it in a fresh VM with Ubuntu 16 (RC3) with the following settings:
settings1
settings2
settings3
The delay started working, but clicking Login again gave me a proxy error:
https://ufile.io/dfr4o

Also, what's the expected behavior? After reaching the limit of attempts without delays, should the user be unable to log in with an incorrect credentials message?
logs.tar.gz

@natt-tester natt-tester reopened this Sep 11, 2018

@yurem

This comment has been minimized.

Contributor

yurem commented Sep 11, 2018

@natt-tester can you check if there is no proxy error in new CE version?

@yurem yurem closed this Sep 11, 2018

@natt-tester

This comment has been minimized.

natt-tester commented Sep 12, 2018

@yurem, I can't reproduce the proxy error in RC4. However, the question of the expected behavior still stands. Is the delay time in seconds? What's the limit? If I set it to 600, how should it affect the basic login process with the below settings? There's a typo in the header, please correct it :)

set1

delay1

@willow9886

This comment has been minimized.

Contributor

willow9886 commented Sep 12, 2018

Is the delay time in seconds?

Yes, it should be. All configuration time values are seconds as far as I know.

Ill let @yurem comment on the other questions.

@natt-tester

This comment has been minimized.

natt-tester commented Sep 17, 2018

@yurem, could you please answer the questions above?

@natt-tester natt-tester reopened this Sep 17, 2018

@yurem

This comment has been minimized.

Contributor

yurem commented Sep 17, 2018

@natt-tester If I set it to 600, how should it affect the basic login process with the below settings?: yes, admin can do this. User will get timeout or proxy error.
I've fixed typo in label.

@natt-tester

This comment has been minimized.

natt-tester commented Sep 19, 2018

@yurem I've tested the setup you sent me:
brute

and my observations are as follows:

  • the functionality itself worked, but waiting 1min for oxtrust to load the changes didn't work, had to restart the service to change maximumAllowedAttemptsWithoutDelay (cache clearing and a new incognito window didn't work either)

  • when I changed the delayTime to 200, it broke the functionality: before there was a delay after every 4th login request, and in this scenario the delay occurred after every request starting with the 4th one

  • I still don't know what maximumAllowedAttempts does: changing the value didn't seem to influence the behavior

Could you please provide an exact description on how it's supposed to work? I can do more exploratory testing, but I don't think this is optimal before the release.

@yurem

This comment has been minimized.

Contributor

yurem commented Sep 20, 2018

  1. Can you try if reload configuration work well in RC6?
  2. 200 seconds is wrong value. You will get proxy timeout or other error not related to oxAuth. See original subject. The right value should be at least less than 20 seconds.
  3. I removed maximumAllowedAttempts it in order RC6 to avoid questions. maximumAllowedAttemptsWithoutDelay is enough for this flow.
@natt-tester

This comment has been minimized.

natt-tester commented Sep 21, 2018

@yurem, please tag me in all related comments

  1. Yes, the reload configuration works well now, tested in c7 with several different settings.
  2. I know what the original idea is, but then why do we have these fields editable with no min/max values info? I think it would make a great UX difference, and it's better to ask now than wait for users' questions
  3. Good call ;)
@yurem

This comment has been minimized.

Contributor

yurem commented Sep 24, 2018

@natt-tester From my point of view it's admin responsibility about to make decision about right value.

For example: How we can explain him that we not allow to specify in this field value bigger than 100 or 150?

@yurem

This comment has been minimized.

Contributor

yurem commented Sep 24, 2018

Currently there is no upper limit of each configuration values in oxAuht/oxTrust: https://github.com/GluuFederation/oxTrust/blob/master/static/src/main/resources/META-INF/resources/schema/oxauth-config.xml.json

From my point of view we need to open separate issue and make decisions after discussion that if we need to apply upper limit to all integer value in configure.

@natt-tester

This comment has been minimized.

natt-tester commented Sep 25, 2018

@yurem, it's a good idea to discuss it further -- I think at least an info string for a field would be nice if we don't want to introduce any limits. Apart from that, I've tested the functionality with different settings in RC7 and it seems to be working fine. I'm closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment