[merged] verify: validate layers of system image stored on disk by imsteev · Pull Request #531 · projectatomic/atomic

imsteev · 2016-08-10T21:32:49Z

Integrated the gomtree filesystem verification tool into atomic verify -V <system_image>.

The idea behind this integration of a filesystem verification tool is so that when a user pulls an image (for now only system images) using atomic pull ..., a manifest that details a layer (i.e, file ordering, file attributes like mode, size, sha256digest, etc.) will be created and stored in /var/lib/atomic/gomtree-manifests. Then, when a user performs atomic verify -V <system_image>, these manifests will be validated against the local image the user has on disk.

Ideally, we would be able to do this for the rest of the images as well, and implement it in a similar manner to syscontainers._pull_image_to_ostree()

Since gomtree is still being developed, we need to maintain updated versions of the gomtree binary. I wasn't sure if a go get..., go install... was something we wanted to do in the Makefile. For now, I created a local build of the gomtree tool and added it as part of this commit.

For testing, would the program be able to write to a /var/lib/atomic directory? Because that would be the location where these "gomtree manifests" would be stored. If you want to test it by hand, you can go into your ostree repo and change some content of an image you're not using (although this might affect other images as well, so be careful) and then perform verify with the -V flag.

Signed-off-by: Stephen Chung schung@redhat.com

imsteev · 2016-08-10T21:36:53Z

@vbatts here is an integration of gomtree into the atomic CLI

imsteev · 2016-08-10T22:36:23Z

Atomic/verify.py

+                continue
+            tmp_dir = tempfile.mkdtemp()
+            ref = os.path.join("ociimage", layer)
+            cmd = ['ostree','checkout','--union','--repo=%s' % "/ostree/repo",ref,tmp_dir]


can we make syscontainers._get_repo_location() a non protected function?

giuseppe · 2016-08-11T08:11:30Z

Atomic/syscontainers.py

+            os.makedirs(root)
+        manifestname = os.path.join(root, "%s.mtree" % layer)
+        if os.path.isfile(manifestname):
+            util.write_out("validation manifest for layer %s alredy created" % layer)


small typo: already

giuseppe · 2016-08-11T08:28:56Z

in prune_ostree_images we should check that for each manifest file the layer still exists. If the layer doesn't exist, then remove the manifest file.

For images stored in an OSTree repository, how does this compare to ostree fsck?

rhatdan · 2016-08-11T12:19:52Z

@mitr @runcom PTAL

@mitr at some point we would like to get this to be covered by the signing spec.

rhatdan · 2016-08-11T12:24:20Z

Atomic/util.py

        if not success:
            shutil.rmtree(temp_dir)
-
    return temp_dir


Intentional?

rhatdan · 2016-08-11T12:32:13Z

@baude PTAL

imsteev · 2016-08-11T13:21:26Z

@giuseppe ostree fsck checks for the overall integrity of the image by evaluating only the checksums of a file. If we wanted more details about what exactly about that image has changed, then integrating gomtree would help us do that. Also, being able to have a validation manifest allows us to incorporate this 'consistency check' when we distribute and sign images.

imsteev · 2016-08-11T17:32:34Z

pushed revisions based off feedback. removed the -V flag to make the system validation default behavior.

imsteev · 2016-08-11T21:38:16Z

revised this PR so that now on an atomic verify -g <non-system image>, we can create a validation manifest. Then, when we run atomic verify <non-system image>, the validation manifest will be checked against the docker image (through temporarily mounting the image)

rhatdan · 2016-08-12T12:09:53Z

Atomic/syscontainers.py


 ATOMIC_LIBEXEC = os.environ.get('ATOMIC_LIBEXEC', '/usr/libexec/atomic')
+ATOMIC_VAR = '/var/lib/atomic'



Should this be under /var/lib/containers? As used below?

ok I will change this to /var/lib/containers/atomic

rhatdan · 2016-08-12T12:14:06Z

We need some tests to make sure this works.

giuseppe · 2016-08-12T14:20:04Z

when are these files deleted? Shouldn't the manifest be deleted on prune if the layer is not present anymore?

Integrate go-mtree tool into atomic CLI command, `atomic verify -V <imagename>`. (Note that `gomtree` is still in development and thus we would need some way of obtaining updated gomtree binaries) When a user does an atomic pull, validation manifests for that system image's layers are created and stored in /var/lib/atomic. When user does `atomic verify -V <system image>`, these manifests are then validated against the image that is on disk (which would be stored in ostree). Signed-off-by: Stephen Chung <schung@redhat.com>

imsteev · 2016-08-12T16:22:32Z

added a simple test case to check if atomic pull on a system image will create gomtree manifests. will need to figure out how to change a consistent file in a system image so that we can test the output of gomtree validation

rhatdan · 2016-08-12T18:55:40Z

Ok we can merge this, but @lsm5 is going to have to add gomtree to the atomic build, or we need to get this accepted as a new package into Fedora.

@rh-atomic-bot r+

rh-atomic-bot · 2016-08-12T18:55:41Z

📌 Commit d741e92 has been approved by rhatdan

rh-atomic-bot · 2016-08-12T18:55:47Z

⌛ Testing commit d741e92 with merge d4728ae...

cgwalters · 2016-08-12T19:04:16Z

atomic

+    verifyp.add_argument("-V", "--validate", default=True,
+                                action="store_false",
+                                help=_("disable validating system images"))
+    verifyp.add_argument("-g", "--generate", default=False,


It seems weird to have this option under verify. I'd expect it to be a system-wide setting one can turn on by default.

Also...I'm a bit concerned about the performance of all of this. Our intent with OSTree BTW is to drive immutability down into the storage stack - then you don't really need verify since the files are truly immutable. See discussion in O_OBJECT

Maybe we should move this to another verb
atomic images generate
Which would generate new checksums.

The immutable bit, could be a problem where a user could not remove a file that a hacker had placed on a file system.

That sort of thing is why I'm not using the immutable bit in ostree today. The semantics are annoying. My proposal for O_OBJECT does allow root to unlink the file (and to create new links to it). I.e. the inode can change, but not the contents.

Also, the immutable bit can be removed. With O_OBJECT at least you'd have to have compromised the kernel (or have CAP_SYS_ADMIN to write to the raw disk).

In the bigger picture of course if someone has CAP_SYS_ADMIN on your machine, they can inject code into the kernel to lie to whatever userspace validation tools you have, or things like inject malware into the hard drive controller.

That said, if we did have the mtree data in OCI images we built, one could use them to more efficiently perform offline forensics too. Mount a disk with libguestfs or whatever, download just the mtree metadata from a layer rather than the whole layer tar.

rh-atomic-bot · 2016-08-12T19:09:32Z

☀️ Test successful - status-atomicjenkins
Approved by: rhatdan
Pushing d4728ae to master...

imsteev reviewed Aug 10, 2016
View reviewed changes

imsteev changed the title ~~verify: validate layers of system image stored on disk (needs some discussion)~~ verify: validate layers of system image stored on disk Aug 11, 2016

giuseppe reviewed Aug 11, 2016
View reviewed changes

rhatdan reviewed Aug 11, 2016
View reviewed changes

Atomic/util.py

if not success:

shutil.rmtree(temp_dir)

return temp_dir

Copy link

Member

rhatdan Aug 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional?

rhatdan reviewed Aug 12, 2016
View reviewed changes

cgwalters reviewed Aug 12, 2016
View reviewed changes

rh-atomic-bot changed the title ~~verify: validate layers of system image stored on disk~~ [merged] verify: validate layers of system image stored on disk Aug 12, 2016

rh-atomic-bot closed this in d4728ae Aug 12, 2016

imsteev deleted the image_validation branch August 12, 2016 21:20

rhatdan mentioned this pull request Aug 15, 2016

[merged] Move generate from atomic verify to atomic images #534

Closed


		ATOMIC_LIBEXEC = os.environ.get('ATOMIC_LIBEXEC', '/usr/libexec/atomic')
		ATOMIC_VAR = '/var/lib/atomic'

Conversation

imsteev commented Aug 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imsteev commented Aug 10, 2016

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!

giuseppe commented Aug 11, 2016

Uh oh!

rhatdan commented Aug 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Aug 11, 2016

Uh oh!

imsteev commented Aug 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imsteev commented Aug 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imsteev commented Aug 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Aug 12, 2016

Uh oh!

giuseppe commented Aug 12, 2016

Uh oh!

imsteev commented Aug 12, 2016

Uh oh!

rhatdan commented Aug 12, 2016

Uh oh!

rh-atomic-bot commented Aug 12, 2016

Uh oh!

rh-atomic-bot commented Aug 12, 2016

Uh oh!

cgwalters Aug 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

rh-atomic-bot commented Aug 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

imsteev commented Aug 10, 2016 •

edited

Loading

imsteev commented Aug 11, 2016 •

edited

Loading

imsteev commented Aug 11, 2016 •

edited

Loading

cgwalters Aug 12, 2016 •

edited

Loading