Avoid race conditions in AppRole #3561
Conversation
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if roleIDIndex == nil { |
There was a problem hiding this comment.
Wouldn't the right thing to do here be to instead ensure that it's created?
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
This changes the quotes from single quotes to double quotes, if it matters.
There was a problem hiding this comment.
@calvn %q add the double quotes automatically.
There was a problem hiding this comment.
Right, however the old syntax printed the values wrapped around single quotes so just pointing that difference out :)
There was a problem hiding this comment.
Oh, I had misread your comment. Change from single to double quotes shouldn't matter IMO.
There was a problem hiding this comment.
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'").
There was a problem hiding this comment.
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{ |
There was a problem hiding this comment.
Would we need the write lock in this case?
There was a problem hiding this comment.
Yes, we do! Good catch.
| data := structs.New(role).Map() | ||
| delete(data, "role_id") | ||
| delete(data, "hmac_key") | ||
| // Convert the 'time.Duration' values to second. |
There was a problem hiding this comment.
If we switch to a write lock, this will now persist the wrong values. You can just set the resp data explicitly.
There was a problem hiding this comment.
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() |
| @@ -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 | |||
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
Good suggestion. Done!
No description provided.