1210538 - Add antivirus checks to release promotion graph

Status: RESOLVED FIXED

(Release Engineering :: Release Automation: Other, defect)

Comment 1

[Kim Moir [:kmoir] ET]

in tree mh script
not per platform
add task line bouncer
soft blocked by beet mover

Comment 2

[Kim Moir [:kmoir] ET]

talked to jlund today

integrate av check action into beetmover
add new action action - scan with clamd before uploading to s3 
when push to releases bucket, copy to releases dir
new functionality: automation to continuously run and scan files on s3

Comment 3

[Kim Moir [:kmoir] ET]

Attachment: bug1210538.patch

So in previous discussions, we thought it would be good to have this as a separate action but since each file is moved separately I just scanned them separately before they are uploaded in upload_bit.  Not sure if this should be changed - I could copy them all to a tmp dir when they are uploaded and then scan afterwards but not sure that if would be more efficient given that most files are not scanned according to the exclusion list.  

I also used clamscan instead of clamdscan (daemon) because I couldn't get clamdscan to work in my docker container.

Comment 4

[Jordan Lund (:jlund)]

Comment on attachment 8716394 [details] [diff] [review]
bug1210538.patch

Review of attachment 8716394 [details] [diff] [review]:
-----------------------------------------------------------------

so I think if we are to scan all beets at once, we should probably upload all the beets at the end after we scan. That way we don't upload potentially bad files and have to work backwards after.

patch looks great but I think as is, it accommodates a 'cache' idea of scanning multiple files.

imo - we should either:

1) have three actions: --download-bits --scan-bits --upload-bits

2) do what we are doing now but get rid of the 'rmtree', 'mkdir', 'copyfile', cache stuff and just 'download, scan, upload, and remove' locally in the same path as one action.

what do you think? out of those two options, I am leaning towards (1).

::: testing/mozharness/scripts/release/beet_mover.py
@@ +270,5 @@
> +        #todo more efficient to scan all at once?
> +        if self._matches_exclude(file_name):
> +            self.info("Excluding {} from virus scan".format(file_name))
> +        else:
> +            if os.path.exists(self.dest_dir):

typically in mozharness all artifacts and deps per each script are self contained in a work_dir. could we put the cache dir in there?:

self.dest_dir = os.path.join(self.query_abs_dirs()['abs_work_dir'], 'cache')

@@ +272,5 @@
> +            self.info("Excluding {} from virus scan".format(file_name))
> +        else:
> +            if os.path.exists(self.dest_dir):
> +                self.info('Emptying {}'.format(self.dest_dir))
> +                shutil.rmtree(self.dest_dir)

can we use self.rmtree() here? adds some mh logging and retry

https://dxr.mozilla.org/build-central/source/mozharness/mozharness/base/script.py#162

@@ +273,5 @@
> +        else:
> +            if os.path.exists(self.dest_dir):
> +                self.info('Emptying {}'.format(self.dest_dir))
> +                shutil.rmtree(self.dest_dir)
> +                os.makedirs(self.dest_dir)

self.mkdir() https://dxr.mozilla.org/build-central/source/mozharness/mozharness/base/script.py#137

@@ +275,5 @@
> +                self.info('Emptying {}'.format(self.dest_dir))
> +                shutil.rmtree(self.dest_dir)
> +                os.makedirs(self.dest_dir)
> +                self.info('Copying {} to {}'.format(file_name,self.dest_dir))
> +                shutil.copy(file_name, self.dest_dir)

self.copyfile uses shutil.copy too but with some logging.

https://dxr.mozilla.org/build-central/source/mozharness/mozharness/base/script.py#517

Comment 5

[Kim Moir [:kmoir] ET]

Yes, I think 1) is the better option, I'll refactor toward that

Comment 6

[Kim Moir [:kmoir] ET]

Attachment: bug1210538-2.patch

updated patch to address feedback comments

Comment 7

[Kim Moir [:kmoir] ET]

Comment on attachment 8717525 [details] [diff] [review]
bug1210538-2.patch

asking rail for feedback since jlund is away

not sure how we parse antivirus failures currently or if this is covered by parsing the mozharness output

Comment 8

[Rail Aliiev [:rail]]

Comment on attachment 8717525 [details] [diff] [review]
bug1210538-2.patch

Review of attachment 8717525 [details] [diff] [review]:
-----------------------------------------------------------------

Just some cosmetic changes.

::: testing/mozharness/scripts/release/beet_mover.py
@@ +171,5 @@
> +        # if excludes is set from command line, use it otherwise use defaults
> +        if 'excludes' in self.config:
> +            self.excludes = self.config['excludes']
> +        else:
> +            self.excludes = DEFAULT_EXCLUDES

or in one line:
  
  self.excludes = self.config.get('excludes', DEFAULT_EXCLUDES)

@@ +173,5 @@
> +            self.excludes = self.config['excludes']
> +        else:
> +            self.excludes = DEFAULT_EXCLUDES
> +        dirs = self.query_abs_dirs()
> +        self.dest_dir = os.path.join(dirs['abs_work_dir'],CACHE_DIR)

a nit: add a space after coma.

@@ +245,5 @@
> +        conn = boto.connect_s3(self.aws_key_id, self.aws_secret_key)
> +
> +        # TODO - do we want to mirror/upload to more than one region?
> +        dirs = self.query_abs_dirs()
> +        boto = self.virtualenv_imports['boto']

Looks like "boto = self..." is used twice here. Also, it doesn't look that it's used in this method. You can kill 3 lines of code and 1 comment line here! :)

@@ +277,5 @@
>              for deliverable in self.manifest['mapping'][locale]:
>                  self.log("uploading '{}' deliverable for '{}' locale".format(deliverable, locale))
> +                #we have already downloaded the files locally so we can use that version
> +                source = self.manifest['mapping'][locale][deliverable]['artifact']
> +                downloaded_file = os.path.join(dirs['abs_work_dir'],source.split(os.sep)[-1])

os.sep won't work for URLs on windows (who cares!), but it's be better to either explicitly split by "/".

I think, it'd be better to either pass the same file name to self.download_file() and use it here or use something like

  downloaded_file = os.path.join(dirs['abs_work_dir'], self.get_filename_from_url(source))

@@ +320,5 @@
> +        """Gets a copy of extract_and_run_command.py from tools, and the supporting mar.py,
> +        so that we can unpack various files for clam to scan them."""
> +        remote_file = "{}/raw-file/{}/stage/extract_and_run_command.py".format(self.config["tools_repo"],
> +                                                                               self.config["tools_revision"])
> +        self.download_file(remote_file, file_name="extract_and_run_command.py")

I don't think we use extract_and_run_command.py anywhere except AV scan. Maybe we can just drop a copy of it into "external_tools" to reduce network operations?

@@ +325,3 @@
>  
>  
> +    def scan_bits(self):

shouldn't it be "scan_beets"? :D

Just kidding.

@@ +329,5 @@
> +        dirs = self.query_abs_dirs()
> +        self._get_extract_script()
> +
> +        file_names = []
> +        from os.path import isfile, join

can you move this import to the top?

@@ +330,5 @@
> +        self._get_extract_script()
> +
> +        file_names = []
> +        from os.path import isfile, join
> +        filenames = [f for f in listdir(dirs['abs_work_dir']) if isfile(join(dirs['abs_work_dir'], f))]

Assuming this is a flat directory? os.listdir() won't walk sub directories.

@@ +332,5 @@
> +        file_names = []
> +        from os.path import isfile, join
> +        filenames = [f for f in listdir(dirs['abs_work_dir']) if isfile(join(dirs['abs_work_dir'], f))]
> +        if not os.path.exists(self.dest_dir):
> +            self.mkdir_p(self.dest_dir)

I'd just self.mkdir_p without if - it won't complain if that directory exists.

@@ +337,5 @@
> +        for file_name in filenames:
> +            if self._matches_exclude(file_name):
> +                self.info("Excluding {} from virus scan".format(file_name))
> +            else:
> +                self.info('Copying {} to {}'.format(file_name,self.dest_dir))

a nit: space after coma.

@@ +354,5 @@
> +    def _matches_exclude(self, keyname):
> +         for exclude in self.excludes:
> +             if re.search(exclude, keyname):
> +                 return True
> +         return False

you can make it shorter! :)

  return any(re.search(exclude, keyname) for exclude in self.excludes)

Comment 9

[Kim Moir [:kmoir] ET]

Attachment: bug1210538-3.patch

updated patch with latest suggestions

Comment 10

[Rail Aliiev [:rail]]

Comment on attachment 8718080 [details] [diff] [review]
bug1210538-3.patch

Review of attachment 8718080 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM! Just one nit:

::: testing/mozharness/scripts/release/beet_mover.py
@@ +232,5 @@
> +        self.log('downloading and uploading artifacts to self_dest_dir...')
> +
> +        # TODO - do we want to mirror/upload to more than one region?
> +        dirs = self.query_abs_dirs()
> +        boto = self.virtualenv_imports['boto']

boto is still here and unused. you can kill it.

Comment 11

[Kim Moir [:kmoir] ET]

Attachment: bug1210538-4.patch

Comment 12

[Rail Aliiev [:rail]]

Comment on attachment 8718307 [details] [diff] [review]
bug1210538-4.patch

Review of attachment 8718307 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mozharness/scripts/release/beet_mover.py
@@ +323,5 @@
> +
> +    def _scan_files(self):
> +        """Scan the files we've collected. We do the download and scan concurrently to make
> +        it easier to have a coherent log afterwards. Uses the venv python."""
> +        self.run_command([self.query_python_path(), 'extract_and_run_command.py',

Sorry, I missed this in my previous review. I think mozharness won't be able to find this file in PATH, so need to explicitly specify the full path to it

You can use a trick similar to 
https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/purge.py#13-17

(at the top)
import mozharness


external_tools_path = os.path.join(
    os.path.abspath(os.path.dirname(os.path.dirname(mozharness.__file__))),
    'external_tools',
)

(and here)
self.run_command([self.query_python_path(), os.path.join(external_tools_path, 'extract_and_run_command.py'), ...

Comment 13

[Kim Moir [:kmoir] ET]

Attachment: bug1210538-5.patch

Comment 14

[Rail Aliiev [:rail]]

Comment on attachment 8719863 [details] [diff] [review]
bug1210538-5.patch

Review of attachment 8719863 [details] [diff] [review]:
-----------------------------------------------------------------

Ship it!

Comment 15

[Jordan Lund (:jlund)]

Comment on attachment 8719863 [details] [diff] [review]
bug1210538-5.patch

Review of attachment 8719863 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mozharness/scripts/release/beet_mover.py
@@ -208,5 @@
>          dirs = self.query_abs_dirs()
>          boto = self.virtualenv_imports['boto']
>  
> -        # download locally
> -        file_name = self.retry(self.download_file,

I don't think we can delete the var file_name as it is used in this method. maybe we should change `file_name` refs in upload_bit() body to `source`?

Comment 16

[Kim Moir [:kmoir] ET]

Attachment: bug1210538-6.patch

apologies I missed this in the earlier patch, I had to comment out the upload part during testing because my aws credentials don't work.  Found an unused variable too

Comment 17

[Jordan Lund (:jlund)]

Comment on attachment 8720395 [details] [diff] [review]
bug1210538-6.patch

Review of attachment 8720395 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm :)

Comment 19

[Kim Moir [:kmoir] ET]

Looking at this bug, it appears that the virus scanning worked for en-US

https://tools.taskcluster.net/task-inspector/#3BcGngNCTzKFAMf77SXabQ/0
https://public-artifacts.taskcluster.net/3BcGngNCTzKFAMf77SXabQ/0/public/logs/live_backing.log

There is the message 

"no key specified to validate 1 signature"

but this appears to be harmless according to this bug

https://bugzilla.mozilla.org/show_bug.cgi?id=755114

I'll land the patches on m-i and then I can close this bug

Comment 20

[Kim Moir [:kmoir] ET]

Attachment: bug1210538m-i.patch

patches to upload to m-i. I also included my Dockerfile from bug 1247428

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22b111e834dd

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/22b111e834dd