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

fix: remove logging of credentials #1534

Merged
merged 1 commit into from
Apr 12, 2023
Merged

fix: remove logging of credentials #1534

merged 1 commit into from
Apr 12, 2023

Conversation

piksel
Copy link
Member

@piksel piksel commented Jan 22, 2023

This PR will remove most of the unsafe logging when running with --trace. The reason for this change is that it might be used as a vulnerability and there is no good scenario for when they would be useful (since we don't want users including the information in the issues/discussions anyway).

If the user/developer needs that information, the corresponding line can just be uncommented and built locally:

❯ grep -r 'CREDENTIAL:' .
./pkg/registry/auth/auth.go:            // CREDENTIAL: Uncomment to log registry credentials
./pkg/registry/digest/digest.go:        // CREDENTIAL: Uncomment to log the request token
./pkg/registry/trust.go:                // CREDENTIAL: Uncomment to log REPO_PASS environment variable
./pkg/registry/trust.go:        // CREDENTIAL: Uncomment to log docker config password
./pkg/registry/registry.go:     // CREDENTIAL: Uncomment to log docker config auth

The two places where the logging still remains is the notification URL generated by updating legacy notifications to Shoutrrr URLs, and the docker image/container info logged when watchtower does not have enough information to update a container.

@piksel piksel requested a review from simskij January 22, 2023 08:37
@codecov
Copy link

codecov bot commented Jan 22, 2023

Codecov Report

Base: 66.19% // Head: 66.17% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (4a09d02) compared to base (c16ac96).
Patch coverage: 28.57% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1534      +/-   ##
==========================================
- Coverage   66.19%   66.17%   -0.02%     
==========================================
  Files          24       24              
  Lines        2313     2312       -1     
==========================================
- Hits         1531     1530       -1     
  Misses        682      682              
  Partials      100      100              
Impacted Files Coverage Δ
pkg/registry/auth/auth.go 42.95% <0.00%> (-0.31%) ⬇️
pkg/registry/digest/digest.go 41.46% <0.00%> (-1.40%) ⬇️
pkg/registry/registry.go 26.66% <ø> (+1.66%) ⬆️
pkg/registry/trust.go 47.69% <50.00%> (+0.07%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@piksel piksel merged commit cfcbcac into main Apr 12, 2023
@piksel piksel deleted the fix/remove-trace-creds branch April 12, 2023 06:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant