logical/aws: Harden WAL entry creation #5202
Conversation
If AWS IAM user creation failed in any way, the WAL corresponding to the IAM user would get left around and Vault would try to roll it back. However, because the user never existed, the rollback failed. Thus, the WAL would essentially get "stuck" and Vault would continually attempt to roll it back, failing every time. A similar situation could arise if the IAM user that Vault created got deleted out of band, or if Vault deleted it but was unable to write the lease revocation back to storage (e.g., a storage failure). This attempts to harden it in two ways. One is by deleting the WAL log entry if the IAM user creation fails. However, the WAL deletion could still fail, and this wouldn't help where the user is deleted out of band, so second, consider the user rolled back if the user just doesn't exist, under certain circumstances. Fixes hashicorp#5190
TestExpiration_Tidy was passing in a leaseEntry that had a nil Secret, which then caused a segfault as the changes to revokeEntry didn't check whether Secret was nil; this is probably unlikely to occur in real life, but good to be extra cautious.
Missed the else...
|
Can you back out the expiration manager changes? A bunch of work has been done in here and I am not sure this is necessary. If it is, can you create a separate PR? |
|
@chrishoffman -- I think the expiration manager changes are still necessary, but I don't know how to test for it in a good way. For the same reason that Why do you want a separate PR? |
|
Mainly because it is not related to the proposed change, although I could be missing something on why this change is necessary. We normally only pass the information that is absolutely necessary to the backend and I am not sure IssueDate is required for revoke operations. |
| // least 5 minutes old as well. | ||
| if aerr, ok := err.(awserr.Error); ok { | ||
| acceptMissingIamUsers := false | ||
| if req.Secret == nil || time.Since(req.Secret.IssueTime) > time.Duration(minAwsUserRollbackAge) { |
There was a problem hiding this comment.
@chrishoffman -- this line is where I need the expiration manager changes, and the comments above try to explain why.
If AWS IAM user creation failed in any way, the WAL corresponding to the
IAM user would get left around and Vault would try to roll it back.
However, because the user never existed, the rollback failed. Thus, the
WAL would essentially get "stuck" and Vault would continually attempt to
roll it back, failing every time. A similar situation could arise if the
IAM user that Vault created got deleted out of band, or if Vault deleted
it but was unable to write the lease revocation back to storage (e.g., a
storage failure).
This attempts to harden it in two ways. One is by deleting the WAL log
entry if the IAM user creation fails. However, the WAL deletion could
still fail, and this wouldn't help where the user is deleted out of
band, so second, consider the user rolled back if the user just doesn't
exist, under certain circumstances.
Fixes #5190