Skip to content

Commit 0fb3fa2

Browse files
authored
chore: code review cleanup (security regex, log levels, casing, blob IDs, tests) (#45)
* fix(types): reject single/double quotes and control chars in password The shellUnsafe regex missed ', " and control bytes (\x00-\x1f, \x7f). The cloud-init runcmd template at metadata/metadata.go:42 embeds the password inside a single-quoted YAML scalar and shells out via 'echo '...' | chpasswd' — a single-quote in the password breaks out of the YAML scalar and runs arbitrary commands as root. Newlines also terminate the YAML line and bypass the chpasswd pipeline. Adds tests for each rejected class. * fix(log): upgrade integrity-relevant failures from Warnf to Errorf Three persist/unlock failure paths were logged as warnings: - storage/json: unlock failure on the index file is an integrity signal, not graceful degradation - hypervisor.MarkError: failure to flip a VM to error state leaves callers blind to the stuck record - hypervisor.RollbackCreate: failure to roll back a placeholder record leaks a phantom VM entry Use the structured Errorf form per cocoon log convention. * fix(hypervisor): lowercase sentinel error messages Match the cocoon convention (network/network.go, images/images.go) of lowercase, action-oriented error text. All callers already check via errors.Is so no behavior change. Fixes ErrNotFound, ErrNotRunning, ErrAmbiguous. * fix(cmd): lowercase NIC and SNAPSHOT error messages Cocoon error convention is lowercase, action-oriented. "NIC count" and "SNAPSHOT is required" both shouted at the user; bring them in line with the rest of the codebase. * refactor(network/cni): rename create.go to lifecycle.go The file holds Prepare/Add/Remove/cniDel/tapNameForVM/ensureNetns — the full CNI lifecycle, not just creation. Same for the platform splits (create_linux.go, create_darwin.go). Pure rename, no logic change; imports unaffected since the package name is unchanged. * docs(snapshot): document SnapshotMeta CPU/Memory backend semantics Only Firecracker populates SnapshotMeta.CPU/Memory; CH leaves both zero and reads from rec.Config instead (CH's native config.json already round-trips CPU/Memory). Add a one-line godoc so the next reader doesn't have to grep the backend implementations. * docs(cmd,ch): clarify CommandContext fallback and qemu-img subprocess - CommandContext: explain that the Background fallback only exists for unit-test callers; the cobra root always installs a signal ctx. - cloudhypervisor.ExtendQcow2: point the qemu-img resize subprocess at the package-level rationale in utils/qemuimg.go so it's discoverable from the call site. * test: cover DevPath, decompressKernel, parseTAPName, loadConfLists Adds table-driven tests for four pure helpers that previously had no unit coverage: - firecracker.DevPath: vda..vdz / vdaa..vdaz boundaries - firecracker.decompressKernel: gzip + zstd, header-prefix variants - bridge.parseTAPName: valid and rejected names (Linux-only) - cni.loadConfLists: empty dir, single + multi conflists, parse error No production code changes. * chore: trim multi-line godocs introduced by this branch to 1 line
1 parent d03dec4 commit 0fb3fa2

16 files changed

Lines changed: 317 additions & 15 deletions

File tree

cmd/core/helpers.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ func (h BaseHandler) Conf() (*config.Config, error) {
7171
return conf, nil
7272
}
7373

74-
// CommandContext returns ctx from cmd or Background as fallback.
74+
// CommandContext returns cmd.Context() or Background (test-only fallback).
7575
func CommandContext(cmd *cobra.Command) context.Context {
7676
if cmd != nil && cmd.Context() != nil {
7777
return cmd.Context()
@@ -411,7 +411,7 @@ func CloneVMConfigFromFlags(cmd *cobra.Command, snapCfg types.SnapshotConfig) (*
411411
// snapshot, Name/Network from the VM (CNI namespace survives restore).
412412
func RestoreVMConfigFromFlags(cmd *cobra.Command, vm *types.VM, snapCfg types.SnapshotConfig) (*types.VMConfig, error) {
413413
if snapCfg.NICs != len(vm.NetworkConfigs) {
414-
return nil, fmt.Errorf("NIC count mismatch: vm has %d, snapshot has %d",
414+
return nil, fmt.Errorf("nic count mismatch: vm has %d, snapshot has %d",
415415
len(vm.NetworkConfigs), snapCfg.NICs)
416416
}
417417
cfg := snapCfg.Config

cmd/vm/run.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -323,7 +323,7 @@ func snapshotSource(cmd *cobra.Command, args []string, baseArgs int) (string, st
323323
return fromDir, "", nil
324324
}
325325
if len(args) <= baseArgs {
326-
return "", "", fmt.Errorf("SNAPSHOT is required (or use --from-dir)")
326+
return "", "", fmt.Errorf("snapshot is required (or use --from-dir)")
327327
}
328328
return "", args[baseArgs], nil
329329
}

hypervisor/cloudhypervisor/utils.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ func qemuExpandImage(ctx context.Context, path string, targetSize int64, directB
4747
if targetSize <= virtualSize {
4848
return nil
4949
}
50+
// shell out: qemu-img is the authoritative qcow2 tool (see utils/qemuimg.go).
5051
if err := utils.RunQemuImg(ctx, "resize", path, strconv.FormatInt(targetSize, 10)); err != nil {
5152
return fmt.Errorf("resize %s: %w", path, err)
5253
}

hypervisor/create.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ func (b *Backend) RollbackCreate(ctx context.Context, id, name string) {
4444
}
4545
return nil
4646
}); err != nil {
47-
log.WithFunc(b.Typ+".RollbackCreate").Warnf(ctx, "rollback VM %s (name=%s): %v", id, name, err)
47+
log.WithFunc(b.Typ+".RollbackCreate").Errorf(ctx, err, "rollback VM %s (name=%s)", id, name)
4848
}
4949
}
5050

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
package firecracker
2+
3+
import (
4+
"bytes"
5+
"compress/gzip"
6+
"testing"
7+
8+
"github.com/klauspost/compress/zstd"
9+
)
10+
11+
func TestDevPath(t *testing.T) {
12+
tests := []struct {
13+
idx int
14+
want string
15+
}{
16+
{0, "/dev/vda"},
17+
{1, "/dev/vdb"},
18+
{25, "/dev/vdz"},
19+
{26, "/dev/vdaa"},
20+
{27, "/dev/vdab"},
21+
{51, "/dev/vdaz"},
22+
{52, "/dev/vdba"},
23+
{77, "/dev/vdbz"},
24+
{78, "/dev/vdca"},
25+
}
26+
for _, tt := range tests {
27+
t.Run(tt.want, func(t *testing.T) {
28+
got := DevPath(tt.idx)
29+
if got != tt.want {
30+
t.Errorf("DevPath(%d) = %q, want %q", tt.idx, got, tt.want)
31+
}
32+
})
33+
}
34+
}
35+
36+
func fakeELF() []byte {
37+
out := []byte{0x7f, 'E', 'L', 'F'}
38+
out = append(out, bytes.Repeat([]byte{0x00}, 60)...)
39+
return out
40+
}
41+
42+
func gzipCompress(t *testing.T, data []byte) []byte {
43+
t.Helper()
44+
var buf bytes.Buffer
45+
gw := gzip.NewWriter(&buf)
46+
if _, err := gw.Write(data); err != nil {
47+
t.Fatalf("gzip write: %v", err)
48+
}
49+
if err := gw.Close(); err != nil {
50+
t.Fatalf("gzip close: %v", err)
51+
}
52+
return buf.Bytes()
53+
}
54+
55+
func zstdCompress(t *testing.T, data []byte) []byte {
56+
t.Helper()
57+
enc, err := zstd.NewWriter(nil)
58+
if err != nil {
59+
t.Fatalf("zstd writer: %v", err)
60+
}
61+
out := enc.EncodeAll(data, nil)
62+
if err := enc.Close(); err != nil {
63+
t.Fatalf("zstd close: %v", err)
64+
}
65+
return out
66+
}
67+
68+
func TestDecompressKernel(t *testing.T) {
69+
elf := fakeELF()
70+
71+
tests := []struct {
72+
name string
73+
data []byte
74+
wantErr bool
75+
}{
76+
{
77+
name: "gzip",
78+
data: gzipCompress(t, elf),
79+
},
80+
{
81+
name: "zstd",
82+
data: zstdCompress(t, elf),
83+
},
84+
{
85+
name: "gzip with header prefix (bzImage-style)",
86+
data: append(bytes.Repeat([]byte{0xff}, 64), gzipCompress(t, elf)...),
87+
},
88+
{
89+
name: "zstd with header prefix (bzImage-style)",
90+
data: append(bytes.Repeat([]byte{0xff}, 64), zstdCompress(t, elf)...),
91+
},
92+
{
93+
name: "no recognized magic",
94+
data: bytes.Repeat([]byte{0xff}, 128),
95+
wantErr: true,
96+
},
97+
{
98+
name: "gzip header but corrupt payload",
99+
data: append([]byte{0x1f, 0x8b, 0x08}, bytes.Repeat([]byte{0xff}, 32)...),
100+
wantErr: true,
101+
},
102+
}
103+
104+
for _, tt := range tests {
105+
t.Run(tt.name, func(t *testing.T) {
106+
got, err := decompressKernel(tt.data)
107+
if tt.wantErr {
108+
if err == nil {
109+
t.Fatalf("want error, got nil")
110+
}
111+
return
112+
}
113+
if err != nil {
114+
t.Fatalf("unexpected err: %v", err)
115+
}
116+
if !bytes.HasPrefix(got, []byte{0x7f, 'E', 'L', 'F'}) {
117+
t.Errorf("output does not start with ELF magic: %x", got[:min(16, len(got))])
118+
}
119+
})
120+
}
121+
}

hypervisor/hypervisor.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,9 @@ import (
1010
)
1111

1212
var (
13-
ErrNotFound = errors.New("VM not found")
14-
ErrNotRunning = errors.New("VM not running")
15-
ErrAmbiguous = errors.New("VM ref resolves to multiple backends")
13+
ErrNotFound = errors.New("vm not found")
14+
ErrNotRunning = errors.New("vm not running")
15+
ErrAmbiguous = errors.New("vm ref resolves to multiple backends")
1616
)
1717

1818
// Hypervisor manages VM lifecycle. Implemented by each backend.

hypervisor/snapshot.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,9 @@ const SnapshotMetaFile = "cocoon.json"
2020
type SnapshotMeta struct {
2121
StorageConfigs []*types.StorageConfig `json:"storage_configs"`
2222
BootConfig *types.BootConfig `json:"boot_config,omitempty"`
23-
CPU int `json:"cpu,omitempty"`
24-
Memory int64 `json:"memory,omitempty"`
23+
// CPU/Memory populated by FC only; CH reads them from config.json on restore.
24+
CPU int `json:"cpu,omitempty"`
25+
Memory int64 `json:"memory,omitempty"`
2526
}
2627

2728
func SaveSnapshotMeta(dir string, meta *SnapshotMeta) error {

hypervisor/state.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ func (b *Backend) UpdateStates(ctx context.Context, ids []string, state types.VM
8686
// MarkError flips a single VM's state to VMStateError, logging on persist failure.
8787
func (b *Backend) MarkError(ctx context.Context, id string) {
8888
if err := b.UpdateStates(ctx, []string{id}, types.VMStateError); err != nil {
89-
log.WithFunc(b.Typ+".MarkError").Warnf(ctx, "mark VM %s error: %v", id, err)
89+
log.WithFunc(b.Typ+".MarkError").Errorf(ctx, err, "mark VM %s error", id)
9090
}
9191
}
9292

network/bridge/gc_linux_test.go

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
//go:build linux
2+
3+
package bridge
4+
5+
import "testing"
6+
7+
func TestParseTAPName(t *testing.T) {
8+
tests := []struct {
9+
name string
10+
wantPrefix string
11+
wantOK bool
12+
}{
13+
{name: "bt12345678-0", wantPrefix: "12345678", wantOK: true},
14+
{name: "bt12345678-1", wantPrefix: "12345678", wantOK: true},
15+
{name: "btabc-3", wantPrefix: "abc", wantOK: true},
16+
{name: "btabc-def-5", wantPrefix: "abc-def", wantOK: true},
17+
18+
// negative
19+
{name: "wrong-prefix-0"},
20+
{name: "bt"},
21+
{name: "bt-0"}, // empty prefix
22+
{name: "bt12345678"},
23+
{name: ""},
24+
}
25+
for _, tt := range tests {
26+
label := tt.name
27+
if label == "" {
28+
label = "<empty>"
29+
}
30+
t.Run(label, func(t *testing.T) {
31+
gotPrefix, gotOK := parseTAPName(tt.name)
32+
if gotOK != tt.wantOK {
33+
t.Errorf("parseTAPName(%q) ok = %v, want %v", tt.name, gotOK, tt.wantOK)
34+
}
35+
if gotPrefix != tt.wantPrefix {
36+
t.Errorf("parseTAPName(%q) prefix = %q, want %q", tt.name, gotPrefix, tt.wantPrefix)
37+
}
38+
})
39+
}
40+
}

network/cni/cni_test.go

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
package cni
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"strings"
7+
"testing"
8+
)
9+
10+
const (
11+
bridgeConflist = `{
12+
"cniVersion": "1.0.0",
13+
"name": "cni-bridge",
14+
"plugins": [
15+
{"type": "bridge", "bridge": "br0"}
16+
]
17+
}`
18+
macvlanConflist = `{
19+
"cniVersion": "1.0.0",
20+
"name": "cni-macvlan",
21+
"plugins": [
22+
{"type": "macvlan", "master": "eth0"}
23+
]
24+
}`
25+
hostNetConflist = `{
26+
"cniVersion": "1.0.0",
27+
"name": "cni-host",
28+
"plugins": [
29+
{"type": "host-local"}
30+
]
31+
}`
32+
)
33+
34+
func writeFile(t *testing.T, path, content string) {
35+
t.Helper()
36+
if err := os.WriteFile(path, []byte(content), 0o600); err != nil {
37+
t.Fatalf("write %s: %v", path, err)
38+
}
39+
}
40+
41+
func TestLoadConfLists(t *testing.T) {
42+
t.Run("empty dir errors", func(t *testing.T) {
43+
dir := t.TempDir()
44+
_, _, err := loadConfLists(dir)
45+
if err == nil {
46+
t.Fatalf("expected error on empty dir, got nil")
47+
}
48+
if !strings.Contains(err.Error(), "no .conflist files") {
49+
t.Errorf("unexpected error: %v", err)
50+
}
51+
})
52+
53+
t.Run("non-conflist files ignored", func(t *testing.T) {
54+
dir := t.TempDir()
55+
writeFile(t, filepath.Join(dir, "10-something.conf"), bridgeConflist)
56+
writeFile(t, filepath.Join(dir, "20-readme.txt"), "ignored")
57+
_, _, err := loadConfLists(dir)
58+
if err == nil {
59+
t.Fatalf("expected error when only non-.conflist files present")
60+
}
61+
})
62+
63+
t.Run("single conflist becomes default", func(t *testing.T) {
64+
dir := t.TempDir()
65+
writeFile(t, filepath.Join(dir, "10-bridge.conflist"), bridgeConflist)
66+
lists, def, err := loadConfLists(dir)
67+
if err != nil {
68+
t.Fatalf("unexpected err: %v", err)
69+
}
70+
if def != "cni-bridge" {
71+
t.Errorf("default = %q, want cni-bridge", def)
72+
}
73+
if _, ok := lists["cni-bridge"]; !ok {
74+
t.Errorf("cni-bridge missing from %v", lists)
75+
}
76+
})
77+
78+
t.Run("default is alphabetically first by filename", func(t *testing.T) {
79+
dir := t.TempDir()
80+
// libcni.ConfFiles sorts by filename; lex order is 10-, 20-, 30-.
81+
writeFile(t, filepath.Join(dir, "30-host.conflist"), hostNetConflist)
82+
writeFile(t, filepath.Join(dir, "10-bridge.conflist"), bridgeConflist)
83+
writeFile(t, filepath.Join(dir, "20-macvlan.conflist"), macvlanConflist)
84+
85+
lists, def, err := loadConfLists(dir)
86+
if err != nil {
87+
t.Fatalf("unexpected err: %v", err)
88+
}
89+
if def != "cni-bridge" {
90+
t.Errorf("default = %q, want cni-bridge (10- prefix wins)", def)
91+
}
92+
for _, want := range []string{"cni-bridge", "cni-macvlan", "cni-host"} {
93+
if _, ok := lists[want]; !ok {
94+
t.Errorf("missing conflist %q in %v", want, lists)
95+
}
96+
}
97+
if len(lists) != 3 {
98+
t.Errorf("got %d conflists, want 3", len(lists))
99+
}
100+
})
101+
102+
t.Run("bad conflist surfaces parse error", func(t *testing.T) {
103+
dir := t.TempDir()
104+
writeFile(t, filepath.Join(dir, "bad.conflist"), "{not json")
105+
_, _, err := loadConfLists(dir)
106+
if err == nil {
107+
t.Fatalf("expected parse error, got nil")
108+
}
109+
})
110+
}

0 commit comments

Comments
 (0)