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

Feature - Manage payloads #2989

Merged
merged 6 commits into from
Jun 17, 2024

Conversation

jbaptperez
Copy link
Contributor

Description

  • Updates GET /payloads: Adds optional GET parameters:
    • sort: Boolean, sorts payload names,
    • exclude_plugins: Boolean, excludes payloads from plugins/<plugin-name/data/payloads directories (only data/payloads),
    • add_path: Boolean adds relative path to the payload from the caldera root,
  • Adds POST /payloads: Add a payload to data/payloads,
  • Adds DELETE /payloads/{name}: Delete a payload from data/payloads.

This is the first PR (out of two) to add payload management (add/delete) directly from the Caldera user interface.
I'll send a second PR to the magma repository (which depends on this one) for the frontend part, once this one is merged.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

As Python tests are broken for now, I only tested this code locally using the swagger, an HTTP client but also with a dedicated Vue.js frontend (next magma PR).

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

@jbaptperez jbaptperez force-pushed the feature/manage-payloads branch 3 times, most recently from 8016b60 to 3e9e877 Compare May 29, 2024 17:07
@elegantmoose elegantmoose self-assigned this May 31, 2024
Copy link
Contributor

@elegantmoose elegantmoose left a comment

Choose a reason for hiding this comment

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

See inline comments for minor requested changes.

Tested locally, all functionality works as detailed.

Overall looks great.

app/api/v2/handlers/payload_api.py Outdated Show resolved Hide resolved
app/api/v2/handlers/payload_api.py Outdated Show resolved Hide resolved
app/api/v2/handlers/payload_api.py Outdated Show resolved Hide resolved
@elegantmoose
Copy link
Contributor

@jbaptperez Re-ping me when complete minor changes. TY!

return file_name, file_path

@staticmethod
def __save_file(target_file_path: str, io_base_src: IOBase):
Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies, didnt notice this before but is this file saving method necessary over just using the file saving method from the File service (https://github.com/mitre/caldera/blob/master/app/service/file_svc.py#L65)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elegantmoose, relevant remark.

Actually, it would be good to use it but I cannot directly.
When defining the post_payloads() method (which indirectly calls __save_file()), I used the @aiohttp_apispec.form_schema(PayloadCreateRequestSchema) annotation in order to have a proper swagger form and documentation.

However, this annotation (hidden function) calls request.multipart() before I even enter the post_payloads() method, to check the form according to the given schema.
Then the file can be read from another special request dictionary object (see aiohttp_apispec documentation).
I cannot read twice the multipart form using standard aiohttp methods because the second time, I get no data.

Therefore, I cannot the file_svc.save_multipart_file_upload() or file_svc.save_file() directly because they assume nothing has called the aiohttp multipart methods yet.

However, I can try to do one of the following changes:

  1. Adapt the current file_svc.save_multipart_file_upload() so that according to the given parameters, the file will be read the aiohttp standard way or using a special aiohttp_apispec object produced by the @aiohttp_apispec.form_schema annotation,
  2. Add another dedicated method to handle the aiohttp_apispec file upload case.

In both cases, I would try to use as maximum common code as possible.

Note that the file_svc.save_multipart_file_upload() method reads entirely the uploaded file and gives it as a payload parameter to file_svc.save_file(), which can lead to performance issues in the case of big files, but that's another story.

So tell me what you prefer (including another proposal).

Copy link
Contributor

Choose a reason for hiding this comment

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

I like 2.

But only if not a big lift. Can you take a quick look but cap your effort, dont want to hold this PR up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elegantmoose, I had a look to the file_svc.py code.
It handles general file upload for other services or the (old v1?) API (I'm new to Cladera and I don't have the whole history).

Beside, this payload upload code looks the first API v2 file upload implementation (what is more, using aiohttp_apispec for swagger integration), and there are some global differences:

  • file_svc.py (v1) handles file encoding (HTTP header) and encryption for storage as my v2 code doesn't,
  • file_svc.py (v1) looks to handle some particular cases (file storage path, file extension),
  • The v2 code follows the aiohttp_apispec to handle uploaded file, so the way the file is read is different than in file_svc.py (however, the implementation I made should supports big file).

Finally, I'm running out of time for this work.

Therefore, I propose the following:

  1. I only change the multipart form field name from payload to file to make my code as generic as possible,
  2. If you agree, you merge this PR, and one nows that there is a base code for general API v2 file upload using aiohttp_apispec,
  3. A future and distinct task / user story will handle a migration from v1 file upload to v2 in file_svc.py, with all features (file encoding, file encryption, aiohttp_apispec constraints for swagger).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elegantmoose, I pushed the field name change (see point 1. above).

I saw you merged from master in the feature/manage-payloads branch: I can rebase it (+ git push --force) to avoid a complex history if you want before validating this PR.

I'll create another PR for the GUI part (magma repository).

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay yea, too much of additional lift for this PR. Thanks for looking into though.

Yea, feel free to rebase it.

Im happy with this PR, I would just like @clenk to do quick look over.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elegantmoose, branch rebased and force pushed.

@jbaptperez jbaptperez force-pushed the feature/manage-payloads branch 2 times, most recently from 067e13a to 78c819d Compare June 7, 2024 19:20
@clenk
Copy link
Contributor

clenk commented Jun 7, 2024

Looks good. Is field_field correct or is it supposed to be file_field? I'm not understanding the naming.

Would be nice to have tests in https://github.com/mitre/caldera/blob/master/tests/api/v2/handlers/test_payloads_api.py for it, but I understand with the failing test harness.... Maybe in a future PR.

Thank you for the contribution!

jean-baptiste-perez-bib added 2 commits June 8, 2024 09:14
Adds optional parameters:
- sort: Sorts the returned list,
- exclude_plugins: Excludes payloads of plugins (only retains
  data/payloads ones).
Adds the optional parameter:
add_path: Adds the relative path to the payload, from the Caldera root
directory.
@jbaptperez
Copy link
Contributor Author

jbaptperez commented Jun 8, 2024

@clenk, I fixed the incorrect field_field variable name, rebased everything onto the last master changes and squashed the commit which switches the upload file form field from payload to file.
This lightens the Git history.

You are right about tests, they are missing.
I quickly forgot about them as tests are broken.
Is there any issue / work in progress about this?
Has someone details about what's going on?

@elegantmoose elegantmoose removed the request for review from clenk June 17, 2024 21:25
@elegantmoose elegantmoose merged commit bababe4 into mitre:master Jun 17, 2024
1 of 6 checks passed
@jbaptperez jbaptperez deleted the feature/manage-payloads branch June 18, 2024 14:24
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.

3 participants