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.

Sign up for GitHub

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

Jump to bottom

Make redaction of common keys in diagnostics case insensitive #6109

Merged
merged 9 commits into from
Dec 3, 2024
Merged

Make redaction of common keys in diagnostics case insensitive #6109

merged 9 commits into from
Dec 3, 2024

Conversation

andrzej-stencel
Copy link
Contributor

@andrzej-stencel andrzej-stencel commented Nov 21, 2024 •
edited
Loading

What does this PR do?

Changes the redaction of common keys like "password" to be case insensitive. This means keys like "PASSWORD" and "PassWord" will also be redacted.

Why is it important?

To prevent leaking secrets into diagnostics.

Checklist

  • My code follows the style guidelines of this project
  • [ ] I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
    • I couldn't find any docs on diagnostics redaction
  • [ ] I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in ./changelog/fragments using the changelog tool
  • [ ] I have added an integration test or an E2E test

Disruptive User Impact

This may cause keys that were previously not redacted to be redacted after the change. For example, the "Key" substring may be common in configuration keys that don't need redaction.

How to test this PR locally

cd internal/pkg/diagnostics
go test .

@andrzej-stencel andrzej-stencel added the enhancement New feature or request label Nov 21, 2024
@andrzej-stencel andrzej-stencel requested a review from a team as a code owner November 21, 2024 10:21
@andrzej-stencel andrzej-stencel added backport-8.x Automated backport to the 8.x branch with mergify backport-8.16 Automated backport with mergify backport-8.17 Automated backport with mergify labels Nov 21, 2024
@andrzej-stencel
Copy link
Contributor Author

Should this be backported into 8.16? I marked this as an "enhancement", but maybe it is a bug fix?

pkoutsovasilis
pkoutsovasilis approved these changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@pkoutsovasilis pkoutsovasilis left a comment

Choose a reason for hiding this comment

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

LGTM

@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Nov 21, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

@pkoutsovasilis
Copy link
Contributor

AFAICT unit-test fail because otel collector never reports healthy (unrelated to this PR) and the serverless ones are due to the BK agent (Error: error creating test runner: error checking region setting: Non-201 status code returned by server: 503)?!

@ycombinator ycombinator requested a review from jkakavas November 21, 2024 15:24
@ycombinator
Copy link
Contributor

Should this be backported into 8.16? I marked this as an "enhancement", but maybe it is a bug fix?

I agree, this feels more like a bug than an enhancement. Added backport labels and bug label instead of enhancement.

@ycombinator
Copy link
Contributor

@andrzej-stencel Please add a changelog fragment to this PR using https://github.com/elastic/elastic-agent-changelog-tool. Thanks!

@ycombinator ycombinator added >bug and removed enhancement New feature or request labels Nov 21, 2024
@elastic-sonarqube Elastic SonarQube
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

@andrzej-stencel andrzej-stencel merged commit 8939f0f into elastic:main Dec 3, 2024
14 checks passed
@andrzej-stencel andrzej-stencel deleted the redact-case-insensitive branch December 3, 2024 16:47
mergify bot pushed a commit that referenced this pull request Dec 3, 2024
@andrzej-stencel @mergify
Make redaction of common keys in diagnostics case insensitive (#6109)
a5abb14 
* Make common key redaction case insensitive

* add changelog entry

* change to bug fix

---------

Co-authored-by: Shaunak Kashyap <[email protected]>
(cherry picked from commit 8939f0f)
mergify bot pushed a commit that referenced this pull request Dec 3, 2024
@andrzej-stencel @mergify
Make redaction of common keys in diagnostics case insensitive (#6109)
b94ea99 
* Make common key redaction case insensitive

* add changelog entry

* change to bug fix

---------

Co-authored-by: Shaunak Kashyap <[email protected]>
(cherry picked from commit 8939f0f)
mergify bot pushed a commit that referenced this pull request Dec 3, 2024
@andrzej-stencel @mergify
Make redaction of common keys in diagnostics case insensitive (#6109)
8cb2b8f 
* Make common key redaction case insensitive

* add changelog entry

* change to bug fix

---------

Co-authored-by: Shaunak Kashyap <[email protected]>
(cherry picked from commit 8939f0f)
andrzej-stencel added a commit that referenced this pull request Dec 4, 2024
@mergify @andrzej-stencel
Make redaction of common keys in diagnostics case insensitive (#6109) (…
026366d 
…#6198)

* Make common key redaction case insensitive

* add changelog entry

* change to bug fix

---------

Co-authored-by: Shaunak Kashyap <[email protected]>
(cherry picked from commit 8939f0f)

Co-authored-by: Andrzej Stencel <[email protected]>
andrzej-stencel added a commit that referenced this pull request Dec 4, 2024
@mergify @andrzej-stencel
Make redaction of common keys in diagnostics case insensitive (#6109) (…
9f1bff2 
…#6196)

* Make common key redaction case insensitive

* add changelog entry

* change to bug fix

---------

Co-authored-by: Shaunak Kashyap <[email protected]>
(cherry picked from commit 8939f0f)

Co-authored-by: Andrzej Stencel <[email protected]>
andrzej-stencel added a commit that referenced this pull request Dec 4, 2024
@mergify @andrzej-stencel
Make redaction of common keys in diagnostics case insensitive (#6109) (…
3a4fb05 
…#6197)

* Make common key redaction case insensitive

* add changelog entry

* change to bug fix

---------

Co-authored-by: Shaunak Kashyap <[email protected]>
(cherry picked from commit 8939f0f)

Co-authored-by: Andrzej Stencel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swiatekm swiatekm swiatekm approved these changes

@pkoutsovasilis pkoutsovasilis pkoutsovasilis approved these changes

@pchila pchila Awaiting requested review from pchila pchila is a code owner automatically assigned from elastic/elastic-agent-control-plane

@jkakavas jkakavas Awaiting requested review from jkakavas

Assignees

@andrzej-stencel andrzej-stencel

Labels
backport-8.x Automated backport to the 8.x branch with mergify backport-8.16 Automated backport with mergify backport-8.17 Automated backport with mergify >bug Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@andrzej-stencel @elasticmachine @pkoutsovasilis @ycombinator @swiatekm @pierrehilbert