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

security considerations for wrapper retrieval #478

Open
FliesLikeABrick opened this issue Apr 24, 2022 · 1 comment
Open

security considerations for wrapper retrieval #478

FliesLikeABrick opened this issue Apr 24, 2022 · 1 comment
Assignees
Labels
bug Something isn't working

Comments

@FliesLikeABrick
Copy link

FliesLikeABrick commented Apr 24, 2022

Snakemake version
Snakemake 7.3.8
Wrappers 1.3.2 (master branch)

Note the Snakemake and snakemake-wrappers version for which you experience the bug.
Always make sure to try out the latest version of both before creating a new bug report.

Describe the bug
Hello,

We are wondering if the developers have considered publishing security recommendations or caveats for the wrapper usage where the wrapper is automatically retrieved from github? It is generally considered a poor practice to directly run code retrieved from a URL, particularly in an automated or programmatic fashion where the contents can change.

This feedback is less about concerns for Snakemake's code integrity (but security compromises are always possible); and more about the possible influence over poor user decisions by indicating that this kind of "always pull in remote code dependencies" pattern is acceptable and secure.

For example, giving a user a command to "curl -s [some url] | bash" as part of a routine operation, versus retrieving the file and running a known good copy. Even if the user hasn't reviewed the script's contents - this at least gives consistency and and ensurance that the contents will not change -- possibly maliciously.

In particular, upon reviewing the documentation at https://snakemake-wrappers.readthedocs.io/en/latest/ , I have the following observations and/or questions:

  • Have the developers considered changing the default recommendation/default strategy to be cloning the repository?
  • Have the developers considered publishing recommendations that productionized code/environments should use a local copy of the wrapper?
  • Cloning the repository or specific wrapper directory has other benefits for performance and reliability, especially if github is having issues.
  • Even if the user is specifying the version tag to be retrieved, this is subject to abuse or error to point to broken or malicious code
  • Since the version tag is specified in the wrapper path for remote retrieval, this seems to be solely a usability move -- instead of the benefit of automatically retrieving the latest updates; in which case a local cached copy is the ultimate in stability.

We understand that merges to master and changes to tags or other metadata come with the usual levels of review, however this does not alleviate the risk to users of problems upstream from infrastructure, operational, or security (credential compromise, or other) influence/risks/unforeseen circumstances

If the above questions and statements seem reasonable and we are not overlooking some published docs/recommendations or other misunderstanding, our suggestions pertain mostly to documentation changes:

  • Update documentation to recommend cloning the repository/directory of the necessary wrappers as a default strategy, but we understand if this is seen as a significant detriment to snakemake-wrappers' usability for new users

  • At least publish caveats of relying on remote retrieval, that there is not a support/availability guarantee and you [the user] understand you are relying on a 3rd party resource when otherwise your data and resources are local

  • There should be a strong recommendation that productionized code/pipelines do not rely on hosted wrappers

  • The documentation be transparent about why the developers consider remote retrieval safe for development/non-production use, such as reference to the contribution and code review guidelines in use. This helps differentiate Snakemake's practices from pulling python, bash, or other code dynamically from non-reviewed/non-controlled sources (like some smaller project's manually maintained website on a Wordpress instance that could more easily be compromised without review/alert)

  • A non-documentation change could be to have the code cache the wrapper on the client if it is pulled from a specific version tag or commit hash. This would provide many of the benefits while reducing dependency on the remote hosting. Additionally, since a version tag or commit hash means the file contents are not expected to change, there is no need for the cache to have aging logic or other long-term maintenance built in -- it can get retrieved once and used forever.

Thank you for your consideration. If you are open to changes in at least the documentation, I am happy to contribute a PR that reflects a reasonable outcome.
Logs
If applicable, any terminal output to help explain your problem.

Minimal example
Add a minimal example for reproducing the bug.

Additional context
Add any other context about the problem here.

@FliesLikeABrick FliesLikeABrick added the bug Something isn't working label Apr 24, 2022
Copy link
Contributor

github-actions bot commented Jan 1, 2024

This issue was marked as stale because it has been open for 6 months with no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants