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

Reduce template dangers #1137

Closed
yajo opened this issue May 14, 2023 · 19 comments · Fixed by #1171
Closed

Reduce template dangers #1137

yajo opened this issue May 14, 2023 · 19 comments · Fixed by #1171
Assignees
Labels
documentation Issue that requires updating docs enhancement
Milestone

Comments

@yajo
Copy link
Member

yajo commented May 14, 2023

The problem is, templates just need these features. Writing good migrations
takes time, but they provide smooth updates; but what if users are able to
turn them off? Then you can't predict the output. Tasks also are needed to
replay last render when updating. And jinja would die if you use it without
the required extensions.

Good point!

So, although it's OK to warn the user, the features are needed for copier
to do its work. Basically, running Copier without --UNSAFE would only work
with templates without these features. At least, we shouldn't require that
flag when the operation won't involve any of those. Also we should provide
a different exit code for this exit path.

👌

think it'd be OK to present a question before starting, which tells all
dangers at once, so user allows them all or exits. In the case of updates,
we need to include replay dangers and migrations if there are any between
the two versions involved.

👍

Copier is a development tool, so it will usually need access to other
development tools expected in that environment.

Also good point! I hadn't considered that.

I don't recommend running malicious templates in a container. I just
recommend not running them 😆.

However, it would be good to have a page in the docs dedicated to our
threat model and possible workarounds. And that could happen even without
implementing any other fix. It could even be "the fix". 🤔

Yes, I think docs plus the additional question when tasks/migrations/extensions are present or the --UNSAFE flag in case of non-interactive usage, as you described above.

I think this sounds like a good actions plan to raise awareness of the dangers and let users make conscious decisions. 👍

Originally posted by @sisp in #1132 (reply in thread)

@yajo yajo added this to the Soon milestone May 14, 2023
@yajo
Copy link
Member Author

yajo commented May 14, 2023

@sisp would you like to tackle this issue?

@sisp
Copy link
Member

sisp commented May 14, 2023

Yes, feel free to assign me to this issue. 👍

@yajo yajo added enhancement documentation Issue that requires updating docs labels May 16, 2023
@yajo yajo linked a pull request Jun 1, 2023 that will close this issue
@yajo yajo closed this as completed in #1171 Jun 1, 2023
yajo pushed a commit that referenced this issue Jun 1, 2023
…1171)

I've disabled the use of unsafe features (Jinja extensions, migrations, and tasks) by default and added a new CLI switch `--UNSAFE` which enables them. Templates that don't use unsafe features are unaffected by this change. But Copier will raise an error for templates that do use unsafe features unless the `--UNSAFE` flag is passed.

I've not added an interactive prompt that asks for consent for using unsafe features because I think it's not clear how to distinguish between interactive prompting and raising an error when `--UNSAFE` is not passed. For this, I think Copier would need a switch that clearly states whether interactive or non-interactive mode is desired. Currently, `--defaults` implies this for questions.

Fixes #1137

BREAKING CHANGE: Copier raises an error when a template uses unsafe features unless the `--UNSAFE` switch is passed
@fcollonval
Copy link

Hey, I understand all the security concerns raised here. But I feel like the --UNSAFE flag is a bit scary for newbies that don't understand all the good reasons for it. I'm wondering if copier could store a list of trusted template source in config file in the user home directory. The idea is to avoid having to set UNSAFE every time you use a trusted template.

We could then add a flag --trust to copier copy that implies --UNSAFE and add the template to the trusted list;

# First time use
copier copy --trust https://github.com/org/trusted-template.git .

# Update does not require the flag
copier update

# Next time use, does not raise
copier copy https://github.com/org/trusted-template.git .

@pawamoy
Copy link
Contributor

pawamoy commented Jun 5, 2023

I feel the same as @fcollonval. I'm exclusively using my own Copier template (I don't think I've ever generated a project using someone else's template), so it feels a bit tedious to add --UNSAFE (ugh, capital letters 😅 I'm such a pedant) each time. I'll probably set an alias in the meantime: alias cc='copier copy --UNSAFE'. Sorry for voicing this concern only afterwards! And thanks for the amazing work on Copier ❤️

@yajo
Copy link
Member Author

yajo commented Jun 5, 2023

Hi! I was expecting something like this 😄

I use fish instead of bash, so adding flags is much cheaper. That can help you.

The solution of having a registry of trusted templates was among the ones we considered, but it didn't land because it was Harper. However feel free to open a PR, as it's still good. You can open an issue first if you want to discuss the implementation.

@carlsylvia
Copy link

The addition of this flag is very problematic and I recommend you consider rolling this back. My guess is the majority of your users are building templates for in-house use, and your attempt to implement some form of "safeness" is not appropriate. How are you determining/defining what is "safe"?

@pawamoy
Copy link
Contributor

pawamoy commented Jun 6, 2023

@carlsylvia you can stay on v7 in the meantime. Unless Copier's updates are automatic within your system?

@carlsylvia
Copy link

A simple change in the semantics would fix the issue immediately. If you simply default to not trusting a template and require the user to provide a --trust flag so that they are explicitly granting trust to the source then all would be well. That simple logic inversion from --UNSAFE to --trust would make all the difference.

@carlsylvia
Copy link

If you want to implement some trusted source repository in the future that is fine, but sometimes the simplest possible solution is the best.

@carlsylvia
Copy link

For the time being we will pin our users to version 7.x until this issue is resolved.

@sisp
Copy link
Member

sisp commented Jun 6, 2023

A simple change in the semantics would fix the issue immediately. If you simply default to not trusting a template and require the user to provide a --trust flag so that they are explicitly granting trust to the source then all would be well. That simple logic inversion from --UNSAFE to --trust would make all the difference.

--UNSAFE and --trust could even coexist as mutually exclusive flags where --UNSAFE is a one-time opt-in to a template with unsafe features and --trust makes it permanent.

I like the trust store idea. And it seems it would be a solution for everybody who has raised concerns about the new --UNSAFE flag until now.

@carlsylvia
Copy link

carlsylvia commented Jun 6, 2023

The --trust flag could record the repo in a local copier metadata file that maintains a list of "trusted" repositories.
Similar to .ssh/known_hosts or authorized_keys
Could even support "trusting" an entire repo and all of the templates contained within.

@sisp
Copy link
Member

sisp commented Jun 6, 2023

Could even support "trusting" an entire repo and all of the templates contained within.

By "entire repo", do you mean a namespace (user namespace or organization/group) on GitHub/GitLab/...?

@carlsylvia
Copy link

Could even support "trusting" an entire repo and all of the templates contained within.

By "entire repo", do you mean a namespace (user namespace or organization/group) on GitHub/GitLab/...?

I was referring to a GitLab group and/or GitHub org, sorry I was not clear. For example if the copier group contained a collection of template projects then all of projects within https://github.com/copier-org could be trusted, or a single project within the group, depending on the user preference. Not exactly sure how they would specify that preference, but could be convenient to flag the entire group rather than individual template projects.

@yajo
Copy link
Member Author

yajo commented Jun 10, 2023

Hi folks, I've got the MVP in #1179. I'll wait for your reviews before the merge.

For now, it just trusts the current execution, just like --UNSAFE. If somebody wants to contribute a more advanced trusting system, like the ones discussed above in #1137 (comment) or #1137 (comment), I think that should be a separate subcommand:

copier copy --trust git+https://example.com/tpl1 ./here # One-time trust

copier trust git+https://example.com/tpl1 # Permanent trust

copier copy git+https://example.com/tpl1 ./here # Now it works because it's trusted

I don't think it's a good idea to trust an entire organization because one could do copier trust https://github.com and shoot your own foot. But trusting once per template doesn't seem that problematic IMHO.

@fcollonval
Copy link

copier trust git+https://example.com/tpl1 # Permanent trust

I would personally prefer to keep the ability to do everything in a single command aka copier copy --trust <src> <dest>. Although I agree that having a new subcommand will be needed if a trusted list is stored to be able to list trusted source, remove some,... . But rather than having a dedicated subcommand, I would suggest going for a config subcommand as storing settings at the user level may be useful for much more things in the future.

yajo added a commit that referenced this issue Jun 12, 2023
@pawamoy
Copy link
Contributor

pawamoy commented Jun 12, 2023

I don't think it's a good idea to trust an entire organization because one could do copier trust https://github.com and shoot your own foot. But trusting once per template doesn't seem that problematic IMHO.

Trusting the entire site is preventable with code: just make sure the argument points to an org and not just github.com.

@yajo
Copy link
Member Author

yajo commented Jun 12, 2023

Yes, but then we have ssh connections to other git providers, non-git templates... well, I'm not telling this isn't feasible, but it's... complex. And the main issue here AFAICS is that some people don't like UNSAFE because it is upper case and scary; nothing more.

@pawamoy
Copy link
Contributor

pawamoy commented Jun 12, 2023

True, not every Git platform stores things under orgs. Git has no concept of organization so this would indeed be complex to support in Copier.

yajo added a commit that referenced this issue Jun 14, 2023
yajo added a commit that referenced this issue Jun 28, 2023
See rationale in #1137 (comment).

Co-authored-by: Sigurd Spieckermann <2206639+sisp@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issue that requires updating docs enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants