mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Till Toenshoff <toensh...@me.com>
Subject Re: Review Request 57763: Fixed environment overwrite warning to not leak possibly sensitive data.
Date Wed, 22 Mar 2017 05:56:06 GMT


> On March 20, 2017, 11:43 a.m., Benjamin Bannier wrote:
> > We already discussed this offline, and I still (maybe even stronger) feel that cases
of duplicate env vars with differing values should be errors, dispite the fact that in the
current implementation Mesos makes sure that isolators' `prepare` are called in deterministic
order (there is no guarantee on the interface level). We should strongly consider tightening
the behavior here if we do not have to support the current behavior for e.g., backwards compatibility
reasons.
> > 
> > Env vars potentially mix user input from `CommandInfo` with output from isolators
configured by an operator. Conflicting values cause settings by either the user or an isolator
to be silently dropped. The first case would provide a bad UX, while the second one could
be a configuration bug or worse from incompatible isolators. Diagnosing either of these through
non-fatal log messages seems to do to little. As a user it would not be enough to just manually
make sure none of these non-fatal errors are reported in `stdout` (not even `stderr`) when
developing the task since an administrator might later on introduce introduce isolators making
use of already taken user variables which would now be be broken by the task's setup. As an
operator I wouldn't even be able to see these messages a user might not care about since they
are reported in the task's output, not e.g., the agent log.
> > 
> > It we'd just rejected tasks with conflicting env settings outright, the situation
would improve for both users and operators. If we'd go that route I believe it would make
sense to add duplicate detection to `validateEnvironment` and directly invoke it here.

I feel very tempted to agree with you here. However, given that the impact is rather harsh,
I would like to leave that up to the shepherd in spe.


- Till


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57763/#review169410
-----------------------------------------------------------


On March 20, 2017, 2:51 a.m., Till Toenshoff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57763/
> -----------------------------------------------------------
> 
> (Updated March 20, 2017, 2:51 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-7264
>     https://issues.apache.org/jira/browse/MESOS-7264
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/launch.cpp 8658525b00e78bed9227d6d400eccccae2cf25dd 
> 
> 
> Diff: https://reviews.apache.org/r/57763/diff/1/
> 
> 
> Testing
> -------
> 
> make check & functional testing...
> 
> ```
> ./src/mesos-execute --name="test" --env='{"key1":"value1"}' --command='sleep 1000' --master=127.0.0.1:5050
> ```
> 
> Within the contents of `stdout` before applying this:
> ```
> Overwriting environment variable 'key1', original: 'value1', new: 'value1'
> ```
> 
> After applying this there is no mention of the actually duplicate but equally valued
variable anymore. Note, this is before applying https://reviews.apache.org/r/57762.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message