Implement utmp/wtmp support #5491

xacrimon · 2021-02-05T13:39:52Z

This PR introduces support for updating utmp and wtmp files on Linux with current and past interactive sessions.

This is a best-effort feature. It is only enabled on platforms where utmp/wtmp exists and will do nothing otherwise. If it lacks permissions to modify the user accounting database a warning will be logged instead of taking down the service as this is not a critical feature that needs to be enabled in all cases.

Fixes #3987

webvictim

Welcome @xacrimon!

Just a few quick nits; I'm sure others will have more detailed analysis!

lib/srv/ctx.go

lib/utils/ttyname.go

lib/utils/uacc.go

lib/utils/uacc.h

lib/srv/ctx.go

lib/utils/uacc.h

a-palchikov

I'd draw some inspiration from https://github.com/ericlagergren/go-gnulib/tree/master/ttyname and https://github.com/ericlagergren/go-gnulib/tree/master/utmp as a way to avoid cgo.

lib/srv/ctx.go

lib/utils/ttyname.go

lib/utils/uacc.go

lib/utils/uacc.h

lib/utils/ttyname.go

lib/utils/uacc.h

lib/srv/termhandlers.go

russjones · 2021-02-06T01:25:19Z

@xacrimon Also a bookkeeping comment, when this PR has been approved by two reviewers make sure you rebase and merge so we get a single commit on master with no merge commit.

lib/srv/ctx.go

lib/srv/termhandlers.go

lib/srv/ctx.go

lib/srv/reexec.go

lib/srv/uacc/uacc.go

lib/srv/reexec.go

lib/srv/uacc/uacc_fallback.go

lib/srv/uacc/uacc_linux.go

lib/srv/reexec.go

lib/srv/uacc/uacc_stub.go

russjones · 2021-02-11T19:18:00Z

lib/srv/reexec.go

+	// The path of the system utmp database.
+	UtmpPath *string `json:"utmp_path"`
+
+	// The path of the system wtmp log.
+	WtmpPath *string `json:"wtmp_path"`


Any reason these are pointers?

Also Go comments start with the name of the variable, so for this (and elsewhere):

// UtmpPath is the path to the system utmp database.

They're pointers because they're optional, if not provided, uacc will default to system defaults.

Adjusted comments

lib/srv/forward/sshserver.go

lib/srv/uacc/uacc_linux.go

lib/srv/ctx.go

lib/srv/exec_test.go

Makefile

integration/db_integration_test.go

awly · 2021-02-12T00:32:07Z

integration/root_integration_test.go

+	"gopkg.in/check.v1"
+)
+
+func TestIntegrationsRoot(t *testing.T) { check.TestingT(t) }


this was marked as resolved but the function isn't removed
i think this whole file can be removed

integration/utmp_integration_test.go

lib/srv/ctx.go

lib/srv/reexec.go

integration/integration_test.go

lib/srv/ctx.go

integration/utmp_integration_test.go

lib/srv/ctx.go

lib/srv/uacc/uacc.h

integration/utmp_integration_test.go

lib/srv/forward/sshserver.go

lib/srv/regular/sshserver.go

lib/srv/uacc/uacc.h

russjones · 2021-02-15T19:16:24Z

Also, what does e point to? It leads to invalid commit for me.

ben-latacora · 2021-07-29T17:08:49Z

lib/srv/uacc/uacc.h

+    }
+    struct utmp *entry = getutent();
+    while (entry != NULL) {
+        if (entry->ut_type == USER_PROCESS && strcmp(user, entry->ut_user) == 0) {


Should this use strncmp since the utmp fields aren't necessarily null terminated? Worst case, it seems like this would read into the hostname field.

ben-latacora · 2021-07-29T17:10:22Z

lib/srv/uacc/uacc_linux.go

+var accountDb sync.Mutex
+
+// Max length of username and hostname as defined by glibc.
+const nameMaxLen = 255


Looks like usernames should be limited at 32 bytes. 256 is only for hostnames. The strncpys should truncate anything that's too long, but it would be good to return helpful errors instead.

xacrimon · 2021-08-03T17:10:23Z

@ben-latacora Thanks! I've posted a new PR addressing your feedback. Good catch.

xacrimon requested review from a-palchikov, russjones and fspmarshall February 5, 2021 13:39

xacrimon requested review from awly, klizhentas, r0mant and webvictim as code owners February 5, 2021 13:39

xacrimon removed request for a-palchikov and fspmarshall February 5, 2021 13:40

xacrimon requested review from a-palchikov and fspmarshall as code owners February 5, 2021 13:44

xacrimon self-assigned this Feb 5, 2021

xacrimon added c-ju Internal Customer Reference c-q7j Internal Customer Reference R3 sales-onboarding Issues related to prospects labels Feb 5, 2021

webvictim reviewed Feb 5, 2021

View reviewed changes

xacrimon removed the sales-onboarding Issues related to prospects label Feb 5, 2021

a-palchikov reviewed Feb 5, 2021

View reviewed changes

russjones reviewed Feb 6, 2021

View reviewed changes

lib/utils/ttyname.go Outdated Show resolved Hide resolved

lib/utils/uacc.h Outdated Show resolved Hide resolved

lib/srv/termhandlers.go Outdated Show resolved Hide resolved

xacrimon force-pushed the joel/utmp-wtmp branch from 4ce407d to fc28273 Compare February 8, 2021 15:08

xacrimon requested review from a-palchikov and russjones February 8, 2021 19:52

a-palchikov reviewed Feb 8, 2021

View reviewed changes

xacrimon requested a review from a-palchikov February 8, 2021 20:50

xacrimon force-pushed the joel/utmp-wtmp branch from beb0586 to 08615f0 Compare February 8, 2021 22:19

awly reviewed Feb 9, 2021

View reviewed changes

xacrimon force-pushed the joel/utmp-wtmp branch from 7fcd399 to d76cc59 Compare February 9, 2021 00:56

russjones reviewed Feb 9, 2021

View reviewed changes

lib/srv/reexec.go Outdated Show resolved Hide resolved

lib/srv/reexec.go Outdated Show resolved Hide resolved

lib/srv/uacc/uacc_stub.go Show resolved Hide resolved

xacrimon force-pushed the joel/utmp-wtmp branch 2 times, most recently from f8f2413 to 58e9887 Compare February 11, 2021 17:30

xacrimon requested a review from russjones February 11, 2021 17:49

xacrimon force-pushed the joel/utmp-wtmp branch from 986a037 to 35ebc89 Compare February 11, 2021 19:04

russjones reviewed Feb 11, 2021

View reviewed changes

xacrimon requested a review from awly February 11, 2021 19:46

xacrimon force-pushed the joel/utmp-wtmp branch from 9511a09 to 68904de Compare February 11, 2021 20:07

awly reviewed Feb 12, 2021

View reviewed changes

xacrimon force-pushed the joel/utmp-wtmp branch 2 times, most recently from cc6b5d9 to 79189e8 Compare February 12, 2021 17:48

xacrimon requested a review from awly February 12, 2021 18:14

a-palchikov reviewed Feb 12, 2021

View reviewed changes

integration/utmp_integration_test.go Outdated Show resolved Hide resolved

lib/srv/ctx.go Outdated Show resolved Hide resolved

lib/srv/ctx.go Outdated Show resolved Hide resolved

lib/srv/uacc/uacc.h Outdated Show resolved Hide resolved

xacrimon requested a review from a-palchikov February 12, 2021 20:01

awly approved these changes Feb 13, 2021

View reviewed changes

integration/utmp_integration_test.go Show resolved Hide resolved

xacrimon force-pushed the joel/utmp-wtmp branch 3 times, most recently from ed27e23 to f49662d Compare February 15, 2021 18:40

russjones reviewed Feb 15, 2021

View reviewed changes

lib/srv/forward/sshserver.go Outdated Show resolved Hide resolved

lib/srv/regular/sshserver.go Outdated Show resolved Hide resolved

lib/srv/uacc/uacc.h Show resolved Hide resolved

russjones approved these changes Feb 15, 2021

View reviewed changes

Implemented utmp/wtmp support.

c575ce5

xacrimon force-pushed the joel/utmp-wtmp branch from c234972 to c575ce5 Compare February 15, 2021 19:45

xacrimon merged commit 324ccda into master Feb 15, 2021

xacrimon deleted the joel/utmp-wtmp branch February 26, 2021 22:25

xacrimon restored the joel/utmp-wtmp branch February 26, 2021 22:25

ben-latacora reviewed Jul 29, 2021

View reviewed changes

xacrimon mentioned this pull request Aug 3, 2021

Fix unsoundness and improve errors in user accounting logic #7785

Merged

zmb3 deleted the joel/utmp-wtmp branch August 4, 2022 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement utmp/wtmp support #5491

Implement utmp/wtmp support #5491

xacrimon commented Feb 5, 2021 •

edited

Loading

webvictim left a comment

a-palchikov left a comment

russjones commented Feb 6, 2021 •

edited

Loading

russjones Feb 11, 2021

xacrimon Feb 11, 2021

xacrimon Feb 11, 2021

awly Feb 12, 2021

russjones commented Feb 15, 2021

ben-latacora Jul 29, 2021

ben-latacora Jul 29, 2021

xacrimon commented Aug 3, 2021

Implement utmp/wtmp support #5491

Implement utmp/wtmp support #5491

Conversation

xacrimon commented Feb 5, 2021 • edited Loading

webvictim left a comment

Choose a reason for hiding this comment

a-palchikov left a comment

Choose a reason for hiding this comment

russjones commented Feb 6, 2021 • edited Loading

russjones Feb 11, 2021

Choose a reason for hiding this comment

xacrimon Feb 11, 2021

Choose a reason for hiding this comment

xacrimon Feb 11, 2021

Choose a reason for hiding this comment

awly Feb 12, 2021

Choose a reason for hiding this comment

russjones commented Feb 15, 2021

ben-latacora Jul 29, 2021

Choose a reason for hiding this comment

ben-latacora Jul 29, 2021

Choose a reason for hiding this comment

xacrimon commented Aug 3, 2021

xacrimon commented Feb 5, 2021 •

edited

Loading

russjones commented Feb 6, 2021 •

edited

Loading