From dfff4dcc6fabdd2fe0ee4619dc3f0f1f05d99628 Mon Sep 17 00:00:00 2001 From: xibz Date: Tue, 21 Jan 2020 22:38:51 +0000 Subject: [PATCH] Remove SeccompLevel from Jailer Firecracker has since removed specifying the seccomp level in the jailer and now is specified in Firecracker instead. This change removes the seccomp level from the jailer and adds it to machine instead. Signed-off-by: xibz --- .buildkite/hooks/pre-exit | 3 + .buildkite/pipeline.yml | 6 +- .gitignore | 2 +- jailer.go | 65 +++----- jailer_test.go | 323 ++++++++++++++++++++++++++------------ machine.go | 28 ++++ machine_test.go | 28 +++- 7 files changed, 307 insertions(+), 148 deletions(-) create mode 100755 .buildkite/hooks/pre-exit diff --git a/.buildkite/hooks/pre-exit b/.buildkite/hooks/pre-exit new file mode 100755 index 00000000..364bad97 --- /dev/null +++ b/.buildkite/hooks/pre-exit @@ -0,0 +1,3 @@ +#!/bin/bash + +sudo rm -rf testdata/logs diff --git a/.buildkite/pipeline.yml b/.buildkite/pipeline.yml index 54d2ef31..51f01059 100644 --- a/.buildkite/pipeline.yml +++ b/.buildkite/pipeline.yml @@ -101,6 +101,9 @@ steps: queue: "${BUILDKITE_AGENT_META_DATA_QUEUE:-default}" distro: "${BUILDKITE_AGENT_META_DATA_DISTRO}" hostname: "${BUILDKITE_AGENT_META_DATA_HOSTNAME}" + # TODO: Remove this once v0.21.0 has been released + soft_fail: # we softfail here since v0.20.0 jailer tests will be broken. + - exit_status: "*" - label: ':hammer: test against firecracker master' env: @@ -120,8 +123,7 @@ steps: queue: "${BUILDKITE_AGENT_META_DATA_QUEUE:-default}" distro: "${BUILDKITE_AGENT_META_DATA_DISTRO}" hostname: "${BUILDKITE_AGENT_META_DATA_HOSTNAME}" - soft_fail: - - exit_status: "*" + # TODO: move soft_fail here once v0.21.0 of firecracker has been released - label: 'go mod tidy' commands: diff --git a/.gitignore b/.gitignore index 617b2ad2..dc4e2706 100644 --- a/.gitignore +++ b/.gitignore @@ -6,4 +6,4 @@ vmlinux root-drive.img TestPID.img build/ - +testdata/logs/ diff --git a/jailer.go b/jailer.go index 3668174a..1ac7757d 100644 --- a/jailer.go +++ b/jailer.go @@ -26,7 +26,7 @@ import ( const ( // defaultJailerPath is the default chroot base directory that the jailer // will use if no other base directory was provided. - defaultJailerPath = "/srv/jailer/firecracker" + defaultJailerPath = "/srv/jailer" defaultJailerBin = "jailer" rootfsFolderName = "root" @@ -38,20 +38,6 @@ var ( ErrMissingJailerConfig = fmt.Errorf("jailer config was not set for use") ) -// SeccompLevelValue represents a secure computing level type. -type SeccompLevelValue int - -// secure computing levels -const ( - // SeccompLevelDisable is the default value. - SeccompLevelDisable SeccompLevelValue = iota - // SeccompLevelBasic prohibits syscalls not whitelisted by Firecracker. - SeccompLevelBasic - // SeccompLevelAdvanced adds further checks on some of the parameters of the - // allowed syscalls. - SeccompLevelAdvanced -) - // JailerConfig is jailer specific configuration needed to execute the jailer. type JailerConfig struct { // GID the jailer switches to as it execs the target binary. @@ -90,15 +76,6 @@ type JailerConfig struct { // STDERR to /dev/null Daemonize bool - // SeccompLevel specifies whether seccomp filters should be installed and how - // restrictive they should be. Possible values are: - // - // 0 : (default): disabled. - // 1 : basic filtering. This prohibits syscalls not whitelisted by Firecracker. - // 2 : advanced filtering. This adds further checks on some of the - // parameters of the allowed syscalls. - SeccompLevel SeccompLevelValue - // ChrootStrategy will dictate how files are transfered to the root drive. ChrootStrategy HandlersAdapter @@ -121,10 +98,10 @@ type JailerCommandBuilder struct { node int // optional params - chrootBaseDir string - netNS string - daemonize bool - seccompLevel SeccompLevelValue + chrootBaseDir string + netNS string + daemonize bool + firecrackerArgs []string stdin io.Reader stdout io.Writer @@ -155,12 +132,15 @@ func (b JailerCommandBuilder) Args() []string { args = append(args, "--netns", b.netNS) } - args = append(args, "--seccomp-level", strconv.Itoa(int(b.seccompLevel))) - if b.daemonize { args = append(args, "--daemonize") } + if len(b.firecrackerArgs) > 0 { + args = append(args, "--") + args = append(args, b.firecrackerArgs...) + } + return args } @@ -229,14 +209,6 @@ func (b JailerCommandBuilder) WithDaemonize(daemonize bool) JailerCommandBuilder return b } -// WithSeccompLevel will set the provided level to the builder. This represents -// the seccomp filters that should be installed and how restrictive they should -// be. -func (b JailerCommandBuilder) WithSeccompLevel(level SeccompLevelValue) JailerCommandBuilder { - b.seccompLevel = level - return b -} - // Stdout will return the stdout that will be used when creating the // firecracker exec.Command func (b JailerCommandBuilder) Stdout() io.Writer { @@ -276,6 +248,13 @@ func (b JailerCommandBuilder) WithStdin(stdin io.Reader) JailerCommandBuilder { return b } +// WithFirecrackerArgs will adds these arguments to the end of the argument +// chain which the jailer will intepret to belonging to Firecracke +func (b JailerCommandBuilder) WithFirecrackerArgs(args ...string) JailerCommandBuilder { + b.firecrackerArgs = args + return b +} + // Build will build a jailer command. func (b JailerCommandBuilder) Build(ctx context.Context) *exec.Cmd { cmd := exec.CommandContext( @@ -304,12 +283,12 @@ func (b JailerCommandBuilder) Build(ctx context.Context) *exec.Cmd { func jail(ctx context.Context, m *Machine, cfg *Config) error { jailerWorkspaceDir := "" if len(cfg.JailerCfg.ChrootBaseDir) > 0 { - jailerWorkspaceDir = filepath.Join(cfg.JailerCfg.ChrootBaseDir, "firecracker", cfg.JailerCfg.ID, rootfsFolderName) + jailerWorkspaceDir = filepath.Join(cfg.JailerCfg.ChrootBaseDir, filepath.Base(cfg.JailerCfg.ExecFile), cfg.JailerCfg.ID, rootfsFolderName) } else { - jailerWorkspaceDir = filepath.Join(defaultJailerPath, cfg.JailerCfg.ID, rootfsFolderName) + jailerWorkspaceDir = filepath.Join(defaultJailerPath, filepath.Base(cfg.JailerCfg.ExecFile), cfg.JailerCfg.ID, rootfsFolderName) } - cfg.SocketPath = filepath.Join(jailerWorkspaceDir, "api.socket") + cfg.SocketPath = filepath.Join(jailerWorkspaceDir, "run", "firecracker.socket") stdout := cfg.JailerCfg.Stdout if stdout == nil { @@ -329,7 +308,9 @@ func jail(ctx context.Context, m *Machine, cfg *Config) error { WithExecFile(cfg.JailerCfg.ExecFile). WithChrootBaseDir(cfg.JailerCfg.ChrootBaseDir). WithDaemonize(cfg.JailerCfg.Daemonize). - WithSeccompLevel(cfg.JailerCfg.SeccompLevel). + WithFirecrackerArgs( + "--seccomp-level", cfg.SeccompLevel.String(), + ). WithStdout(stdout). WithStderr(stderr) diff --git a/jailer_test.go b/jailer_test.go index cb4214c4..c4ff6234 100644 --- a/jailer_test.go +++ b/jailer_test.go @@ -7,106 +7,116 @@ import ( "testing" ) -var testCases = []struct { - name string - jailerCfg JailerConfig - expectedArgs []string - netns string - expectedSockPath string -}{ - { - name: "required fields", - jailerCfg: JailerConfig{ - ID: "my-test-id", - UID: Int(123), - GID: Int(100), - NumaNode: Int(0), - ChrootStrategy: NewNaiveChrootStrategy("path", "kernel-image-path"), - ExecFile: "/path/to/firecracker", - }, - expectedArgs: []string{ - defaultJailerBin, - "--id", - "my-test-id", - "--uid", - "123", - "--gid", - "100", - "--exec-file", - "/path/to/firecracker", - "--node", - "0", - "--seccomp-level", - "0", - }, - expectedSockPath: filepath.Join(defaultJailerPath, "my-test-id", rootfsFolderName, "api.socket"), - }, - { - name: "other jailer binary name", - jailerCfg: JailerConfig{ - ID: "my-test-id", - UID: Int(123), - GID: Int(100), - NumaNode: Int(0), - ChrootStrategy: NewNaiveChrootStrategy("path", "kernel-image-path"), - ExecFile: "/path/to/firecracker", - JailerBinary: "imprisoner", - }, - expectedArgs: []string{ - "imprisoner", - "--id", - "my-test-id", - "--uid", - "123", - "--gid", - "100", - "--exec-file", - "/path/to/firecracker", - "--node", - "0", - "--seccomp-level", - "0", +func TestJailerBuilder(t *testing.T) { + var testCases = []struct { + name string + jailerCfg JailerConfig + expectedArgs []string + netns string + expectedSockPath string + }{ + { + name: "required fields", + jailerCfg: JailerConfig{ + ID: "my-test-id", + UID: Int(123), + GID: Int(100), + NumaNode: Int(0), + ChrootStrategy: NewNaiveChrootStrategy("path", "kernel-image-path"), + ExecFile: "/path/to/firecracker", + }, + expectedArgs: []string{ + defaultJailerBin, + "--id", + "my-test-id", + "--uid", + "123", + "--gid", + "100", + "--exec-file", + "/path/to/firecracker", + "--node", + "0", + }, + expectedSockPath: filepath.Join( + defaultJailerPath, + "firecracker", + "my-test-id", + rootfsFolderName, + "run", + "firecracker.socket"), }, - expectedSockPath: filepath.Join(defaultJailerPath, "my-test-id", rootfsFolderName, "api.socket"), - }, - { - name: "optional fields", - netns: "/path/to/netns", - jailerCfg: JailerConfig{ - ID: "my-test-id", - UID: Int(123), - GID: Int(100), - NumaNode: Int(1), - ChrootStrategy: NewNaiveChrootStrategy("path", "kernel-image-path"), - ExecFile: "/path/to/firecracker", - ChrootBaseDir: "/tmp", - SeccompLevel: SeccompLevelAdvanced, - JailerBinary: "/path/to/the/jailer", + { + name: "other jailer binary name", + jailerCfg: JailerConfig{ + ID: "my-test-id", + UID: Int(123), + GID: Int(100), + NumaNode: Int(0), + ChrootStrategy: NewNaiveChrootStrategy("path", "kernel-image-path"), + ExecFile: "/path/to/firecracker", + JailerBinary: "imprisoner", + }, + expectedArgs: []string{ + "imprisoner", + "--id", + "my-test-id", + "--uid", + "123", + "--gid", + "100", + "--exec-file", + "/path/to/firecracker", + "--node", + "0", + }, + expectedSockPath: filepath.Join( + defaultJailerPath, + "firecracker", + "my-test-id", + rootfsFolderName, + "run", + "firecracker.socket"), }, - expectedArgs: []string{ - "/path/to/the/jailer", - "--id", - "my-test-id", - "--uid", - "123", - "--gid", - "100", - "--exec-file", - "/path/to/firecracker", - "--node", - "1", - "--chroot-base-dir", - "/tmp", - "--netns", - "/path/to/netns", - "--seccomp-level", - "2", + { + name: "optional fields", + netns: "/path/to/netns", + jailerCfg: JailerConfig{ + ID: "my-test-id", + UID: Int(123), + GID: Int(100), + NumaNode: Int(1), + ChrootStrategy: NewNaiveChrootStrategy("path", "kernel-image-path"), + ExecFile: "/path/to/firecracker", + ChrootBaseDir: "/tmp", + JailerBinary: "/path/to/the/jailer", + }, + expectedArgs: []string{ + "/path/to/the/jailer", + "--id", + "my-test-id", + "--uid", + "123", + "--gid", + "100", + "--exec-file", + "/path/to/firecracker", + "--node", + "1", + "--chroot-base-dir", + "/tmp", + "--netns", + "/path/to/netns", + }, + expectedSockPath: filepath.Join( + "/tmp", + "firecracker", + "my-test-id", + rootfsFolderName, + "run", + "firecracker.socket"), }, - expectedSockPath: filepath.Join("/tmp", "firecracker", "my-test-id", rootfsFolderName, "api.socket"), - }, -} - -func TestJailerBuilder(t *testing.T) { + } for _, c := range testCases { t.Run(c.name, func(t *testing.T) { b := NewJailerCommandBuilder(). @@ -114,7 +124,6 @@ func TestJailerBuilder(t *testing.T) { WithUID(IntValue(c.jailerCfg.UID)). WithGID(IntValue(c.jailerCfg.GID)). WithNumaNode(IntValue(c.jailerCfg.NumaNode)). - WithSeccompLevel(c.jailerCfg.SeccompLevel). WithExecFile(c.jailerCfg.ExecFile) if len(c.jailerCfg.JailerBinary) > 0 { @@ -142,6 +151,124 @@ func TestJailerBuilder(t *testing.T) { } func TestJail(t *testing.T) { + var testCases = []struct { + name string + jailerCfg JailerConfig + expectedArgs []string + netns string + expectedSockPath string + }{ + { + name: "required fields", + jailerCfg: JailerConfig{ + ID: "my-test-id", + UID: Int(123), + GID: Int(100), + NumaNode: Int(0), + ChrootStrategy: NewNaiveChrootStrategy("path", "kernel-image-path"), + ExecFile: "/path/to/firecracker", + }, + expectedArgs: []string{ + defaultJailerBin, + "--id", + "my-test-id", + "--uid", + "123", + "--gid", + "100", + "--exec-file", + "/path/to/firecracker", + "--node", + "0", + "--", + "--seccomp-level", + "0", + }, + expectedSockPath: filepath.Join( + defaultJailerPath, + "firecracker", + "my-test-id", + rootfsFolderName, + "run", + "firecracker.socket"), + }, + { + name: "other jailer binary name", + jailerCfg: JailerConfig{ + ID: "my-test-id", + UID: Int(123), + GID: Int(100), + NumaNode: Int(0), + ChrootStrategy: NewNaiveChrootStrategy("path", "kernel-image-path"), + ExecFile: "/path/to/firecracker", + JailerBinary: "imprisoner", + }, + expectedArgs: []string{ + "imprisoner", + "--id", + "my-test-id", + "--uid", + "123", + "--gid", + "100", + "--exec-file", + "/path/to/firecracker", + "--node", + "0", + "--", + "--seccomp-level", + "0", + }, + expectedSockPath: filepath.Join( + defaultJailerPath, + "firecracker", + "my-test-id", + rootfsFolderName, + "run", + "firecracker.socket"), + }, + { + name: "optional fields", + netns: "/path/to/netns", + jailerCfg: JailerConfig{ + ID: "my-test-id", + UID: Int(123), + GID: Int(100), + NumaNode: Int(1), + ChrootStrategy: NewNaiveChrootStrategy("path", "kernel-image-path"), + ExecFile: "/path/to/firecracker", + ChrootBaseDir: "/tmp", + JailerBinary: "/path/to/the/jailer", + }, + expectedArgs: []string{ + "/path/to/the/jailer", + "--id", + "my-test-id", + "--uid", + "123", + "--gid", + "100", + "--exec-file", + "/path/to/firecracker", + "--node", + "1", + "--chroot-base-dir", + "/tmp", + "--netns", + "/path/to/netns", + "--", + "--seccomp-level", + "0", + }, + expectedSockPath: filepath.Join( + "/tmp", + "firecracker", + "my-test-id", + rootfsFolderName, + "run", + "firecracker.socket"), + }, + } for _, c := range testCases { t.Run(c.name, func(t *testing.T) { m := &Machine{ diff --git a/machine.go b/machine.go index a0eb3128..a3b21a47 100644 --- a/machine.go +++ b/machine.go @@ -49,6 +49,24 @@ const ( defaultFirecrackerInitTimeoutSeconds = 3 ) +// SeccompLevelValue represents a secure computing level type. +type SeccompLevelValue int + +// secure computing levels +const ( + // SeccompLevelDisable is the default value. + SeccompLevelDisable SeccompLevelValue = iota + // SeccompLevelBasic prohibits syscalls not whitelisted by Firecracker. + SeccompLevelBasic + // SeccompLevelAdvanced adds further checks on some of the parameters of the + // allowed syscalls. + SeccompLevelAdvanced +) + +func (level SeccompLevelValue) String() string { + return strconv.Itoa(int(level)) +} + // ErrAlreadyStarted signifies that the Machine has already started and cannot // be started again. var ErrAlreadyStarted = errors.New("firecracker: machine already started") @@ -126,6 +144,15 @@ type Config struct { // ForwardSignals is an optional list of signals to catch and forward to // firecracker. If not provided, the default signals will be used. ForwardSignals []os.Signal + + // SeccompLevel specifies whether seccomp filters should be installed and how + // restrictive they should be. Possible values are: + // + // 0 : (default): disabled. + // 1 : basic filtering. This prohibits syscalls not whitelisted by Firecracker. + // 2 : advanced filtering. This adds further checks on some of the + // parameters of the allowed syscalls. + SeccompLevel SeccompLevelValue } // Validate will ensure that the required fields are set and that @@ -289,6 +316,7 @@ func NewMachine(ctx context.Context, cfg Config, opts ...Opt) (*Machine, error) m.Handlers.Validation = m.Handlers.Validation.Append(ConfigValidationHandler) m.cmd = defaultFirecrackerVMMCommandBuilder. WithSocketPath(cfg.SocketPath). + AddArgs("--seccomp-level", cfg.SeccompLevel.String()). Build(ctx) } diff --git a/machine_test.go b/machine_test.go index 2ee7945d..244d3956 100644 --- a/machine_test.go +++ b/machine_test.go @@ -59,9 +59,10 @@ const ( ) var ( - skipTuntap bool - testDataPath = "./testdata" - testDataBin = filepath.Join(testDataPath, "bin") + skipTuntap bool + testDataPath = "./testdata" + testDataLogPath = filepath.Join(testDataPath, "logs") + testDataBin = filepath.Join(testDataPath, "bin") testRootfs = filepath.Join(testDataPath, "root-drive.img") ) @@ -72,6 +73,10 @@ func init() { if val := os.Getenv(testDataPathEnv); val != "" { testDataPath = val } + + if err := os.MkdirAll(testDataLogPath, 0777); err != nil { + panic(err) + } } // Ensure that we can create a new machine @@ -106,9 +111,12 @@ func TestJailerMicroVMExecution(t *testing.T) { } fctesting.RequiresRoot(t) + logPath := filepath.Join(testDataLogPath, "TestJailerMicroVMExecution") + err := os.MkdirAll(logPath, 0777) + require.NoError(t, err, "unable to create %s path", logPath) + jailerUID := 123 jailerGID := 100 - var err error if v := os.Getenv(sudoUID); v != "" { if jailerUID, err = strconv.Atoi(v); err != nil { t.Fatalf("Failed to parse %q", sudoUID) @@ -145,7 +153,7 @@ func TestJailerMicroVMExecution(t *testing.T) { // short names and directory to prevent SUN_LEN error id := "b" jailerTestPath := tmpDir - jailerFullRootPath := filepath.Join(jailerTestPath, "firecracker", id) + jailerFullRootPath := filepath.Join(jailerTestPath, filepath.Base(getFirecrackerBinaryPath()), id) os.MkdirAll(jailerTestPath, 0777) socketPath := filepath.Join(jailerTestPath, "firecracker", "TestJailerMicroVMExecution.socket") @@ -156,6 +164,7 @@ func TestJailerMicroVMExecution(t *testing.T) { require.NoError(t, err, "failed to open fifo writer file") defer func() { fw.Close() + exec.Command("cp", capturedLog, logPath).Run() os.Remove(capturedLog) os.Remove(socketPath) os.Remove(logFifo) @@ -163,6 +172,13 @@ func TestJailerMicroVMExecution(t *testing.T) { os.RemoveAll(tmpDir) }() + logFd, err := os.OpenFile( + filepath.Join(logPath, "TestJailerMicroVMExecution.log"), + os.O_CREATE|os.O_RDWR, + 0666) + require.NoError(t, err, "failed to create log file") + defer logFd.Close() + cfg := Config{ Debug: true, SocketPath: socketPath, @@ -193,6 +209,8 @@ func TestJailerMicroVMExecution(t *testing.T) { ChrootBaseDir: jailerTestPath, ExecFile: getFirecrackerBinaryPath(), ChrootStrategy: NewNaiveChrootStrategy(jailerFullRootPath, vmlinuxPath), + Stdout: logFd, + Stderr: logFd, }, FifoLogWriter: fw, }