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

Avoid race conditions in AppRole #3561

Merged
merged 7 commits into from
Nov 10, 2017
Merged

Avoid race conditions in AppRole #3561

merged 7 commits into from
Nov 10, 2017

Conversation

vishalnayak
Copy link
Member

No description provided.

@vishalnayak vishalnayak added this to the 0.9.0 milestone Nov 9, 2017
if err != nil {
return nil, err
}
if roleIDIndex == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't the right thing to do here be to instead ensure that it's created?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought of that too and considered that to be “magic”. It’s would be a cover up and may not lead us to the actual problem.

@@ -780,7 +792,7 @@ func (b *backend) pathRoleCreateUpdate(req *logical.Request, data *framework.Fie
role.Period = time.Second * time.Duration(data.Get("period").(int))
}
if role.Period > b.System().MaxLeaseTTL() {
return logical.ErrorResponse(fmt.Sprintf("'period' of '%s' is greater than the backend's maximum lease TTL of '%s'", role.Period.String(), b.System().MaxLeaseTTL().String())), nil
return logical.ErrorResponse(fmt.Sprintf("period of %q is greater than the backend's maximum lease TTL of %q", role.Period.String(), b.System().MaxLeaseTTL().String())), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes the quotes from single quotes to double quotes, if it matters.

Copy link
Member Author

Choose a reason for hiding this comment

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

@calvn %q add the double quotes automatically.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, however the old syntax printed the values wrapped around single quotes so just pointing that difference out :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I had misread your comment. Change from single to double quotes shouldn't matter IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only potential change I see is that whenever this error gets returned back as JSON, the message will contain escaped quotes which may not look as clean ("\"error value\"" vs "'error value'").

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. Its also true for a lot of places in Vault. It may not look as clean to humans, but parsing it and handling it is perfectly legal. I am inclined towards thinking that this is okay.

if roleIDIndex == nil {
// The index should never be nil for a valid role. If it is, create
// a new one.
err = b.setRoleIDEntry(req.Storage, role.RoleID, &roleIDStorageEntry{
Copy link
Member

Choose a reason for hiding this comment

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

Would we need the write lock in this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we do! Good catch.

jefferai
jefferai previously approved these changes Nov 9, 2017
Copy link
Member

@jefferai jefferai left a comment

Choose a reason for hiding this comment

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

Just the one question!

calvn
calvn previously approved these changes Nov 10, 2017
@vishalnayak vishalnayak dismissed stale reviews from calvn and jefferai via 6d67c3f November 10, 2017 15:47
data := structs.New(role).Map()
delete(data, "role_id")
delete(data, "hmac_key")
// Convert the 'time.Duration' values to second.
Copy link
Member

Choose a reason for hiding this comment

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

If we switch to a write lock, this will now persist the wrong values. You can just set the resp data explicitly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that we are not persisting the role. We are only creating an index for the role ID. But still we hold the write lock on the role since the index is tightly coupled to a role entry.

Data: data,
}
// Create a map of data to be returned and remove sensitive information from it
respData := structs.New(role).Map()
Copy link
Member

Choose a reason for hiding this comment

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

No structs!

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@@ -857,55 +857,79 @@ func (b *backend) pathRoleRead(req *logical.Request, data *framework.FieldData)

lock := b.roleLock(roleName)
lock.RLock()
defer lock.RUnlock()
writeLockHeld := false
Copy link
Member

Choose a reason for hiding this comment

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

Rather than this bool, since the signatures are the same, you can just create a function like lockRelease := lock.RUnlock and below if you switch to a write lock you can reassign to lockRelease = lock.Unlock. THen below you just call lockRelease().

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion. Done!

@vishalnayak vishalnayak merged commit 017c0ec into master Nov 10, 2017
@vishalnayak vishalnayak deleted the approle-locking branch November 10, 2017 16:32
@vishalnayak vishalnayak mentioned this pull request Nov 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants