Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VIRTS-1807 Adding exfil file download capability to the UI #2060

Merged
merged 3 commits into from
Mar 16, 2021
Merged

Conversation

mrengstrom
Copy link
Contributor

@mrengstrom mrengstrom commented Feb 24, 2021

Description

adding the capability to download the exfilled files from agents via the UI

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

I created multiple directories and operations and tested to make sure exfilled files were properly shown in the UI and that when downloaded files were un-encrypted and able to be extracted/read without having to pass through any additional scripts.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@codecov
Copy link

codecov bot commented Feb 24, 2021

Codecov Report

Merging #2060 (1168eb0) into master (a689901) will decrease coverage by 0.47%.
The diff coverage is 21.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2060      +/-   ##
==========================================
- Coverage   64.55%   64.07%   -0.48%     
==========================================
  Files          61       61              
  Lines        4672     4724      +52     
==========================================
+ Hits         3016     3027      +11     
- Misses       1656     1697      +41     
Impacted Files Coverage Δ
app/service/file_svc.py 53.98% <7.69%> (-4.02%) ⬇️
app/api/rest_api.py 58.62% <18.18%> (-9.47%) ⬇️
app/service/rest_svc.py 42.25% <18.18%> (-0.61%) ⬇️
app/api/packs/advanced.py 75.00% <66.66%> (-1.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a689901...1168eb0. Read the comment docs.

@unkempthenry unkempthenry requested a review from clenk February 25, 2021 00:58
Copy link
Contributor

@clenk clenk left a comment

Choose a reason for hiding this comment

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

Looks good aside from one bug.

And a couple things I don't think should hold this PR up:

  • In the future we may want to add a "check all" button. The ability I used when testing only exfilled a single file, but if it's very many this would be easier than having to check each checkbox.
  • I noticed we aren't consistent as far as capitalization with our http error messages, such as in download_exfil_file(). This isn't limited to this PR though; see also
    raise web.HTTPBadRequest(body='This operation has already finished.')
    elif state not in op[0].states.values():
    raise web.HTTPBadRequest(body='state must be one of {}'.format(op[0].states.values()))

startdir = self.get_config('exfil_dir')

exfil_files = dict()
exfil_folders = [f.path for f in os.scandir(startdir) if f.is_dir()]
Copy link
Contributor

Choose a reason for hiding this comment

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

I went to the Exfills UI before running any operations, selected "all" and got an Internal Server Error on this line:
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/caldera'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, looks like I forgot to check that, a simple validation should help prevent that. I'll push up a change.

@mrengstrom
Copy link
Contributor Author

Also @clenk I can address you other two issues when I push up my fix, both should be fairly simple to address, and I agree that we should have a consistency in our messages returned to the user.

@unkempthenry unkempthenry removed their assignment Mar 4, 2021
@wbooth wbooth merged commit 8a1f0bb into master Mar 16, 2021
wbooth added a commit that referenced this pull request Mar 16, 2021
@mrengstrom mrengstrom deleted the virts-1807 branch March 23, 2021 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants