Use lowercase usernames #723
Conversation
adombeck
force-pushed
the
UDENG-4518-case-insensitive-usernames
branch
7 times, most recently
from
2c9dd18 to
b990470
Compare
All the brokers we currently support (msentraid and google) use case-insensitive usernames. Currently, logging in with a username which differs in upper- or lowercase from the username in the broker results in an authentication failure. We want to fix that. The easiest way to do that is to convert all usernames to lowercase before storing them in our database or passing them to the broker, and also convert the Name argument of GetPassdByName to lowercase before querying the database. If we (or anyone else) ever want to add a broker for a provider which does *not* use case-insensitive usernames, we will have to revisit this and decide whether we want to support that.
To be able to treat usernames as case-insensitive, we store them in lowercase in our database and query them in lowercase. To make that work with existing databases, we need to convert the usernames of all existing user records in the database to lowercase. Note that we don't modify the home directory. If an uppercase username is part of the home directory, it will still be in uppercase.
Currently, we only support groups with msentraid. The names of those groups are case-insensitive: https://learn.microsoft.com/en-us/azure/azure-resource-manager/management/resource-name-rules So we also store those group names case-insensitive, same as for usernames. Note that Unix groups are case-sensitive, so we treat local groups as case-sensitive.
The username passed to SelectBroker is converted to lowercase, so the test doesn't check what it's supposed to check. We already have a test for that in internal/users/internal_test.go though.
It was hard to figure out why a test failed with:
no validator for UI layout type ""
It's easier when we return an error instead of just an empty UI layout.
adombeck
force-pushed
the
UDENG-4518-case-insensitive-usernames
branch
from
b990470 to
2d28ca5
Compare
There was a problem hiding this comment.
So... There are some improvements to do in the code (mostly the handling of the passwd file is my main concern).
However, in general I'm not fully sure that this is the right path to take if we want to be consistent with the authd architecture and we want to be future proof, I mean:
- Authd is meant to be brokers-agnostic, the fact we've two brokers handling with domain-like names right now, it doesn't imply that we may support very different other kinds in future that instead require having case-sensitive names.
- Unix usernames are case-insensitive? authd user names should folllow it.
- It should be the up to the brokers work to do the username sanitizing and telling authd if two usernames match
So, this said... I'm still thinking that we should take the path of adding further method(s)? to the brokers (and maybe NSS) API in order to achieve something like this without touching authd "core" too much.
If we want to do some kind of migration, maybe brokers can provide features "flags" telling us how to behave, or we can make them do some user-matching check...
| @@ -136,6 +136,15 @@ func TestUpdateUserEntry(t *testing.T) { | |||
| // These values don't matter. We just want to make sure they are the same as the ones provided by the manager. | |||
| LastPwdChange: -1, MaxPwdAge: -1, PwdWarnPeriod: -1, PwdInactivity: -1, MinPwdAge: -1, ExpirationDate: -1, | |||
| }, | |||
| "user1-in-caps": { | |||
| Name: "User1", | |||
There was a problem hiding this comment.
To match the test name it should be USER1 instead? so we're also checking that all chars are converted (since we didn't IIRC)
| log.Warningf(context.Background(), "Error migrating database to lowercase usernames: %v", err) | ||
| } else { |
There was a problem hiding this comment.
Early return instead of else
internal/users/cache/migration.go
Outdated
| if version == "" || semver.Compare(version, initialLowercaseUsernamesVersion) < 0 { | ||
| log.Infof(context.Background(), "Migrating database to lowercase usernames (database version: %q)", version) | ||
| if err := migrateToLowercaseUsernames(c); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
if version != "" && semver.Compare(version, initialLowercaseUsernamesVersion) >= 0 {
return nil;
}
log.Infof(context.Background(), "Migrating database to lowercase usernames (database version: %q)", version)
return migrateToLowercaseUsernames(c);| if err != nil { | ||
| return fmt.Errorf("error reading /etc/group: %w", err) | ||
| } | ||
| content = bytes.ReplaceAll(content, []byte(oldName), []byte(newName)) |
There was a problem hiding this comment.
Isn't this dangerous?
It may change an user created by another tool, called MarcoAntonio doing s/Marco/marco/g.
It's also potentially race if other processes are working with it, so I think that in case we should still always go via passwd, fixing our NSS module in case it tries to be in the middle.
All the brokers we currently support (msentraid and google) use case-insensitive usernames. Currently, logging in with a username which differs in upper- or lowercase from the username in the broker results in an authentication failure. We want to fix that.
The easiest way to do that is to convert all usernames to lowercase before storing them in our database or passing them to the broker, and also convert the Name argument of GetPassdByName to lowercase before querying the database.
If we (or anyone else) ever wants to add a broker for a provider which does not use case-insensitive usernames, we will have to revisit this and decide whether we want to support that.
Review and merge together with ubuntu/authd-oidc-brokers#314
Closes #530, #531
UDENG-4518
UDENG-4519
TODO
initialLowercaseUserAndGroupNamesVersionis set to the correct version