Skip to content

Commit

Permalink
Do not include image annotations when building spec
Browse files Browse the repository at this point in the history
These annotations can have security implications - crun, for
example, allows rootless containers to preserve the user's groups
through an annotation. We absolutely should not include
annotations from an untrusted image off the internet by default.

We may consider whitelisting some annotations (e.g. the legacy
WASM annotations), but given that there is now a more explicit
way of specifying an image uses the WASM runtime in the OCI image
spec, I'm just tearing this out entirely for now.

Signed-off-by: Matt Heon <[email protected]>
  • Loading branch information
mheon authored and openshift-cherrypick-robot committed May 22, 2023
1 parent c926b12 commit 0f18a01
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 7 deletions.
10 changes: 3 additions & 7 deletions pkg/specgen/generate/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,6 @@ func CompleteSpec(ctx context.Context, r *libpod.Runtime, s *specgen.SpecGenerat
s.Env = envLib.Join(defaultEnvs, s.Env)

// Labels and Annotations
annotations := make(map[string]string)
if newImage != nil {
labels, err := newImage.Labels(ctx)
if err != nil {
Expand All @@ -183,12 +182,8 @@ func CompleteSpec(ctx context.Context, r *libpod.Runtime, s *specgen.SpecGenerat
}
}

// Add annotations from the image
for k, v := range inspectData.Annotations {
if !define.IsReservedAnnotation(k) {
annotations[k] = v
}
}
// Do NOT include image annotations - these can have security
// implications, we don't want untrusted images setting them.
}

// in the event this container is in a pod, and the pod has an infra container
Expand All @@ -199,6 +194,7 @@ func CompleteSpec(ctx context.Context, r *libpod.Runtime, s *specgen.SpecGenerat
// VM, which is the default behavior
// - "container" denotes the container should join the VM of the SandboxID
// (the infra container)
annotations := make(map[string]string)
if len(s.Pod) > 0 {
p, err := r.LookupPod(s.Pod)
if err != nil {
Expand Down
2 changes: 2 additions & 0 deletions test/e2e/build/basicalpine/Containerfile.with_label
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
FROM quay.io/libpod/alpine:latest
LABEL testlabel=testvalue
26 changes: 26 additions & 0 deletions test/e2e/run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2071,4 +2071,30 @@ WORKDIR /madethis`, BB)
Expect(t).To(BeTrue())
Expect(strings[0]).Should(ContainSubstring("size=10240k"))
})

It("podman run does not preserve image annotations", func() {
annoName := "test.annotation.present"
annoValue := "annovalue"
imgName := "basicalpine"
build := podmanTest.Podman([]string{"build", "-f", "build/basicalpine/Containerfile.with_label", "--annotation", fmt.Sprintf("%s=%s", annoName, annoValue), "-t", imgName})
build.WaitWithDefaultTimeout()
Expect(build).Should(Exit(0))
Expect(build.ErrorToString()).To(BeEmpty(), "build error logged")

ctrName := "ctr1"
run := podmanTest.Podman([]string{"run", "-d", "--name", ctrName, imgName, "top"})
run.WaitWithDefaultTimeout()
Expect(run).Should(Exit(0))
Expect(run.ErrorToString()).To(BeEmpty(), "run error logged")

inspect := podmanTest.Podman([]string{"inspect", ctrName})
inspect.WaitWithDefaultTimeout()
Expect(inspect).Should(Exit(0))
Expect(inspect.ErrorToString()).To(BeEmpty(), "inspect error logged")

inspectData := inspect.InspectContainerToJSON()
Expect(inspectData).To(HaveLen(1))
Expect(inspectData[0].Config.Annotations).To(Not(HaveKey(annoName)))
Expect(inspectData[0].Config.Annotations).To(Not(HaveKey("testlabel")))
})
})

0 comments on commit 0f18a01

Please sign in to comment.