-
Notifications
You must be signed in to change notification settings - Fork 219
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
Conversation
(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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
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.
I am working on adding the sha256 checks for all curl'ed files of this script. Expect an update to this PR soon. |
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. |
@njsmith merge? |
Thanks! |
I just cut a release of auditwheel and uploaded to pypi too. |
Thanks @rmcgibbo! |
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.).
And #47 should finish this off. 🎆 thanks everyone, 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. |
Yes, I agree - this was a great team effort, enjoyable, informative, productive. Thanks to y'all for your fine work. |
effective teamwork is truly awesome yeah :-) |
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)