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

Whitelist environment variables to be shown on error #466

Closed
The-Compiler opened this Issue Mar 1, 2017 · 7 comments

Comments

Projects
2 participants
@The-Compiler
Contributor

The-Compiler commented Mar 1, 2017

When installing dependencies failed, tox shows the whole environment:

ERROR: invocation failed (exit code 1), logfile: /home/florian/proj/qutebrowser/git/.tox/fail/log/fail-1.log
ERROR: actionid: fail
msg: getenv
cmdargs: ['/home/florian/proj/qutebrowser/git/.tox/fail/bin/pip', 'install', 'afiojafsioajfaisjf']
env: {'LANG': 'en_US.utf8', 'DISPLAY': ':0', 'SHLVL': '1', 'LOGNAME': 'florian', 'XDG_VTNR': '1', 'INVOCATION_ID': 'f2f60c8361c1410aba3e990a0eed88a5', 'GIT_PAGER': 'less +g', 'PWD': '/home/florian/proj/qutebrowser/git', 'HG': '/usr/bin/hg', 'MOZ_PLUGIN_PATH': '/usr/lib/mozilla/plugins', 'LANGUAGE': 'en_US.utf8', 'TERMINAL': 'termite', 'QT_QPA_PLATFORMTHEME': 'gtk2', 'JOURNAL_STREAM': '8:16050', 'COLORTERM': 'truecolor', 'XDG_SESSION_ID': 'c1', 'CCACHE_PREFIX': '', 'DESKTOP_SESSION': 'herbstluftwm-locked', 'XDG_RUNTIME_DIR': '/run/user/1000', 'XAUTHORITY': '/home/florian/.Xauthority', 'LC_MEASUREMENT': 'en_GB.UTF-8', 'LC_PAPER': 'en_GB.UTF-8', 'DBUS_SESSION_BUS_ADDRESS': 'unix:path=/run/user/1000/bus', 'VTE_VERSION': '4601', 'MAIL': '/var/spool/mail/florian', 'HLWM_LOADED': '1', 'CCACHE_COMPRESS': '1', 'CCACHE_BASEDIR': '/home/florian', 'SHELL': '/usr/bin/zsh', 'LC_TIME': 'en_GB.UTF-8', 'WINDOWID': '25165827', 'TERM': 'xterm-termite', 'GTK_MODULES': 'canberra-gtk-module', 'SUDO_PROMPT': '[sudo] password for %u@%h (-> %U): ', 'EDITOR': 'emacs', 'LC_COLLATE': 'C', 'HOME': '/home/florian', 'BROWSER': 'qutebrowser', 'VISUAL': 'emacs', 'XDG_SEAT': 'seat0', 'VIEW_PDF': 'zathura', 'USER': 'florian', 'PATH': '/home/florian/proj/qutebrowser/git/.tox/fail/bin:/usr/share/perl5/vendor_perl/auto/share/dist/Cope:/usr/lib/ccache/bin/:/usr/lib/hardening-wrapper/bin:/usr/local/sbin:/usr/local/bin:/usr/bin:/usr/lib/jvm/default/bin:/usr/bin/site_perl:/usr/bin/vendor_perl:/usr/bin/core_perl:/home/florian/bin:/home/florian/bin/go/bin', 'OLDPWD': '/home/florian', 'LS_COLORS': 'rs=0:di=01;34:ln=01;36:mh=00:pi=40;33:so=01;35:do=01;35:bd=40;33;01:cd=40;33;01:or=40;31;01:mi=00:su=37;41:sg=30;43:ca=30;41:tw=30;42:ow=34;42:st=37;44:ex=01;32:*.tar=01;31:*.tgz=01;31:*.arc=01;31:*.arj=01;31:*.taz=01;31:*.lha=01;31:*.lz4=01;31:*.lzh=01;31:*.lzma=01;31:*.tlz=01;31:*.txz=01;31:*.tzo=01;31:*.t7z=01;31:*.zip=01;31:*.z=01;31:*.Z=01;31:*.dz=01;31:*.gz=01;31:*.lrz=01;31:*.lz=01;31:*.lzo=01;31:*.xz=01;31:*.zst=01;31:*.tzst=01;31:*.bz2=01;31:*.bz=01;31:*.tbz=01;31:*.tbz2=01;31:*.tz=01;31:*.deb=01;31:*.rpm=01;31:*.jar=01;31:*.war=01;31:*.ear=01;31:*.sar=01;31:*.rar=01;31:*.alz=01;31:*.ace=01;31:*.zoo=01;31:*.cpio=01;31:*.7z=01;31:*.rz=01;31:*.cab=01;31:*.jpg=01;35:*.jpeg=01;35:*.mjpg=01;35:*.mjpeg=01;35:*.gif=01;35:*.bmp=01;35:*.pbm=01;35:*.pgm=01;35:*.ppm=01;35:*.tga=01;35:*.xbm=01;35:*.xpm=01;35:*.tif=01;35:*.tiff=01;35:*.png=01;35:*.svg=01;35:*.svgz=01;35:*.mng=01;35:*.pcx=01;35:*.mov=01;35:*.mpg=01;35:*.mpeg=01;35:*.m2v=01;35:*.mkv=01;35:*.webm=01;35:*.ogm=01;35:*.mp4=01;35:*.m4v=01;35:*.mp4v=01;35:*.vob=01;35:*.qt=01;35:*.nuv=01;35:*.wmv=01;35:*.asf=01;35:*.rm=01;35:*.rmvb=01;35:*.flc=01;35:*.avi=01;35:*.fli=01;35:*.flv=01;35:*.gl=01;35:*.dl=01;35:*.xcf=01;35:*.xwd=01;35:*.yuv=01;35:*.cgm=01;35:*.emf=01;35:*.ogv=01;35:*.ogx=01;35:*.aac=00;36:*.au=00;36:*.flac=00;36:*.m4a=00;36:*.mid=00;36:*.midi=00;36:*.mka=00;36:*.mp3=00;36:*.mpc=00;36:*.ogg=00;36:*.ra=00;36:*.wav=00;36:*.oga=00;36:*.opus=00;36:*.spx=00;36:*.xspf=00;36:', 'LESS': '-R -M +g', 'LESS_TERMCAP_me': '\x1b[0m', 'LESS_TERMCAP_se': '\x1b[0m', 'LESS_TERMCAP_ue': '\x1b[0m', 'LESS_TERMCAP_mb': '\x1b[1;32m', 'LESS_TERMCAP_md': '\x1b[1;34m', 'LESS_TERMCAP_us': '\x1b[1;32m', 'LESS_TERMCAP_so': '\x1b[1;44;1m', 'VIRTUAL_ENV_DISABLE_PROMPT': '1', 'LC_MESSAGES': 'en_US.utf8', '_': '/usr/bin/tox', 'QT_QPA_PLATFORM_PLUGIN_PATH': '/home/florian/proj/qutebrowser/git/.tox/fail/Lib/site-packages/PyQt5/plugins/platforms', 'PYTEST_QT_API': 'pyqt5', 'PYTHONHASHSEED': '4060969334', 'VIRTUAL_ENV': '/home/florian/proj/qutebrowser/git/.tox/fail'}

apart from being a lot of visual noise, this can leak things like passwords or API keys (i.e. secure environment variables) on CI systems like Travis.

Seems like that happened here: https://github.com/GoogleCloudPlatform/google-auth-library-python/pull/102/files#diff-a09db41a73f7fd39327cf6a25e928626

Instead, I think there should be a configurable whitelist of what to show, being something like PATH, LANG, LC_*, PYTHON* by default.

@obestwalter

This comment has been minimized.

Member

obestwalter commented Mar 6, 2017

Hi @The-Compiler - I also think it is definitely necessary to change this behaviour. I would even shoot for the next release.

From a user view who suffers from severe featuritis I would wish for the following:

  1. A set of standard env vars that are always shown, because they pose no security risk
  2. A possibility to switch showing of env vars fully on and fully off
  3. A configuration option to whitelist specific env vars

From developer point of view, who might have to implement and maintain this I would think:

  1. would need knowledge about which env vars are relevant and risk free on which OSs. There is also high bikeshedding probability regarding discussions about which env vars should be included and which not, so I would immediately scrap this idea and say that this is not worth it.
  2. Looks easy to implement and easy to understand from a user point of view. Default would be fully off and if I need extra debug output I could switch it fully on. Seems feasible, but then if all builds are happening in the open, I might want the possivbility to add a black list to explicitly filter sensitive vars. This would complicate things again. Would I want a blacklist and a white list? How would they co exist? Lot's of questions.
  3. also seems easy to implement and although it poses the most work on the user it is also the safest.

So I would also opt for adding the suggested white list. I might send a PR soon if no one else shouts that they want to implement it.

@The-Compiler

This comment has been minimized.

Contributor

The-Compiler commented Mar 6, 2017

Another thing to consider is if we still want to show the environment variable names for the ones which are not whitelisted. It makes it clear those are still set (otherwise the output might be confusing), but hides the values.

@The-Compiler

This comment has been minimized.

Contributor

The-Compiler commented Apr 12, 2017

This also seems to affect devpi (via --result-json?) which then leaks all environment variables with the world (e.g. here).

I've had people refuse to run devpi test for me because they didn't want to share their environment, and people ask me to delete their results at pytest/tox/devpi trainings I gave because it shared too much from their work machines.

@obestwalter obestwalter added this to issues in [released] 3.0.x Sep 4, 2017

@obestwalter obestwalter self-assigned this Sep 7, 2017

obestwalter added a commit to Avira/tox that referenced this issue Sep 10, 2017

obestwalter added a commit to Avira/tox that referenced this issue Sep 10, 2017

@obestwalter

This comment has been minimized.

Member

obestwalter commented Sep 10, 2017

I completely overlooked that one ... should have been fixed long ago :(

I created a very simple patch to just not leak that info, release that as 2.8.2 and integrate the fix also into master.

obestwalter added a commit to Avira/tox that referenced this issue Sep 10, 2017

obestwalter added a commit to Avira/tox that referenced this issue Sep 10, 2017

obestwalter added a commit that referenced this issue Sep 10, 2017

obestwalter added a commit to Avira/tox that referenced this issue Sep 10, 2017

@obestwalter

This comment has been minimized.

Member

obestwalter commented Sep 10, 2017

Released 2.8.2 containing a simple fix to just not print env vars in that case.

@obestwalter

This comment has been minimized.

Member

obestwalter commented Sep 11, 2017

I would even tend to close this as wontfix then, as I don't really see the value in printing out env vars at all, now that I had a closer look.

@obestwalter

This comment has been minimized.

Member

obestwalter commented Sep 15, 2017

Closing this now. The actual problem (leaking information) was solved. If anyone wants to pick this up and reintroduce controlled environment variable reporting in certain stages of the testrun, this could be addressed in a plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment