approle: Make invalid role_id a 400 error instead of 500 #4470
Conversation
| // Checks if the Role represented by the RoleID still exists | ||
| func (b *backend) validateRoleID(ctx context.Context, s logical.Storage, roleID string) (*roleStorageEntry, string, error) { | ||
| // Validates the supplied RoleID and SecretID | ||
| func (b *backend) validateCredentials(ctx context.Context, req *logical.Request, data *framework.FieldData) (*roleStorageEntry, string, map[string]string, string, error, error) { |
There was a problem hiding this comment.
Consider naming the return parameters. The list is long, many are strings, and much of the usage is nil, "",... which makes it hard to infer what they even are.
There was a problem hiding this comment.
Good catch. Removed.
| @@ -152,7 +138,7 @@ func (b *backend) validateCredentials(ctx context.Context, req *logical.Request, | |||
| } | |||
| } | |||
|
|
|||
| return role, roleName, metadata, secretID, nil, nil | |||
| return role, roleIDIndex.Name, metadata, secretID, nil, nil | |||
There was a problem hiding this comment.
Why do we return secretID? The only caller doesn't even use it.
There was a problem hiding this comment.
I removed the function as it had a single caller.
|
|
||
| roleLock := b.roleLock(roleName) | ||
| roleLock.RLock() | ||
| defer roleLock.RUnlock() |
There was a problem hiding this comment.
Do you actually need the lock after the roleEntry call? If not, can you just manually unlock instead of deferring until the end? That way you're not holding on to the role lock while also contending for the read/write locks for the secret ID.
There was a problem hiding this comment.
We don't need the lock on the role for long. Releasing it sooner now as per feedback.
| secretIDLock := b.secretIDLock(secretIDHMAC) | ||
| secretIDLock.RLock() | ||
| secretIDUnlockFunc := secretIDLock.RUnlock | ||
| defer func() { |
There was a problem hiding this comment.
Honestly, I'd prefer if you didn't do this. In the two return scenarios below you could just unlock manually. Then when you're in the switch, in the first case you know it'll stay a read lock and you can immediately defer the RUnlock; in the second case you know you'll need to switch it so you can runlock, lock, immediately defer the write lock.
Fixes #4451