feat(services/pam): Do not expose internal errors to client #420
Conversation
This is to mimic a little better the behavior of other PAM modules. They show a generic error message to avoid leaking information that could potentially help attackers.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #420 +/- ##
==========================================
+ Coverage 84.44% 84.61% +0.17%
==========================================
Files 77 77
Lines 6713 6749 +36
Branches 75 75
==========================================
+ Hits 5669 5711 +42
+ Misses 732 728 -4
+ Partials 312 310 -2 ☔ View full report in Codecov by Sentry. |
| func redactError(err *error) { | ||
| if *err == nil { | ||
| return | ||
| } | ||
| slog.Debug(fmt.Sprintf("%v", err)) | ||
| *err = errGeneric | ||
| } |
There was a problem hiding this comment.
So, from my POV this is a bit too agrressive, since it's also changing the messages that the broker wants us to write with a generic "authentication failure".
While things such as invalid password 'really, it's not a goodpass!', should be 'goodpass' which are the broker auth messages should be exposed by the UI as they are.
Not sure what's the best way to filter them though. But ideally it's up to the broker not to send private details as Authentication message, while it may indeed provide more useful information for other kind of failures (for example on time out, or when something requires an user action).
There was a problem hiding this comment.
I agree and disagree also. I agree that this is indeed aggressive, but that's what other PAM modules tend to do as well to avoid leaking anything that could potentially be used by an attacker.
Also, the whole point of authd is to be a layer between 3rd party brokers and the machine. It's our business logic to ensure that what we show the user is what can be shown. Expecting the brokers to respect and match what we do locally defeats part of the purpose.
This is to mimic the behavior of other PAM modules better. They show a generic error message to avoid leaking information that could help attackers.
UDENG-3420
UDENG-3419
UDENG-3412
UDENG-3418