-
-
Notifications
You must be signed in to change notification settings - Fork 358
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
fix: log credentials verifier error details #713
fix: log credentials verifier error details #713
Conversation
This change introduced a possible side effect, now the decisions API also returns the stack trace as the response, thus end users get the full Oathkeeper stack trace - disclosing which tooling is used to secure requests. Was this expected as a feature or should it be changed? |
Yeah, we can change that. PRs welcomed |
I just finished reviewing the "possible side effect" into details, seems like the change just brought additional information to the response only if you are using the JSON error handler with the |
Oathkeeper can already be identified by other strings that are returned, even with verbose=false. The fundamental problem here is that with verbose=true, we are returning "black box" strings that could potentially contain sensitive information, so maybe it would be preferable to deprecate the option? Does anyone depend on it? Alternatively, a more considerate solution:
|
#467
@aeneasr
Proposed changes
Log error details reported by CredentialsVerifier instead of discarding them.
Checklist
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further comments
Below is an example output with this change applied. Please review the format of the 'missing kid header' error.