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

Normalize error response to stop information leakage #23072

Closed

Conversation

jamiewri
Copy link
Member

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.

Copy link
Contributor

@hghaf099 hghaf099 left a 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
Copy link
Contributor

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!

Copy link
Contributor

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
Copy link
Contributor

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.

@hghaf099
Copy link
Contributor

@jamiewri we are looking forward to getting this PR merged. Are you still interested in working on the required fixes in the feedback?

@hghaf099
Copy link
Contributor

@jamiewri Thanks for raising the issue. I am closing this PR in favour of #23786. The fix matches what you have implemented. We are looking forward to your further contributions to the Vault project.

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.

2 participants