diff --git a/README.md b/README.md index 38863765..42a2639e 100644 --- a/README.md +++ b/README.md @@ -80,6 +80,13 @@ The [default template](https://github.com/ubuntu/aad-auth/blob/main/conf/aad.con # ; %d - domain # shell = /bin/bash ; default shell for the user +## UID range configuration +## WARNING! Changing these values may effect existing users. +min_uid = 100000 ; minimum UID to assign (inclusive) +max_uid = 2147483647 ; maximum UID to assign (exclusive) + ; the default max_uid is 4294967295 (max uint32), however this causes issues with commonly used software, such as xdg-desktop-portal-gnome + ; this value (max int32) works with most software, while still providing a large UID space. + ### overriding values for a specific domain, every value inside a section is optional # [domain.com] # tenant_id = aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa diff --git a/conf/aad.conf.template b/conf/aad.conf.template index 44797aff..5c2a97f4 100644 --- a/conf/aad.conf.template +++ b/conf/aad.conf.template @@ -16,6 +16,13 @@ # ; %d - domain # shell = /bin/bash ; default shell for the user +## UID range configuration +## WARNING! Changing these values may effect existing users. +min_uid = 100000 ; minimum UID to assign (inclusive) +max_uid = 2147483647 ; maximum UID to assign (exclusive) + ; the default max_uid is 4294967295 (max uint32), however this causes issues with commonly used software, such as xdg-desktop-portal-gnome + ; this value (max int32) works with most software, while still providing a large UID space. + ### overriding values for a specific domain, every value inside a section is optional # [domain.com] # tenant_id = aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa diff --git a/internal/cache/cache.go b/internal/cache/cache.go index 26e86eab..b9633947 100644 --- a/internal/cache/cache.go +++ b/internal/cache/cache.go @@ -29,6 +29,8 @@ var ( ErrOfflineCredentialsExpired = errors.New("offline credentials expired") // ErrOfflineAuthDisabled is returned when offline authentication is disabled by using a negative value in aad.conf. ErrOfflineAuthDisabled = errors.New("offline authentication is disabled") + // ErrNoUIDAvailable is returned when there is no available UID within the specified range + ErrNoUIDAvailable = errors.New("no uid available") ) const ( @@ -330,13 +332,13 @@ func (c *Cache) CanAuthenticate(ctx context.Context, username, password string) } // Update creates and update user nss cache when there has been an online verification. -func (c *Cache) Update(ctx context.Context, username, password, homeDirPattern, shell string) (err error) { +func (c *Cache) Update(ctx context.Context, username, password, homeDirPattern, shell string, minUID, maxUID uint32) (err error) { defer decorate.OnError(&err, i18n.G("couldn't create/open cache for nss database")) user, err := c.GetUserByName(ctx, username) if errors.Is(err, ErrNoEnt) { // Try creating the user - id, err := c.generateUIDForUser(ctx, username) + id, err := c.generateUIDForUser(ctx, username, minUID, maxUID) if err != nil { return err } @@ -405,25 +407,30 @@ func encryptPassword(ctx context.Context, username, password string) (string, er } // generateUIDForUser returns an unique uid for the user to create. -func (c *Cache) generateUIDForUser(ctx context.Context, username string) (uid uint32, err error) { +func (c *Cache) generateUIDForUser(ctx context.Context, username string, minUID, maxUID uint32) (uid uint32, err error) { defer decorate.OnError(&err, i18n.G("failed to generate uid for user %q"), username) - logger.Debug(ctx, "generate user id for user %q", username) + logger.Debug(ctx, "generate user id for user %q (min %d, max %d)", username, minUID, maxUID) // compute uid for user - var offset uint32 = 100000 uid = 1 for _, c := range username { uid = (uid * uint32(c)) % math.MaxUint32 } - uid = uid%(math.MaxUint32-offset) + offset + uid = uid%(maxUID-minUID) + minUID // check collision or increment + initialUid := uid for { if exists, err := uidOrGidExists(c.db, uid, username); err != nil { return 0, err } else if exists { - uid++ + // Loop round between offset and modulus until a uid is found + uid = (uid-minUID+1)%(maxUID-minUID) + minUID + // If we've cycled all the way round, we should error + if uid == initialUid { + return 0, ErrNoUIDAvailable + } continue } diff --git a/internal/cache/cache_test.go b/internal/cache/cache_test.go index 7eb3e89f..234b91e4 100644 --- a/internal/cache/cache_test.go +++ b/internal/cache/cache_test.go @@ -5,6 +5,7 @@ import ( "flag" "fmt" "io/fs" + "math" "os" "path/filepath" "testing" @@ -293,7 +294,7 @@ func TestUpdate(t *testing.T) { var lastUID int64 for _, n := range tc.userNames { start := time.Now() - err := c.Update(context.Background(), n, "my password", "/home/%f", "/bin/bash") + err := c.Update(context.Background(), n, "my password", "/home/%f", "/bin/bash", 100000, math.MaxUint32) end := time.Now() if tc.wantErr { require.Error(t, err, "Update should have returned an error but hasn't") @@ -327,7 +328,7 @@ func TestUpdate(t *testing.T) { // we need one second as we are storing an unix timestamp for last online auth time.Sleep(time.Second) - err = c.Update(context.Background(), n, "other password", "/home/%f", "/bin/bash") + err = c.Update(context.Background(), n, "other password", "/home/%f", "/bin/bash", 100000, math.MaxUint32) if tc.wantErrRefresh { require.Error(t, err, "Second update should have returned an error but hasn't") return @@ -542,6 +543,122 @@ func TestQueryPasswdAttribute(t *testing.T) { } } +func TestGenerateUIDForUser(t *testing.T) { + t.Parallel() + + cacheDir := t.TempDir() + cacheDB := "users_in_db" + testutils.PrepareDBsForTests(t, cacheDir, cacheDB) + c := testutils.NewCacheForTests(t, cacheDir) + + tests := []struct { + username string + offset uint32 + modulus uint32 + expected uint32 + }{ + // Default offset and modulus + {"jacob.otoole@mevitae.com", 100000, math.MaxUint32, 1982826144}, + {"frederick.shaw@mevitae.com", 100000, math.MaxUint32, 155813536}, + // An example of a collision + {"frankie.fisher@mevitae.com", 100000, math.MaxUint32, 155813536}, + + // Test a smaller UID range + {"frankie.fisher@mevitae.com", 2000, 3000, 0}, + {"frederick.shaw@mevitae.com", 2000, 3000, 0}, + {"jacob.otoole@mevitae.com", 2000, 3000, 0}, + {"a", 2000, 30000, 0}, + {"aa", 2000, 30000, 0}, + {"aaa", 2000, 30000, 0}, + {"aaaa", 2000, 30000, 0}, + {"aaaaa", 2000, 30000, 0}, + {"aaaaaa", 2000, 30000, 0}, + {"aaaaaaa", 2000, 30000, 0}, + {"aaaaaaaa", 2000, 30000, 0}, + } + for _, tc := range tests { + tc := tc + t.Run(fmt.Sprint("generateUidFor ", tc.username, " from ", tc.offset, " to ", tc.modulus), func(t *testing.T) { + uid, err := c.GenerateUIDForUser(context.Background(), tc.username, tc.offset, tc.modulus) + require.NoError(t, err) + if tc.expected != 0 { + require.Equal(t, tc.expected, uid) + } + require.LessOrEqual(t, tc.offset, uid) + require.Greater(t, tc.modulus, uid) + }) + } +} + +func TestUpdateUIDs(t *testing.T) { + t.Parallel() + + cacheDir := t.TempDir() + cacheDB := "users_in_db" + testutils.PrepareDBsForTests(t, cacheDir, cacheDB) + c := testutils.NewCacheForTests(t, cacheDir) + + tests := []struct { + username string + offset uint32 + modulus uint32 + expected uint32 + err error + }{ + // Default offset and modulus + {"jacob.otoole@mevitae.com", 100000, math.MaxUint32, 1982826144, nil}, + {"frederick.shaw@mevitae.com", 100000, math.MaxUint32, 155813536, nil}, + // A collision example, since it's using the database, it is incremented + {"frankie.fisher@mevitae.com", 100000, math.MaxUint32, 155813537, nil}, + + // Test a smaller UID range + {"frankie.fisher1@mevitae.com", 2000, 30000, 0, nil}, + {"frederick.shaw1@mevitae.com", 2000, 30000, 0, nil}, + {"jacob.otoole1@mevitae.com", 2000, 30000, 0, nil}, + {"a", 2000, 30000, 0, nil}, + {"aa", 2000, 30000, 0, nil}, + {"aaa", 2000, 30000, 0, nil}, + {"aaaa", 2000, 30000, 0, nil}, + {"aaaaa", 2000, 30000, 0, nil}, + {"aaaaaa", 2000, 30000, 0, nil}, + {"aaaaaaa", 2000, 30000, 0, nil}, + {"aaaaaaaa", 2000, 30000, 0, nil}, + + // A short UID range causes an overflow + {"b", 1000, 1002, 1000, nil}, + {"bb", 1000, 1002, 1001, nil}, + {"bbb", 1000, 1002, 1002, cache.ErrNoUIDAvailable}, + + // We should also be able to handle overflow at the top of the uint32 space, and certainly not overflow to be root!!! + {"c", math.MaxUint32 - 2, math.MaxUint32, math.MaxUint32 - 1, nil}, + {"cc", math.MaxUint32 - 2, math.MaxUint32, math.MaxUint32 - 2, nil}, + {"ccc", math.MaxUint32 - 2, math.MaxUint32, 0, cache.ErrNoUIDAvailable}, + {"cccc", math.MaxUint32 - 2, math.MaxUint32, 1, cache.ErrNoUIDAvailable}, + } + for _, tc := range tests { + tc := tc + t.Run(fmt.Sprint("Update UID test for ", tc.username, " from ", tc.offset, " to ", tc.modulus), func(t *testing.T) { + // Update the user + err := c.Update(context.Background(), tc.username, "", "/home/%U", "/bin/sh", tc.offset, tc.modulus) + if tc.err == nil { + require.NoError(t, err, "Update should not error") + } else { + require.ErrorAs(t, err, &tc.err) + return + } + // Check the UID which was generated + uidInterface, err := c.QueryPasswdAttribute(context.Background(), tc.username, "uid") + uid := uint32(uidInterface.(int64)) + require.NoError(t, err, "QueryPasswdAttribute should not error") + if tc.expected != 0 { + require.Equal(t, tc.expected, uid) + } + require.LessOrEqual(t, tc.offset, uid) + require.Greater(t, tc.modulus, uid) + }) + } +} + func TestMain(m *testing.M) { testutils.InstallUpdateFlag() flag.Parse() diff --git a/internal/cache/cache_uid_test.go b/internal/cache/cache_uid_test.go new file mode 100644 index 00000000..82069e07 --- /dev/null +++ b/internal/cache/cache_uid_test.go @@ -0,0 +1,9 @@ +package cache + +import ( + "context" +) + +func (c *Cache) GenerateUIDForUser(ctx context.Context, username string, minUID, maxUID uint32) (uint32, error) { + return c.generateUIDForUser(ctx, username, minUID, maxUID) +} diff --git a/internal/config/config.go b/internal/config/config.go index ea8f49fd..187d60c4 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -4,6 +4,7 @@ package config import ( "context" "fmt" + "math" "path/filepath" "github.com/go-ini/ini" @@ -26,6 +27,10 @@ type AAD struct { OfflineCredentialsExpiration *int `ini:"offline_credentials_expiration"` HomeDirPattern string `ini:"homedir"` Shell string `ini:"shell"` + // MinUID is the minimum allowed UID to be assigned + MinUID uint32 `ini:"min_uid"` + // MaxUID is one above the maximum UID to be assigned + MaxUID uint32 `ini:"max_uid"` } // ToIni reflects the configuration values to an ini.File representation. @@ -64,6 +69,8 @@ func Load(ctx context.Context, p, domain string, opts ...Option) (config AAD, er config = AAD{ HomeDirPattern: defaultHomePattern, Shell: defaultShell, + MinUID: 100000, + MaxUID: math.MaxUint32, } // Tries to load the defaults from the adduser.conf @@ -93,6 +100,9 @@ func Load(ctx context.Context, p, domain string, opts ...Option) (config AAD, er if config.AppID == "" { return AAD{}, fmt.Errorf("missing required 'app_id' entry in configuration file") } + if config.MinUID >= config.MaxUID { + return AAD{}, fmt.Errorf("'min_uid' must be less than 'max_uid' in configuration file") + } return config, nil } diff --git a/internal/pam/pam.go b/internal/pam/pam.go index 6d3e612c..8dc7a64c 100644 --- a/internal/pam/pam.go +++ b/internal/pam/pam.go @@ -107,7 +107,7 @@ func Authenticate(ctx context.Context, username, password, conf string, opts ... } // Successful online login, update cache. - if err := c.Update(ctx, username, password, cfg.HomeDirPattern, cfg.Shell); err != nil { + if err := c.Update(ctx, username, password, cfg.HomeDirPattern, cfg.Shell, cfg.MinUID, cfg.MaxUID); err != nil { logError(ctx, i18n.G("%w. Denying access."), err) return ErrPamAuth }