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

fix!: Validate paths for Job Bundles #171

Merged
merged 1 commit into from
Feb 7, 2024
Merged

Conversation

marofke
Copy link
Contributor

@marofke marofke commented Feb 2, 2024

What was the problem/requirement? (What/Why)

We need to safeguard users when uploading inputs. There's a few cases here:

  1. A Job bundle directory cannot contain symlinks that resolve outside of the bundle directory
  2. The default PATH values in the template defined in the bundle:
    a. cannot have default parameters that are absolute, and
    b. the default relative paths must resolve inside the bundle directory (so no escaping with ../)
  3. For input paths in the Asset References file, any PATH values in the Job bundle parameters file, as well as any queue environment PATH parameters:
    a. If the path resolves outside of the job bundle directory AND is not part of a configured storage profile location for the queue being submitted to, we need to warn the user, so they're aware files will be uploaded from an unexpected area

What was the solution? (How)

  1. When loading a job bundle, walk the directory and ensure all paths resolve inside the resolved job bundle directory (as the Job Bundle directory could itself be a symlink)
    a. This was mostly added in src/deadline/client/job_bundle/parameters.py and src/deadline/client/job_bundle/loader.py
  2. Reorganize the submission workflow, adding a new function to src/deadline/job_attachments/upload.py to first process all asset paths (these are combined with the validated Job Bundle, parameters file, queue environment parameters, and asset references files), and prompt the user to confirm if they want to continue submission if files are found outside of the Job Bundle dir and any storage profile locations for the queue
    a. Note that I moved the confirmation popup that happens AFTER hashing to BEFORE hashing to include a combined confirmation message
    b. Also note: when submitting through the GUI, since we move the job bundle to the ~/.deadline/job_history/ dir, the actual input files for the job bundle do not move, so if the files in the original job bundle are not part of a Storage Profile filesystem location, it will always prompt the user
  3. I also took the opportunity to clean up our GUI a little bit (I haven't done much QT work before, so it was fun to try paying the beauty tax)
    a. Fixed some text that didn't render as it was using HTML tags
    b. Grouped the progress bars so it's more clear

What is the impact of this change?

Users should have more information about what they're submitting when they attempt to submit a Job Bundle. This protects users from job bundles that could contain symlinks that resolve outside of the job bundle directory. Also, the UI is a little nicer to look at (at least I think so ;) )

How was this change tested?

  • unit / integration tests pass
  • LOOOOTS of manual submission testing (for both CLI and GUI workflows):
    • Job bundle with symlinks that resolve within the job bundle -> submits successfully
    • Job bundle with symlinks that resolve outside the job bundle -> raises an error
    • Job bundle with relative default PATH values that resolve outside the bundle dir -> raises an error
    • Job bundle with absolute default PATH -> raises an error
    • Set a storage profile filesystem location, included asset references within that directory -> normal submission with no warnings
    • Set a storage profile filesystem location, included asset references outside that directory -> makes a warning popup
    • Added a queue environment with IN and INOUT PATH parameters. Confirmed that absolute paths that resolved outside of my configured storage profile locations presented a warning popup, and relative / absolute paths that resolved within storage profiles did not

UI Changes

Old UI
  • The status boxes at the bottom had cut off text and white borders that didn't fit with the overall theme
    Screenshot 2024-02-02 at 5 19 52 PM
  • Submission progress UI didn't have information clearly separated
    Screenshot 2024-02-02 at 5 20 04 PM
New UI
  • Made the credential stats boxes (at the bottom) look more in line with the rest of the styling
    Screenshot 2024-02-07 at 10 29 47 AM
    Screenshot 2024-02-07 at 10 29 55 AM
    Screenshot 2024-02-07 at 10 30 15 AM
    Screenshot 2024-02-07 at 10 30 28 AM
  • Made the submission progress UI more clear
    Screenshot 2024-02-07 at 10 42 52 AM
  • Warning when files are outside of SP file location / job bundle dir
    Screenshot 2024-02-02 at 3 37 04 PM
  • What happens if you hit cancel
    Screenshot 2024-02-02 at 3 37 18 PM
  • Error when you try to load a Job Bundle with paths that resolve outside of the bundle dir
    Screenshot 2024-02-02 at 3 41 33 PM
  • What the submission UI looks like if you have all valid paths within a SP file location / bundle dir
    Screenshot 2024-02-02 at 3 43 26 PM
  • Successful submission UI
    Screenshot 2024-02-07 at 10 43 02 AM
    Screenshot 2024-02-07 at 10 43 22 AM

Was this change documented?

Not yet

Is this a breaking change?

Yes, the interface for the Job Attachments S3AssetManager has changed (should only affect custom Python scripts that import the library)

  • hash_assets_and_create_manifest takes different arguments
  • prepare_paths_for_upload was added and needs to be called before hash_assets_and_create_manifest

Also:

  • the user is prompted for confirmation before hashing when submitting (previously, they'd be prompted after hashing, before uploading)
  • can no longer open job bundles with paths that resolve outside of the job bundle directory
  • can no longer open job bundles withe default PATH parameters in the template, and/or relative paths that resolve outside of the job bundle directory

@marofke marofke force-pushed the marofke/symlink-inpath-fix branch 5 times, most recently from f7cc051 to 15adea0 Compare February 5, 2024 18:09
@marofke marofke marked this pull request as ready for review February 5, 2024 18:12
@marofke marofke requested a review from a team as a code owner February 5, 2024 18:12
@marofke marofke changed the title fix: Enforce relative PATHs for job bundle defaults fix!: Enforce relative PATHs for job bundle defaults Feb 5, 2024
@marofke marofke changed the title fix!: Enforce relative PATHs for job bundle defaults fix!: Validate paths for Job Bundles Feb 5, 2024
@marofke marofke force-pushed the marofke/symlink-inpath-fix branch 2 times, most recently from 1032d21 to 7c62124 Compare February 6, 2024 19:42
src/deadline/client/job_bundle/loader.py Outdated Show resolved Hide resolved
src/deadline/client/job_bundle/loader.py Outdated Show resolved Hide resolved
src/deadline/client/job_bundle/parameters.py Show resolved Hide resolved
@marofke marofke force-pushed the marofke/symlink-inpath-fix branch 2 times, most recently from 8c15f35 to 17c200b Compare February 7, 2024 16:31
@marofke
Copy link
Contributor Author

marofke commented Feb 7, 2024

I've also updated the screenshots with some UI changes I made based on Mark's feedback, take another look at the description!

Copy link
Contributor

@gahyusuh gahyusuh left a comment

Choose a reason for hiding this comment

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

Thanks for making lots of refactoring and making the GUI submitter prettier! I posted minor comments mainly related to variable names.

src/deadline/job_attachments/upload.py Outdated Show resolved Hide resolved
scripted_tests/upload_cancel_test.py Outdated Show resolved Hide resolved
scripted_tests/upload_scale_test.py Outdated Show resolved Hide resolved
src/deadline/client/cli/_groups/bundle_group.py Outdated Show resolved Hide resolved
Copy link
Contributor

@gahyusuh gahyusuh left a comment

Choose a reason for hiding this comment

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

Look great to me, thank you!

Breaking change!

Adds validation to Job Bundles, including the following:
- default PATH values in Job Bundle templates MUST be relative
  AND must resolve within the bundle directory
- that includes any symlinks within the bundle itself
- if any paths in the overall submission resolve outside of
  the job bundle directory or any configured storage profile
  filesystem locations, a warning message is raised
- `S3AssetManager` changes:
  - `prepare_paths_for_upload` was added and needs to be called
    before hash_assets_and_create_manifest
  - `hash_assets_and_create_manifest` interface changed

Signed-off-by: Caden Marofke <marofke@amazon.com>
@marofke marofke merged commit 278e4f6 into mainline Feb 7, 2024
18 checks passed
YutongLi291 pushed a commit that referenced this pull request Feb 14, 2024
Breaking change!

Adds validation to Job Bundles, including the following:
- default PATH values in Job Bundle templates MUST be relative
  AND must resolve within the bundle directory
- that includes any symlinks within the bundle itself
- if any paths in the overall submission resolve outside of
  the job bundle directory or any configured storage profile
  filesystem locations, a warning message is raised
- `S3AssetManager` changes:
  - `prepare_paths_for_upload` was added and needs to be called
    before hash_assets_and_create_manifest
  - `hash_assets_and_create_manifest` interface changed

Signed-off-by: Caden Marofke <marofke@amazon.com>
@marofke marofke deleted the marofke/symlink-inpath-fix branch February 21, 2024 23:34
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.

3 participants