-
Notifications
You must be signed in to change notification settings - Fork 139
[merged] verify: validate layers of system image stored on disk #531
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,5 +5,4 @@ build | |
| *.log | ||
| htmlcov | ||
| .coverage | ||
|
|
||
| tests/test-images/Dockerfile.secret | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ | |
| ['return_code', 'stdout', 'stderr']) | ||
| ATOMIC_CONF = os.environ.get('ATOMIC_CONF', '/etc/atomic.conf') | ||
| ATOMIC_CONFD = os.environ.get('ATOMIC_CONFD', '/etc/atomic.d/') | ||
| ATOMIC_LIBEXEC = os.environ.get('ATOMIC_LIBEXEC', '/usr/libexec/atomic') | ||
|
|
||
| def check_if_python2(): | ||
| if int(sys.version_info[0]) < 3: | ||
|
|
@@ -296,7 +297,6 @@ def skopeo_layers(image, args=None, layers=None): | |
| finally: | ||
| if not success: | ||
| shutil.rmtree(temp_dir) | ||
|
|
||
| return temp_dir | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Intentional? |
||
|
|
||
|
|
||
|
|
@@ -435,3 +435,39 @@ def find_remote_image(client, image): | |
|
|
||
| def is_user_mode(): | ||
| return os.geteuid() != 0 | ||
|
|
||
| def generate_validation_manifest(img_rootfs=None, img_tar=None, keywords=""): | ||
| """ | ||
| Executes the gomtree validation manifest creation command | ||
| :param img_rootfs: path to directory | ||
| :param img_tar: path to tar file (or tgz file) | ||
| :param keywords: use only the keywords specified to create the manifest | ||
| :return: output of gomtree validation manifest creation | ||
| """ | ||
| if img_rootfs == None and img_tar == None: | ||
| write_out("no source for gomtree to generate a manifest from") | ||
| if img_rootfs: | ||
| cmd = [ATOMIC_LIBEXEC + '/gomtree','-c','-p',img_rootfs] | ||
| elif img_tar: | ||
| cmd = [ATOMIC_LIBEXEC + '/gomtree','-c','-T',img_tar] | ||
| if keywords: | ||
| cmd += ['-k',keywords] | ||
| return subp(cmd) | ||
|
|
||
| def validate_manifest(spec, img_rootfs=None, img_tar=None, keywords=""): | ||
| """ | ||
| Executes the gomtree validation manife st validation command | ||
| :param img_rootfs: path to directory | ||
| :param img_tar: path to tar file (or tgz file) | ||
| :param keywords: use only the keywords specified to validate the manifest | ||
| :return: output of gomtree validation manifest validation | ||
| """ | ||
| if img_rootfs == None and img_tar == None: | ||
| write_out("no source for gomtree to validate a manifest") | ||
| if img_rootfs: | ||
| cmd = [ATOMIC_LIBEXEC + '/gomtree', '-p', img_rootfs, '-f', spec] | ||
| elif img_tar: | ||
| cmd = [ATOMIC_LIBEXEC + '/gomtree','-T',img_tar, '-f', spec] | ||
| if keywords: | ||
| cmd += ['-k',keywords] | ||
| return subp(cmd) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,9 +5,14 @@ | |
| from operator import itemgetter | ||
| from .atomic import AtomicError | ||
| from .syscontainers import SystemContainers | ||
| from .mount import Mount | ||
| import shutil | ||
| import itertools | ||
| import tempfile | ||
| import subprocess | ||
|
|
||
| ATOMIC_VAR = '/var/lib/containers/atomic' | ||
|
|
||
| class Verify(Atomic): | ||
| def __init__(self): | ||
| super(Verify, self).__init__() | ||
|
|
@@ -16,6 +21,10 @@ def __init__(self): | |
| def verify_system_image(self): | ||
| manifest = self.syscontainers.get_manifest(self.image) | ||
| layers = SystemContainers.get_layers_from_manifest(manifest) | ||
|
|
||
| if hasattr(self.args,"validate") and self.args.validate: | ||
| self.validate_system_image_manifests(layers) | ||
|
|
||
| remote = True | ||
| try: | ||
| remote_manifest = self.syscontainers.get_manifest(self.image, remote=True) | ||
|
|
@@ -88,6 +97,12 @@ def fix_layers(layers): | |
| except AtomicError: | ||
| self._no_such_image() | ||
|
|
||
| if hasattr(self.args,"generate") and self.args.generate: | ||
| self.generate_docker_validation_manifest() | ||
|
|
||
| elif hasattr(self.args,"validate") and self.args.validate: | ||
| self.validate_docker_image_manifest() | ||
|
|
||
| layers = fix_layers(self.get_layers()) | ||
| if self.debug: | ||
| for l in layers: | ||
|
|
@@ -343,6 +358,98 @@ def assemble_nvr(self, image, image_name=None): | |
| else: | ||
| return nvr | ||
|
|
||
| def validate_system_image_manifests(self,layers): | ||
| """ | ||
| Validate a system image's layers against the the associated validation manifests | ||
| created from those image layers on atomic pull. | ||
| :param layers: list of the names of the layers to validate | ||
| :return: None | ||
| """ | ||
| missing_manifests = [] | ||
| for layer in layers: | ||
| layer = layer.replace("sha256:","") | ||
| manifestpath = self.get_gomtree_manifest(layer) | ||
| if not manifestpath: | ||
| missing_manifests.append(layer) | ||
| continue | ||
| tmp_dir = tempfile.mkdtemp() | ||
| ref = os.path.join("ociimage", layer) | ||
| cmd = ['ostree','checkout','--union','--repo=%s' % self.syscontainers.get_ostree_repo_location(),ref,tmp_dir] | ||
| r = util.subp(cmd) | ||
| if r.return_code != 0: | ||
| util.write_err(r.stderr) | ||
| continue | ||
| r = util.validate_manifest(manifestpath, img_rootfs=tmp_dir,keywords="type,uid,gid,mode,size,sha256digest") | ||
| if r.return_code != 0: | ||
| util.write_out("modifications in layer %s layer:\n" % layer) | ||
| if r.return_code > 1: | ||
| util.write_err(r.stderr) | ||
| else: | ||
| util.write_err(r.stdout) | ||
| shutil.rmtree(tmp_dir) | ||
| if len(missing_manifests): | ||
| util.write_out("validation manifests for the following layers do not exist:\n") | ||
| for layer in missing_manifests: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this go to stderr?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it should, yeah. I'll change it to |
||
| util.write_out("\t%s" % layer) | ||
| util.write_out("\n") | ||
| util.write_out("perform an `atomic pull \"%s\"` if you want to generate these manifests\n" % self.image) | ||
|
|
||
| def generate_docker_validation_manifest(self): | ||
| """ | ||
| Generates a gomtree validation manifest for a non-system image and stores it in | ||
| ATOMIC_VAR | ||
| :param: | ||
| :return: None | ||
| """ | ||
| iid = self._is_image(self.image) | ||
| if os.path.exists(os.path.join(ATOMIC_VAR,"gomtree-manifests/%s.mtree" % iid)): | ||
| return | ||
| if not os.path.exists(os.path.join(ATOMIC_VAR,"gomtree-manifests")): | ||
| os.makedirs(os.path.join(ATOMIC_VAR,"gomtree-manifests")) | ||
| manifestname = os.path.join(ATOMIC_VAR, "gomtree-manifests/%s.mtree" % iid) | ||
| tmpdir = tempfile.mkdtemp() | ||
| m = Mount() | ||
| m.args = [] | ||
| m.image = self.image | ||
| m.mountpoint = tmpdir | ||
| m.mount() | ||
| r = util.generate_validation_manifest(img_rootfs=tmpdir, keywords="type,uid,gid,mode,size,sha256digest") | ||
| m.unmount() | ||
| with open(manifestname,"w",0) as f: | ||
| f.write(r.stdout) | ||
| shutil.rmtree(tmpdir) | ||
|
|
||
| def validate_docker_image_manifest(self): | ||
| """ | ||
| Validates a docker image by mounting the image on a rootfs and validate that | ||
| rootfs against the manifests that were created. Note that it won't be validated | ||
| layer by layer. | ||
| :param: | ||
| :return: None | ||
| """ | ||
| iid = self._is_image(self.image) | ||
| if not os.path.exists(os.path.join(ATOMIC_VAR,"gomtree-manifests/%s.mtree" % iid)): | ||
| return | ||
| manifestname = os.path.join(ATOMIC_VAR, "gomtree-manifests/%s.mtree" % iid) | ||
| tmpdir = tempfile.mkdtemp() | ||
| m = Mount() | ||
| m.args = [] | ||
| m.image = self.image | ||
| m.mountpoint = tmpdir | ||
| m.mount() | ||
| r = util.validate_manifest(manifestname, img_rootfs=tmpdir, keywords="type,uid,gid,mode,size,sha256digest") | ||
| m.unmount() | ||
| if r.return_code != 0: | ||
| util.write_err(r.stdout) | ||
| shutil.rmtree(tmpdir) | ||
|
|
||
| @staticmethod | ||
| def get_gomtree_manifest(layer, root=os.path.join(ATOMIC_VAR, "gomtree-manifests")): | ||
| manifestpath = os.path.join(root,"%s.mtree" % layer) | ||
| if os.path.isfile(manifestpath): | ||
| return manifestpath | ||
| return None | ||
|
|
||
| @staticmethod | ||
| def get_local_version(name, layers): | ||
| for layer in layers: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,6 @@ VERSION=$(shell $(PYTHON) setup.py --version) | |
| all: python-build docs pylint-check dockertar-sha256-helper | ||
|
|
||
| .PHONY: test-python3-pylint | ||
| test-python3-pylint: | ||
| $(PYTHON3_PYLINT) --disable=all --enable=E --enable=W --additional-builtins=_ *.py atomic Atomic tests/unit/*.py -d=no-absolute-import,print-statement,no-absolute-import,bad-builtin | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a mistake, probably due to rebasing. I'll fix it |
||
|
|
||
| .PHONY: test check | ||
|
|
@@ -62,7 +61,7 @@ install-only: | |
| ln -fs ../share/atomic/atomic $(DESTDIR)/usr/bin/atomic | ||
|
|
||
| install -d -m 0755 $(DESTDIR)/usr/libexec/atomic | ||
| install -m 0755 dockertar-sha256-helper migrate.sh gotar $(DESTDIR)/usr/libexec/atomic | ||
| install -m 0755 dockertar-sha256-helper migrate.sh gotar gomtree $(DESTDIR)/usr/libexec/atomic | ||
|
|
||
| [ -d $(SYSCONFDIR) ] || mkdir -p $(SYSCONFDIR) | ||
| install -m 644 atomic.sysconfig $(SYSCONFDIR)/atomic | ||
|
|
@@ -86,4 +85,3 @@ install: all install-only | |
| .PHONY: install-openscap | ||
| install-openscap: | ||
| install -m 644 atomic.d/openscap $(DESTDIR)/etc/atomic.d | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,7 +66,6 @@ class HelpByDefaultArgumentParser(argparse.ArgumentParser): | |
| def print_usage(self, message="too few arguments"): | ||
| self.prog = " ".join(sys.argv) | ||
| self.error(message) | ||
|
|
||
|
|
||
| # Code for python2 copied from Adrian Sampson hack | ||
| # https://gist.github.com/sampsyo/471779 | ||
|
|
@@ -683,6 +682,12 @@ def create_parser(help_text): | |
| verifyp.add_argument("-v", "--verbose", default=False, | ||
| action="store_true", | ||
| help=_("Report status of each layer")) | ||
| verifyp.add_argument("-V", "--validate", default=True, | ||
| action="store_false", | ||
| help=_("disable validating system images")) | ||
| verifyp.add_argument("-g", "--generate", default=False, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems weird to have this option under 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should move this to another verb
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The immutable bit, could be a problem where a user could not remove a file that a hacker had placed on a file system.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. |
||
| action="store_true", | ||
| help=_("generate gomtree manifest of image and store it")) | ||
| return parser | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this default to true?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we want the default behavior to do this then yes. Since there was other functionality that
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I just think this might be more important functionality then what we had before. I would like to see verify grow to
|
||
|
|
||
| if __name__ == '__main__': | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,9 @@ IMAGE | |
| # DESCRIPTION | ||
| **atomic verify** checks whether there is a newer image available and scans | ||
| through all layers to see if any of the layers, which are base images themselves, have a new version available. | ||
| If the tool finds an out of date image, it will report as such. | ||
| If the tool finds an out of date image, it will report as such. If the image is a system image, it will | ||
| also look through the layers and validate each layer to determine if it has been tampered with and output | ||
| details of these changes (if at all). | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we want to switch this definition to include all of the things that validate will attempt to do. Do them by default and then add options to disable features. |
||
| If the image or any of its layers are pulled from a repository, it will attempt to check the repository | ||
| to see if there is a new image and capture any of its relevant information like version (where applicable). | ||
|
|
@@ -29,6 +31,9 @@ the version information. | |
| **-v** **--verbose** | ||
| Will output the status of each base image that makes up the image being verified. | ||
|
|
||
| **-g** **--generate** | ||
| Generates a gomtree validation manifest when user specifies a non-system image | ||
|
|
||
| # EXAMPLES | ||
| Verify the Red Hat rsyslog image | ||
|
|
||
|
|
@@ -43,9 +48,20 @@ Verify the Red Hat rsyslog image and show status of each image layer | |
| redhat/rhel7-7.1-24 redhat/rhel7-7.1-24 | ||
|
|
||
| * = version difference | ||
| Verify a system image | ||
|
|
||
| # sudo atomic verify busybox | ||
| validation output for layer a3ed95caeb02ffe68cdd9fd84406680ae93d633cb16422d00e8a7c22955b46d4: | ||
|
|
||
| (no changes detected) | ||
|
|
||
| validation output for layer 8ddc19f16526912237dd8af81971d5e4dd0587907234be2b83e249518d5b673f: | ||
|
|
||
| "etc/shadow": keyword "size": expected 243; got 268 | ||
| "etc/shadow": keyword "sha256digest": expected 22d9cee21ee808c52af44ac...; got 7a07ac69054c2a3533569874c2... | ||
|
|
||
|
|
||
| # HISTORY | ||
| May 2015, Originally compiled by Daniel Walsh (dwalsh at redhat dot com) | ||
|
|
||
| Nov 2015, Updated for remote inspect by Brent Baude (bbaude at redhat dot com) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be under /var/lib/containers? As used below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I will change this to
/var/lib/containers/atomic