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

Use patchelf 0.9 + Nathaniel's fixes #44

Merged
merged 4 commits into from
Apr 3, 2016
Merged

Conversation

ogrisel
Copy link
Contributor

@ogrisel ogrisel commented Apr 2, 2016

As soon as @matthew-brett's test show that @njsmith's fixes to patchelf are safe, I think we can merge those to the official manylinux docker image.

pypa/auditwheel#25 (comment)

(cd patchelf-0.9 && ./configure && make && make install)
rm -rf patchelf-0.9.tar.gz patchelf-0.9
# Install patchelf and auditwheel (latest with unreleased bug fixes)
curl -sLO http://nipy.bic.berkeley.edu/manylinux/patchelf-0.9njs2.tar.gz
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now that I think about it, it's kinda not-so-great that we're installing this directly over a plain unencrypted http link to a random server. It means that if nipy.bic.berkeley.edu gets 0wned or MITMed, then some large proportion of the internet gets 0wned too (basically everyone using pip on linux). We can't help having a somewhat broad trust footprint (github, travis, docker hub for our base image, fedora, python.org, pypi for auditwheel, the TLS certificate system), but we should still try to keep it down if we can :-).

Maybe the best strategy would be to check the hash, like we do with openssl?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably once this goes into master on patchelf, we can just pull from github?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly no - the bootstrap autoconf stuff doesn't run on our image, so I had to run this on a more modern machine and then upload the tarball.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not really a problem. We could just compile and install a newer version of autoconf on the image.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure - or use the patched tarball via https until NixOS does a new release.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checking the hash like we do with openssl would be even more secure than https -- in addition to protecting against attacks on the data in transit, it also protects against someone compromising nipy.bic.berkeley.edu.

it's always hard to judge how seriously to take these kinds of security risks -- I guess anyone who compromises nipy.bic.berkeley.edu can probably add trojans to all kinds of fun stuff that @matthew-brett builds and then distributes all over the place :-). But I'm a little nervous about this docker image in particular, because srsly anyone who can sneak code into here can pull of a pretty apocalyptic security breach -- there are a lot of linux systems in this world that use pip install, and probably all of them are going to effectively end up running arbitrary code picked by anyone who compromises this docker image. So taking some simple precautions to reduce our exposure seems like reasonable prudence :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed if someone finds a way to rootkit our manylinux1 gcc or patchelf binaries the consequences could be dramatic. I think we need to think about hashing all the files of that image and keep an offline copy somewhere and setup a process to audit periodically the public manylinux1 images from quay.io so as to be able to detect compromised binaries.

PyPI is similarly a potential target and there is a draft PEP to devise a more robust security model for it: https://www.python.org/dev/peps/pep-0480/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upper in the same script there is another unsafe http URL:

http://people.centos.org/tru/devtools-2/devtools-2.repo

This one does not have an HTTPS counterpart.

@ogrisel
Copy link
Contributor Author

ogrisel commented Apr 3, 2016

I am working on adding the sha256 checks for all curl'ed files of this script. Expect an update to this PR soon.

@ogrisel
Copy link
Contributor Author

ogrisel commented Apr 3, 2016

This is looking good. All the sha256 checks passed. Travis is now building all the Pythons.

I also tried locally to change the reference hash values to verify that the build script properly stops in case of bad hash. It works as expected.

@ogrisel
Copy link
Contributor Author

ogrisel commented Apr 3, 2016

@njsmith merge?

@njsmith
Copy link
Member

njsmith commented Apr 3, 2016

Thanks!

@njsmith njsmith merged commit 9113539 into pypa:master Apr 3, 2016
@rmcgibbo
Copy link
Member

rmcgibbo commented Apr 3, 2016

I just cut a release of auditwheel and uploaded to pypi too.

@ogrisel
Copy link
Contributor Author

ogrisel commented Apr 3, 2016

Thanks @rmcgibbo!

njsmith added a commit to njsmith/manylinux that referenced this pull request Apr 3, 2016
Rebuilding the docker images to pick up auditwheel 1.3.0 should complete
the changes needed to fix the problems @matthew-brett found with
vendoring libgfortran (see: pypa/auditwheel#24, pypa/auditwheel#25, pypa#43,
 pypa#44, etc.).
@njsmith
Copy link
Member

njsmith commented Apr 3, 2016

And #47 should finish this off. 🎆 thanks everyone, this was a lot more complicated than I expected :-)

@ogrisel
Copy link
Contributor Author

ogrisel commented Apr 3, 2016

this was a lot more complicated than I expected :-)

But I was amazed by the speed of problem detection, diagnostic and solution of the team. It was a great experience to take part in this. I expect that @matthew-brett will start uploading manylinux1 wheels for the scipy stack in the coming days. I just posted a message on the scikit-learn mailing list pointing theo @matthew-brett's post on the numpy ML to tell people to test the wheels.

@matthew-brett
Copy link
Contributor

Yes, I agree - this was a great team effort, enjoyable, informative, productive. Thanks to y'all for your fine work.

@njsmith
Copy link
Member

njsmith commented Apr 7, 2016

effective teamwork is truly awesome yeah :-)

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

Successfully merging this pull request may close these issues.

None yet

4 participants