Skip to content
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

Upgrading securedrop-app-code should remove unused Python dependencies #856

Closed
garrettr opened this issue Jan 29, 2015 · 9 comments · Fixed by #5487
Closed

Upgrading securedrop-app-code should remove unused Python dependencies #856

garrettr opened this issue Jan 29, 2015 · 9 comments · Fixed by #5487

Comments

@garrettr
Copy link
Contributor

The postinst script of securedrop-app-code upgrades the Python dependencies with the following command:

pip install --no-index --find-links=/var/securedrop/wheelhouse -r /var/www/securedrop/requirements/securedrop-requirements.txt

The only downside here is that it does not automatically remove unused dependencies. For example, when I upgraded the Python dependencies with pip_update.sh in 9868f3a, argparse and wsgiref were no longer being used and were automatically removed from the requirements file by pip-dump. However, upgrading the package on the server (which runs the above pip install) does not trigger their removal.

@garrettr garrettr added this to the 0.4 milestone Jan 29, 2015
@garrettr
Copy link
Contributor Author

We should revisit this the next time we have to upgrade the Python dependencies the securedrop-app-code .deb package.

@garrettr
Copy link
Contributor Author

Just discussed this with @conorsch. We started working on this because of #856 (comment), and @fowlslegs has a good start towards a resolution in #1302. However, that solution makes me a little nervous and would require significant testing prior to release. Furthermore, this issue is not high priority. Since the outdated dependencies are no longer used by the application, I do not think they pose a significant security risk.

As a result, I am removing this from the 0.3.7 milestone. We should revisit it again in the near future.

@garrettr garrettr removed this from the 0.3.7 milestone May 23, 2016
@garrettr
Copy link
Contributor Author

@fowlslegs and I discussed this in person. We agreed that it is difficult to do this well because SecureDrop's Python dependencies are installed into the system-wide pool of Python packages, which makes it hard to disambiguate SecureDrop's Python packages from other Python packages.

The "right answer" is to properly isolate SecureDrop's deployment environment. There are numerous techniques for doing this, and doing so would be a significant change in the current deployment stack; therefore, they should be evaluated carefully and considered for a later release.

@garrettr
Copy link
Contributor Author

In general, pip-sync seems like a good tool to use here. The only thing preventing us from just dropping it in is:

SecureDrop's Python dependencies are installed into the system-wide pool of Python packages, which makes it hard to disambiguate SecureDrop's Python packages from other Python packages.

from #856 (comment). If we isolate those environments, then it should be straightforward to use pip-sync to keep the installed dependencies in sync with the requirements files.

@psivesely
Copy link
Contributor

SecureDrop's Python dependencies are installed into the system-wide pool of Python packages, which makes it hard to disambiguate SecureDrop's Python packages from other Python packages.

Actually, this seems not to be the case. Since SecureDrop installs it's Python packages using pip in the postinst script, they are installed to /usr/local/lib/python2.7/dist-packages/. This is opposed to the way that most Debian packages install Python packages, which is by installing the files to /usr/lib/python2.7/dist-packages/ with apt and then compiling the Python bytecode with pycompile like so:

if which pycompile >/dev/null 2>&1; then
  pycompile -p python-six
fi

I could not find any other examples of Debian packages providing Python packages that use pip like we do, which is sensible because then there is no requirement for pip, a developer's tool, just to install libraries for some application. I also confirmed in a VM that all files under /usr/local/lib/python2.7/dist-packages were installed by securedrop-app-code via pip. I think it makes sense that we begin to follow this example and eliminate the dependency of pip (at least in production). More discussion on how we can (and if we should) leverage the Replaces field in the Debian package control file should follow, but I'd need to investigate this more first.

In my opinion, installing neither pip, nor pip-tools, greatly simplifies things and is the preferable solution.

@psivesely
Copy link
Contributor

#1472, if done correctly, will prevent the problem of deprecated dependency buildup in case of future dependency turnover, however, it doesn't address past buildup. I think it'd be safe to blow away the contents of /usr/local/lib/python2.7/dist-packages/. What I could do is provision a VM exactly as normal, except only install the dependencies of securedrop-app-code instead of the package itself. If there's nothing in /usr/local/lib/python2.7/dist-packages/ as I suspect, we should be fine rm -rfing the dir in the postinst, since we'll have already built and installed the current SD Python dependencies in /usr/lib/python2.7/dist-packages.

@eloquence
Copy link
Member

Note that the dependency management story has significantly changed with the introduction of dh-virtualenv in #4622; however, this may still be a valid issue.

@eloquence
Copy link
Member

@rmol will do a quick test to check whether this is still an issue (likely not with current implementation) and close if not.

@rmol
Copy link
Contributor

rmol commented Sep 4, 2020

The current packaging does result in dropped dependencies being deleted from the production virtualenv, except for their .pyc files.

We could start using the mod_wsgi configuration directive WSGIDontWriteBytecode, or conversely, ensure the package contains compiled Python code, but I think a cruder, more thorough, and retroactive solution would be adding a function in postinst to delete all byte-compiled files in the venv.

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

Successfully merging a pull request may close this issue.

5 participants