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

Add ability to deploy remote packages #143

Merged
merged 6 commits into from
Nov 4, 2021

Conversation

YrrepNoj
Copy link
Contributor

@YrrepNoj YrrepNoj commented Oct 29, 2021

Adding the ability for Zarf to deploy remote packages. This checks if the deployPath given is a URL with a file extension we recognize. If the path is a valid package it gets downloaded to /temp and processed.

This also renames packageName to packagePath to try to show that the variable is more than just a name and could either be a full path to a file or even a URL to some online resource.

Potential Issues To Add After Merge:

  • Add more verification checks on the file being downloaded (in addition to the file extension and sha256 check we're currently doing?)
  • Add credentials to access the url provided, currently we are only able to download packages that are public.
  • Check remote filetype instead of simply checking the file extension.

@YrrepNoj
Copy link
Contributor Author

/test all

@YrrepNoj YrrepNoj marked this pull request as draft October 30, 2021 02:37
@YrrepNoj YrrepNoj changed the title WIP: Add ability to deploy remote packages Add ability to deploy remote packages Nov 1, 2021
@YrrepNoj YrrepNoj linked an issue Nov 1, 2021 that may be closed by this pull request
@YrrepNoj YrrepNoj marked this pull request as ready for review November 1, 2021 16:23
@YrrepNoj
Copy link
Contributor Author

YrrepNoj commented Nov 1, 2021

/test all

@YrrepNoj YrrepNoj self-assigned this Nov 1, 2021
@jeff-mccoy
Copy link
Contributor

The PR looks good, though we might want to breakup that Deploy() function a little, it's getting heft. However I do have concerns with this idea overall until we have more SBOM components. If we aren't validating a signature or even SHA hash then we have no way to vet the quality of this content or that it should be trusted. I'm not certain what the right answer is, but I think we should discuss that concern a little before merging. CC @RothAndrew @mikhailswift

@RothAndrew
Copy link
Contributor

RothAndrew commented Nov 1, 2021

The PR looks good, though we might want to breakup that Deploy() function a little, it's getting heft. However I do have concerns with this idea overall until we have more SBOM components. If we aren't validating a signature or even SHA hash then we have no way to vet the quality of this content or that it should be trusted. I'm not certain what the right answer is, but I think we should discuss that concern a little before merging. CC @RothAndrew @mikhailswift

I do find value in this from an ease-of-use perspective, though I definitely do see the security implications.

We could potentially require the user to add a --shasum flag, or add an --insecure flag if they don't want to. That would at least make them internalize that this is not a secure thing to do.

Example:

# Won't work
zarf package deploy https://example.com/some-package.tar.zst
ERROR: --shasum or --insecure required to proceed

# The right way
zarf package deploy https://example.com/some-package.tar.zst --shasum aec070645fe53ee3b3763059376134f058cc337247c978add178b6ccdfb0019f

# The insecure way
zarf package deploy https://example.com/some-package.tar.zst --insecure

We could also require --insecure if they don't specify https://

@jeff-mccoy
Copy link
Contributor

I like that format, think it should be part of this PR before merge @YrrepNoj

@YrrepNoj YrrepNoj force-pushed the feature/deploy-remote-package branch from 1238d64 to 239cb0c Compare November 2, 2021 00:50
@YrrepNoj
Copy link
Contributor Author

YrrepNoj commented Nov 2, 2021

/test all

cli/cmd/package.go Outdated Show resolved Hide resolved
cli/cmd/package.go Outdated Show resolved Hide resolved
cli/internal/packager/deploy.go Outdated Show resolved Hide resolved
cli/internal/packager/deploy.go Outdated Show resolved Hide resolved
cli/internal/packager/deploy.go Outdated Show resolved Hide resolved
test/e2e/e2e_general_cli_test.go Show resolved Hide resolved
@YrrepNoj
Copy link
Contributor Author

YrrepNoj commented Nov 4, 2021

/test all

@jeff-mccoy jeff-mccoy self-requested a review November 4, 2021 14:30
jeff-mccoy
jeff-mccoy previously approved these changes Nov 4, 2021
Copy link
Contributor

@jeff-mccoy jeff-mccoy left a comment

Choose a reason for hiding this comment

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

lgtm. Only comment on the slice constant problem. We could make it private and use a getter with a by value return, I think that would get after the immutability quality of a constant.

@YrrepNoj
Copy link
Contributor Author

YrrepNoj commented Nov 4, 2021

/test all

@jeff-mccoy jeff-mccoy merged commit 3795096 into master Nov 4, 2021
@jeff-mccoy jeff-mccoy deleted the feature/deploy-remote-package branch November 4, 2021 20:17
jeff-mccoy pushed a commit that referenced this pull request Feb 8, 2022
* Add ability to deploy remote packages

* Rename 'packagePath' variable in deploy code to better represent value

* Update package deploy help message to describe URL usage

* Add shasum check for remote package deployments

* Add e2e test for deploying remote packages

* Add initializer function for ValidFileExtensions variable

Signed-off-by: Jeff McCoy <code@jeffm.us>
Noxsios pushed a commit that referenced this pull request Mar 8, 2023
* Add ability to deploy remote packages

* Rename 'packagePath' variable in deploy code to better represent value

* Update package deploy help message to describe URL usage

* Add shasum check for remote package deployments

* Add e2e test for deploying remote packages

* Add initializer function for ValidFileExtensions variable

Signed-off-by: Jeff McCoy <code@jeffm.us>
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.

Deploy package from remote source
3 participants