Auditd integration #14948

jakule · 2022-07-27T16:43:17Z

This PR implements integration with auditd. Events sent to Linux Audit System (auditd) should closely match those sends by OpenSSH.

Closes #13785

lib/auditd/auditd_linux.go

nklaassen · 2022-08-16T20:21:47Z

lib/auditd/auditd_linux.go

+	// if value is not set, logind PAM module will set it to the correct value
+	// after fork. 4294967295 is -1 converted to uint32.
+	return loginuid != 4294967295


I notice getSelfLoginUID() returns an int64 but it's compared with the max value of a uint32, not sure of both of those are intentional

Yes. All uint32 values can be represented as int64, and I didn't want any value to be incorrectly "rounded" when reading it from /proc/self/loginuid as strconv.ParseInt returns int64.

lib/auditd/auditd_linux.go

lib/service/service.go

nklaassen · 2022-08-16T21:34:51Z

lib/service/service.go

@@ -2206,6 +2207,11 @@ func (process *TeleportProcess) initSSH() error {
 			return trace.Wrap(err)
 		}

+		if auditd.IsLoginUIDSet() {
+			log.Warnf("login UID is set, but it shouldn't. Incorrect login UID breaks session ID when using auditd. " +


Should this be an error? I guess that wouldn't be backward compatible. Will things break if this occurs, or the feature will be disabled?

Should this be an error?

Questionable. One field in the event log will be "incorrect". Each SSH session has a unique session ID but only when the login UID is not set. That makes it easy to trace it. If the login UID is set in the main process, then all sessions share the same session ID, so it's hard to distinguish which event belongs to each session. You can still try to filter by PID or username and at least have partial data.
We cannot also fully disable the feature here. When PAM is enabled, some events are generated outside of Teleport, so disabling anything on our side would make it even more confusing, in my opinion. Then auditd would receive "incorrect" events from PAM and none from Teleport.
The good news is that most people should run Teleport from systemd. Then login UID is correct, and the problem doesn't exist. For those few cases when you run Teleport from bash (probably the most common scenario) as a root we will print that warning.

nklaassen · 2022-08-16T21:36:49Z

lib/srv/reexec.go

+		return errorWriter, teleport.RemoteCommandFailure, trace.Errorf("failed to login user start: %v", err)
+	}
+
+	defer func(err *error) {


I don't think you need to get so tricky with passing the address of err as an argument, the deferred function can just capture err since it is a named return value

Good point, fixed.

lib/srv/reexec.go

lib/auditd/auditd.go

lib/auditd/auditd_linux.go

Joerger · 2022-08-18T17:40:05Z

lib/auditd/auditd_linux.go

+	return status, nil
+}
+
+// SendMsg sends a message. Client will create a new connection if not connected already.


As Nic mentioned, it currently looks like SendMsg and SendEvent are always creating a new client rather than using the cached client.

I might be missing something, but I'd recommend changing the API to function like this:

client, err := auditd.NewClient() if err != nil { return trace.Wrap(err) } defer client.Close() if err := client.SendEvent(event); err != nil { return trace.Wrap(err) }

Then in auditd.go you can just introduce a mock client so that creating the client and sending events both produce noops.

I was thinking about it, and the initial implementation looked like that. My main issue with this approach is that it is a bit verbose compared to only one function call. Also, you need to remember to close the connection at the end. That's why I decided to wrap everything in a function.

There are, of course, a few more things to consider if we use only SendEvent():

If we create a new connection for each message, we probably create more connections when needed. This is not exactly true. In my implementation, I found only one place where we could re-use the connection. I don't think that is a big issue then.

Why then my client internally holds a mutex if the connection is not being used from more than one goroutine? Mainly for correctness. the auditd package exposes the client, so if anyone decides to use it in the future, it may create a race condition without knowing it.

I think there are three possible actions here:

Remove the internal mutex to be explicit about the fact that it is not really needed. Make NewClient() constructor private, so the only API in that package is the SendEvent() function. That would make the API more explicit.

Leave it as it is. Currently, the auditd package exposes two APIs. Higher lever SendEvent() function that allows you to send one message and NewClient() that can create a new client that is thread-safe.

I could use the auditd client in that one place where the connection can be reused to reduce the overhead where possible and make use of the NewClient() API.

WDYGT?
@Joerger @nklaassen

Looks like the connection is being reused properly now, so looks good to me as is.

I could use the auditd client in that one place where the connection can be reused to reduce the overhead where possible and make use of the NewClient() API.

I think it would make sense to use NewClient in that one place, otherwise we might as well remove the NewClient API and simplify the connection logic for a single use connection.

lib/srv/reexec.go

Joerger · 2022-08-24T18:02:28Z

lib/auditd/auditd_linux.go

+	return status, nil
+}
+
+// SendMsg sends a message. Client will create a new connection if not connected already.


Looks like the connection is being reused properly now, so looks good to me as is.

I could use the auditd client in that one place where the connection can be reused to reduce the overhead where possible and make use of the NewClient() API.

I think it would make sense to use NewClient in that one place, otherwise we might as well remove the NewClient API and simplify the connection logic for a single use connection.

Joerger · 2022-08-24T18:06:21Z

lib/auditd/auditd_linux.go

+	if !hasCapabilities() {
+		// Do nothing when not running as root.
+		return nil
+	}


Should the NewClient API also include this check? Maybe NewClient can return a no-op client / an error, or clt.SendMsg can also do this check and return nil?

r0mant

Code looks ok to me, my main worry is how error scenarios are handled. We've got to make sure we don't block or freeze connections if something's wrong. I would test things like what happens if auditd goes down.

r0mant · 2022-08-26T15:36:34Z

lib/auditd/common.go

+	Close() error
+}
+
+// Message is a


Unfinished comment.

lib/srv/authhandlers.go

r0mant · 2022-08-26T15:38:47Z

lib/srv/reexec.go

+	TerminalName string `json:"terminal_name"`
+
+	ClientAddress string `json:"client_address"`


Nit: Add godocs on these.

lib/auditd/common.go

r0mant · 2022-08-26T15:54:23Z

lib/srv/reexec.go

+		TTYName:      c.TerminalName,
+	}
+
+	if err := auditd.SendEvent(auditd.AuditUserLogin, auditd.Success, auditdMsg); err != nil {


This will emit events for both interactive and non-interactive ("run command") sessions, correct?

Also, does this work with automatic user provisioning? It should, but might be worth double-checking.

Checked, correct events are generated in both cases (run command and user provisioning).

lib/auditd/auditd_linux.go

r0mant · 2022-08-26T16:22:20Z

lib/auditd/auditd_linux.go

+	if err := template.Must(template.New("auditd-message").
+		Parse(msgDataTmpl)).


Can template.Must(template.New("...").Parse(...)) be moved to a global scope variable so this function just executes it?

r0mant · 2022-08-26T16:25:34Z

lib/auditd/auditd_linux.go

+		Data: MsgData,
+	}
+
+	resp, err := c.conn.Execute(msg)


Can this block or something if auditd is down or not responding? Leading to the client being deadlocked due to a mutex.

russjones · 2022-09-01T16:29:05Z

@Joerger Can you re-review so we can get this merged in? We need it for Teleport 10.2.

Fix tty name

…tlink

Added tests

Add client IP and TTY name to auditd events

Fix AuditUserEnd message

Co-authored-by: Roman Tkachenko <roman@goteleport.com>

github-actions · 2022-09-05T18:29:06Z

@jakule See the table below for backport results.

Branch	Result
branch/v10	Failed

Add auditd integration. Co-authored-by: Nic Klaassen <nic@goteleport.com> Co-authored-by: Roman Tkachenko <roman@goteleport.com>

Backport of #14948 Add auditd integration.

Add auditd integration. Co-authored-by: Nic Klaassen <nic@goteleport.com> Co-authored-by: Roman Tkachenko <roman@goteleport.com>

jakule force-pushed the jakule/auditd branch from a46a35d to 59a2faa Compare July 27, 2022 16:47

jakule force-pushed the jakule/auditd branch from b2e4af1 to 352eb7d Compare August 9, 2022 19:17

jakule mentioned this pull request Aug 15, 2022

Add Auditd docs #15530

Merged

jakule marked this pull request as ready for review August 15, 2022 21:40

github-actions bot requested review from Joerger and nklaassen August 15, 2022 21:40

nklaassen reviewed Aug 16, 2022

View reviewed changes

Joerger reviewed Aug 18, 2022

View reviewed changes

jakule force-pushed the jakule/auditd branch from bc5aea6 to 11651a4 Compare August 23, 2022 16:07

jakule requested review from Joerger and nklaassen August 24, 2022 15:46

Joerger reviewed Aug 24, 2022

View reviewed changes

r0mant reviewed Aug 26, 2022

View reviewed changes

r0mant approved these changes Aug 26, 2022

View reviewed changes

Joerger approved these changes Sep 1, 2022

View reviewed changes

jakule added 14 commits September 1, 2022 23:43

WIP Auditd

c23f23c

Add end event

4d032dc

Fix tty name

Migrate from github.com/mozilla/libaudit-go to github.com/mdlayher/ne…

9fe4102

…tlink

WIP

8f1316c

Refactoring

8b3bf26

Added tests

Refactoring

d621031

Fix cross-platform build

04ec50d

Add tests

cecd10d

go mod tidy

8d68575

Minor refactoring

ee7c1bf

go mod tidy

8afe79a

Cosmetic changes

5ee2428

Refactoring

735d210

Add client IP and TTY name to auditd events

Add warning message if login UID is set.

ea160e5

jakule and others added 10 commits September 1, 2022 23:43

Refactor auditd message generation

956db03

Make auditd test less verbose

bab6a5b

Refactor auditd client.

7855b2c

Add more testcases

555b428

Fix AuditUserEnd message

Add package godoc

687100c

Check for nil connection in auditd Close() client.

22903b5

Apply suggestions from code review

9f30b3c

Co-authored-by: Roman Tkachenko <roman@goteleport.com>

Address code review comments.

97526ba

go mod tidy

5d56c4f

Fix linter issue.

21c2d12

jakule force-pushed the jakule/auditd branch from 7c76ada to 21c2d12 Compare September 2, 2022 03:44

jakule added 3 commits September 2, 2022 00:12

Fix test

5d6ea44

Merge branch 'master' into jakule/auditd

7a77d38

Do not return the error to the caller if auditd is disabled

42a124e

jakule added the backport/branch/v10 label Sep 2, 2022

jakule added 7 commits September 2, 2022 14:46

Merge branch 'master' into jakule/auditd

740cf3b

Do not fail SSH connection on auditd error.

c3e53e0

Merge branch 'master' into jakule/auditd

67853ca

Merge branch 'master' into jakule/auditd

0280d7f

Fix nil dereference when term is nil.

fae2204

Fix linter error

4d211c0

Merge branch 'master' into jakule/auditd

9f27eab

jakule enabled auto-merge (squash) September 5, 2022 13:49

Merge branch 'master' into jakule/auditd

758c351

jakule merged commit 7744f72 into master Sep 5, 2022

jakule added a commit that referenced this pull request Sep 6, 2022

Auditd integration (#14948)

c9894c1

Add auditd integration. Co-authored-by: Nic Klaassen <nic@goteleport.com> Co-authored-by: Roman Tkachenko <roman@goteleport.com>

jakule mentioned this pull request Sep 6, 2022

[v10] Auditd integration #16140

Merged

jakule added a commit that referenced this pull request Sep 6, 2022

Auditd integration (#14948) (#16140)

26b8921

Backport of #14948 Add auditd integration.

capnspacehook pushed a commit that referenced this pull request Sep 12, 2022

Auditd integration (#14948)

d6a140c

Add auditd integration. Co-authored-by: Nic Klaassen <nic@goteleport.com> Co-authored-by: Roman Tkachenko <roman@goteleport.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auditd integration #14948

Auditd integration #14948

jakule commented Jul 27, 2022 •

edited

Loading

nklaassen Aug 16, 2022

jakule Aug 23, 2022

nklaassen Aug 16, 2022

jakule Aug 24, 2022

nklaassen Aug 16, 2022

jakule Aug 24, 2022

Joerger Aug 18, 2022

jakule Aug 24, 2022

Joerger Aug 24, 2022

Joerger Aug 24, 2022

Joerger Aug 24, 2022

r0mant left a comment

r0mant Aug 26, 2022

r0mant Aug 26, 2022

r0mant Aug 26, 2022

r0mant Aug 26, 2022

jakule Sep 2, 2022

r0mant Aug 26, 2022

r0mant Aug 26, 2022

russjones commented Sep 1, 2022

github-actions bot commented Sep 5, 2022

		TerminalName string `json:"terminal_name"`

		ClientAddress string `json:"client_address"`

		if err := template.Must(template.New("auditd-message").
		Parse(msgDataTmpl)).

Auditd integration #14948

Auditd integration #14948

Conversation

jakule commented Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r0mant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russjones commented Sep 1, 2022

github-actions bot commented Sep 5, 2022

jakule commented Jul 27, 2022 •

edited

Loading