Re: [BuildStream] Protect against plugin modifications of artifacts

[William Salmon <will salmon codethink co uk>]

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)

And I don't think one is safer than the other but the second is much 
less complicated for a plugin author.

> Yes it is possible to have non reproducible output also from within the sandbox, but the chances are lower 
> and the tools being used are deterministic, which is kind of the whole point.
>
> Permutations within the sandbox is what BuildStream provides and ensures, hacking at the content with plugin 
> code breaks this contract.
>
> This is only considering the point (A) above which is the most important/relevant, other than this you also 
> have a problematic (C) which you raised yourself.
>
> While it is quite obvious that input configurations such as commands provided in `Plugin.configure()` must be 
> a part of the unique key, this is less obvious for code changes in manual strong arming of the sandbox in 
> python code, and more likely to be overlooked in third party plugin code (which should not be easy to get 
> wrong).
>
> Regarding the other comment about multiple ways of doing things: it is impossible to remove the possibility 
> of manipulating files within the sandbox, as this is kind of the primary function of BuildStream (while 
> manipulating files with plugin code was never been a function of BuildStream in the first place).
>
> Cheers,
>      -Tristan