Redact internal error messages to prevent leaks

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.

internal/services/pam/pam.go

@@ -6,6 +6,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"log/slog"
 	"os/user"
 
 	"github.com/ubuntu/authd"
@@ -58,7 +59,9 @@ func (s Service) AvailableBrokers(ctx context.Context, _ *authd.Empty) (*authd.A
 
 // GetPreviousBroker returns the previous broker set for a given user, if any.
 // If the user is not in our cache, it will try to check if it’s on the system, and return then "local".
-func (s Service) GetPreviousBroker(ctx context.Context, req *authd.GPBRequest) (*authd.GPBResponse, error) {
+func (s Service) GetPreviousBroker(ctx context.Context, req *authd.GPBRequest) (gpbr *authd.GPBResponse, err error) {
+	defer redactError(&err)
+
 	// Use in memory cache first
 	if b := s.brokerManager.BrokerForUser(req.GetUsername()); b != nil {
 		return &authd.GPBResponse{PreviousBroker: b.ID}, nil
@@ -101,6 +104,7 @@ func (s Service) GetPreviousBroker(ctx context.Context, req *authd.GPBRequest) (
 
 // SelectBroker starts a new session and selects the requested broker for the user.
 func (s Service) SelectBroker(ctx context.Context, req *authd.SBRequest) (resp *authd.SBResponse, err error) {
+	defer redactError(&err)
 	defer decorate.OnError(&err, "can't start authentication transaction")
 
 	username := req.GetUsername()
@@ -141,6 +145,7 @@ func (s Service) SelectBroker(ctx context.Context, req *authd.SBRequest) (resp *
 
 // GetAuthenticationModes fetches a list of authentication modes supported by the broker depending on the session information.
 func (s Service) GetAuthenticationModes(ctx context.Context, req *authd.GAMRequest) (resp *authd.GAMResponse, err error) {
+	defer redactError(&err)
 	defer decorate.OnError(&err, "could not get authentication modes")
 
 	sessionID := req.GetSessionId()
@@ -182,6 +187,7 @@ func (s Service) GetAuthenticationModes(ctx context.Context, req *authd.GAMReque
 
 // SelectAuthenticationMode set given authentication mode as selected for this sessionID to the broker.
 func (s Service) SelectAuthenticationMode(ctx context.Context, req *authd.SAMRequest) (resp *authd.SAMResponse, err error) {
+	defer redactError(&err)
 	defer decorate.OnError(&err, "can't select authentication mode")
 
 	sessionID := req.GetSessionId()
@@ -211,6 +217,7 @@ func (s Service) SelectAuthenticationMode(ctx context.Context, req *authd.SAMReq
 
 // IsAuthenticated returns broker answer to authentication request.
 func (s Service) IsAuthenticated(ctx context.Context, req *authd.IARequest) (resp *authd.IAResponse, err error) {
+	defer redactError(&err)
 	defer decorate.OnError(&err, "can't check authentication")
 
 	sessionID := req.GetSessionId()
@@ -248,6 +255,7 @@ func (s Service) IsAuthenticated(ctx context.Context, req *authd.IARequest) (res
 		data = ""
 	}
 
+	data = redactMessage(data)
 	return &authd.IAResponse{
 		Access: access,
 		Msg:    data,
@@ -256,6 +264,7 @@ func (s Service) IsAuthenticated(ctx context.Context, req *authd.IARequest) (res
 
 // SetDefaultBrokerForUser sets the default broker for the given user.
 func (s Service) SetDefaultBrokerForUser(ctx context.Context, req *authd.SDBFURequest) (empty *authd.Empty, err error) {
+	defer redactError(&err)
 	defer decorate.OnError(&err, "can't set default broker %q for user %q", req.GetBrokerId(), req.GetUsername())
 
 	if req.GetUsername() == "" {
@@ -279,6 +288,7 @@ func (s Service) SetDefaultBrokerForUser(ctx context.Context, req *authd.SDBFURe
 
 // EndSession asks the broker associated with the sessionID to end the session.
 func (s Service) EndSession(ctx context.Context, req *authd.ESRequest) (empty *authd.Empty, err error) {
+	defer redactError(&err)
 	defer decorate.OnError(&err, "could not abort session")
 
 	sessionID := req.GetSessionId()
@@ -335,3 +345,37 @@ func mapToUILayout(layout map[string]string) (r *authd.UILayout) {
 		Code:    &code,
 	}
 }
+
+// errGeneric is the error to return when the broker returns an error.
+//
+// This error is returned to the client to prevent information leaks.
+var errGeneric = errors.New("authentication failure")
+
+// redactError replaces the error with a generic one to prevent information leaks.
+//
+// Since the error messages contain useful information for debugging, the original error message
+// is written in the system logs.
+func redactError(err *error) {
+	if *err == nil {
+		return
+	}
+	slog.Debug(fmt.Sprintf("%v", err))
+	*err = errGeneric
+}
+
+// genericErrorMessage is the message to return when the broker returns an error message.
+//
+// This message is returned to the client to prevent information leaks.
+const genericErrorMessage string = `{"message":"authentication failure"}`
+
+// redactMessage replaces the message with a generic one to prevent information leaks.
+//
+// Since the message contains useful information for debugging, the original message is written
+// in the system logs.
+func redactMessage(msg string) string {
+	if msg == "{}" || msg == "" {
+		return msg
+	}
+	slog.Debug(fmt.Sprintf("Got broker message: %q", msg))
+	return genericErrorMessage
+}

internal/services/pam/testdata/TestIsAuthenticated/golden/denies_authentication_when_broker_times_out/IsAuthenticated

@@ -1,4 +1,4 @@
 FIRST CALL:
 	access: denied
-	msg: {"message": "denied by time out"}
+	msg: {"message":"authentication failure"}
 	err: <nil>

internal/services/pam/testdata/TestIsAuthenticated/golden/error_on_empty_data_even_if_granted/IsAuthenticated

@@ -1,4 +1,4 @@
 FIRST CALL:
 	access: 
 	msg: 
-	err: rpc error: code = Unknown desc = can't check authentication: missing key "userinfo" in returned message, got: {}
+	err: rpc error: code = Unknown desc = authentication failure

internal/services/pam/testdata/TestIsAuthenticated/golden/error_on_updating_local_groups_with_unexisting_file/IsAuthenticated

@@ -1,4 +1,4 @@
 FIRST CALL:
 	access: 
 	msg: 
-	err: rpc error: code = Unknown desc = can't check authentication: failed to update user "TestIsAuthenticated/Error_on_updating_local_groups_with_unexisting_file_separator_success_with_local_groups": could not update local groups for user "TestIsAuthenticated/Error_on_updating_local_groups_with_unexisting_file_separator_success_with_local_groups": could not fetch existing local group: open testdata/TestIsAuthenticated/does_not_exists.group: no such file or directory
+	err: rpc error: code = Unknown desc = authentication failure

internal/services/pam/testdata/TestIsAuthenticated/golden/error_when_authenticating/IsAuthenticated

@@ -1,4 +1,4 @@
 FIRST CALL:
 	access: 
 	msg: 
-	err: rpc error: code = Unknown desc = can't check authentication: broker "BrokerMock": IsAuthenticated errored out
+	err: rpc error: code = Unknown desc = authentication failure

internal/services/pam/testdata/TestIsAuthenticated/golden/error_when_broker_returns_invalid_access/IsAuthenticated

@@ -1,4 +1,4 @@
 FIRST CALL:
 	access: 
 	msg: 
-	err: rpc error: code = Unknown desc = can't check authentication: invalid access authentication key: invalid
+	err: rpc error: code = Unknown desc = authentication failure

internal/services/pam/testdata/TestIsAuthenticated/golden/error_when_broker_returns_invalid_data/IsAuthenticated

@@ -1,5 +1,4 @@
 FIRST CALL:
 	access: 
 	msg: 
-	err: rpc error: code = Unknown desc = can't check authentication: response returned by the broker is not a valid json: invalid character 'i' looking for beginning of value
-Broker returned: invalid
+	err: rpc error: code = Unknown desc = authentication failure

internal/services/pam/testdata/TestIsAuthenticated/golden/error_when_broker_returns_invalid_userinfo/IsAuthenticated

@@ -1,4 +1,4 @@
 FIRST CALL:
 	access: 
 	msg: 
-	err: rpc error: code = Unknown desc = can't check authentication: message is not JSON formatted: json: cannot unmarshal string into Go value of type brokers.userInfo
+	err: rpc error: code = Unknown desc = authentication failure

internal/services/pam/testdata/TestIsAuthenticated/golden/error_when_broker_returns_username_different_than_the_one_selected/IsAuthenticated

@@ -1,4 +1,4 @@
 FIRST CALL:
 	access: 
 	msg: 
-	err: rpc error: code = Unknown desc = can't check authentication: provided userinfo is invalid: username "different_username" does not match the selected username "TestIsAuthenticated/Error_when_broker_returns_username_different_than_the_one_selected_separator_IA_info_mismatching_user_name"
+	err: rpc error: code = Unknown desc = authentication failure

internal/services/pam/testdata/TestIsAuthenticated/golden/error_when_calling_second_time_without_cancelling/IsAuthenticated

@@ -5,4 +5,4 @@ FIRST CALL:
 SECOND CALL:
 	access: 
 	msg: 
-	err: rpc error: code = Unknown desc = can't check authentication: broker "BrokerMock": IsAuthenticated already running for session "TestIsAuthenticated/Error_when_calling_second_time_without_cancelling_separator_IA_second_call-session_id"
+	err: rpc error: code = Unknown desc = authentication failure

internal/services/pam/testdata/TestIsAuthenticated/golden/error_when_sessionid_is_empty/IsAuthenticated

@@ -1,4 +1,4 @@
 FIRST CALL:
 	access: 
 	msg: 
-	err: rpc error: code = InvalidArgument desc = can't check authentication: rpc error: code = InvalidArgument desc = no session ID provided
+	err: rpc error: code = Unknown desc = authentication failure

internal/services/pam/testdata/TestIsAuthenticated/golden/error_when_there_is_no_broker/IsAuthenticated

@@ -1,4 +1,4 @@
 FIRST CALL:
 	access: 
 	msg: 
-	err: rpc error: code = Unknown desc = can't check authentication: no broker found for session "invalid-session"
+	err: rpc error: code = Unknown desc = authentication failure

pam/integration-tests/gdm_test.go

@@ -213,7 +213,7 @@ func TestGdmModule(t *testing.T) {
 		"Error on missing user": {
 			pamUser: ptrValue(""),
 			wantPamErrorMessages: []string{
-				"can't select broker: rpc error: code = InvalidArgument desc = can't start authentication transaction: rpc error: code = InvalidArgument desc = no user name provided",
+				"can't select broker: rpc error: code = Unknown desc = authentication failure",
 			},
 			wantError:       pam.ErrSystem,
 			wantAcctMgmtErr: pam_test.ErrIgnore,
@@ -279,7 +279,7 @@ func TestGdmModule(t *testing.T) {
 				},
 			},
 			wantPamErrorMessages: []string{
-				"invalid password 'really, it's not a goodpass!', should be 'goodpass'",
+				"authentication failure",
 			},
 			wantError:       pam.ErrAuth,
 			wantAcctMgmtErr: pam_test.ErrIgnore,
@@ -294,7 +294,7 @@ func TestGdmModule(t *testing.T) {
 				},
 			},
 			wantPamErrorMessages: []string{
-				"user not found",
+				"authentication failure",
 			},
 			wantError:       pam.ErrAuth,
 			wantAcctMgmtErr: pam_test.ErrIgnore,
@@ -311,7 +311,7 @@ func TestGdmModule(t *testing.T) {
 				},
 			},
 			wantPamErrorMessages: []string{
-				fido1AuthID + " should have wait set to true",
+				"authentication failure",
 			},
 			wantError:       pam.ErrAuth,
 			wantAcctMgmtErr: pam_test.ErrIgnore,

pam/integration-tests/testdata/TestCLIAuthenticate/golden/deny_authentication_if_max_attempts_reached

@@ -133,7 +133,7 @@ Gimme your password
 > ./pam_authd login socket=${AUTHD_TESTS_CLI_AUTHENTICATE_TESTS_SOCK}
 Gimme your password
 >
-invalid password 'wrongpass', should be 'goodpass'
+authentication failure
 
 
 
@@ -166,7 +166,7 @@ invalid password 'wrongpass', should be 'goodpass'
 > ./pam_authd login socket=${AUTHD_TESTS_CLI_AUTHENTICATE_TESTS_SOCK}
 Gimme your password
 >
-invalid password 'wrongpass', should be 'goodpass'
+authentication failure
 
 
 
@@ -199,7 +199,7 @@ invalid password 'wrongpass', should be 'goodpass'
 > ./pam_authd login socket=${AUTHD_TESTS_CLI_AUTHENTICATE_TESTS_SOCK}
 Gimme your password
 >
-invalid password 'wrongpass', should be 'goodpass'
+authentication failure
 
 
 
@@ -232,7 +232,7 @@ invalid password 'wrongpass', should be 'goodpass'
 > ./pam_authd login socket=${AUTHD_TESTS_CLI_AUTHENTICATE_TESTS_SOCK}
 Gimme your password
 >
-invalid password 'wrongpass', should be 'goodpass'
+authentication failure
 
 
 
@@ -265,7 +265,7 @@ invalid password 'wrongpass', should be 'goodpass'
 > ./pam_authd login socket=${AUTHD_TESTS_CLI_AUTHENTICATE_TESTS_SOCK}
 Gimme your password
 >
-PAM Error Message: invalid password 'wrongpass', should be 'goodpass'
+PAM Error Message: authentication failure
 PAM Authenticate() for user "user-integration-max-attempts" exited with error (PAM exit code: 7)
 : Authentication failure
 PAM Info Message: acct=incomplete
@@ -298,7 +298,7 @@ dispatch
 > ./pam_authd login socket=${AUTHD_TESTS_CLI_AUTHENTICATE_TESTS_SOCK}
 Gimme your password
 >
-PAM Error Message: invalid password 'wrongpass', should be 'goodpass'
+PAM Error Message: authentication failure
 PAM Authenticate() for user "user-integration-max-attempts" exited with error (PAM exit code: 7)
 : Authentication failure
 PAM Info Message: acct=incomplete

pam/integration-tests/testdata/TestCLIAuthenticate/golden/deny_authentication_if_user_does_not_exist

@@ -121,14 +121,14 @@ Username: user-unexistent
 
 
 
-PAM Error Message: can't select broker: rpc error: code = Unknown desc = can't start authenticat
-ion transaction: user "user-unexistent" does not exist
+PAM Error Message: can't select broker: rpc error: code = Unknown desc = authentication failure
 PAM Authenticate() for user "user-unexistent" exited with error (PAM exit code: 4): System error
 PAM Info Message: acct=incomplete
 PAM AcctMgmt() exited with error (PAM exit code: 25): The return value should be ignored by PAM
 dispatch
 >
 
+
 ────────────────────────────────────────────────────────────────────────────────
 > ./pam_authd login socket=${AUTHD_TESTS_CLI_AUTHENTICATE_TESTS_SOCK}
   Select your provider
@@ -154,12 +154,12 @@ dispatch
 
 
 
-PAM Error Message: can't select broker: rpc error: code = Unknown desc = can't start authenticat
-ion transaction: user "user-unexistent" does not exist
+PAM Error Message: can't select broker: rpc error: code = Unknown desc = authentication failure
 PAM Authenticate() for user "user-unexistent" exited with error (PAM exit code: 4): System error
 PAM Info Message: acct=incomplete
 PAM AcctMgmt() exited with error (PAM exit code: 25): The return value should be ignored by PAM
 dispatch
 >
 
+
 ────────────────────────────────────────────────────────────────────────────────

pam/integration-tests/testdata/TestCLIAuthenticate/golden/deny_authentication_if_usernames_dont_match

@@ -132,9 +132,8 @@ Gimme your password
 ────────────────────────────────────────────────────────────────────────────────
 > ./pam_authd login socket=${AUTHD_TESTS_CLI_AUTHENTICATE_TESTS_SOCK}
 Gimme your password
-PAM Error Message: authentication status failure: rpc error: code = Unknown desc = can't check a
-uthentication: provided userinfo is invalid: username "mismatching-username" does not match the
-selected username "user-mismatching-name"
+PAM Error Message: authentication status failure: rpc error: code = Unknown desc = authenticatio
+n failure
 PAM Authenticate() for user "user-mismatching-name" exited with error (PAM exit code: 4): System
  error
 PAM Info Message: acct=incomplete
@@ -160,14 +159,14 @@ dispatch
 
 
 
+
 
 
 ────────────────────────────────────────────────────────────────────────────────
 > ./pam_authd login socket=${AUTHD_TESTS_CLI_AUTHENTICATE_TESTS_SOCK}
 Gimme your password
-PAM Error Message: authentication status failure: rpc error: code = Unknown desc = can't check a
-uthentication: provided userinfo is invalid: username "mismatching-username" does not match the
-selected username "user-mismatching-name"
+PAM Error Message: authentication status failure: rpc error: code = Unknown desc = authenticatio
+n failure
 PAM Authenticate() for user "user-mismatching-name" exited with error (PAM exit code: 4): System
  error
 PAM Info Message: acct=incomplete
@@ -193,6 +192,7 @@ dispatch
 
 
 
+
 
 
 ────────────────────────────────────────────────────────────────────────────────

pam/integration-tests/testdata/TestCLIChangeAuthTok/golden/prevent_change_password_if_auth_fails

@@ -133,7 +133,7 @@ Gimme your password
 > ./pam_authd passwd socket=${AUTHD_TESTS_CLI_AUTHTOK_TESTS_SOCK}
 Gimme your password
 >
-invalid password 'wrongpass', should be 'goodpass'
+authentication failure
 
 
 
@@ -166,7 +166,7 @@ invalid password 'wrongpass', should be 'goodpass'
 > ./pam_authd passwd socket=${AUTHD_TESTS_CLI_AUTHTOK_TESTS_SOCK}
 Gimme your password
 >
-invalid password 'wrongpass', should be 'goodpass'
+authentication failure
 
 
 
@@ -199,7 +199,7 @@ invalid password 'wrongpass', should be 'goodpass'
 > ./pam_authd passwd socket=${AUTHD_TESTS_CLI_AUTHTOK_TESTS_SOCK}
 Gimme your password
 >
-invalid password 'wrongpass', should be 'goodpass'
+authentication failure
 
 
 
@@ -232,7 +232,7 @@ invalid password 'wrongpass', should be 'goodpass'
 > ./pam_authd passwd socket=${AUTHD_TESTS_CLI_AUTHTOK_TESTS_SOCK}
 Gimme your password
 >
-invalid password 'wrongpass', should be 'goodpass'
+authentication failure
 
 
 
@@ -265,7 +265,7 @@ invalid password 'wrongpass', should be 'goodpass'
 > ./pam_authd passwd socket=${AUTHD_TESTS_CLI_AUTHTOK_TESTS_SOCK}
 Gimme your password
 >
-PAM Error Message: invalid password 'wrongpass', should be 'goodpass'
+PAM Error Message: authentication failure
 PAM ChangeAuthTok() for user "user-integration-max-attempts" exited with error (PAM exit code: 7
 ): Authentication failure
 PAM Info Message: acct=incomplete
@@ -298,7 +298,7 @@ dispatch
 > ./pam_authd passwd socket=${AUTHD_TESTS_CLI_AUTHTOK_TESTS_SOCK}
 Gimme your password
 >
-PAM Error Message: invalid password 'wrongpass', should be 'goodpass'
+PAM Error Message: authentication failure
 PAM ChangeAuthTok() for user "user-integration-max-attempts" exited with error (PAM exit code: 7
 ): Authentication failure
 PAM Info Message: acct=incomplete