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.

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

logical/aws: Harden WAL entry creation #5202

Merged
merged 7 commits into from
Sep 27, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion builtin/logical/aws/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func Backend() *backend {
},

WALRollback: b.walRollback,
WALRollbackMinAge: 5 * time.Minute,
WALRollbackMinAge: minAwsUserRollbackAge,
BackendType: logical.TypeLogical,
}

Expand Down Expand Up @@ -134,3 +134,5 @@ func (b *backend) clientSTS(ctx context.Context, s logical.Storage) (stsiface.ST

return b.stsClient, nil
}

const minAwsUserRollbackAge = 5 * time.Minute
31 changes: 31 additions & 0 deletions builtin/logical/aws/path_user.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@ import (
"context"
"fmt"
"strings"
"time"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/awserr"
"github.com/aws/aws-sdk-go/service/iam"
"github.com/hashicorp/errwrap"
"github.com/hashicorp/vault/helper/strutil"
Expand Down Expand Up @@ -130,6 +132,35 @@ func pathUserRollback(ctx context.Context, req *logical.Request, _kind string, d
MaxItems: aws.Int64(1000),
})
if err != nil {
// This isn't guaranteed to be perfect; for example, an IAM user
// might have gotten put into the WAL but then the IAM user creation
// failed (e.g., Vault didn't have permissions) and then the WAL
// deletion failed as well. Then, if Vault doesn't have access to
// call iam:ListGroupsForUser, AWS will return an access denied error
// and the WAL will never get cleaned up. But this is better than
// just having Vault "forget" about a user it actually created.
//
// BEWARE a potential race condition -- where this is called
// immediately after a user is created. AWS eventual consistency
// might say the user doesn't exist when the user does in fact
// exist, and this could cause Vault to forget about the user.
// This won't happen if the user creation fails (because the WAL
// minimum age is 5 minutes, and AWS eventual consistency is, in
// practice, never that long), but it could happen if a lease holder
// asks immediately after getting a user to revoke the lease, causing
// Vault to leak the secret, which would be a Very Bad Thing to allow.
// So we make sure that, if there's an associated lease, it must be at
// 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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chrishoffman -- this line is where I need the expiration manager changes, and the comments above try to explain why.

// WAL rollback
acceptMissingIamUsers = true
}
if aerr.Code() == iam.ErrCodeNoSuchEntityException && acceptMissingIamUsers {
return nil
}
}
return err
}
groups := groupsResp.Groups
Expand Down
4 changes: 4 additions & 0 deletions builtin/logical/aws/secret_access_keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,10 @@ func (b *backend) secretAccessKeysCreate(
UserName: aws.String(username),
})
if err != nil {
if walErr := framework.DeleteWAL(ctx, s, walId); walErr != nil {
iamErr := errwrap.Wrapf("error creating IAM user: {{err}}", err)
return nil, errwrap.Wrap(errwrap.Wrapf("failed to delete WAL entry: {{err}}", walErr), iamErr)
}
return logical.ErrorResponse(fmt.Sprintf(
"Error creating IAM user: %s", err)), awsutil.CheckAWSError(err)
}
Expand Down
2 changes: 1 addition & 1 deletion logical/lease.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ type LeaseOptions struct {
Increment time.Duration `json:"-"`

// IssueTime is the time of issue for the original lease. This is
// only available on a Renew operation and has no effect when returning
// only available on Renew and Revoke operations and has no effect when returning
// a response. It can be used to enforce maximum lease periods by
// a logical backend.
IssueTime time.Time `json:"-"`
Expand Down
7 changes: 7 additions & 0 deletions vault/expiration.go
Original file line number Diff line number Diff line change
Expand Up @@ -1277,6 +1277,13 @@ func (m *ExpirationManager) revokeEntry(ctx context.Context, le *leaseEntry) err
return nil
}

if le.Secret != nil {
// not sure if this is really valid to have a leaseEntry with a nil Secret
// (if there's a nil Secret, what are you really leasing?), but the tests
// create one, and good to be defensive
le.Secret.IssueTime = le.IssueTime
}

// Make sure we're operating in the right namespace
nsCtx := namespace.ContextWithNamespace(ctx, le.namespace)

Expand Down