mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 57763: Fixed environment overwrite warning to not leak possibly sensitive data.
Date Mon, 20 Mar 2017 11:43:38 GMT

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



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.

- Benjamin Bannier


On March 20, 2017, 3: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, 3: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