Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve kube exec Audit Log events #12831

Merged
merged 4 commits into from
May 24, 2022
Merged

Improve kube exec Audit Log events #12831

merged 4 commits into from
May 24, 2022

Conversation

tigrato
Copy link
Contributor

@tigrato tigrato commented May 23, 2022

Improve the kube exec flow in Audit log.

  • Non-interactive shells
    Adds the session.start and session.end events to the non-interactive exec commands.
    Before this commit, the Exec event was not recorded in the Audit Log because it missed the Metadata.Code field and failed the validation.
    This code is only known after the exec is triggered (FAILURE or SUCCESS) and so it requires that Exec event insertion into the Audit Log must be delayed until the execution finishes (i.e. like in SSH).
    Given that, to prevent delayed log entries (execution is blocked for a long time), we require the usage of SessionStart and SessionEnd events even if the session is non-interactive.

  • Interactive shells:
    Add the Exec event to catch the initial command status (i.e. executable not found, forbidden, exit code) and let the session recording record the user actions in the shell.

Fixes #12283
Fixes #12764

lib/kube/proxy/forwarder.go Outdated Show resolved Hide resolved
lib/kube/proxy/forwarder.go Outdated Show resolved Hide resolved
Comment on lines +1081 to +1080
if err := f.cfg.StreamEmitter.EmitAuditEvent(f.ctx, execEvent); err != nil {
f.log.WithError(err).Warn("Failed to emit exec event.")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we sending the ExecEvent at the end of the session rather than at the beginning?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before the Exec event was triggered before executing the code but it was never committed to Audit Log because it failed the validation caused by missing the Metadata.Code. This field can have two values for exec: ExecFailureCode and ExecCode.
In order for us to know if the execution failed we must trigger it and wait for its result.
This is what this MR does, in order to correctly populate the Metadata.Code with the correct value it waits until the execution finishes and commits the Exec event with the correct code, as we already do in SSH connections.

Comment on lines +511 to +514
defer func() {
// catch err by reference so we can access any write into it in the two calls that follow
onFinished(err)
}()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this isn't equivalent to defer onFinished(err)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, when you do the call to onFinished it will contain the current value for err and not catch it by reference as you can see here: https://go.dev/play/p/KapPnQZiZ7B

lib/kube/proxy/sess.go Outdated Show resolved Hide resolved
@tigrato tigrato requested a review from xacrimon May 23, 2022 17:39
tigrato added 4 commits May 24, 2022 10:03
… exec

This commit adds the `session.start` and `session.end` events to the non interactive exec commands.
Before this commit the `Exec` event was not recorded in Audit Log because it missed the `Metadata.Code` field and failed the validation.
This code is only known after the exec is triggered (FAILURE or SUCCESS) and so it requires that `Exec` event insertion into Audit Log  must be delayed until the execution finishes (i.e. like in SSH).
Given that, in order to prevent delayed log entries (execution is blocked for long time) we require the usage of  `SessionStart` and `SessionEnd` events even if the session is non-interactive.

Fixes #12764
…turn code

After `SessionStart` event we must catch if the execution was successfull and log it in Audit Log. The session can be denied for several reasons like missing executable, RBAC restriction to the user/group...

The `Exec` will only catch the initial command status (i.e. executable not found, forbidden, exit code) and the session recording will record the user actions in the shell.

Fixes #12283
@tigrato tigrato force-pushed the tigrato/exec_fix_err_codes branch from 2e738f5 to 547a930 Compare May 24, 2022 09:03
@tigrato tigrato merged commit 1266a5d into master May 24, 2022
@tigrato tigrato deleted the tigrato/exec_fix_err_codes branch May 24, 2022 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants