Skip to content

Conversation

@austinbyers
Copy link
Collaborator

@austinbyers austinbyers commented Sep 6, 2017

This fixes a few bugs and adds some improvements to the Analyzer.

Backwards-Incompatible Changes

  • Renames LambdaVersion to AnalyzerVersion (resolves: Rename LambdaVersion DynamoDB column to AnalyzerVersion #44)
    • WARNING: This is a breaking change to the DynamoDB schema. Terraform will destroy the table and create a new one, so be sure to copy your existing DynamoDB table if you want it backed up.

Changes

  • Adds type annotations to the analyzer (contributes to: Use type annotations #34)
  • Uses higher-level boto3 resources for simpler logic (contributes to: Use higher-level boto3 resources #42)
  • Includes all object metadata and object modification time in the YARA match alerts
    • Standardizes filepath as the S3 metadata key which is used for YARA rules which look at the filepath
    • The SNS alert format should have changed as part of Add CarbonBlack downloader #52
  • Fixes a bug where identical S3 objects were skipped during a batch analysis
  • Removes try: except block when building the YARA analyzer in main.py
  • Removes most custom boto3 mocks in favor of simple mock.patching

Tested

  • CI: unit tests, pylint, coverage
  • maage.py live_test alerts as expected with no errors in any Lambda logs
  • Manually tested the CarbonBlack downloader after a deploy

Reviewers

to: @ryandeivert
cc: @airbnb/binaryalert-maintainers

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 89.456% when pulling fac6719 on abb--analyzer-fixes into 4c60632 on master.

Copy link
Contributor

@ryandeivert ryandeivert left a comment

Choose a reason for hiding this comment

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

LGTM - however I'd suggest calling out the Dynamo warning in the PR body a little more since it seems very important

@austinbyers
Copy link
Collaborator Author

@ryandeivert I updated the description. We'll also make sure to call it out in the CHANGELOG

@austinbyers
Copy link
Collaborator Author

Coveralls is taking too long; I've made it not a required check for now

@austinbyers austinbyers merged commit 510f94a into master Sep 6, 2017
@austinbyers austinbyers deleted the abb--analyzer-fixes branch September 6, 2017 20:48
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 89.456% when pulling f3ae9fd on abb--analyzer-fixes into 4c60632 on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rename LambdaVersion DynamoDB column to AnalyzerVersion

4 participants