ubuntu / authd

Authentication daemon for external Brokers
GNU Lesser General Public License v3.0
110 stars 9 forks source link

Reduce debug logs #489

Closed adombeck closed 2 months ago

adombeck commented 2 months ago

Reduce some debug logs which are too verbose or even leak sensitive information.

UDENG-4105

codecov-commenter commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 84.91%. Comparing base (a99a014) to head (ca47562). Report is 9 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #489 +/- ## ========================================== + Coverage 84.83% 84.91% +0.08% ========================================== Files 79 79 Lines 6922 6927 +5 Branches 75 75 ========================================== + Hits 5872 5882 +10 + Misses 731 730 -1 + Partials 319 315 -4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

adombeck commented 2 months ago

But please add a rationale for the changes in the PR description, even though the title is self-explicative, we always try to describe what triggered the changes and/or why this is better than we had before (only exceptions are dependencies bumps, since we try to keep the code as updated as possible).

Ok! I always try to explain the reason for the changes in detail in the commit messages, because those are easier to find later, but I'll try to write better PR descriptions as well.

3v1n0 commented 2 months ago

Sorry, I've missed this PR when it was on review, but I hardly find a reason why writing such information on the logs would be a problem when debugging is enabled, since debugging is per se the level that has not to be used in production but that is useful for us when developing or analyzing problems. While only in few specific cases reporters should use it (especially in the PAM modules).

I understand one rationale is not to make users to leak such information when pasting logs here, but we may avoid it by sanitizing it a collecting level.

Regarding reducing the polling logging, I agree it can be convenient, but it's also I would like not to happen when running tests (integration, especially) and in general I would like to always print the related requests in normal mode, to understand better what's going on if a problem happen (and I did had to debug these cases in the past).

So, in general I would be happy if you could look a the comments I left and can adapt them so that it's still possible to have full-debugging when a specific parameter (or build parameter) is defined.

As future plan, I think we should make debug=true to be actually an alias for something like debug=N where N is a debug level and depending on the value we show more or less information, given that for sure we're already too conservative at times with showing debug data (e.g. I personally have sometimes to do fmt.Println-based debugging, and I consider it a failure of our logging strategy).

adombeck commented 2 months ago

I hardly find a reason why writing such information on the logs would be a problem when debugging is enabled, since debugging is per se the level that has not to be used in production but that is useful for us when developing or analyzing problems.

I disagree on this. A verbose or debug mode is not only targeted at developers but also at users who encounter a problem and want to figure out what's going wrong or attach detailed logs to a bug report. In fact, we ask our users to enable debug logging for the PAM module in https://github.com/ubuntu/authd/wiki/06--Troubleshooting-reference#pam-module. I am very much of the opinion that debug logs should not contain user passwords in cleartext and I'm glad that we found and fixed this before any of our users posted those logs in a public GitHub issue.