The default bug view has changed. See this FAQ.

Add antivirus checks to release promotion graph

RESOLVED FIXED

Status

Release Engineering
Release Automation
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: rail, Assigned: kmoir)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

a year ago
Assignee: nobody → kmoir
(Assignee)

Comment 1

a year ago
in tree mh script
not per platform
add task line bouncer
soft blocked by beet mover
(Assignee)

Comment 2

a year ago
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
(Assignee)

Comment 3

a year ago
Created attachment 8716394 [details] [diff] [review]
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.
(Assignee)

Updated

a year ago
Attachment #8716394 - Flags: feedback?(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
Attachment #8716394 - Flags: feedback?(jlund) → feedback+
(Assignee)

Comment 5

a year ago
Yes, I think 1) is the better option, I'll refactor toward that
(Assignee)

Comment 6

a year ago
Created attachment 8717525 [details] [diff] [review]
bug1210538-2.patch

updated patch to address feedback comments
Attachment #8716394 - Attachment is obsolete: true
(Assignee)

Comment 7

a year ago
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
Attachment #8717525 - Flags: feedback?(rail)
(Reporter)

Comment 8

a year ago
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)
Attachment #8717525 - Flags: feedback?(rail) → feedback+
(Assignee)

Comment 9

a year ago
Created attachment 8718080 [details] [diff] [review]
bug1210538-3.patch

updated patch with latest suggestions
Attachment #8717525 - Attachment is obsolete: true
Attachment #8718080 - Flags: feedback?(rail)
(Reporter)

Comment 10

a year ago
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.
Attachment #8718080 - Flags: feedback?(rail) → feedback+
(Assignee)

Comment 11

a year ago
Created attachment 8718307 [details] [diff] [review]
bug1210538-4.patch
Attachment #8718080 - Attachment is obsolete: true
Attachment #8718307 - Flags: review?(rail)
(Reporter)

Comment 12

a year ago
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'), ...
Attachment #8718307 - Flags: review?(rail) → review-
(Assignee)

Comment 13

a year ago
Created attachment 8719863 [details] [diff] [review]
bug1210538-5.patch
Attachment #8718307 - Attachment is obsolete: true
Attachment #8719863 - Flags: review?(rail)
(Reporter)

Comment 14

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

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

Ship it!
Attachment #8719863 - Flags: review?(rail) → review+
(Assignee)

Updated

a year ago
Attachment #8719863 - Flags: checked-in+
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`?
(Assignee)

Comment 16

a year ago
Created attachment 8720395 [details] [diff] [review]
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
Attachment #8720395 - Flags: review?(jlund)
Comment on attachment 8720395 [details] [diff] [review]
bug1210538-6.patch

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

lgtm :)
Attachment #8720395 - Flags: review?(jlund) → review+
(Assignee)

Updated

a year ago
Attachment #8720395 - Flags: checked-in+
(Reporter)

Updated

a year ago
Duplicate of this bug: 699579
(Assignee)

Comment 19

a year ago
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
(Assignee)

Comment 20

a year ago
Created attachment 8722138 [details] [diff] [review]
bug1210538m-i.patch

patches to upload to m-i. I also included my Dockerfile from bug 1247428
Attachment #8722138 - Flags: review?(rail)
(Reporter)

Updated

a year ago
Attachment #8722138 - Flags: review?(rail) → review+

Comment 21

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/22b111e834dd
(Assignee)

Updated

a year ago
Attachment #8722138 - Flags: checked-in+
(Assignee)

Updated

a year ago
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/22b111e834dd
You need to log in before you can comment on or make changes to this bug.