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

Prepping API #329

Closed
1 task done
StandingPadAnimations opened this issue Aug 4, 2022 · 8 comments
Closed
1 task done

Prepping API #329

StandingPadAnimations opened this issue Aug 4, 2022 · 8 comments
Labels
enhancement Feature requests or new functionality suggestions

Comments

@StandingPadAnimations
Copy link
Collaborator

Check against existing requests

  • I have checked known issues, and my problem is different

Describe the context

Currently, prep materials only works in Cycles, EEVEE, or Blender Internal, and the materials are pretty much set in stone. As such, an API for people to override the prep materials function would be helpful for people using engines like Luxcore.

How do you imagine your feature works?

Overview

This expands upon #274. While #274 makes overriding the default material easy on paper, it's extremely hard to implement in practice and doesn't solve issues with other render engines. Moreover, it expects the user to know the custom node names MCprep uses for image textures

Proposal

The solution would be a basic API that allows developers to generate custom materials using Python and Bpy. An example of such an API (this isn't set in stone right now but just an idea):

import bpy

def prep_mat(mcprep_ctx, bpy_ctx):
    image = mcprep_ctx.get_diffuse_tex()
    image_node = mcprep.create_diffuse(image)
    # Do some stuff with image_node like generating a custom material...


MCPREP_INFO = {
    "engine" : "CYCLES",
    "min_bv" : 3.2,
    "prep_func" : prep_mat
}

Implementation on the MCprep side

Such an API would be somewhat simple to implement on the MCprep side of things. MCprep could have a folder where the user drags in a file designed for the Prepping API and MCprep registers the file when Blender starts up (i.e. during the addon registration process)

Potentional Security Risks

With any API that can be exploited, there will be people attempting to write code that could harm a user. For example, it is possible to delete a user's operating system using the os module. While Blender does make installation of 3rd party packages a nightmare, there's nothing stopping someone from using Python's standard library

I'll use Blender and Better Discord (a modification of the Discord client) as examples. With both software, they assume that the user knows what they're doing. Better Discord takes a step further by curating a list of plugins on their website that have been confirmed to be safe and are put under strict guidelines. In Blender's case meanwhile, it's just extremely hard to spread malicious code in the first place

However, we also have to consider that a significant portion of the MCprep userbase may not understand security issues in the first place (either because they're in an environment that doesn't fully understand digital security, or they're simply too naive to assume that people have bad intentions).

To counteract this, I have a 2 part solution. The first part of the solution is to make the Prepping API opt-in by default, so a user would have to actively enable it in the first place. The second part is modeled after Better Discord's solution to plugins; a GitHub repository with Prepping API scripts that are confirmed to be safe and follow strict guidelines. Those guidelines can be as follows:

  • Must be open sourced
  • Must have a public Github repo
  • Can not use requests over the internet
  • Can not access local files of a user
  • Etc.

This wouldn't stop people from using scripts that aren't from the list (unless we make it so only confirmed scripts can be installed, but that would be hard to enforce at the MCprep level and brings issues with creating new Prepping API scripts), but it can alleviate some of the security risks.

I know this part of the proposal is massive, but with any API security must be a priority, even an API as simple as the suggested Prepping API.

Conclusion

The Prepping API concept would solve some of the fundamental issues that #274 brings and would actually bring more benefits than #274. It's both easier on the user (provided we make the API easy to use) and us MCprep developers (in terms of maintaining and implementing), and since Python is a big language (which can be a double-edged sword as I mentioned in the Security section), it allows users to do certain operations with textures that may not be possible in #274 (such as upscaling and color adjustments)

What existing workaround (or closest thing to a workaround) do you have today (within Blender, MCprep, or any software)? If there is no workaround, explain why you feel this way.

For overriding default materials, it requires a lot of work with sync materials. For engines other then Cycles, EEVEE, or Blender Internal, the only workaround is hoping that those engines can support or convert nodes from Cycles/EEVEE/Blender Internal.

@StandingPadAnimations StandingPadAnimations added the enhancement Feature requests or new functionality suggestions label Aug 4, 2022
@zNightlord
Copy link
Contributor

zNightlord commented Aug 21, 2022

This is very interesting, having this would be nice for advanced user. In Rigify there is a thing call finalize script or post process script to process the rig after done with generation. They do have some util handy function in the addon to use as API.

I make this just to preping into Malt material.
image

@StandingPadAnimations
Copy link
Collaborator Author

To address the security concerns, I think we could add key signing so that MCprep's own prepping scripts can be used without worrying about tampering. It would rely on a 3rd party Python module, though.

One issue however is Windows, since the easiest way would be PGP signing (as it already exists, and we can take advantage of it) which isn't supported by default. Another way of verifying security (though I don't like this route) would be that MCprep would request the user to read and verify scripts every time one is updated (similar to how we Arch users verify AUR PKGBUILD files). It's the simplest, but most MCprep users don't know what to look out for.

@zNightlord
Copy link
Contributor

zNightlord commented May 8, 2023

I'm going to leave this here. If anyone come across this and want to look into this. https://wiki.blender.org/wiki/Process/Addons/Rigify/FeatureSets
Rigify is an interesting addon for rigging is builtin into Blender.
What make it so powerful? It is an rigging framework, an API that let technical user add custom rigs or "Featureset" on top of it with a package zip file and also that post process script I mentioned above.
So a feature sets added can register load another python addon, can extend even override the existing base addon.

@StandingPadAnimations
Copy link
Collaborator Author

StandingPadAnimations commented May 16, 2023

Regarding this issue, after a disscussion on the MCprep Discord server, I think it's fine if we add this feature provided we require the Prepping API to be enabled in the actual MCprep Python code itself (i.e. don't put a checkbox to enable the Prepping API).

The reason I say this is because I have 0 faith that MCprep users will read and understand random Python scripts they download off the internet, and this feature will allow arbitrary Python scripts to be loadable. As Murphy's Law states:

Anything that can go wrong will go wrong

We MCprep maintainers have a responsibility in keeping users' systems safe. If we make the Prepping API too easy to enable and users suffer as a result, I personally believe the blame would lie on us as we could have made this far harder to exploit. As such, I will personally block any pull request related to the Prepping API from being merged if it has the following:

  • Putting the Prepping API as an option in the UI
  • Having the Prepping API enabled by default or always enabled
  • Compromise security in any other way beyond the obvious without addressing it

I know developers and users will be mad at these requirements, but we're dealing with something that could perform some serious stuff. Python can:

  • Execute arbitrary shell commands
  • Delete files
  • Download resources from the internet
  • and much, much more

"Other addons and programs don't care"
Cause either their userbases aren't full of naive 12 year olds that don't understand basic computer literacy (no offense to our usebase, but it has to be said) or they have their own scripting languages where doing messing up the system is impossible, and we're not going to write our own scripting language (that would be slow, and unmaintainable; I've written many compilers and interpreters, and it's not something most developers will grasp)

"What about blocking imports?"
Yeah that's not enough. People will find ways around it. Many modules in the Python standard library depend on other modules; should we ban half of the standard library to prevent a few modules from ever being loaded?

"What about a sandbox?"
The only proper way would be a process sandbox, which Blender does not come with. We can't make a sandbox at the Python level cause that's impossible; that was learned with pysandbox:

I now agree that putting a sandbox in CPython is the wrong design. There are too many ways to escape the untrusted namespace using the various introspection features of the Python language.

The Python language is weird in that we can do whatever to objects. Heck, we already do this for 2.7x support. The downside is that it makes sandboxing at the language damn near impossible. Pysandbox was at the interpreter level and it stilled failed, how badly would a sandbox implemented in Python fail?

Tldr: I'll allow development on this feature, but personally block pull requests related to this if they (repeating from earlier):

  • Putting the Prepping API as an option in the UI
  • Having the Prepping API enabled by default or always enabled
  • Compromise security in any other way beyond the obvious without addressing it

StandingPadAnimations added a commit that referenced this issue May 17, 2023
Since Prepping API security is no laughing matter, I've went ahead and
added some basic code for a security check. It's test based and will ask
the user to answer a bunch of questions related to Python and malicious
scripting.

Am I harsh in the comments? Yes, but I think it's needed as we're
dealing with something that if done wrong could compromise user systems.
As I stated in #329
> If we make the Prepping API too easy to enable and users suffer as a
> result, I personally believe the blame would lie on us as we could
> have made this far harder to exploit.

I don't care about what other addons do, our audience is full of 12 year
olds that want to have fun by making Minecraft animations, security is
certainly not on their minds.

I'll be pulling example code from Python malware I find on the internet
(to those that submit that code on GitHub for people to be aware and
comment it well, I'm grateful), so there's going to be a varity of code
in these questions.
@StandingPadAnimations
Copy link
Collaborator Author

I've made a Prepping API branch and pushed d62872c. I'm starting with security since that's my number one priority.

@StandingPadAnimations
Copy link
Collaborator Author

Ok I think I've changed my mind on having a custom scripting language. I'll start a repo for a new Lua based language (I'll call it PrepScript for now) and see how it goes

I will still add support for Python, but make enabling Python support extremely hard

@StandingPadAnimations
Copy link
Collaborator Author

StandingPadAnimations commented May 19, 2023

This is all under the assumption that we have people that can maintain PrepScript in the future.

@TheDuckCow, just want your opinion on this. I'm perfectly fine not creating a custom scripting language if you feel it would be too hard to maintain (that's the reason I initially didn't want to make one)

@StandingPadAnimations
Copy link
Collaborator Author

I'll be closing this issue since I feel that for such a niche feature, the workarounds needed aren't worth it

@StandingPadAnimations StandingPadAnimations closed this as not planned Won't fix, can't repro, duplicate, stale Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests or new functionality suggestions
Projects
None yet
Development

No branches or pull requests

2 participants