Skip to content
This repository has been archived by the owner on Sep 14, 2020. It is now read-only.

[305] Hide authorisation headers of exceptions' reprs #306

Merged
merged 3 commits into from
Feb 4, 2020

Conversation

nolar
Copy link
Contributor

@nolar nolar commented Feb 3, 2020

What do these changes do?

Prevent leaking Authorisation: and maybe other sensitive headers of the K8s API requests and responses when failing to post an event.

Description

Previously, Kopf was logging the whole repr of an error in case of failures. With aiohttp.ClientResponseError, this seems to be a problem, as its repr includes all the request headers, including Authorization: ….

When this header is logged, then those who have access to the logs can also get access to the K8s API (especially if the K8s API tokens are not expired, which is the case on its own).

With this change, it will only show string forms of any errors. For aiohttp.ClientResponseError, this means only the HTTP status, message, and an URL. And, as a special case, in K8s-event posting, only the HTTP status and message.

Issues/PRs

Issues: fixes #305

Type of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • The code addresses only the mentioned problem, and this problem only
  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt

@nolar nolar added the enhancement New feature or request label Feb 3, 2020
@zincr
Copy link

zincr bot commented Feb 3, 2020

🤖 zincr found 0 problems , 0 warnings

✅ Large Commits
✅ Approvals
✅ Specification
✅ Dependency Licensing

@zincr
Copy link

zincr bot commented Feb 3, 2020

🤖 zincr found 1 problem , 0 warnings

❌ Approvals
✅ Large Commits
✅ Specification
✅ Dependency Licensing

Details on how to resolve are provided below


Approvals

All proposed changes must be reviewed by project maintainers before they can be merged

Not enough people have approved this pull request - please ensure that 1 additional user, who have not contributed to this pull request approve the changes.

  • ✅ Approved by PR author @nolar
  • ❌ 1 additional approval needed
     

nolar added 2 commits February 4, 2020 09:50
They can leak in other ways though. But that is difficult to simulate.
For example, in generic failure of a root task (resource watcher).
@nolar nolar merged commit 405ff50 into zalando-incubator:master Feb 4, 2020
@nolar nolar deleted the security branch February 4, 2020 09:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Censor Authorization header in "Failed to post an event" logs
2 participants