Skip to content

Commit 4172b36

Browse files
mxpvmxpv
authored
Merge pull request #218 from mxpv/validation
Enforce ID validation
2 parents 849563d + fd60447 commit 4172b36

File tree

6 files changed

+164
-31
lines changed

6 files changed

+164
-31
lines changed

‎agent/service.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ func (ts *TaskService) Create(requestCtx context.Context, req *taskAPI.CreateTas
156156

157157
err = bundleDir.MountRootfs(drive.Path(), "ext4", nil)
158158
if err != nil {
159-
return nil, errors.Wrap(err, "failed to mount rootfs")
159+
return nil, errors.Wrapf(err, "failed to mount rootfs %q", drive.Path())
160160
}
161161

162162
req.Bundle = bundleDir.RootPath()

‎firecracker-control/local.go

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,14 @@ func (s *local) CreateVM(requestCtx context.Context, req *proto.CreateVMRequest)
116116
// If we're here, there is no pre-existing shim for this VMID, so we spawn a new one
117117
defer shimSocket.Close()
118118

119-
shimDir := vm.ShimDir(ns, id)
120-
err = shimDir.Create()
119+
shimDir, err := vm.ShimDir(ns, id)
120+
if err != nil {
121+
err = errors.Wrapf(err, "failed to build shim path")
122+
s.logger.WithError(err).Error()
123+
return nil, err
124+
}
125+
126+
err = shimDir.Mkdir()
121127
if err != nil {
122128
err = errors.Wrapf(err, "failed to create VM dir %q", shimDir.RootPath())
123129
s.logger.WithError(err).Error()
@@ -266,7 +272,14 @@ func (s *local) newShim(ns, vmID, containerdAddress string, shimSocket *net.Unix
266272

267273
cmd := exec.Command(internal.ShimBinaryName, args...)
268274

269-
cmd.Dir = vm.ShimDir(ns, vmID).RootPath()
275+
shimDir, err := vm.ShimDir(ns, vmID)
276+
if err != nil {
277+
err = errors.Wrap(err, "failed to create shim dir")
278+
logger.WithError(err).Error()
279+
return nil, err
280+
}
281+
282+
cmd.Dir = shimDir.RootPath()
270283

271284
shimSocketFile, err := shimSocket.File()
272285
if err != nil {

‎internal/vm/dir.go

Lines changed: 37 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"os"
2020
"path/filepath"
2121

22+
"github.com/containerd/containerd/identifiers"
2223
"github.com/containerd/containerd/runtime/v2/shim"
2324
"github.com/containerd/fifo"
2425
"github.com/firecracker-microvm/firecracker-containerd/internal"
@@ -33,8 +34,16 @@ const (
3334

3435
// ShimDir holds files, sockets and FIFOs scoped to a single shim managing the
3536
// VM with the given VMID. It is unique per-VM and containerd namespace.
36-
func ShimDir(namespace, vmID string) Dir {
37-
return Dir(filepath.Join(varRunDir, namespace, vmID))
37+
func ShimDir(namespace, vmID string) (Dir, error) {
38+
if err := identifiers.Validate(namespace); err != nil {
39+
return "", errors.Wrap(err, "invalid namespace")
40+
}
41+
42+
if err := identifiers.Validate(vmID); err != nil {
43+
return "", errors.Wrap(err, "invalid vm id")
44+
}
45+
46+
return Dir(filepath.Join(varRunDir, namespace, vmID)), nil
3847
}
3948

4049
// Dir represents the root of a firecracker-containerd VM directory, which
@@ -46,9 +55,9 @@ func (d Dir) RootPath() string {
4655
return string(d)
4756
}
4857

49-
// Create will mkdir the RootPath with correct permissions, or no-op if it
58+
// Mkdir will mkdir the RootPath with correct permissions, or no-op if it
5059
// already exists
51-
func (d Dir) Create() error {
60+
func (d Dir) Mkdir() error {
5261
return os.MkdirAll(d.RootPath(), 0700)
5362
}
5463

@@ -88,26 +97,45 @@ func (d Dir) FirecrackerMetricsFifoPath() string {
8897

8998
// BundleLink returns the path to the symlink to the bundle dir for a given container running
9099
// inside the VM of this vm dir.
91-
func (d Dir) BundleLink(containerID string) bundle.Dir {
92-
return bundle.Dir(filepath.Join(d.RootPath(), containerID))
100+
func (d Dir) BundleLink(containerID string) (bundle.Dir, error) {
101+
if err := identifiers.Validate(containerID); err != nil {
102+
return "", errors.Wrapf(err, "invalid container id %q", containerID)
103+
}
104+
105+
return bundle.Dir(filepath.Join(d.RootPath(), containerID)), nil
93106
}
94107

95108
// CreateBundleLink creates the BundleLink by symlinking to the provided bundle dir
96109
func (d Dir) CreateBundleLink(containerID string, bundleDir bundle.Dir) error {
97-
return createSymlink(bundleDir.RootPath(), d.BundleLink(containerID).RootPath(), "bundle")
110+
path, err := d.BundleLink(containerID)
111+
if err != nil {
112+
return err
113+
}
114+
115+
return createSymlink(bundleDir.RootPath(), path.RootPath(), "bundle")
98116
}
99117

100118
// CreateAddressLink creates a symlink from the VM dir to the bundle dir for the shim address file.
101119
// This symlink is read by containerd.
102120
// CreateAddressLink assumes that CreateBundleLink has been called.
103121
func (d Dir) CreateAddressLink(containerID string) error {
104-
return createSymlink(d.AddrFilePath(), d.BundleLink(containerID).AddrFilePath(), "shim address file")
122+
path, err := d.BundleLink(containerID)
123+
if err != nil {
124+
return err
125+
}
126+
127+
return createSymlink(d.AddrFilePath(), path.AddrFilePath(), "shim address file")
105128
}
106129

107130
// CreateShimLogFifoLink creates a symlink from the bundleDir of the provided container.
108131
// CreateAddressLink assumes that CreateBundleLink has been called.
109132
func (d Dir) CreateShimLogFifoLink(containerID string) error {
110-
return createSymlink(d.BundleLink(containerID).LogFifoPath(), d.LogFifoPath(), "shim log fifo")
133+
path, err := d.BundleLink(containerID)
134+
if err != nil {
135+
return err
136+
}
137+
138+
return createSymlink(path.LogFifoPath(), d.LogFifoPath(), "shim log fifo")
111139
}
112140

113141
// WriteAddress will write the actual address file in the VM dir.

‎internal/vm/dir_test.go

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
// Copyright 2018-2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License"). You may
4+
// not use this file except in compliance with the License. A copy of the
5+
// License is located at
6+
//
7+
// http://aws.amazon.com/apache2.0/
8+
//
9+
// or in the "license" file accompanying this file. This file is distributed
10+
// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
11+
// express or implied. See the License for the specific language governing
12+
// permissions and limitations under the License.
13+
14+
package vm
15+
16+
import (
17+
"strings"
18+
"testing"
19+
20+
"github.com/stretchr/testify/assert"
21+
)
22+
23+
var invalidContainerIDs = []string{"", "id?", "*", "id/1", "id\\"}
24+
25+
func TestShimDir(t *testing.T) {
26+
tests := []struct {
27+
name string
28+
ns string
29+
id string
30+
outDir string
31+
outErr string
32+
}{
33+
{name: "empty ns", outErr: "invalid namespace: identifier must not be empty"},
34+
{name: "ns with /", ns: "/", outErr: `invalid namespace: identifier "/" must match`},
35+
{name: "ns with ?", ns: "?", outErr: `invalid namespace: identifier "?" must match`},
36+
{name: "ns with *", ns: "*", outErr: `invalid namespace: identifier "*" must match`},
37+
{name: "ns with ,", ns: ",", outErr: `invalid namespace: identifier "," must match`},
38+
{name: "empty id", ns: "test", outErr: "invalid vm id: identifier must not be empty"},
39+
{name: "id with /", ns: "test", id: "/", outErr: `invalid vm id: identifier "/" must match`},
40+
{name: "id with ?", ns: "test", id: "?", outErr: `invalid vm id: identifier "?" must match`},
41+
{name: "id with *", ns: "test", id: "*", outErr: `invalid vm id: identifier "*" must match`},
42+
{name: "id with ,", ns: "test", id: ",", outErr: `invalid vm id: identifier "," must match`},
43+
{name: "valid", ns: "ns", id: "1", outDir: "/var/run/firecracker-containerd/ns/1"},
44+
{name: "valid with dashes", ns: "test-123", id: "123-456", outDir: "/var/run/firecracker-containerd/test-123/123-456"},
45+
{name: "valid with dots", ns: "test.123", id: "123.456", outDir: "/var/run/firecracker-containerd/test.123/123.456"},
46+
}
47+
48+
for _, tc := range tests {
49+
test := tc
50+
t.Run(test.name, func(t *testing.T) {
51+
dir, err := ShimDir(test.ns, test.id)
52+
53+
if test.outErr != "" {
54+
assert.Error(t, err)
55+
assert.True(t, strings.Contains(err.Error(), test.outErr), err.Error())
56+
} else {
57+
assert.NoError(t, err)
58+
assert.EqualValues(t, dir, test.outDir)
59+
}
60+
})
61+
}
62+
}
63+
64+
func TestBundleLink(t *testing.T) {
65+
var root Dir = "/root/"
66+
67+
dir, err := root.BundleLink("1")
68+
assert.NoError(t, err)
69+
assert.EqualValues(t, "/root/1", dir)
70+
71+
dir, err = root.BundleLink("test-1")
72+
assert.NoError(t, err)
73+
assert.EqualValues(t, "/root/test-1", dir)
74+
}
75+
76+
func TestBundleLinkInvalidID(t *testing.T) {
77+
var (
78+
root Dir = "/root/"
79+
err error
80+
)
81+
82+
for _, id := range invalidContainerIDs {
83+
_, err = root.BundleLink(id)
84+
assert.Error(t, err)
85+
}
86+
}

‎runtime/service.go

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,9 @@ type service struct {
113113
shimCtx context.Context
114114
shimCancel func()
115115

116-
vmID string
116+
vmID string
117+
shimDir vm.Dir
118+
117119
config *Config
118120

119121
// vmReady is closed once CreateVM has been successfully called
@@ -161,10 +163,16 @@ func NewService(shimCtx context.Context, id string, remotePublisher shim.Publish
161163
namespace = namespaces.Default
162164
}
163165

166+
var shimDir vm.Dir
164167
vmID := os.Getenv(internal.VMIDEnvVarKey)
165168
logger := log.G(shimCtx)
166169
if vmID != "" {
167170
logger = logger.WithField("vmID", vmID)
171+
172+
shimDir, err = vm.ShimDir(namespace, vmID)
173+
if err != nil {
174+
return nil, errors.Wrap(err, "invalid shim directory")
175+
}
168176
}
169177

170178
s := &service{
@@ -176,13 +184,15 @@ func NewService(shimCtx context.Context, id string, remotePublisher shim.Publish
176184
shimCtx: shimCtx,
177185
shimCancel: shimCancel,
178186

179-
vmID: vmID,
187+
vmID: vmID,
188+
shimDir: shimDir,
189+
180190
config: config,
181191

182192
vmReady: make(chan struct{}),
183193
}
184194

185-
s.stubDriveHandler = newStubDriveHandler(s.shimDir().RootPath(), logger)
195+
s.stubDriveHandler = newStubDriveHandler(s.shimDir.RootPath(), logger)
186196
s.startEventForwarders(remotePublisher)
187197

188198
err = s.serveFCControl()
@@ -453,7 +463,7 @@ func (s *service) createVM(requestCtx context.Context, request *proto.CreateVMRe
453463

454464
cmd := firecracker.VMCommandBuilder{}.
455465
WithBin(s.config.FirecrackerBinaryPath).
456-
WithSocketPath(s.shimDir().FirecrackerSockPath()).
466+
WithSocketPath(s.shimDir.FirecrackerSockPath()).
457467
Build(s.shimCtx) // shimCtx so the VM process is only killed when the shim shuts down
458468

459469
// use shimCtx so the VM is killed when the shim shuts down
@@ -563,18 +573,14 @@ func (s *service) SetVMMetadata(requestCtx context.Context, request *proto.SetVM
563573
return &empty.Empty{}, nil
564574
}
565575

566-
func (s *service) shimDir() vm.Dir {
567-
return vm.ShimDir(s.namespace, s.vmID)
568-
}
569-
570576
func (s *service) buildVMConfiguration(req *proto.CreateVMRequest) (*firecracker.Config, error) {
571577
logger := s.logger.WithField("cid", s.machineCID)
572578

573579
cfg := firecracker.Config{
574-
SocketPath: s.shimDir().FirecrackerSockPath(),
580+
SocketPath: s.shimDir.FirecrackerSockPath(),
575581
VsockDevices: []firecracker.VsockDevice{{Path: "root", CID: s.machineCID}},
576-
LogFifo: s.shimDir().FirecrackerLogFifoPath(),
577-
MetricsFifo: s.shimDir().FirecrackerMetricsFifoPath(),
582+
LogFifo: s.shimDir.FirecrackerLogFifoPath(),
583+
MetricsFifo: s.shimDir.FirecrackerMetricsFifoPath(),
578584
MachineCfg: machineConfigurationFromProto(s.config, req.MachineCfg),
579585
}
580586

@@ -662,14 +668,14 @@ func (s *service) Create(requestCtx context.Context, request *taskAPI.CreateTask
662668
}).Debug("creating task")
663669

664670
bundleDir := bundle.Dir(request.Bundle)
665-
err = s.shimDir().CreateBundleLink(request.ID, bundleDir)
671+
err = s.shimDir.CreateBundleLink(request.ID, bundleDir)
666672
if err != nil {
667673
err = errors.Wrap(err, "failed to create VM dir bundle link")
668674
logger.WithError(err).Error()
669675
return nil, err
670676
}
671677

672-
err = s.shimDir().CreateAddressLink(request.ID)
678+
err = s.shimDir.CreateAddressLink(request.ID)
673679
if err != nil {
674680
err = errors.Wrap(err, "failed to create shim address symlink")
675681
logger.WithError(err).Error()
@@ -983,9 +989,9 @@ func (s *service) Shutdown(requestCtx context.Context, req *taskAPI.ShutdownRequ
983989
shutdownErr = multierror.Append(shutdownErr, errors.Wrap(err, "failed to gracefully stop VM"))
984990
}
985991

986-
err = os.RemoveAll(s.shimDir().RootPath())
992+
err = os.RemoveAll(s.shimDir.RootPath())
987993
if err != nil {
988-
shutdownErr = multierror.Append(shutdownErr, errors.Wrapf(err, "failed to remove VM dir %q during shutdown", s.shimDir().RootPath()))
994+
shutdownErr = multierror.Append(shutdownErr, errors.Wrapf(err, "failed to remove VM dir %q during shutdown", s.shimDir.RootPath()))
989995
}
990996

991997
if shutdownErr != nil {

‎runtime/service_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -186,10 +186,10 @@ func TestBuildVMConfiguration(t *testing.T) {
186186

187187
svc.stubDriveHandler = newStubDriveHandler(tempDir, svc.logger)
188188
// For values that remain constant between tests, they are written here
189-
tc.expectedCfg.SocketPath = svc.shimDir().FirecrackerSockPath()
189+
tc.expectedCfg.SocketPath = svc.shimDir.FirecrackerSockPath()
190190
tc.expectedCfg.VsockDevices = []firecracker.VsockDevice{{Path: "root", CID: svc.machineCID}}
191-
tc.expectedCfg.LogFifo = svc.shimDir().FirecrackerLogFifoPath()
192-
tc.expectedCfg.MetricsFifo = svc.shimDir().FirecrackerMetricsFifoPath()
191+
tc.expectedCfg.LogFifo = svc.shimDir.FirecrackerLogFifoPath()
192+
tc.expectedCfg.MetricsFifo = svc.shimDir.FirecrackerMetricsFifoPath()
193193
tc.expectedCfg.Drives[0].PathOnHost = firecracker.String(filepath.Join(tempDir, "stub0"))
194194

195195
actualCfg, err := svc.buildVMConfiguration(tc.request)

0 commit comments

Comments
 (0)