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

Sanitize Central config request URI and headers in logs #1471

Merged
merged 8 commits into from
Sep 3, 2021

Conversation

russcam
Copy link
Contributor

@russcam russcam commented Aug 16, 2021

This commit sanitizes the request URI and headers of
request to fetch central configuration, to redact username/password info
and sensitive headers.

  • Move sanitization methods to separate Santization helper class.
  • Add test to assert central config sensitive request details are redacted.
  • Use Consts.Redacted wherever "[REDACTED]" is needed.

Fixes #1376

This commit sanitizes the request URI and headers of
request to fetch central configuration, to redact username/password info
and sensitive headers.

- Move sanitization methods to separate Santization helper class.
- Add test to assert central config sensitive request details are redacted.
- Use Consts.Redacted wherever "[REDACTED]" is needed.

Fixes elastic#1376
@apmmachine
Copy link
Contributor

apmmachine commented Aug 16, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-08-25T00:39:09.537+0000

  • Duration: 56 min 33 sec

  • Commit: 646d368

Test stats 🧪

Test Results
Failed 0
Passed 19669
Skipped 126
Total 19795

Trends 🧪

Image of Build Times

Image of Tests

@gregkalapos gregkalapos self-requested a review August 18, 2021 21:08
Copy link
Contributor

@gregkalapos gregkalapos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM.

Somewhat related: do we want to maybe change the log level of the log here. We print this as a warning (or even error?) by default and people already asked about it multiple times. When central config is not enabled in Kibana, this is expected and normal behaviour - so at least in that case I think an info log would be enough.

@russcam
Copy link
Contributor Author

russcam commented Aug 23, 2021

Info log level sounds good. It can probably do with some further refactoring to trim the log down to the essential information, and to detect, if possible, if central config is not enabled and log/not log appropriately.

russcam and others added 4 commits August 23, 2021 18:39
This commit uses the log level defined on the FailedToFetchConfigException
for logging the exception. This is Debug for 403 and 404 responses and Error
for everything else.
@russcam
Copy link
Contributor Author

russcam commented Aug 24, 2021

I've updated use the log level defined on the FailedToFetchConfigException thrown, for logging the exception. This is Debug for 403 and 404 responses and Error for everything else. I think this can be refactored in future, to simplify the flow further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-dotnet bug Something isn't working v1.11.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] CentralConfigFetcher logs SecretToken and ApiKey from request headers
3 participants