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

security: Redact credentials when marshalled to YAML #6186

Merged
merged 6 commits into from
May 19, 2022
Merged

security: Redact credentials when marshalled to YAML #6186

merged 6 commits into from
May 19, 2022

Conversation

chaudum
Copy link
Contributor

@chaudum chaudum commented May 18, 2022 •
edited
Loading

What this PR does / why we need it

security: Redact BOS credentials when marshalled to YAML

Change the type of the credentials flags for the BOS client configuration from string to flagext.Secret, which automatically redacts to ******** when marshalled to YAML.

Use flagext.Secret for S3 object client credentials

instead of a package private, similar implementation of a secret type flag. The redacted output of the value changes from redacted to ********.

Which issue(s) this PR fixes

Fixes #6184

Special notes for your reviewer

No changelog entry because the BOS storage feature is unreleased.

Checklist

  • Documentation added
  • Tests updated
  • Is this an important fix or new feature? Add an entry in the CHANGELOG.md.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

@chaudum
security: Redact BOS credentials when marshalled to YAML
2cef06c 
Change the type of the credentials flags for the BOS client
configuration from string to flagext.Secret, which
automatically redacts to `********` when marshalled to YAML.

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
@chaudum chaudum force-pushed the chaudum/redact-bos-credentials branch from cb02927 to 0b86cc9 Compare May 18, 2022 09:06
@chaudum
fixup! security: Redact BOS credentials when marshalled to YAML
84893f9 
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
@chaudum
Use flagext.Secret for S3 object client credentials
f2d5c73 
instead of a package private, similar implementation of a secret type flag

The redacted output of the value changes from `redacted` to `********`.

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
@chaudum chaudum force-pushed the chaudum/redact-bos-credentials branch from 0b86cc9 to f2d5c73 Compare May 18, 2022 09:24
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@chaudum chaudum marked this pull request as ready for review May 18, 2022 09:43
@chaudum chaudum requested a review from a team as a code owner May 18, 2022 09:43
@chaudum
Only redact secret_access_key and leave access_key_id plain string flag
c740e3b 
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
@chaudum
fixup! Only redact secret_access_key and leave access_key_id plain st…
9640edd 
…ring flag

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

simonswine
simonswine approved these changes May 18, 2022
View reviewed changes
Copy link
Contributor

@simonswine simonswine 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 fixing this so quickly.

LGTM, one non-blocking nit

pkg/storage/chunk/client/baidubce/bos_storage_client.go Show resolved Hide resolved
@chaudum
Add credential redaction test
eb95c23 
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
@pull-request-size pull-request-size bot added size/L and removed size/M labels May 18, 2022
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

kavirajk
kavirajk approved these changes May 19, 2022
View reviewed changes
Copy link
Contributor

@kavirajk kavirajk left a comment

Choose a reason for hiding this comment

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

👍

@kavirajk kavirajk merged commit b073ec9 into main May 19, 2022
@kavirajk kavirajk deleted the chaudum/redact-bos-credentials branch May 19, 2022 07:49
This was referenced Jun 28, 2022
Closed
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonswine simonswine simonswine approved these changes

@kavirajk kavirajk kavirajk approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Secrets leak through the config endpoint
4 participants
@chaudum @grafanabot @simonswine @kavirajk