From 64ca6ba60072d363093fd339d89f50589d5cfaaa Mon Sep 17 00:00:00 2001 From: Noor Eldeen Mansour Date: Wed, 24 Jun 2026 09:37:35 +0300 Subject: [PATCH 1/4] broker/pam: wrap granted response in {userinfo,message} envelope The Entra password+MFA login failed with "invalid character 'Y' looking for beginning of value" after a successful grant. The broker emitted the success message as a bare string while the consumer (dataToMsg) expects a {"message": ...} envelope, so the PAM client's parse aborted an already-granted login. Forward a consistent {userinfo, message} envelope from IsAuthenticated so consumers always parse the same shape regardless of whether the broker attached a notice. Encode IAResponse.Msg as {"message": ...} matching the format used for non-granted replies. Non-string values in the broker's message field are treated as absent rather than rejected, so a malformed cosmetic field never blocks an already-granted login at the broker boundary. Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> --- internal/brokers/broker.go | 50 ++++++++++++++----- internal/brokers/broker_test.go | 2 + ...ult_groups_even_if_broker_did_not_set_them | 2 +- ...res_non_string_message_in_granted_response | 4 ++ ...n_broker_returns_userinfo_with_empty_gecos | 2 +- ...eturns_userinfo_with_group_with_empty_UGID | 2 +- ...returns_userinfo_with_mismatching_username | 2 +- .../Successfully_authenticate | 2 +- ...y_authenticate_after_cancelling_first_call | 2 +- ...icate_after_second_call_without_cancelling | 4 +- ...essfully_authenticate_with_granted_message | 4 ++ internal/services/pam/pam.go | 26 ++++++++-- internal/services/pam/pam_test.go | 1 + .../IsAuthenticated | 4 ++ .../cache.db | 20 ++++++++ internal/testutils/broker.go | 8 +++ 16 files changed, 111 insertions(+), 24 deletions(-) create mode 100644 internal/brokers/testdata/golden/TestIsAuthenticated/Ignores_non_string_message_in_granted_response create mode 100644 internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_granted_message create mode 100644 internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_granted_message/IsAuthenticated create mode 100644 internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_granted_message/cache.db diff --git a/internal/brokers/broker.go b/internal/brokers/broker.go index a29c45052c..58d9a65a67 100644 --- a/internal/brokers/broker.go +++ b/internal/brokers/broker.go @@ -22,6 +22,15 @@ import ( // LocalBrokerName is the name of the local broker. const LocalBrokerName = "local" +// grantedData is the canonical envelope used to carry a granted authentication +// result between the broker layer and the PAM service. The optional message is +// an authd-controlled, user-facing notice (for example, a caching indicator) +// that the broker may attach on a successful login. +type grantedData struct { + UserInfo types.UserInfo `json:"userinfo"` + Message string `json:"message,omitempty"` +} + type brokerer interface { NewSession(ctx context.Context, username, lang, mode string) (sessionID, encryptionKey string, err error) GetAuthenticationModes(ctx context.Context, sessionID string, supportedUILayouts []map[string]string) (authenticationModes []map[string]string, err error) @@ -207,9 +216,14 @@ func (b Broker) IsAuthenticated(ctx context.Context, sessionID, authenticationDa switch access { case auth.Granted: - rawUserInfo, err := unmarshalAndGetKey(data, "userinfo") - if err != nil { - return "", "", err + var rawData map[string]json.RawMessage + if err := json.Unmarshal([]byte(data), &rawData); err != nil { + return "", "", fmt.Errorf("response returned by the broker is not a valid json: %v\nBroker returned: %v", err, data) + } + + rawUserInfo, ok := rawData["userinfo"] + if !ok { + return "", "", fmt.Errorf("missing key %q in returned message, got: %v", "userinfo", data) } info, err := unmarshalUserInfo(rawUserInfo) @@ -221,14 +235,25 @@ func (b Broker) IsAuthenticated(ctx context.Context, sessionID, authenticationDa return "", "", err } - d, err := json.Marshal(info) + var message string + if rawMessage := rawData["message"]; rawMessage != nil { + if err := json.Unmarshal(rawMessage, &message); err != nil { + // A non-string message must not fail an already-granted login; it's cosmetic. + log.Warningf(ctx, "Ignoring non-string message in broker granted response: %v", err) + } + } + + // Always forward a consistent {"userinfo": ..., "message": ...} envelope + // (message omitted when empty) so the consumer always parses the same + // shape regardless of whether the broker attached a success message. + d, err := json.Marshal(grantedData{UserInfo: info, Message: message}) if err != nil { return "", "", fmt.Errorf("can't marshal UserInfo: %v", err) } data = string(d) case auth.Denied, auth.Retry: - if _, err := unmarshalAndGetKey(data, "message"); err != nil { + if err := requireKey(data, "message"); err != nil { return "", "", err } @@ -236,7 +261,7 @@ func (b Broker) IsAuthenticated(ctx context.Context, sessionID, authenticationDa if data == "{}" { break } - if _, err := unmarshalAndGetKey(data, "message"); err != nil { + if err := requireKey(data, "message"); err != nil { return "", "", err } @@ -411,17 +436,16 @@ func validateUserInfo(uInfo types.UserInfo) (err error) { return nil } -// unmarshalAndGetKey tries to unmarshal the content in data and returns the value of the requested key. -func unmarshalAndGetKey(data, key string) (json.RawMessage, error) { +// requireKey unmarshals data and returns an error if the given key is missing. +func requireKey(data, key string) error { var returnedData map[string]json.RawMessage if err := json.Unmarshal([]byte(data), &returnedData); err != nil { - return nil, fmt.Errorf("response returned by the broker is not a valid json: %v\nBroker returned: %v", err, data) + return fmt.Errorf("response returned by the broker is not a valid json: %v\nBroker returned: %v", err, data) } - rawMsg, ok := returnedData[key] - if !ok { - return nil, fmt.Errorf("missing key %q in returned message, got: %v", key, data) + if _, ok := returnedData[key]; !ok { + return fmt.Errorf("missing key %q in returned message, got: %v", key, data) } - return rawMsg, nil + return nil } diff --git a/internal/brokers/broker_test.go b/internal/brokers/broker_test.go index 268fdd06e5..57d2185d62 100644 --- a/internal/brokers/broker_test.go +++ b/internal/brokers/broker_test.go @@ -213,6 +213,8 @@ func TestIsAuthenticated(t *testing.T) { cancelFirstCall bool }{ "Successfully_authenticate": {sessionID: "success"}, + "Successfully_authenticate_with_granted_message": {sessionID: "ia_granted_with_data"}, + "Ignores_non_string_message_in_granted_response": {sessionID: "ia_granted_with_non_string_message"}, "Successfully_authenticate_after_cancelling_first_call": {sessionID: "ia_second_call", secondCall: true}, "Denies_authentication_when_broker_times_out": {sessionID: "ia_timeout"}, "Adds_default_groups_even_if_broker_did_not_set_them": {sessionID: "ia_info_empty_groups"}, diff --git a/internal/brokers/testdata/golden/TestIsAuthenticated/Adds_default_groups_even_if_broker_did_not_set_them b/internal/brokers/testdata/golden/TestIsAuthenticated/Adds_default_groups_even_if_broker_did_not_set_them index 6f5d818a13..2c2399c1dd 100644 --- a/internal/brokers/testdata/golden/TestIsAuthenticated/Adds_default_groups_even_if_broker_did_not_set_them +++ b/internal/brokers/testdata/golden/TestIsAuthenticated/Adds_default_groups_even_if_broker_did_not_set_them @@ -1,4 +1,4 @@ FIRST CALL: access: granted - data: {"Name":"ia_info_empty_groups@example.com","UID":0,"Gecos":"gecos for ia_info_empty_groups@example.com","Dir":"/home/ia_info_empty_groups@example.com","Shell":"/bin/sh/ia_info_empty_groups@example.com","Groups":[]} + data: {"userinfo":{"Name":"ia_info_empty_groups@example.com","UID":0,"Gecos":"gecos for ia_info_empty_groups@example.com","Dir":"/home/ia_info_empty_groups@example.com","Shell":"/bin/sh/ia_info_empty_groups@example.com","Groups":[]}} err: diff --git a/internal/brokers/testdata/golden/TestIsAuthenticated/Ignores_non_string_message_in_granted_response b/internal/brokers/testdata/golden/TestIsAuthenticated/Ignores_non_string_message_in_granted_response new file mode 100644 index 0000000000..5bcd4bb9fd --- /dev/null +++ b/internal/brokers/testdata/golden/TestIsAuthenticated/Ignores_non_string_message_in_granted_response @@ -0,0 +1,4 @@ +FIRST CALL: + access: granted + data: {"userinfo":{"Name":"ia_granted_with_non_string_message@example.com","UID":0,"Gecos":"gecos for ia_granted_with_non_string_message@example.com","Dir":"/home/ia_granted_with_non_string_message@example.com","Shell":"/bin/sh/ia_granted_with_non_string_message@example.com","Groups":[{"Name":"group-ia_granted_with_non_string_message@example.com","GID":null,"UGID":"ugid-ia_granted_with_non_string_message@example.com"}]}} + err: diff --git a/internal/brokers/testdata/golden/TestIsAuthenticated/No_error_when_broker_returns_userinfo_with_empty_gecos b/internal/brokers/testdata/golden/TestIsAuthenticated/No_error_when_broker_returns_userinfo_with_empty_gecos index 4da9b1bc19..64f24f10e3 100644 --- a/internal/brokers/testdata/golden/TestIsAuthenticated/No_error_when_broker_returns_userinfo_with_empty_gecos +++ b/internal/brokers/testdata/golden/TestIsAuthenticated/No_error_when_broker_returns_userinfo_with_empty_gecos @@ -1,4 +1,4 @@ FIRST CALL: access: granted - data: {"Name":"ia_info_empty_gecos@example.com","UID":0,"Gecos":"","Dir":"/home/ia_info_empty_gecos@example.com","Shell":"/bin/sh/ia_info_empty_gecos@example.com","Groups":[{"Name":"group-ia_info_empty_gecos@example.com","GID":null,"UGID":"ugid-ia_info_empty_gecos@example.com"}]} + data: {"userinfo":{"Name":"ia_info_empty_gecos@example.com","UID":0,"Gecos":"","Dir":"/home/ia_info_empty_gecos@example.com","Shell":"/bin/sh/ia_info_empty_gecos@example.com","Groups":[{"Name":"group-ia_info_empty_gecos@example.com","GID":null,"UGID":"ugid-ia_info_empty_gecos@example.com"}]}} err: diff --git a/internal/brokers/testdata/golden/TestIsAuthenticated/No_error_when_broker_returns_userinfo_with_group_with_empty_UGID b/internal/brokers/testdata/golden/TestIsAuthenticated/No_error_when_broker_returns_userinfo_with_group_with_empty_UGID index e3343d5e7a..5f72cc28ca 100644 --- a/internal/brokers/testdata/golden/TestIsAuthenticated/No_error_when_broker_returns_userinfo_with_group_with_empty_UGID +++ b/internal/brokers/testdata/golden/TestIsAuthenticated/No_error_when_broker_returns_userinfo_with_group_with_empty_UGID @@ -1,4 +1,4 @@ FIRST CALL: access: granted - data: {"Name":"ia_info_empty_ugid@example.com","UID":0,"Gecos":"gecos for ia_info_empty_ugid@example.com","Dir":"/home/ia_info_empty_ugid@example.com","Shell":"/bin/sh/ia_info_empty_ugid@example.com","Groups":[{"Name":"group-ia_info_empty_ugid@example.com","GID":null,"UGID":""}]} + data: {"userinfo":{"Name":"ia_info_empty_ugid@example.com","UID":0,"Gecos":"gecos for ia_info_empty_ugid@example.com","Dir":"/home/ia_info_empty_ugid@example.com","Shell":"/bin/sh/ia_info_empty_ugid@example.com","Groups":[{"Name":"group-ia_info_empty_ugid@example.com","GID":null,"UGID":""}]}} err: diff --git a/internal/brokers/testdata/golden/TestIsAuthenticated/No_error_when_broker_returns_userinfo_with_mismatching_username b/internal/brokers/testdata/golden/TestIsAuthenticated/No_error_when_broker_returns_userinfo_with_mismatching_username index 6883f59433..750efbc904 100644 --- a/internal/brokers/testdata/golden/TestIsAuthenticated/No_error_when_broker_returns_userinfo_with_mismatching_username +++ b/internal/brokers/testdata/golden/TestIsAuthenticated/No_error_when_broker_returns_userinfo_with_mismatching_username @@ -1,4 +1,4 @@ FIRST CALL: access: granted - data: {"Name":"different_username@example.com","UID":0,"Gecos":"gecos for ia_info_mismatching_user_name@example.com","Dir":"/home/ia_info_mismatching_user_name@example.com","Shell":"/bin/sh/ia_info_mismatching_user_name@example.com","Groups":[{"Name":"group-ia_info_mismatching_user_name@example.com","GID":null,"UGID":"ugid-ia_info_mismatching_user_name@example.com"}]} + data: {"userinfo":{"Name":"different_username@example.com","UID":0,"Gecos":"gecos for ia_info_mismatching_user_name@example.com","Dir":"/home/ia_info_mismatching_user_name@example.com","Shell":"/bin/sh/ia_info_mismatching_user_name@example.com","Groups":[{"Name":"group-ia_info_mismatching_user_name@example.com","GID":null,"UGID":"ugid-ia_info_mismatching_user_name@example.com"}]}} err: diff --git a/internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate b/internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate index e2d4f94097..ff0807ac84 100644 --- a/internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate +++ b/internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate @@ -1,4 +1,4 @@ FIRST CALL: access: granted - data: {"Name":"success@example.com","UID":0,"Gecos":"gecos for success@example.com","Dir":"/home/success@example.com","Shell":"/bin/sh/success@example.com","Groups":[{"Name":"group-success@example.com","GID":null,"UGID":"ugid-success@example.com"}]} + data: {"userinfo":{"Name":"success@example.com","UID":0,"Gecos":"gecos for success@example.com","Dir":"/home/success@example.com","Shell":"/bin/sh/success@example.com","Groups":[{"Name":"group-success@example.com","GID":null,"UGID":"ugid-success@example.com"}]}} err: diff --git a/internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate_after_cancelling_first_call b/internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate_after_cancelling_first_call index 84a6f76fcc..abf9519375 100644 --- a/internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate_after_cancelling_first_call +++ b/internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate_after_cancelling_first_call @@ -4,5 +4,5 @@ FIRST CALL: err: SECOND CALL: access: granted - data: {"Name":"ia_second_call@example.com","UID":0,"Gecos":"gecos for ia_second_call@example.com","Dir":"/home/ia_second_call@example.com","Shell":"/bin/sh/ia_second_call@example.com","Groups":[{"Name":"group-ia_second_call@example.com","GID":null,"UGID":"ugid-ia_second_call@example.com"}]} + data: {"userinfo":{"Name":"ia_second_call@example.com","UID":0,"Gecos":"gecos for ia_second_call@example.com","Dir":"/home/ia_second_call@example.com","Shell":"/bin/sh/ia_second_call@example.com","Groups":[{"Name":"group-ia_second_call@example.com","GID":null,"UGID":"ugid-ia_second_call@example.com"}]}} err: diff --git a/internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate_after_second_call_without_cancelling b/internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate_after_second_call_without_cancelling index 1f160ed6dd..c65f851cdf 100644 --- a/internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate_after_second_call_without_cancelling +++ b/internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate_after_second_call_without_cancelling @@ -1,8 +1,8 @@ FIRST CALL: access: granted - data: {"Name":"ia_second_call@example.com","UID":0,"Gecos":"gecos for ia_second_call@example.com","Dir":"/home/ia_second_call@example.com","Shell":"/bin/sh/ia_second_call@example.com","Groups":[{"Name":"group-ia_second_call@example.com","GID":null,"UGID":"ugid-ia_second_call@example.com"}]} + data: {"userinfo":{"Name":"ia_second_call@example.com","UID":0,"Gecos":"gecos for ia_second_call@example.com","Dir":"/home/ia_second_call@example.com","Shell":"/bin/sh/ia_second_call@example.com","Groups":[{"Name":"group-ia_second_call@example.com","GID":null,"UGID":"ugid-ia_second_call@example.com"}]}} err: SECOND CALL: access: granted - data: {"Name":"ia_second_call@example.com","UID":0,"Gecos":"gecos for ia_second_call@example.com","Dir":"/home/ia_second_call@example.com","Shell":"/bin/sh/ia_second_call@example.com","Groups":[{"Name":"group-ia_second_call@example.com","GID":null,"UGID":"ugid-ia_second_call@example.com"}]} + data: {"userinfo":{"Name":"ia_second_call@example.com","UID":0,"Gecos":"gecos for ia_second_call@example.com","Dir":"/home/ia_second_call@example.com","Shell":"/bin/sh/ia_second_call@example.com","Groups":[{"Name":"group-ia_second_call@example.com","GID":null,"UGID":"ugid-ia_second_call@example.com"}]}} err: diff --git a/internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_granted_message b/internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_granted_message new file mode 100644 index 0000000000..0569dd5696 --- /dev/null +++ b/internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_granted_message @@ -0,0 +1,4 @@ +FIRST CALL: + access: granted + data: {"userinfo":{"Name":"ia_granted_with_data@example.com","UID":0,"Gecos":"gecos for ia_granted_with_data@example.com","Dir":"/home/ia_granted_with_data@example.com","Shell":"/bin/sh/ia_granted_with_data@example.com","Groups":[{"Name":"group-ia_granted_with_data@example.com","GID":null,"UGID":"ugid-ia_granted_with_data@example.com"}]},"message":"Your password was cached for offline login."} + err: diff --git a/internal/services/pam/pam.go b/internal/services/pam/pam.go index 3f0d9349ae..923114075d 100644 --- a/internal/services/pam/pam.go +++ b/internal/services/pam/pam.go @@ -282,11 +282,15 @@ func (s Service) IsAuthenticated(ctx context.Context, req *authd.IARequest) (res }, nil } - var uInfo types.UserInfo - if err := json.Unmarshal([]byte(data), &uInfo); err != nil { + var grantedData struct { + UserInfo types.UserInfo `json:"userinfo"` + Message string `json:"message,omitempty"` + } + if err := json.Unmarshal([]byte(data), &grantedData); err != nil { log.Errorf(ctx, "IsAuthenticated: Could not unmarshal user data for session %q: %v", sessionID, err) return nil, fmt.Errorf("user data from broker invalid: %v", err) } + uInfo := grantedData.UserInfo // authd uses lowercase user and group names uInfo.Name = strings.ToLower(uInfo.Name) @@ -315,9 +319,25 @@ func (s Service) IsAuthenticated(ctx context.Context, req *authd.IARequest) (res return nil, err } + // IAResponse.Msg always carries a JSON {"message": ...} envelope (or an + // empty string when there is no message), matching the format used for + // non-granted responses and expected by the PAM client's dataToMsg parser. + // A granted login must never depend on this purely cosmetic message, so we + // only fail here if the broker-supplied message cannot be re-encoded, which + // should not happen for a valid string. + msg := "" + if grantedData.Message != "" { + m, err := json.Marshal(map[string]string{"message": grantedData.Message}) + if err != nil { + log.Errorf(ctx, "IsAuthenticated: Could not marshal granted message for session %q: %v", sessionID, err) + return nil, fmt.Errorf("could not marshal granted message: %v", err) + } + msg = string(m) + } + return &authd.IAResponse{ Access: access, - Msg: "", + Msg: msg, }, nil } diff --git a/internal/services/pam/pam_test.go b/internal/services/pam/pam_test.go index 8c952b9900..e5ade6bf89 100644 --- a/internal/services/pam/pam_test.go +++ b/internal/services/pam/pam_test.go @@ -437,6 +437,7 @@ func TestIsAuthenticated(t *testing.T) { // There is no wantErr as it's stored in the golden file. }{ "Successfully_authenticate": {username: "success@example.com"}, + "Successfully_authenticate_with_granted_message": {username: "ia_granted_with_data@example.com"}, "Successfully_authenticate_if_first_call_is_canceled": {username: "ia_second_call@example.com", secondCall: true, cancelFirstCall: true}, "Denies_authentication_when_broker_times_out": {username: "ia_timeout@example.com"}, "Update_existing_DB_on_success": {username: "success@example.com", existingDB: "cache-with-user.db"}, diff --git a/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_granted_message/IsAuthenticated b/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_granted_message/IsAuthenticated new file mode 100644 index 0000000000..a124a85872 --- /dev/null +++ b/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_granted_message/IsAuthenticated @@ -0,0 +1,4 @@ +FIRST CALL: + access: granted + msg: {"message":"Your password was cached for offline login."} + err: diff --git a/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_granted_message/cache.db b/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_granted_message/cache.db new file mode 100644 index 0000000000..166e2e4fd7 --- /dev/null +++ b/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_granted_message/cache.db @@ -0,0 +1,20 @@ +users: + - name: ia_granted_with_data@example.com + uid: 1111 + gid: 1111 + gecos: gecos for ia_granted_with_data@example.com + dir: /home/ia_granted_with_data@example.com + shell: /bin/sh/ia_granted_with_data@example.com +groups: + - name: ia_granted_with_data@example.com + gid: 1111 + ugid: ia_granted_with_data@example.com + - name: group-ia_granted_with_data@example.com + gid: 22222 + ugid: ugid-ia_granted_with_data@example.com +users_to_groups: + - uid: 1111 + gid: 1111 + - uid: 1111 + gid: 22222 +schema_version: 2 diff --git a/internal/testutils/broker.go b/internal/testutils/broker.go index d49780abed..7a705ce441 100644 --- a/internal/testutils/broker.go +++ b/internal/testutils/broker.go @@ -310,6 +310,14 @@ func (b *BrokerBusMock) IsAuthenticated(sessionID, authenticationData string) (a access = authNext data = `{"message": "It's fine to show a message here"}` + case "ia_granted_with_data": + access = authGranted + data = fmt.Sprintf(`{"userinfo": %s, "message": "Your password was cached for offline login."}`, userInfoFromName(sessionID, nil)) + + case "ia_granted_with_non_string_message": + access = authGranted + data = fmt.Sprintf(`{"userinfo": %s, "message": 42}`, userInfoFromName(sessionID, nil)) + case "ia_next_with_invalid_data": access = authNext data = `{"msg": "there should not be a message here"}` From 2ebf1fdc7c2dae5824f9455f707aa3086974ff0b Mon Sep 17 00:00:00 2001 From: Noor Eldeen Mansour Date: Wed, 24 Jun 2026 09:37:40 +0300 Subject: [PATCH 2/4] pam: demote malformed granted message to a warning A cosmetic notice attached to a granted IAResponse must never abort an already-granted login. Treat a parse error on the Msg field as a warning and fall back to showing no notice. Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> --- pam/internal/adapter/authentication.go | 14 ++++++-- pam/internal/adapter/gdmmodel.go | 6 ++++ pam/internal/adapter/gdmmodel_test.go | 46 ++++++++++++++++++++++++++ pam/internal/adapter/nativemodel.go | 6 ++++ 4 files changed, 69 insertions(+), 3 deletions(-) diff --git a/pam/internal/adapter/authentication.go b/pam/internal/adapter/authentication.go index 0e0846a6d9..df27428cab 100644 --- a/pam/internal/adapter/authentication.go +++ b/pam/internal/adapter/authentication.go @@ -352,11 +352,19 @@ func (m authenticationModel) Update(msg tea.Msg) (authModel authenticationModel, var authMsg string if msg.access != auth.Cancelled { - msg, err := dataToMsg(msg.msg) + parsedMsg, err := dataToMsg(msg.msg) if err != nil { - return m, sendEvent(pamError{status: pam.ErrSystem, msg: err.Error()}) + // A malformed or unexpected message must never fail an + // authentication that the broker has already granted: the + // message is a purely cosmetic notice. For any other access we + // still surface the error. + if msg.access != auth.Granted { + return m, sendEvent(pamError{status: pam.ErrSystem, msg: err.Error()}) + } + log.Warningf(context.TODO(), "Ignoring invalid granted message: %v", err) + } else { + authMsg = parsedMsg } - authMsg = msg } switch msg.access { diff --git a/pam/internal/adapter/gdmmodel.go b/pam/internal/adapter/gdmmodel.go index 3cfd7ce83f..270b3e7a31 100644 --- a/pam/internal/adapter/gdmmodel.go +++ b/pam/internal/adapter/gdmmodel.go @@ -266,6 +266,12 @@ func (m gdmModel) Update(msg tea.Msg) (gdmModel, tea.Cmd) { case gdmIsAuthenticatedResultReceived: access := msg.access authMsg, err := dataToMsg(msg.msg) + if err != nil && access == auth.Granted { + // A malformed granted message must never fail an already-granted + // authentication; it is a purely cosmetic notice. + log.Warningf(context.TODO(), "Ignoring invalid granted message: %v", err) + authMsg, err = "", nil + } if err != nil { return m, sendEvent(pamError{status: pam.ErrSystem, msg: err.Error()}) } diff --git a/pam/internal/adapter/gdmmodel_test.go b/pam/internal/adapter/gdmmodel_test.go index b8b3c8a091..7cd9634b23 100644 --- a/pam/internal/adapter/gdmmodel_test.go +++ b/pam/internal/adapter/gdmmodel_test.go @@ -409,6 +409,52 @@ func TestGdmModel(t *testing.T) { msg: "Hi GDM, it's a pleasure to get you in!", }, }, + "Authenticated_with_invalid_message_still_succeeds_with_preset_PAM_user_and_server_side_broker_and_authMode_selection": { + clientOptions: append(slices.Clone(multiBrokerClientOptions), + pam_test.WithGetPreviousBrokerReturn(firstBrokerInfo.Id, nil), + pam_test.WithIsAuthenticatedReturn(&authd.IAResponse{ + Access: auth.Granted, + // A bare (non-JSON) message must never fail an + // already-granted authentication: it is dropped, not fatal. + Msg: "You're in, but this is not a valid JSON envelope!", + }, nil), + ), + pamUser: "pam-preset-user-and-daemon-selected-broker", + messages: []tea.Msg{ + gdmTestWaitForStage{ + stage: proto.Stage_challenge, + commands: []tea.Cmd{ + sendEvent(gdmTestSendAuthDataWhenReady{&authd.IARequest_AuthenticationData_Secret{ + Secret: "gdm-good-password", + }}), + }, + }, + }, + wantSelectedBroker: firstBrokerInfo.Id, + wantGdmRequests: []gdm.RequestType{ + gdm.RequestType_uiLayoutCapabilities, + gdm.RequestType_changeStage, // -> broker Selection + gdm.RequestType_changeStage, // -> authMode Selection + gdm.RequestType_changeStage, // -> password + }, + wantGdmEvents: []gdm.EventType{ + gdm.EventType_userSelected, + gdm.EventType_brokersReceived, + gdm.EventType_brokerSelected, + gdm.EventType_authModeSelected, + gdm.EventType_uiLayoutReceived, + gdm.EventType_authEvent, + gdm.EventType_startAuthentication, + }, + wantStage: proto.Stage_challenge, + wantGdmAuthRes: []*authd.IAResponse{{ + Access: auth.Granted, + Msg: "", + }}, + wantExitStatus: PamSuccess{ + BrokerID: firstBrokerInfo.Id, + }, + }, "New_password_changed_after_server_side_broker_and_authMode_selection": { clientOptions: append(slices.Clone(singleBrokerNewPasswordClientOptions), pam_test.WithGetPreviousBrokerReturn(firstBrokerInfo.Id, nil), diff --git a/pam/internal/adapter/nativemodel.go b/pam/internal/adapter/nativemodel.go index 1fc32787bd..11c0c55b38 100644 --- a/pam/internal/adapter/nativemodel.go +++ b/pam/internal/adapter/nativemodel.go @@ -318,6 +318,12 @@ func (m nativeModel) Update(msg tea.Msg) (nativeModel, tea.Cmd) { case isAuthenticatedResultReceived: access := msg.access authMsg, err := dataToMsg(msg.msg) + if err != nil && access == auth.Granted { + // A malformed granted message must never fail an already-granted + // authentication; it is a purely cosmetic notice. + log.Warningf(context.TODO(), "Ignoring invalid granted message: %v", err) + authMsg, err = "", nil + } if cmd := maybeSendPamError(err); cmd != nil { return m, cmd } From 23f8e55ef2794c834177dcd9d5235e5640a77760 Mon Sep 17 00:00:00 2001 From: Noor Eldeen Mansour Date: Tue, 23 Jun 2026 20:46:07 +0300 Subject: [PATCH 3/4] broker: attach caching notice after Entra password login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After the Entra password+MFA flow caches the user's password for offline login, the user had no indication that their local password was set to their Entra password Attach a broker-owned notice to the granted response so it surfaces through the PAM conversation. The notice lives in the broker — the component that knows when caching occurred — rather than being hardcoded in authd. Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> --- authd-oidc-brokers/internal/broker/authresponses.go | 7 +++++++ authd-oidc-brokers/internal/broker/broker.go | 5 +++++ authd-oidc-brokers/internal/broker/broker_test.go | 7 +++++++ authd-oidc-brokers/internal/broker/export_test.go | 3 +++ .../Successfully_authenticate_with_granted_message | 2 +- .../IsAuthenticated | 2 +- internal/testutils/broker.go | 2 +- 7 files changed, 25 insertions(+), 3 deletions(-) diff --git a/authd-oidc-brokers/internal/broker/authresponses.go b/authd-oidc-brokers/internal/broker/authresponses.go index eae9c4b625..3e8a551948 100644 --- a/authd-oidc-brokers/internal/broker/authresponses.go +++ b/authd-oidc-brokers/internal/broker/authresponses.go @@ -2,6 +2,12 @@ package broker import "github.com/canonical/authd/authd-oidc-brokers/internal/providers/info" +// cachedPasswordMessage is the user-facing notice attached to a granted +// response after the user's Entra password is saved as the local password +// (during the Entra password + MFA flow). It is broker-owned so it can be +// localized independently of authd. +const cachedPasswordMessage = "Your local password has been set to your Entra password" + type isAuthenticatedDataResponse interface { isAuthenticatedDataResponse() } @@ -9,6 +15,7 @@ type isAuthenticatedDataResponse interface { // userInfoMessage represents the user information message that is returned to authd. type userInfoMessage struct { UserInfo info.User `json:"userinfo"` + Message string `json:"message,omitempty"` } func (userInfoMessage) isAuthenticatedDataResponse() {} diff --git a/authd-oidc-brokers/internal/broker/broker.go b/authd-oidc-brokers/internal/broker/broker.go index 98892df0ec..7369cb322e 100644 --- a/authd-oidc-brokers/internal/broker/broker.go +++ b/authd-oidc-brokers/internal/broker/broker.go @@ -1476,6 +1476,11 @@ func (b *Broker) finishEntraAuth(ctx context.Context, session *session, mfaToken return AuthDenied, unexpectedErrMsg("failed to store password") } session.entraPasswordHash = "" + + if msg, ok := data.(userInfoMessage); ok { + msg.Message = cachedPasswordMessage + data = msg + } } return access, data diff --git a/authd-oidc-brokers/internal/broker/broker_test.go b/authd-oidc-brokers/internal/broker/broker_test.go index 3485842399..1f700283d5 100644 --- a/authd-oidc-brokers/internal/broker/broker_test.go +++ b/authd-oidc-brokers/internal/broker/broker_test.go @@ -1916,6 +1916,13 @@ func TestIsAuthenticatedEntraMFAWaitStartsPollingAtOne(t *testing.T) { require.Equal(t, []int{1}, provider.recordedPollAttempts) require.Equal(t, []string{""}, provider.recordedChallengeData) + var grantPayload struct { + Message string `json:"message"` + } + require.NoError(t, json.Unmarshal([]byte(data), &grantPayload)) + require.Equal(t, broker.CachedPasswordMessage, grantPayload.Message, + "Entra MFA completion should attach the offline-password caching notice") + _, err = os.Stat(b.PasswordFilepathForSession(sessionID)) require.NoError(t, err, "Entra MFA completion should cache the offline password") _, err = os.Stat(b.TokenPathForSession(sessionID)) diff --git a/authd-oidc-brokers/internal/broker/export_test.go b/authd-oidc-brokers/internal/broker/export_test.go index 40bf8d37d9..055dec96cc 100644 --- a/authd-oidc-brokers/internal/broker/export_test.go +++ b/authd-oidc-brokers/internal/broker/export_test.go @@ -215,3 +215,6 @@ const MaxRequestDuration = maxRequestDuration // MaxAuthAttempts exposes the broker's maxAuthAttempts for tests. const MaxAuthAttempts = maxAuthAttempts + +// CachedPasswordMessage exposes the broker's cachedPasswordMessage for tests. +const CachedPasswordMessage = cachedPasswordMessage diff --git a/internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_granted_message b/internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_granted_message index 0569dd5696..a2a6bcc576 100644 --- a/internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_granted_message +++ b/internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_granted_message @@ -1,4 +1,4 @@ FIRST CALL: access: granted - data: {"userinfo":{"Name":"ia_granted_with_data@example.com","UID":0,"Gecos":"gecos for ia_granted_with_data@example.com","Dir":"/home/ia_granted_with_data@example.com","Shell":"/bin/sh/ia_granted_with_data@example.com","Groups":[{"Name":"group-ia_granted_with_data@example.com","GID":null,"UGID":"ugid-ia_granted_with_data@example.com"}]},"message":"Your password was cached for offline login."} + data: {"userinfo":{"Name":"ia_granted_with_data@example.com","UID":0,"Gecos":"gecos for ia_granted_with_data@example.com","Dir":"/home/ia_granted_with_data@example.com","Shell":"/bin/sh/ia_granted_with_data@example.com","Groups":[{"Name":"group-ia_granted_with_data@example.com","GID":null,"UGID":"ugid-ia_granted_with_data@example.com"}]},"message":"Offline login is enabled with your Entra password"} err: diff --git a/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_granted_message/IsAuthenticated b/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_granted_message/IsAuthenticated index a124a85872..01503cec02 100644 --- a/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_granted_message/IsAuthenticated +++ b/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_granted_message/IsAuthenticated @@ -1,4 +1,4 @@ FIRST CALL: access: granted - msg: {"message":"Your password was cached for offline login."} + msg: {"message":"Offline login is enabled with your Entra password"} err: diff --git a/internal/testutils/broker.go b/internal/testutils/broker.go index 7a705ce441..89eaba63ee 100644 --- a/internal/testutils/broker.go +++ b/internal/testutils/broker.go @@ -312,7 +312,7 @@ func (b *BrokerBusMock) IsAuthenticated(sessionID, authenticationData string) (a case "ia_granted_with_data": access = authGranted - data = fmt.Sprintf(`{"userinfo": %s, "message": "Your password was cached for offline login."}`, userInfoFromName(sessionID, nil)) + data = fmt.Sprintf(`{"userinfo": %s, "message": "Offline login is enabled with your Entra password"}`, userInfoFromName(sessionID, nil)) case "ia_granted_with_non_string_message": access = authGranted From 08f3d19b9ef2454380f16f4c862355967eb4df6f Mon Sep 17 00:00:00 2001 From: Noor Eldeen Mansour Date: Tue, 23 Jun 2026 20:46:14 +0300 Subject: [PATCH 4/4] pam: avoid duplicate success notice on native clients Native (SSH, non-TTY) clients receive the granted message twice: once through nativeModel.sendInfo and again through the PAM TextInfo conversation echo in sendReturnMessageToPam. GDM and interactive- terminal clients do not go through the native sendInfo path, so they must keep receiving the echo. Suppress the redundant PAM-conversation echo only for Native clients by returning false from shouldSendAuthMessage when clientType == Native and the response is a success. Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> --- pam/pam.go | 26 ++++++++++++++++-- pam/pam_session_test.go | 59 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 2 deletions(-) create mode 100644 pam/pam_session_test.go diff --git a/pam/pam.go b/pam/pam.go index 84c4a47f43..712206a4af 100644 --- a/pam/pam.go +++ b/pam/pam.go @@ -119,6 +119,21 @@ func sendReturnMessageToPam(mTx pam.ModuleTransaction, retStatus adapter.PamRetu } } +func shouldSendAuthMessage(clientType adapter.PamClientType, msg string, isSuccess bool) bool { + if msg == "" { + return false + } + + if isSuccess { + // Native clients (SSH, non-TTY) already display the success message + // via the native model's sendInfo path; skip the PAM-conversation echo + // to avoid printing it twice. + return clientType != adapter.Native + } + + return true +} + // initLogging initializes the logging given the passed parameters. // It returns a function that should be called in order to reset the logging to // the default and potentially close the opened resources. @@ -326,19 +341,26 @@ func (h *pamModule) handleAuthRequest(mode authd.SessionMode, mTx pam.ModuleTran return pam.ErrAbort } - sendReturnMessageToPam(mTx, exitStatus) - switch exitStatus := exitStatus.(type) { case adapter.PamSuccess: + if shouldSendAuthMessage(pamClientType, exitStatus.Message(), true) { + sendReturnMessageToPam(mTx, exitStatus) + } if err := mTx.SetData(authenticationBrokerIDKey, exitStatus.BrokerID); err != nil { return err } return nil case adapter.PamReturnError: + if shouldSendAuthMessage(pamClientType, exitStatus.Message(), false) { + sendReturnMessageToPam(mTx, exitStatus) + } return fmt.Errorf("%w: %s", exitStatus.Status(), exitStatus.Message()) default: + // Preserve the previous behavior of showing any message associated with + // unexpected exit statuses before returning the system error. + sendReturnMessageToPam(mTx, exitStatus) return fmt.Errorf("%w: unknown exit code: %#v", pam.ErrSystem, exitStatus) } } diff --git a/pam/pam_session_test.go b/pam/pam_session_test.go new file mode 100644 index 0000000000..898776a05a --- /dev/null +++ b/pam/pam_session_test.go @@ -0,0 +1,59 @@ +package main + +import ( + "testing" + + "github.com/canonical/authd/pam/internal/adapter" + "github.com/stretchr/testify/require" +) + +func TestShouldSendAuthMessage(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + clientType adapter.PamClientType + msg string + isSuccess bool + + want bool + }{ + "Does_not_send_native_success_messages_again": { + clientType: adapter.Native, + msg: "cached", + isSuccess: true, + want: false, + }, + "Sends_gdm_success_messages_via_pam_conversation": { + clientType: adapter.Gdm, + msg: "cached", + isSuccess: true, + want: true, + }, + "Sends_interactive_terminal_success_messages": { + clientType: adapter.InteractiveTerminal, + msg: "cached", + isSuccess: true, + want: true, + }, + "Sends_error_messages": { + clientType: adapter.Native, + msg: "denied", + isSuccess: false, + want: true, + }, + "Ignores_empty_messages": { + clientType: adapter.Native, + msg: "", + isSuccess: true, + want: false, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + require.Equal(t, tc.want, shouldSendAuthMessage(tc.clientType, tc.msg, tc.isSuccess)) + }) + } +}