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

Integrate the eBPF based AppArmor recorder into the API #2296

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
a3f1b00
Integrate the eBPF based apparmor recorder into the API
ccojocar Jun 9, 2024
caa1b8f
Fix some lint warnings
ccojocar Jun 9, 2024
4d060ad
Fix more lint warnings
ccojocar Jun 9, 2024
d9964c8
Fix even more lint warnings
ccojocar Jun 9, 2024
40ba469
Fix implicit memory aliasing warning
ccojocar Jun 9, 2024
1705742
Fix formatting
ccojocar Jun 9, 2024
4603c6b
Remove unecessary blank line
ccojocar Jun 9, 2024
da87658
Fixing the formatting to make linters happy
ccojocar Jun 9, 2024
620c35d
Fix the profile recorder after refactoring to get the existing unit t…
ccojocar Jun 9, 2024
5667551
Update the recording webhook to set the security context also for app…
ccojocar Jun 9, 2024
0bcdf41
Add a unit test to admission webhook recording to check if apparmor e…
ccojocar Jun 9, 2024
567d559
Delete unused function to make linter happy
ccojocar Jun 9, 2024
52dcda2
Add some unit test in bpfrecorder to cover the apparmor recorder
ccojocar Jun 9, 2024
abc111b
Add unit tests in profilerecorder to cover the apparmor eBPF recorder
ccojocar Jun 9, 2024
c569511
Fix lint warnings by allowing duplicated code in tests
ccojocar Jun 9, 2024
6c57243
Fix formatting
ccojocar Jun 9, 2024
af23002
Add some documentation which describes how to record a apparmor profile
ccojocar Jun 9, 2024
9a7272a
Remove not used nonlint directive
ccojocar Jun 9, 2024
09c9438
Fix typos
ccojocar Jun 9, 2024
570bd25
Fix the bpfrecoder unit test
ccojocar Jun 9, 2024
ba4f714
Update comment to be more clear
ccojocar Jun 15, 2024
e0ee07f
Cleanup commented code
ccojocar Jun 15, 2024
3d6631b
Fix some lint warnings
ccojocar Jun 16, 2024
e89ccb7
Fix formatting
ccojocar Jun 16, 2024
3d80c99
Fix unit tests after refactoring
ccojocar Jun 16, 2024
01f8a52
Enable the BPF LSM during unit testing
ccojocar Jun 16, 2024
6a4197c
Make sure the E2E_TEST_BPF_LSM_ENABLED env variable is set for all bp…
ccojocar Jun 17, 2024
8570c78
Set the env variable in each prepare function
ccojocar Jun 17, 2024
c1f319c
Wrap the bpfrecorder unit tests into a Run in order to set the env va…
ccojocar Jun 17, 2024
ea95d47
Fix lint warnings
ccojocar Jun 17, 2024
3da8ba8
Fix more lint warnings
ccojocar Jun 17, 2024
694bc1e
Avoid running the bpfrecorder test in parallel
ccojocar Jun 17, 2024
af72a87
Remove unused nolint directive
ccojocar Jun 17, 2024
a354478
Fix the spoc e2e tests when the proc is started externally
ccojocar Jun 23, 2024
f3e98d4
Fix some lint warnings
ccojocar Jun 23, 2024
6b2958e
Fix even more lint warnings
ccojocar Jun 23, 2024
d022451
Reduce file permissions
ccojocar Jun 23, 2024
d1e3267
Fix compilation error
ccojocar Jun 23, 2024
71aebad
Increase the sleep time of target command in spoc e2e no-proc
ccojocar Jun 23, 2024
32e6bb9
Fix typo
ccojocar Jun 23, 2024
1666e86
Include extra ssh config to vagrant files if present
ccojocar Jun 30, 2024
b68834e
Do not fail the bpfrecorder if the BPF program cannot load
ccojocar Jun 30, 2024
69670fe
Run the bpfrecorder apparmor test in parallel
ccojocar Jun 30, 2024
898ae03
Skip the spoc e2e tests if apparmor or BPF_LSM is disabled
ccojocar Jun 30, 2024
211b175
Run the spoc e2e test in Debian 12 image which supports BPF LSM
ccojocar Jun 30, 2024
704076a
Fix typo
ccojocar Jun 30, 2024
5d76008
Add an option to disable the image build for Debian
ccojocar Jun 30, 2024
9de138c
Use the profilerecording name when disabeling a recorded profile
ccojocar Jun 30, 2024
21946d2
Fix typo in variable name
ccojocar Jun 30, 2024
0f49aa7
Update the instalaltion and usage doc for eBPF base recording
ccojocar Jul 1, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix the spoc e2e tests when the proc is started externally
Change-Id: I1c0502d9a7972868d15b32d06ce318898686a7ab
Signed-off-by: Cosmin Cojocar <[email protected]>
  • Loading branch information
ccojocar committed Jun 29, 2024
commit a3544789104c79a31a161557743d9c8e6f2f79bd
6 changes: 6 additions & 0 deletions internal/pkg/cli/recorder/impl.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (

"sigs.k8s.io/security-profiles-operator/internal/pkg/cli/command"
"sigs.k8s.io/security-profiles-operator/internal/pkg/daemon/bpfrecorder"
"sigs.k8s.io/security-profiles-operator/internal/pkg/util"
)

type defaultImpl struct{}
Expand All @@ -58,6 +59,7 @@ type impl interface {
PrintObj(printers.YAMLPrinter, runtime.Object, io.Writer) error
GoArchToSeccompArch(string) (seccomp.Arch, error)
Notify(chan<- os.Signal, ...os.Signal)
ProcessIDByName(name string) (int, error)
}

func (*defaultImpl) LoadBpfRecorder(b *bpfrecorder.BpfRecorder) error {
Expand Down Expand Up @@ -123,3 +125,7 @@ func (*defaultImpl) GoArchToSeccompArch(arch string) (seccomp.Arch, error) {
func (*defaultImpl) Notify(c chan<- os.Signal, sig ...os.Signal) {
signal.Notify(c, sig...)
}

func (*defaultImpl) ProcessIDByName(name string) (int, error) {
return util.ProcessIDByName(name)
}
18 changes: 17 additions & 1 deletion internal/pkg/cli/recorder/recorder.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,22 @@ func (r *Recorder) Run() error {
ch := make(chan os.Signal, 1)
r.Notify(ch, os.Interrupt)
log.Print(WaitForSigIntMessage)

// searching the mntns of the process started outside of spoc
cmd := r.options.commandOptions.Command()
if err := util.Retry(func() (err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For seccomp, it currently works as follows if --no-proc is passed:

  1. We record syscalls for all mount namespaces by binaries with the target program name.
  2. We merge them below in processSeccomp.

Is there a reason why we don't want to do the same for AppArmor, i.e. merge all recorded profiles?

Copy link
Contributor Author

@ccojocar ccojocar Jul 2, 2024

Choose a reason for hiding this comment

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

My understanding of this flag is that the process is not started by the spoc CLI but externally and spoc will start recording the profile for it as soon as the PID comes live.

I think in the previous implementation, it was recording everything for all process running into the system until it got interrupted.

In this implementation, it will look up first the PID by command name, and then the namespace where that PID is running. This is required to retrieve the recorded information from the maps by namespace.

Now we create an apparmor profile for an entire namespace where the process is running. This is also the case for seccomp. This is not ideal, if other processes run in parallel in that namespace when the CLI is recording. The profiles will contain more stuff than expected. I think for CLI this is acceptable, since people will most likely use this feature to record profiles for a single process running into a wider system, and that process will have its own namesapce unless they will start it in an already existing namesapce.

I would argue that this feature is probably not the best option, if you want to record a profile for an entire container (e.g. form outside). In that case, I would use the in-cluster option.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think in the previous implementation, it was recording everything for all process running into the system until it got interrupted.

Not quite. The ebpf code itself checks the program name and only records events for matching processes. So yes, you would get all events for "curl", no matter from which namespace, but you would not get events from the rest of the system. This is why the seccomp implementation takes all namespaces into account (mntns is 0) with --no-proc. I think we could do the same for AppArmor and avoid the PID race.

pid, err := r.ProcessIDByName(cmd)
if err != nil {
return fmt.Errorf("getting PID by name: %w", err)
}
mntns, err = r.FindProcMountNamespace(r.bpfRecorder, uint32(pid))
if err != nil {
return fmt.Errorf("finding mntns of PID %d: %w", pid, err)
}
return nil
}, func(err error) bool { return true }); err != nil {
return fmt.Errorf("searching the PID of %q command: %w", cmd, err)
}
<-ch
} else {
cmd := command.New(r.options.commandOptions)
Expand All @@ -105,7 +121,7 @@ func (r *Recorder) Run() error {

mntns, err = r.FindProcMountNamespace(r.bpfRecorder, pid)
if err != nil {
return fmt.Errorf("finding mntns for command PID %d: %w", pid, err)
return fmt.Errorf("finding mntns of PID %d: %w", pid, err)
}

if err := r.CommandWait(cmd); err != nil {
Expand Down
79 changes: 79 additions & 0 deletions internal/pkg/cli/recorder/recorderfakes/fake_impl.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

114 changes: 114 additions & 0 deletions internal/pkg/util/process.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
/*
Copyright 2024 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http:https://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package util

import (
"errors"
"fmt"
"os"
"path"
"path/filepath"
"regexp"
"strconv"
"strings"
)

const (
defaultPID = -1
processRoot = "/proc"
)

// ErrEmptyPIDName indicates an erorr for a PID with an empty process name
var ErrEmptyPIDName = errors.New("process Name of given PID is empty")

var procRegexp = regexp.MustCompile(`[/]*proc/[\d]+/cmdline`)

// ProcessIDByName is looking up the PID by process name
func ProcessIDByName(name string) (int, error) {
p := &proc{
name: name,
pid: defaultPID,
}
return p.findPIDByName(processRoot)
}

type proc struct {
name string
pid int
}

func (p *proc) findPIDByName(root string) (int, error) {
err := filepath.Walk(root, p.walkProc)
if err != nil {
return -1, fmt.Errorf("looking for pid of process %q: %w", p.name, err)
}
if p.pid != defaultPID {
return p.pid, nil
}
return -1, fmt.Errorf("could not find a valid pid for process name %q", p.name)
}

func (p *proc) walkProc(path string, info os.FileInfo, err error) error {
// Return already if there is an existing error due to for instance
// insufficient privileges.
if err != nil {
return nil
}

// Skip paths which doesn't look like /proc/<pid>cmdline.
if !procRegexp.Match([]byte(path)) {
return nil
}

pid, err := parsePID(path)
if err != nil {
return fmt.Errorf("parsing PID from path %s: %w", path, err)
}

name, err := parseName(path)
if err != nil {
// skip pids without empty command and keep going
if err == ErrEmptyPIDName {
return nil
}
return fmt.Errorf("parsing process name from path %s: %w", path, err)
}

// Update the PID if we found the process
if strings.HasPrefix(name, p.name) {
p.pid = pid
}

return nil
}

func parsePID(dir string) (int, error) {
pidDir, _ := path.Split(dir)
return strconv.Atoi(path.Base(pidDir))
}

func parseName(path string) (string, error) {
f, err := os.ReadFile(path)
if err != nil {
return "", fmt.Errorf("reading from %s: %w", path, err)
}
name := strings.TrimSpace(string(f))
if len(name) > 0 {
return name, nil
}
return "", ErrEmptyPIDName
}
122 changes: 122 additions & 0 deletions internal/pkg/util/process_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
/*
Copyright 2024 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http:https://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package util

import (
"fmt"
"os"
"path"
"testing"

"github.com/stretchr/testify/require"
)

func TestFindPIDByName(t *testing.T) {
t.Parallel()

cases := []struct {
name string
pid int
cmd string
cmdline string
create bool
skipCmd bool
emptyCmd bool
wantError bool
}{
{
name: "Find PID successfully",
pid: 123,
cmd: "/security-profiles-operator/test/spoc/demobinary",
cmdline: "/security-profiles-operator/test/spoc/demobinary--net-tcp--sleep60",
create: true,
skipCmd: false,
wantError: false,
},
{
name: "No pid available",
create: false,
wantError: true,
},
{
name: "No cmd available",
pid: 123,
cmd: "test-cmd",
create: true,
skipCmd: true,
wantError: true,
},
{
name: "Find PID successfully",
pid: 123,
cmd: "test-cmd",
cmdline: "test-cmd",
create: true,
skipCmd: false,
emptyCmd: true,
wantError: false,
},
}

for _, tc := range cases {
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
tempDir := t.TempDir()
if tc.create {
err := createProcData(tempDir, tc.pid, tc.cmdline, tc.skipCmd, tc.emptyCmd)
require.NoError(t, err)
}
p := &proc{name: tc.cmd, pid: defaultPID}
gotPid, gotErr := p.findPIDByName(tempDir)
if !tc.wantError {
require.NoError(t, gotErr)
require.Equal(t, tc.pid, gotPid, "should find a valid PID by process name")
} else {
require.Error(t, gotErr)
}
})
}
}

func createProcData(root string, pid int, cmd string, skipCmd bool, emptyCmd bool) error {
procPath := path.Join(root, processRoot)
if err := os.Mkdir(procPath, 0700); err != nil {
return fmt.Errorf("creating proc root dir: %w", err)
}
procDir := path.Join(procPath, fmt.Sprintf("%d", pid))
if err := os.Mkdir(procDir, 0700); err != nil {
return fmt.Errorf("creating proc dir: %w", err)
}
if !skipCmd {
cmdFile := path.Join(procDir, "cmdline")
if err := os.WriteFile(cmdFile, []byte(cmd), 0644); err != nil {
return fmt.Errorf("creating cmd file: %w", err)
}
}
if emptyCmd {
procDir := path.Join(procPath, "567")
if err := os.Mkdir(procDir, 0700); err != nil {
return fmt.Errorf("creating proc dir: %w", err)
}
cmdFile := path.Join(procDir, "cmdline")
if err := os.WriteFile(cmdFile, []byte(""), 0644); err != nil {
return fmt.Errorf("creating cmd file: %w", err)
}
}
return nil
}
Loading