Re: [BuildStream] Protect against plugin modifications of artifacts

[William Salmon <will salmon codethink co uk>]

On 22/06/2020 16:52, Tristan Van Berkom wrote:
> Hi again,
>
> > On Jun 22, 2020, at 10:50 PM, William Salmon <will salmon codethink co uk> wrote:
> >
> > 
> >
> > > On 22/06/2020 14:26, Tristan Van Berkom wrote:
> > > Hi,
> > > > > On Jun 22, 2020, at 9:37 PM, William Salmon <will salmon codethink co uk> wrote:
> > > >
> > > > 
> > > >
> > > > > On 22/06/2020 12:24, Tristan Van Berkom wrote:
> > > > > Hi William,
> > > > > Thanks for engaging in this.
> > > > > > On Mon, 2020-06-22 at 11:02 +0100, William Salmon wrote:
> > > > > > Reply in line.
> > > > > [...]
> > > > [..]
> > > > > The main differences here are:
> > > > >    A.) The guarantees we can provide about running safe sandboxed code
> > > > >        compared with untrusted local python interpreter code.
> > > > >        This would lead us down a path of providing ever more carefully
> > > > >        crafted APIs in the core in order to produce reproducible output
> > > > >        in the artifact, all the while not being certain what python APIs
> > > > >        a plugin might use to produce output.
> > > >
> > > > Else were you mention that you don't want to get in to a debate about if we can trust python. So lets not, 
> > > > lets decide were things should live and then decide if we need to change our plugin language separately.
> > > >
> > > > If a plugin creates a config file with logic dependent on config options and variables etc and then the 
> > > > plugin adds a configure command that calls cat to add this text to a file. This seems just as susceptible to 
> > > > plugin author or plugin language issues as if the plugin directly adds a file to the cas (via the nice api we 
> > > > have now) so it can be included in the sandbox.
> > > >
> > > > These seem equivalent except that using cat is more complex and requires cat be a build dependency. There are 
> > > > many alternatives to cat but they still require some extra bin be in the sandbox and adding new configure 
> > > > commands.
> > > It is not equivalent.
> > > `cat`, or whatever you use inside the sandbox, is part of a controlled environment, the python code your 
> > > plugin is running, is not.
> >
> > Yes but sometimes the logic to generate the conf is non trivial so in those cases were the plugin needs to 
> > effect the text, the danger is in creating the PLUGIN_GENERATED_CONTENT and is a separate issue.
> >
> > Once PLUGIN_GENERATED_CONTENT is created we can ether
> >
> > configuration_commands.append("""
> > cat > /path/to/build/file.conf << EOF
> > """ + PLUGIN_GENERATED_CONTENT + """
> > EOF
> > """)
> >
> > or
> >
> > file.write(PLUGIN_GENERATED_CONTENT)
>
> Definitely agree.
>
> The thing is, both are highly problematic.

This is my issue, creating PLUGIN_GENERATED_CONTENT is some times highly 
desirable / necessary*

So we should A) make it as easy as possible to make sure 
PLUGIN_GENERATED_CONTENT is unique to a cache key, B) minimize the risk 
of PLUGIN_GENERATED_CONTENT not getting correctly translated in to the 
sand box.

Writing via the vdir API to the cas is a much better way to get 
PLUGIN_GENERATED_CONTENT in to the sandbox, IMO because it dose not have 
to be escaped to get through sh and cat, dose not need extra deps and 
dose not needs manipulation of configuration commands etc.


> The fact that generating data in python is problematic is not a 
reason to avoid fixing illegal writing to the sandbox (which probably 
mostly happens *due* to the latter problem), its a reason to ensure that 
we *also* ensure that doesn’t happen.
>

While the addition may have been a error in your eyes, If writing to the 
sandbox was not public API I would have be arguing that it should be. 
For the reasons above.

Directory access API has been used by plugin authors for a long time and 
it is the cleanest way to do a variety of things and has been used by a 
variety of plugin authors.

Regardless of why/how the API was added this thread is saying that we 
should get rid of the most reliable way to place 
PLUGIN_GENERATED_CONTENT in to the sandbox.

I am not saying that this API should not be improved or that there is no 
room to make it better or more tied to a cache key etc to reduce risks 
of things going wrong. But to completely remove this feature would seem 
to me to be a major step in the wrong direction.


* If we are ever going to generate a good image creation element or 
maybe something like a better OSTree element then having plugins 
generate config/content for the underlying tool is needed.