Untested: Enforce runtime architecture #752

mtrmac · 2019-11-14T20:45:27Z

When pulling images for consumption by the current runtime, reject images that target a different architecture: https://bugzilla.redhat.com/show_bug.cgi?id=1753019 .

WIP:

Absolutely untested, beyond that it compiles.
I have no idea how this interacts with manifest lists yet.
We might want to look for images that would be broken by this (is it possible to run i386 images on x86_64?)
This should ideally be tested directly in CRI-O to ensure it behaves reasonably, and the error is propagated all the way up to the pod status.
I’d really love to hear opinions on the semantics change of MustMatchRuntimeOS.

vrothberg

A somehow related libpod PR: containers/podman#4505

I think it's valid to enforce the OS and arch unless explicitly requested by the user. We could add an option to ignore the checks but I haven't heard any user request until now.

vrothberg · 2019-11-18T09:21:53Z

copy/copy.go

-		c, err := src.OCIConfig(ctx)
-		if err != nil {
-			return errors.Wrapf(err, "Error parsing image configuration")
+		if (wantedOS == "windows" && c.OS == "linux") || (wantedOS != "windows" && c.OS == "windows") {


Why not if wantedOS != c.OS?

This keeps the previous logic unchanged. I didn’t look into why it is written exactly this way.

It dates back to 0c75281 but I can't imagine why it was written that way. According to spec, we should understand all values of GOOS (https://golang.org/doc/install/source#environment) which is currently not the case - at least for this specific check.

This is somehow still outstanding. The logic predates this PR but I'm convinced it's wrong. @mtrmac, if you prefer, I can open a PR after and take ownership of the consequences.

*shrug* added a commit to simplify it now:

Looking back at older images, docker://busybox:1.21-ubuntu from 2015 does already have "os" and "architecture" filled in, so this seems safe enough.

I’m not exactly sure how we are going to test that this PR does not break compatibility with obscure images… but if we figure that out, we might just as well test both changes at once.

copy/copy.go

vrothberg · 2019-11-18T09:29:19Z

copy/copy.go

-		if err != nil {
-			return errors.Wrapf(err, "Error parsing image configuration")
+		if (wantedOS == "windows" && c.OS == "linux") || (wantedOS != "windows" && c.OS == "windows") {
+			return fmt.Errorf("image operating system %q cannot be used on %q", c.OS, wantedOS)


The error message can be misleading as we could force to exchange the image and host via the SystemContext, such that it claims that the "image operating system "linux" cannot be used on "windows"" (when running a linux host). Maybe we can change the wording to "mismatching image operating system: received %q, expected %q"? Same applies to the architecture.

Will fix, though I expect the implementation, and so possibly the reporting, will change drastically after the full scope of #753 .

Improved the texts now.

…ture This has arguably been implied (OTOH, also arguably, it's a breaking change), make it explicit. This does not yet implement the semantics. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

... to make it easier to add architecture checks. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Now it applies better to OSChoice overrides (it does not claim that the OSChoice is the current runtime OS). Signed-off-by: Miloslav Trmač <mitr@redhat.com>

It's unknown why the previous check was so complex. Looking back at older images, docker://busybox:1.21-ubuntu from 2015 does already have "os" and "architecture" filled in, so this seems safe enough. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

(i.e. containers-storage:, ostree:, local docker-daemon: ). This uses the same logic as manifest list choices, so it should be safe, but it's not 100% certain (e.g. this will probably prevent pulling 386 containers on x86_64, which is currently possible for non-multi-arch images, at least). Signed-off-by: Miloslav Trmač <mitr@redhat.com>

vrothberg

LGTM, thanks @mtrmac

rhatdan · 2019-11-22T20:30:46Z

LGTM

mtrmac force-pushed the enforce-architecture branch from 9cb3112 to 7c319f8 Compare November 14, 2019 20:46

vrothberg reviewed Nov 18, 2019

View reviewed changes

mtrmac added 3 commits November 22, 2019 15:46

Redefine ImageDestination.MustMatchRuntimeOS to also include architec…

0d94869

…ture This has arguably been implied (OTOH, also arguably, it's a breaking change), make it explicit. This does not yet implement the semantics. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Rearrange checkImageDestinationForCurrentRuntimeOS a bit

aad7b77

... to make it easier to add architecture checks. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Improve OS mismatch error message

13f621b

Now it applies better to OSChoice overrides (it does not claim that the OSChoice is the current runtime OS). Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac force-pushed the enforce-architecture branch from 7c319f8 to d68f82e Compare November 22, 2019 14:59

mtrmac added 2 commits November 22, 2019 16:11

Simplify the OS equivalence check

20477a8

It's unknown why the previous check was so complex. Looking back at older images, docker://busybox:1.21-ubuntu from 2015 does already have "os" and "architecture" filled in, so this seems safe enough. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac force-pushed the enforce-architecture branch from d68f82e to d13dc8a Compare November 22, 2019 15:11

mtrmac changed the title ~~WIP RFC: Enforce runtime architecture~~ Untested: Enforce runtime architecture Nov 22, 2019

vrothberg reviewed Nov 22, 2019

View reviewed changes

rhatdan merged commit f680ba3 into containers:master Nov 22, 2019

mtrmac deleted the enforce-architecture branch November 23, 2019 04:48

mtrmac mentioned this pull request Dec 11, 2019

Image decryption support for CRIO cri-o/cri-o#2813

Merged

mtrmac mentioned this pull request Jan 13, 2020

Pulling cross platform images containers/podman#4849

Closed

mtrmac mentioned this pull request Jan 23, 2020

Drop the architecture check in checkImageDestinationForCurrentRuntime #799

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Untested: Enforce runtime architecture #752

Untested: Enforce runtime architecture #752

Uh oh!

mtrmac commented Nov 14, 2019

Uh oh!

vrothberg left a comment

Uh oh!

vrothberg Nov 18, 2019

Uh oh!

mtrmac Nov 18, 2019

Uh oh!

vrothberg Nov 19, 2019

Uh oh!

vrothberg Nov 22, 2019

Uh oh!

mtrmac Nov 22, 2019

Uh oh!

Uh oh!

Uh oh!

vrothberg Nov 18, 2019

Uh oh!

mtrmac Nov 18, 2019

Uh oh!

mtrmac Nov 22, 2019

Uh oh!

vrothberg left a comment

Uh oh!

rhatdan commented Nov 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Untested: Enforce runtime architecture #752

Untested: Enforce runtime architecture #752

Uh oh!

Conversation

mtrmac commented Nov 14, 2019

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Nov 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants