Generate unique UIDs and GIDs #663

adombeck · 2024-12-02T11:05:09Z

These changes have already partly been reviewed in https://github.com/canonical/authd-private/pull/9.

Closes #509
UDENG-5416
UDENG-4352

internal/users/localgroups/getpwent_c.go

internal/testsdetection/testsdetection.go

internal/users/localgroups/getpwent_c.go

internal/users/manager.go

internal/users/localentries/getpwent_c.go

internal/users/localentries/getpwent_test.go

internal/users/manager.go

adombeck · 2024-12-05T23:18:41Z

rebased on main and resolved conflicts

adombeck · 2024-12-21T16:21:43Z

I extracted the new code to handle temporary users and groups into a separate package and added some more tests.

Should have been removed by 4fd03a4.

It seems to be unused since ddbba26.

Combine TestUserByID and TestUserByName, TestGroupByID and TestGroupByName, because the test bodies were identical (except for the called function).

* Remove obsolete tests: They were testing behavior which has changed. * Fix tests by using either predictable UIDs or replacing UIDs in golden files with placeholders (normalizing) * Limit number of pre-auth user records to avoid denial of service by sending many SSH requests for unique users to fill up the UID range. * Remove field UID from users.UserInfo struct because it's not set in internal/brokers/broker.go anymore and there is no need to expose this field. * Rename localgroups package to localentries because it now also provides functions for passwd entries. * Check for error when calling getpwent or getgrent

3v1n0

I think we're almost there, some further nits and suggestions.

I was wondering, since there are some common code paths in groups and users temporary entries registration, would be possible to do some logic deduplication, or is it not possible?

3v1n0 · 2025-01-09T02:10:34Z

internal/users/manager_test.go

+
+func TestMain(m *testing.M) {
+	log.SetLevel(log.DebugLevel)
+	os.Exit(m.Run())


Just use m.Run() as per #691

internal/services/nss/nss.go

3v1n0 · 2025-01-09T02:18:30Z

internal/users/tempentries/groups.go

+		registerMutex: sync.Mutex{},
+		rwMutex:       sync.RWMutex{},


We normally call them just Mu in the codebase here, so can shorten the suffixes

Yeah, I've seen that. I thought rwMutex would be a bit easier to understand than rwMu, but I don't care much, so I'll change it.

3v1n0 · 2025-01-09T02:20:42Z

internal/users/tempentries/groups.go

+func (r *temporaryGroupRecords) groupEntry(group groupRecord) types.GroupEntry {
+	return types.GroupEntry{Name: group.name, GID: group.gid, Passwd: group.passwd}
+}


Does this need to be part of temporaryGroupRecords?

3v1n0 · 2025-01-09T02:22:37Z

internal/users/tempentries/groups.go

+			if cleanupErr := cleanup(); cleanupErr != nil {
+				err = errors.Join(err, cleanupErr)
+			}


Could it just be errors.Join(err, cleanup())?

It could, but I would find that harder to read. When you read that, you need to know and remember that errors.Join ignores nil arguments.

internal/users/localentries/getgrent_c.go

3v1n0 · 2025-01-09T02:50:12Z

internal/users/localentries/getpwent_c.go

+void unset_errno(void) {
+  errno = 0;
+}
+
+int get_errno(void) {
+  return errno;
+}


Maybe this will fit better in the new errno.go file?

When I try to move it:

diff --git a/internal/users/localentries/errno.go b/internal/users/localentries/errno.go index 8f345f3c..1cbf30ba 100644 --- a/internal/users/localentries/errno.go +++ b/internal/users/localentries/errno.go @@ -2,4 +2,22 @@ package localentries import "sync" +/* +#include <stdlib.h> +#include <pwd.h> +#include <grp.h> +#include <errno.h> +#include <string.h> + +void unset_errno(void) { + errno = 0; +} + +int get_errno(void) { + return errno; +} +*/ +//#cgo LDFLAGS: -Wl,--allow-multiple-definition +import "C" + var errnoMutex sync.Mutex diff --git a/internal/users/localentries/getgrent_c.go b/internal/users/localentries/getgrent_c.go index e99bc254..c4a5a8a2 100644 --- a/internal/users/localentries/getgrent_c.go +++ b/internal/users/localentries/getgrent_c.go @@ -7,16 +7,7 @@ package localentries #include <stdlib.h> #include <pwd.h> #include <grp.h> -#include <errno.h> #include <string.h> - -void unset_errno(void) { - errno = 0; -} - -int get_errno(void) { - return errno; -} */ //#cgo LDFLAGS: -Wl,--allow-multiple-definition import "C" diff --git a/internal/users/localentries/getpwent_c.go b/internal/users/localentries/getpwent_c.go index b635b474..aca04aed 100644 --- a/internal/users/localentries/getpwent_c.go +++ b/internal/users/localentries/getpwent_c.go @@ -7,16 +7,7 @@ package localentries #include <stdlib.h> #include <pwd.h> #include <grp.h> -#include <errno.h> #include <string.h> - -void unset_errno(void) { - errno = 0; -} - -int get_errno(void) { - return errno; -} */ //#cgo LDFLAGS: -Wl,--allow-multiple-definition import "C"

then building the package fails:

go test ./internal/users/localentries/... # github.com/ubuntu/authd/internal/users/localentries internal/users/localentries/getgrent_c.go:94:30: could not determine kind of name for C.EBADF internal/users/localentries/getgrent_c.go:40:15: could not determine kind of name for C.ENOENT internal/users/localentries/getgrent_c.go:94:39: could not determine kind of name for C.EPERM internal/users/localentries/getgrent_c.go:94:21: could not determine kind of name for C.ESRCH internal/users/localentries/getgrent_c.go:37:12: could not determine kind of name for C.get_errno internal/users/localentries/getgrent_c.go:34:2: could not determine kind of name for C.unset_errno FAIL github.com/ubuntu/authd/internal/users/localentries [build failed] FAIL github.com/ubuntu/authd/internal/users/localentries/testutils [build failed] FAIL

I've done some cleanups on this regards, please see commits at https://github.com/3v1n0/authd/commits/509-random-unique-ids/

not sure if you get a notification, I have a question here

Yeah, I think I did, however check the branch again as I've moved things to a more central place and added tests, in case we need to reuse it

internal/users/localentries/getpwent_c.go

3v1n0 · 2025-01-10T17:41:07Z

internal/users/localentries/errno.go

+
+import "sync"
+
+var errnoMutex sync.Mutex


Suggested change

var errnoMutex sync.Mutex

var errnoMu sync.Mutex

3v1n0 · 2025-01-10T17:41:23Z

internal/users/localentries/getgrent_c.go

+	Passwd string
+}
+
+var getgrentMutex sync.Mutex


Suggested change

var getgrentMutex sync.Mutex

var getgrentMu sync.Mutex

3v1n0 · 2025-01-10T17:41:48Z

internal/users/localentries/getpwent_c.go

+	Gecos string
+}
+
+var getpwentMutex sync.Mutex


Suggested change

var getpwentMutex sync.Mutex

var getpwentMu sync.Mutex

3v1n0 · 2025-01-10T18:00:00Z

internal/users/tempentries/tempentries.go

@@ -61,7 +61,7 @@ func (r *TemporaryRecords) UserByName(name string) (types.UserEntry, error) {
 //
 // Returns the generated UID and a cleanup function that should be called to remove the temporary user once the user was
 // added to the database.
-func (r *TemporaryRecords) RegisterUser(name string) (uid uint32, cleanup func() error, err error) {
+func (r *TemporaryRecords) RegisterUser(name string) (uid uint32, cleanup func(), err error) {


Do we have a reason to have named values here (a part from clarity, that I agree is nice)?

As the cleanup can be a bit dangerous, since it's going to be set to nil by default, and calling it by mistake before being set, will lead to a crash without being caught before.

adombeck force-pushed the 509-random-unique-ids branch 3 times, most recently from cb2ba1b to 8e15caf Compare December 3, 2024 00:05