RFD 158: Description of how we should address a targeted account auth DoS#35533
RFD 158: Description of how we should address a targeted account auth DoS#35533
Conversation
jentfoo
requested review from
codingllama,
kimlisa,
klizhentas,
rosstimothy and
russjones
|
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 |
jentfoo
force-pushed
the
rfd/0158-account-recovery
branch
from
77e4ae4 to
44dfcb8
Compare
|
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 |
jentfoo
added
the
no-changelog
jentfoo
changed the title
jentfoo
changed the title
klizhentas
left a comment
klizhentas
left a comment
There was a problem hiding this comment.
@jentfoo after reading this, not clear to me 100% what are you proposing. Are you proposing not to lock out the account after trying recovery token 3 times and failing?
@klizhentas That is correct. It looks like the account reset mechanisms already takes in inputs that can not be brute forced. So I am recommending we remove this lock out in order to allow accounts that are under a targeted attack to be able to be accessed. |
zmb3
left a comment
zmb3
left a comment
There was a problem hiding this comment.
There are a lot of sections in the RFD template that are not included here.
While this is a relatively targeted change and you can probably get away without all of them, I think there are some that are worth discussing.
- required approvers
- any gRPC or API changes? Any RPCs that will be deprecated by the change?
- audit/observability: should we introduce any new metrics to track account recovery attempts?
|
|
||
| The [Account Lifecycle RFD](0029-account-lifecycle.md) defines how an account can be recovered after the loss of credentials. However it does not ensure that an account is recoverable under the condition that it is under a targeted attack. | ||
|
|
||
| Account authentication can (and should be) locked out after invalid attempts. Because passwords are user supplied, there are real brute force risks that must be considered. However this lockout mechanism can also prevent accounts from being accessible by the legitimate user. We must allow users access to their accounts even under conditions where they are under a targeted attack. |
There was a problem hiding this comment.
We must allow users access to their accounts even under conditions where they are under a targeted attack.
Brute forcing a password and locking the account is also a targeted attack isn't it? Are you proposing any changes here, or are we okay with not allowing access during this type of targeted attack?
There was a problem hiding this comment.
The password reset mechanism unlocks the account from failed password auth failures. Which is a common pattern since theoretically the user likely entered an incorrect password repeatedly before starting the password reset flow.
Please see the linked PR #35325. There is some discussion on how much we should remove from gRPC. I don't feel strongly one way or another, so looking for your guidance
These audit events already exist. I feel like it's more obvious when reviewing the linked PR above. |
|
@zmb3 @klizhentas friendly ping on this RFD and the change #35325 The external researcher is asking for updates on this design. I would like to be able to respond if this is something we plan to address. |
|
@jentfoo I'm having trouble approving this because there isn't a lot of detail here. While I understand that you've already made the change, "please see the linked PR" isn't really the spirit of an RFD. I'd expect that we use the RFD to discuss the scope of the change, and then after we've agreed upon what is in scope and what is not we can review the implementation. |
|
@zmb3 I have added the requested sections, let me know if there is anything else I can answer. Thank you! |
|
|
||
| ### Fix | ||
|
|
||
| Because reset tokens are not user controlled, and neither tokens nor MFA devices are able to be brute forced, this lockout on reset is unnecessary. We should remove this mechanism so that legitimate users who have control over reset tokens or associated MFA can maintain access even under attack conditions. |
There was a problem hiding this comment.
What length are reset tokens at the moment ? Do we need to increase their length if we're removing the 3 attempt limit.
There was a problem hiding this comment.
The length varies due to us using a combination of words. Each recovery code consists of 8 words. When considering how brute forcible this is, it's tempting to look at the character count (which on average is close to our max input length of 72 characters), but instead we need to recognize the word list is well understood. There are 7775 possible words. which means the probability per guess is (1/7775)^8 (result is 7.5E-32).
This is an extremely low probability, well outside of the possibility of brute forcing. Using a similar length fully random string would technically provide more entropy and brute force resistance, but considering the scale I don't feel that it's a necessary change.
public-teleport-github-review-bot
Bot
removed request for
codingllama,
kimlisa,
rosstimothy and
russjones
|
I plan to merge this RFD next week, let me know if anyone has any additional feedback. Thank you! |
Please make sure you have all the necessary approvals, including product. I would suggest seeking security approval as well (apart from yourself), so you get an extra pair of eyes there too. Otherwise, happy merging ;) |
My understanding is that after 2 weeks of asking for review, it is assumed to be able to be merged. This was mentioned in the Engineering All Hands some months ago. I still would like any additional feedback, but I have poked people several times now. |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
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.
* 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 * Further GRPC cleanup after PR feedback * Apply PR Feedback
* 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
* 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
5 participants
This RFD describes how we should address the security issue captured in https://github.com/gravitational/security-findings/issues/75
The removal of this feature is already available as a PR under: #35325