-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Normalize error response to stop information leakage #23072
Normalize error response to stop information leakage #23072
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution. I have provided some initial feedback on the PR. I will take a deeper look later, and I need to discuss this internally of the implications of the change before we can accept it.
@@ -0,0 +1,3 @@ | |||
```release-note:security |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to another issue, please call this out as a change!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the message is unnecessarily too long. Would you please make it consistent with the other change:
auth/approle: Normalize error response messages when invalid credentials are provided
@@ -99,7 +99,7 @@ func (b *backend) pathLoginResolveRole(ctx context.Context, req *logical.Request | |||
return nil, err | |||
} | |||
if roleIDIndex == nil { | |||
return logical.ErrorResponse("invalid role ID"), nil | |||
return logical.ErrorResponse("invalid role or secret ID"), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the error response could also be changed to logical.ErrInvalidCredentials
for here and the rest of the changes.
@jamiewri we are looking forward to getting this PR merged. Are you still interested in working on the required fixes in the feedback? |
Further to the work done in this PR to stop information leakage via HTTP error codes with partially correct log ins. This PR normalizes the returned error message for the same reasons.