Restrict username characters to those allowed in Ubuntu/Debian#1452
Restrict username characters to those allowed in Ubuntu/Debian#1452
Conversation
- Add ValidateUsername() function in internal/users/types/ using regex ^[a-z_][-a-z0-9_]*[$]?$ following shadow-utils/useradd rules - Call ValidateUsername() in validateUserInfo() in brokers/broker.go to validate system usernames returned by brokers before they are stored - Update test mock broker to return valid system usernames (remove @example.com suffix, add ia_info_invalid_username case) - Add TestValidateUsername unit test with valid and invalid username cases - Add Error_when_broker_returns_userinfo_with_invalid_username test cases to both broker and PAM service tests - Update all golden files to reflect new valid username format Agent-Logs-Url: https://github.com/canonical/authd/sessions/0deae640-34db-4f1f-bcb1-e06aa4f75083 Co-authored-by: adombeck <18482300+adombeck@users.noreply.github.com>
- Update valid.group to use 'success_with_local_groups' instead of 'success_with_local_groups@example.com' to prevent duplicate entries - Update cache-with-locked-user.db to use 'locked' instead of 'locked@example.com' so the locked user check works correctly - Update golden files to reflect correct test behavior Agent-Logs-Url: https://github.com/canonical/authd/sessions/0deae640-34db-4f1f-bcb1-e06aa4f75083 Co-authored-by: adombeck <18482300+adombeck@users.noreply.github.com>
Agent-Logs-Url: https://github.com/canonical/authd/sessions/0deae640-34db-4f1f-bcb1-e06aa4f75083 Co-authored-by: adombeck <18482300+adombeck@users.noreply.github.com>
| // Invalid usernames | ||
| "Error_on_empty_username": {username: "", wantErr: true}, | ||
| "Error_on_uppercase_character": {username: "User", wantErr: true}, | ||
| "Error_on_email_style_name": {username: "user@example.com", wantErr: true}, |
There was a problem hiding this comment.
Not really babe!
This must be valid!
There was a problem hiding this comment.
had to laugh, but you can let me review the PRs I tell copilot to open first, I don't want to waste your time :)
There was a problem hiding this comment.
Fixed in ee16c0a. The regex now allows @ and . — user@example.com is valid. The test case has been updated to Valid_email_style_name.
Remove @example.com from group member names in test data files to be consistent with the new username validation rules Agent-Logs-Url: https://github.com/canonical/authd/sessions/0deae640-34db-4f1f-bcb1-e06aa4f75083 Co-authored-by: adombeck <18482300+adombeck@users.noreply.github.com>
| localgroup3:x:43:otheruser2@example.com,success_with_local_groups | ||
| localgroup4:x:44:otheruser2@example.com | ||
| cloudgroup1:x:9998:otheruser3@example.com | ||
| cloudgroup2:x:9999:otheruser4@example.com |
There was a problem hiding this comment.
@copilot WRONNNNNNGGGGGGGGGGG @ is fully valid for us!
There was a problem hiding this comment.
Fixed in ee16c0a. Reverted the group files to use email-style usernames. The regex is now ^[a-z_][-a-z0-9_.@]*[$]?$ — @ and . are fully valid.
- Update regex to ^[a-z_][-a-z0-9_.@]*[$]?$ to allow @ and . chars which are needed for email-style usernames (e.g. user@example.com) used by cloud identity providers like MS Entra ID and Google IAM - Revert test mock broker to use @example.com email-style usernames - Update ia_info_invalid_username to use -invalid_username (starts with hyphen) as the clearly invalid test case instead - Revert test data files (valid.group, cache-with-locked-user.db) - Update test unit cases: email-style names are valid, not invalid - Regenerate all affected golden files Agent-Logs-Url: https://github.com/canonical/authd/sessions/aab36fdb-b65d-458f-9c58-8bee7bb06733 Co-authored-by: 3v1n0 <345675+3v1n0@users.noreply.github.com>
| "Error_on_name_starting_with_digit": {username: "1user", wantErr: true}, | ||
| "Error_on_name_starting_with_hyphen": {username: "-user", wantErr: true}, | ||
| "Error_on_name_with_dollar_not_at_end": {username: "user$name", wantErr: true}, | ||
| "Error_on_name_with_space": {username: "user name", wantErr: true}, |
There was a problem hiding this comment.
Check if a name has / or other elements that may lead to injections such as quotes and so on
There was a problem hiding this comment.
Done in b8eb81b. Added explicit wantErr: true test cases for /, \, ', ", `, ;, &, |, null byte, newline, tab, :, !, (, ), <, and > — all correctly rejected by the allowlist regex.
Add explicit error test cases for characters that could cause injection or path traversal issues: /, \, ', ", `, ;, &, |, null byte, newline, tab, :, !, (, ), <, >. All are correctly rejected by the allowlist regex. Agent-Logs-Url: https://github.com/canonical/authd/sessions/28fe1c37-8afa-4239-a969-08d92f5a94f5 Co-authored-by: 3v1n0 <345675+3v1n0@users.noreply.github.com>
| // It follows the Debian/Ubuntu username policy as defined by shadow-utils/useradd rules, | ||
| // extended to allow '@' and '.' characters for cloud identity provider email-style usernames | ||
| // (e.g. user@example.com). | ||
| var usernameRegexp = regexp.MustCompile(`^[a-z_][-a-z0-9_.@]*[$]?$`) | ||
|
|
There was a problem hiding this comment.
I wonder how likely it is for this to cause regressions, i.e. there being existing authd users with characters that are allowed by this change, causing login to fail for those users.
We might want to allow all characters that are allowed in email addresses.
There was a problem hiding this comment.
We might want to allow all characters that are allowed in email addresses.
But email addresses allow / which opens the door to path traversal again. Maybe we just shouldn't try to validate the username and keep accepting what the broker gives us?
There was a problem hiding this comment.
i.e. there being existing authd users with characters that are allowed by this change, causing login to fail for those users.
and new users have the same issue if their email address contains characters not allowed by this change. they will have to change their email address to be able to log in successfully. and in the current implementation, they are only told so after successful device auth, which makes the UX even worse.
There was a problem hiding this comment.
Maybe we can just deny-list some relevant chars such as all the ones included in these tests, which even if used by someone, really seems wrong.
// Injection / path traversal characters must be rejected
"Error_on_name_with_slash": {username: "user/name", wantErr: true},
"Error_on_name_with_backslash": {username: `user\name`, wantErr: true},
"Error_on_name_with_single_quote": {username: "user'name", wantErr: true},
"Error_on_name_with_double_quote": {username: `user"name`, wantErr: true},
"Error_on_name_with_backtick": {username: "user`name", wantErr: true},
"Error_on_name_with_semicolon": {username: "user;name", wantErr: true},
"Error_on_name_with_ampersand": {username: "user&name", wantErr: true},
"Error_on_name_with_pipe": {username: "user|name", wantErr: true},
"Error_on_name_with_null_byte": {username: "user\x00name", wantErr: true},
"Error_on_name_with_newline": {username: "user\nname", wantErr: true},
"Error_on_name_with_tab": {username: "user\tname", wantErr: true},
"Error_on_name_with_colon": {username: "user:name", wantErr: true},
"Error_on_name_with_exclamation": {username: "user!name", wantErr: true},
"Error_on_name_with_open_paren": {username: "user(name", wantErr: true},
"Error_on_name_with_close_paren": {username: "user)name", wantErr: true},
"Error_on_name_with_less_than": {username: "user<name", wantErr: true},
"Error_on_name_with_greater_than": {username: "user>name", wantErr: true},Likely including any white-space and non-printable character in general
authd did not validate the characters in system usernames returned by brokers. authd should validate usernames and only allow characters that are also allowed on Ubuntu/Debian systems.
Changes
Validation logic
internal/users/types/userinfo.go: AddedValidateUsername()using regex^[a-z_][-a-z0-9_.@]*[$]?$per shadow-utils/useradd rules, extended to also allow@and.for email-style usernames used by cloud identity providers (e.g.user@example.com)internal/brokers/broker.go: CallsValidateUsername()insidevalidateUserInfo(), the single enforcement point before any broker-returned username is persistedTest updates
TestValidateUsernameunit tests covering valid cases (including email-style names) and invalid cases/,\,',",`,;,&,|, null byte, newline, tab,:,!,(,),<,>) — all correctly rejected by the allowlist regexError_when_broker_returns_userinfo_with_invalid_usernametest cases to both broker and PAM service layers; the invalid test username uses-invalid_username(starts with a hyphen)ia_info_invalid_usernamecase to the mock broker returning-invalid_usernameto exercise the new validation