Skip to content
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.

Sign up for GitHub

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

Jump to bottom

AM-90: mfa verify brute force detection implemented #2220

Merged
merged 1 commit into from
Nov 22, 2022
Merged

AM-90: mfa verify brute force detection implemented #2220

merged 1 commit into from
Nov 22, 2022

Conversation

ashraf706
Copy link
Contributor

jira issue: https://graviteecommunity.atlassian.net/browse/AM-90

how to test the feature:

  1. Create email or sms factor.
  2. Enable the factor in an application and enroll, then verify for a user to make sure everything is fine as before.
  3. Now from User Accounts menu enable brute force detection from the newly added MFA section.
  4. For testing purpose input could be max challenge attempt 3, reset time 1 minutes and enable send email option
  5. Save the settings.
  6. Now trigger login flow and during MFA verify steps enter 3 wrong value and try to verify. You should receive invalid code error as before, no behaviour change in this step.
  7. Provide wrong/right code value on the 4th time and you should receive maximum attempts reached error.
  8. An email will be sent to the user account along with an audit log entry for the attempt reached entry.
  9. Try again after 1 minute with correct value and verification should be completed successfully.

@ashraf706 ashraf706 requested a review from a team as a code owner November 9, 2022 09:45
farmborough
farmborough suggested changes Nov 9, 2022
View reviewed changes
Copy link
Contributor

@farmborough farmborough left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like quite a large PR that perhaps should have been broken down into smaller subtasks - the repo stuff could have been done separately, for example.

gravitee-am-service/src/main/java/io/gravitee/am/service/impl/VerifyAttemptServiceImpl.java Show resolved Hide resolved
...-am-gateway-standalone-distribution/src/main/resources/templates/i18n/messages_en.properties Outdated Show resolved Hide resolved
...-am-gateway-standalone-distribution/src/main/resources/templates/i18n/messages_en.properties Outdated Show resolved Hide resolved
...-am-gateway-standalone-distribution/src/main/resources/templates/i18n/messages_en.properties Outdated Show resolved Hide resolved
...agement-api-standalone-distribution/src/main/resources/templates/i18n/messages_en.properties Outdated Show resolved Hide resolved
leleueri
leleueri requested changes Nov 14, 2022
View reviewed changes
...in/java/io/gravitee/am/gateway/handler/root/resources/endpoint/mfa/MFAChallengeEndpoint.java Outdated
() -> handler.handle(Future.succeededFuture()),
error -> {
logger.error("Could not delete verify attempt", error);
routingContext.fail(401);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to reject the authentication/code validation if the delete action fails ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, we do not as I understand. Delete performs after successful verification. That means in this stage everything is fine (challenge verified) and deletes if there is any existing verification attempts in the db. It shouldn't have any impact in verification process if the delete is failed for unexpected reason in my opinion.

Copy link
Contributor

@leleueri leleueri Nov 15, 2022 •
edited by ashraf706
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok in that case, we should not call routingContext.fail(401); after the log error (maybe it becomes a warn 🤔 ?)
As we do not want to fail if the delete fails, we should call handler.handle(Future.succeededFuture()) instead of routingContext.fail(401);

wdyt?

Agree: changed log to warn and removed the fail 401

gravitee-am-service/src/main/java/io/gravitee/am/service/impl/VerifyAttemptServiceImpl.java Show resolved Hide resolved
gravitee-am-service/src/main/java/io/gravitee/am/service/impl/VerifyAttemptServiceImpl.java Outdated Show resolved Hide resolved
...in/java/io/gravitee/am/gateway/handler/root/resources/endpoint/mfa/MFAChallengeEndpoint.java Outdated Show resolved Hide resolved
...in/java/io/gravitee/am/gateway/handler/root/resources/endpoint/mfa/MFAChallengeEndpoint.java Outdated Show resolved Hide resolved
@ashraf706 ashraf706 force-pushed the AM-90-mfa-challenge-brute-force-detection branch 2 times, most recently from 8bb4ea8 to 6ed373d Compare November 15, 2022 08:30
@ashraf706 ashraf706 requested a review from leleueri November 15, 2022 09:37
leleueri
leleueri requested changes Nov 15, 2022
View reviewed changes
...in/java/io/gravitee/am/gateway/handler/root/resources/endpoint/mfa/MFAChallengeEndpoint.java Outdated
() -> handler.handle(Future.succeededFuture()),
error -> {
logger.error("Could not delete verify attempt", error);
routingContext.fail(401);
Copy link
Contributor

@leleueri leleueri Nov 15, 2022 •
edited by ashraf706
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok in that case, we should not call routingContext.fail(401); after the log error (maybe it becomes a warn 🤔 ?)
As we do not want to fail if the delete fails, we should call handler.handle(Future.succeededFuture()) instead of routingContext.fail(401);

wdyt?

Agree: changed log to warn and removed the fail 401

.../main/java/io/gravitee/am/service/reporter/builder/management/VerifyAttemptAuditBuilder.java Outdated Show resolved Hide resolved
.../main/java/io/gravitee/am/service/reporter/builder/management/VerifyAttemptAuditBuilder.java Outdated Show resolved Hide resolved
@ashraf706 ashraf706 force-pushed the AM-90-mfa-challenge-brute-force-detection branch from 7d7103d to c05d754 Compare November 16, 2022 09:20
@ashraf706 ashraf706 requested a review from leleueri November 16, 2022 09:23
@ashraf706 ashraf706 force-pushed the AM-90-mfa-challenge-brute-force-detection branch from c05d754 to 2f27ab0 Compare November 21, 2022 15:55
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.8% 0.8% Coverage
15.4% 15.4% Duplication

@ashraf706 ashraf706 merged commit 5e484eb into master Nov 22, 2022
@ashraf706 ashraf706 deleted the AM-90-mfa-challenge-brute-force-detection branch November 22, 2022 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@farmborough farmborough farmborough approved these changes

@leleueri leleueri Awaiting requested review from leleueri

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ashraf706 @leleueri @farmborough