Remove account lockout from failed recovery attempts #35325

jentfoo · 2023-12-04T18:39:23Z

This account lockout looks to be unnecessary and potentially problematic. Recovery codes and recovery through MFA are not possible to brute force.

In addition the potential to lockout an account from being able to use a recovery method could result in them being unable to unlock their account from other potential abuse cases (for example an attacker locking the account from failed password attempts).

For that reason I believe we should remove this lock as part of Teleport 15. Opening this PR for discussion, please highlight if there are any concerns which motivated this account lockout and which I may have missed.

github-actions · 2023-12-04T18:39:54Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

lib/auth/accountrecovery_test.go

lib/auth/accountrecovery.go

lib/services/identity.go

rfd/0029-account-lifecycle.md

russjones

@jentfoo Can you link me to the RFD where this change was discussed and approved?

jentfoo · 2023-12-05T22:22:48Z

@russjones The only RFD changes are the removal of the requirement from the existing RFD. As mentioned in the description I opened this PR to prompt exactly that type of discussion.

To reiterate the motivation for bringing this change up for discussion:

There is currently the potential for a targeted attack on a user to result in locking that user locked out and unable to recover. Removing this mechanism would ensure our users have a means to access their account even in the case of a targeted attack.
There is not an obvious need to a lock out on recovery failure. Recovery methods appear to be system controlled and high enough entropy to be brute force resistant.

Let me know what additional details you would like to have captured, and what your thoughts around this change are. Thank you!

russjones · 2023-12-06T01:19:28Z

@jentfoo Is this tied to a compliance goal or issue that will reach it's SLA if not addressed?

jentfoo · 2023-12-06T03:01:51Z

@russjones There is a potential DoS risk here in that a user can be targeted and prevented access to their account. However we have not filed a report against it yet. I wanted to submit this PR for discussion and to explore if this is something we can improve.

If you would like me to file an issue to link to this, I can do that. I just thought this was a better way to open up the discussion.

jentfoo · 2023-12-06T22:09:14Z

@russjones The report that motivated this PR is now filed under https://github.com/gravitational/security-findings/issues/75

Let me know if you have any additional questions, or feedback on this change. Thank you!

russjones · 2023-12-07T00:29:12Z

@jentfoo Please propose changes via our RFD process if you want to change the behavior of Teleport.

jentfoo · 2023-12-08T00:10:05Z

@russjones I submitted the RFD under the PR here: #35533

The RFD is fairly light in content because this is a relatively isolated change, but hopefully it will provides a better description of why we want to make this improvement. Please let me know your thoughts and questions, thank you!

jentfoo · 2024-01-09T20:53:05Z

@zmb3 and @codingllama, I have partially updated this PR. This will depend on the e update I just submitted here: https://github.com/gravitational/teleport.e/pull/3118

I would appreciate a look around the API changes (mostly captured in the most recent commit). I believe I have removed all possible API as discussed in the RFD. I have kept the RecoveryAttemptLockExpires field in the protobuf but renamed as DEPRECATED, let me know if there is a more standard way to maintain compatibility with these changes. Thank you!

github-actions · 2024-01-09T20:55:17Z

🤖 Vercel preview here: https://docs-lmolodche-goteleport.vercel.app/docs/ver/preview

api/proto/teleport/legacy/types/types.proto

github-actions · 2024-01-09T23:08:45Z

🤖 Vercel preview here: https://docs-laqayemcy-goteleport.vercel.app/docs/ver/preview

strideynet · 2024-01-10T09:35:51Z

integrations/operator/crdgen/testdata/protofiles/teleport/legacy/types/types.proto

-    (gogoproto.nullable) = false,
-    (gogoproto.jsontag) = "recovery_attempt_lock_expires,omitempty"
-  ];
+  reserved 5;  // removed RecoveryAttemptLockExpires after lockout was removed


Usually I see these reserve blocks at the top of the message - but unsure if that's a standard thing.

AFAIK this goes either way. Not sure if we have a consensus.

github-actions · 2024-01-24T00:15:39Z

🤖 Vercel preview here: https://docs-kxlh7p8g5-goteleport.vercel.app/docs/ver/preview

github-actions · 2024-01-24T00:25:33Z

🤖 Vercel preview here: https://docs-aac30f8qy-goteleport.vercel.app/docs/ver/preview

zmb3

We're almost there.

lib/auth/accountrecovery.go

lib/services/identity.go

github-actions · 2024-01-24T04:01:27Z

🤖 Vercel preview here: https://docs-8oucg7azh-goteleport.vercel.app/docs/ver/preview

zmb3 · 2024-01-24T18:12:53Z

/excludeflake *

public-teleport-github-review-bot · 2024-01-24T18:38:02Z

@jentfoo See the table below for backport results.

Branch	Result
branch/v15	Failed

* Remove account lockout from failed recovery attempts This account lockout looks to be unecessary and potentially problematic. Recovery codes and recovery through MFA are not possible to brute force. In addition the potential to lockout an account from being able to use a recovery method could result in them being unable to unlock their account from other potential abuse cases (for example an attacker locking the account from failed password attempts). As discussed in the RFD (#35533) this includes the removal of all the API used for this locking mechanism. * accountrecovery: Update `WithLock` function names * accountrecovery: Combine verifyRecoveryCode and verifyRecoveryCodeWithRecord into one function

codingllama · 2024-01-31T22:05:34Z

api/proto/teleport/legacy/types/types.proto

+  reserved 5; // removed "google.protobuf.Timestamp RecoveryAttemptLockExpires" after lockout was removed
+  reserved "RecoveryAttemptLockExpires";


The removal of this field is making UpdateAndSwapUser fail.

If the stored user had the "recovery_attempt_lock_expires" field set, then the comparison always fails. That is because when we read and unmarshal the user the unknown field gets dropped, but the actual CompareAndSwap operation compares using the storage blobs instead.

Reverting the deletion, but keeping the field deprecated should serve as an immediate fix. Arguably, a better fix is to make sure CompareAndSwap compare apples-to-apples: either user vs user, or blob vs blob, but never user vs blob.

@jentfoo @rosstimothy

I just submitted a PR to bring the field back: #37618

Thank you for finding this issue @codingllama!

Also removes the outdated mention of locking accounts after failed password reset attempts. This functionality was removed in #35325 but the docs were not updated. Closes #41135

Also removes the outdated mention of locking accounts after failed password reset attempts. This functionality was removed in #35325 but the docs were not updated. Closes #41135 Co-authored-by: Steven Martin <steven@goteleport.com>

jentfoo requested review from Tener, zmb3, codingllama, rosstimothy and kimlisa December 4, 2023 18:39

jentfoo self-assigned this Dec 4, 2023

github-actions bot requested a review from jimbishopp December 4, 2023 18:39

github-actions bot added rfd Request for Discussion size/sm labels Dec 4, 2023

jentfoo added the no-changelog Indicates that a PR does not require a changelog entry label Dec 4, 2023

codingllama reviewed Dec 5, 2023

View reviewed changes

lib/auth/accountrecovery_test.go Show resolved Hide resolved

lib/auth/accountrecovery.go Show resolved Hide resolved

lib/services/identity.go Show resolved Hide resolved

rfd/0029-account-lifecycle.md Outdated Show resolved Hide resolved

russjones previously requested changes Dec 5, 2023

View reviewed changes

jentfoo mentioned this pull request Dec 8, 2023

RFD 158: Description of how we should address a targeted account auth DoS #35533

Merged

jentfoo requested a review from russjones December 14, 2023 19:31

jentfoo force-pushed the jent/remove_recovery_failure_lockout branch from daf5c3f to c4fabf6 Compare January 9, 2024 20:50

jentfoo temporarily deployed to vercel January 9, 2024 20:50 — with GitHub Actions Inactive

zmb3 reviewed Jan 9, 2024

View reviewed changes

api/proto/teleport/legacy/types/types.proto Outdated Show resolved Hide resolved

jentfoo temporarily deployed to vercel January 9, 2024 23:03 — with GitHub Actions Inactive

strideynet approved these changes Jan 10, 2024

View reviewed changes

jentfoo force-pushed the jent/remove_recovery_failure_lockout branch from 9f8fb88 to c5fbce0 Compare January 10, 2024 18:51

Further GRPC cleanup after PR feedback

b44adb9

jentfoo force-pushed the jent/remove_recovery_failure_lockout branch from 244f60c to b44adb9 Compare January 24, 2024 00:18

jentfoo temporarily deployed to vercel January 24, 2024 00:18 — with GitHub Actions Inactive

zmb3 reviewed Jan 24, 2024

View reviewed changes

lib/auth/accountrecovery.go Outdated Show resolved Hide resolved

lib/auth/accountrecovery.go Outdated Show resolved Hide resolved

lib/services/identity.go Show resolved Hide resolved

Apply PR Feedback

4add256

jentfoo temporarily deployed to vercel January 24, 2024 03:55 — with GitHub Actions Inactive

zmb3 approved these changes Jan 24, 2024

View reviewed changes

public-teleport-github-review-bot bot removed request for jimbishopp and rosstimothy January 24, 2024 18:11

jentfoo enabled auto-merge January 24, 2024 18:16

jentfoo added this pull request to the merge queue Jan 24, 2024

Merged via the queue into master with commit 09a33d8 Jan 24, 2024
40 checks passed

jentfoo deleted the jent/remove_recovery_failure_lockout branch January 24, 2024 18:37

jentfoo mentioned this pull request Jan 24, 2024

[v15] Remove account lockout from failed recovery attempts #37196

Merged

codingllama reviewed Jan 31, 2024

View reviewed changes

espadolini mentioned this pull request Feb 1, 2024

Unmarshal and compare user objects in CompareAndSwapUser #37660

Merged

zmb3 added a commit that referenced this pull request May 6, 2024

docs: add local user password policy

be62a3d

Also removes the outdated mention of locking accounts after failed password reset attempts. This functionality was removed in #35325 but the docs were not updated. Closes #41135

zmb3 mentioned this pull request May 6, 2024

docs: add local user password policy #41234

Merged

zmb3 added a commit that referenced this pull request May 7, 2024

docs: add local user password policy

6acfac4

Also removes the outdated mention of locking accounts after failed password reset attempts. This functionality was removed in #35325 but the docs were not updated. Closes #41135

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove account lockout from failed recovery attempts #35325

Remove account lockout from failed recovery attempts #35325

jentfoo commented Dec 4, 2023

github-actions bot commented Dec 4, 2023

russjones left a comment

jentfoo commented Dec 5, 2023 •

edited

Loading

russjones commented Dec 6, 2023

jentfoo commented Dec 6, 2023

jentfoo commented Dec 6, 2023

russjones commented Dec 7, 2023

jentfoo commented Dec 8, 2023

jentfoo commented Jan 9, 2024

github-actions bot commented Jan 9, 2024

github-actions bot commented Jan 9, 2024

strideynet Jan 10, 2024

codingllama Jan 11, 2024

github-actions bot commented Jan 24, 2024

github-actions bot commented Jan 24, 2024

zmb3 left a comment

github-actions bot commented Jan 24, 2024

zmb3 commented Jan 24, 2024

public-teleport-github-review-bot bot commented Jan 24, 2024

codingllama Jan 31, 2024 •

edited

Loading

jentfoo Jan 31, 2024

		reserved 5; // removed "google.protobuf.Timestamp RecoveryAttemptLockExpires" after lockout was removed
		reserved "RecoveryAttemptLockExpires";

Remove account lockout from failed recovery attempts #35325

Remove account lockout from failed recovery attempts #35325

Conversation

jentfoo commented Dec 4, 2023

github-actions bot commented Dec 4, 2023

russjones left a comment

Choose a reason for hiding this comment

jentfoo commented Dec 5, 2023 • edited Loading

russjones commented Dec 6, 2023

jentfoo commented Dec 6, 2023

jentfoo commented Dec 6, 2023

russjones commented Dec 7, 2023

jentfoo commented Dec 8, 2023

jentfoo commented Jan 9, 2024

github-actions bot commented Jan 9, 2024

github-actions bot commented Jan 9, 2024

strideynet Jan 10, 2024

Choose a reason for hiding this comment

codingllama Jan 11, 2024

Choose a reason for hiding this comment

github-actions bot commented Jan 24, 2024

github-actions bot commented Jan 24, 2024

zmb3 left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 24, 2024

zmb3 commented Jan 24, 2024

public-teleport-github-review-bot bot commented Jan 24, 2024

codingllama Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

jentfoo Jan 31, 2024

Choose a reason for hiding this comment

jentfoo commented Dec 5, 2023 •

edited

Loading

codingllama Jan 31, 2024 •

edited

Loading