Skip to content

bootstrap: enable https for bootstrap curl using wolfssl#11287

Closed
mcmtroffaes wants to merge 1 commit intoNixOS:stagingfrom
mcmtroffaes:feature/curl-bootstrap-https
Closed

bootstrap: enable https for bootstrap curl using wolfssl#11287
mcmtroffaes wants to merge 1 commit intoNixOS:stagingfrom
mcmtroffaes:feature/curl-bootstrap-https

Conversation

@mcmtroffaes
Copy link
Contributor

This was spurred by NixOS/hydra#257 ; also see #11221.

I used wolfssl for this patch, because it is a lot smaller than openssl so perhaps more suitable for a bootstrapping environment (only 300kb as opposed to about 3Mb for openssl), but if openssl (or libressl) is more desirable then it is fairly easy to change the patch accordingly. I've added myself as a maintainer of the wolfssl package (of course I'm happy to pass it on to someone else too).

Tested with:

NIX_PATH=nixpkgs=/path/to/my/local/nixpkgs/ nix-build /path/to/my/local/nixpkgs/pkgs/stdenv/linux/make-bootstrap-tools.nix -A test

(Note that this takes a very long time to complete, a few hours on my machine.)

I've also successfully downloaded files over https with the new curl.

A note about the changes to unpack-bootstrap-tools.sh: for the test to complete, this script has to work on both the old bootstrap package as well as on the new bootstrap package. The old one does not have the wolfssl shared library, so there's an extra check to ensure that patchelf isn't run on a file that isn't present even if it is in the list.

cc @vcunat @peti

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @edolstra, @ambrop72 and @vcunat to be potential reviewers

@nbp nbp added 0.kind: enhancement Add something new or improve an existing system. 1.severity: mass-rebuild 8.has: package (new) This PR adds a new package 8.has: package (update) This PR updates a package to a newer version labels Nov 26, 2015
@vcunat
Copy link
Member

vcunat commented Nov 27, 2015

/cc @wkennington because of WIP with a different approach.

@vcunat
Copy link
Member

vcunat commented Nov 27, 2015

Is the commit adding wolfssl of independent interest as well? That one could be picked to master and even 15.09 immediately.

I do like the general approach of using a lighter version during bootstrapping.

@mcmtroffaes
Copy link
Contributor Author

@wkennington: Apologies - I wasn't aware of another approach being in the works - as I ran into this curl https issue, it seemed like a nice little exercise for me to get to know the internals of nix a bit better.

@vcunat: Yes, the wolfssl commit could be cherrypicked into master. Do you want me to set up a separate pull request for it?

@mcmtroffaes
Copy link
Contributor Author

I now found the work of @wkennington on this. Bits of #8081 could be reused to improve this pull request. For example, the ca certificates probably needs to be copied into the bootstrap bundle - not sure why my test worked without doing so (maybe because I was testing from fedora?).

Thoughts, @wkennington?

vcunat pushed a commit that referenced this pull request Nov 27, 2015
vcunat pushed a commit that referenced this pull request Nov 27, 2015
Picked from #11287.

(cherry picked from commit b5e06b0)
@vcunat
Copy link
Member

vcunat commented Nov 27, 2015

I pushed the new package.

@mcmtroffaes
Copy link
Contributor Author

Thanks @vcunat

@mcmtroffaes mcmtroffaes force-pushed the feature/curl-bootstrap-https branch from f46e195 to 7d1be85 Compare December 17, 2015 15:31
@mcmtroffaes
Copy link
Contributor Author

I've rebased this onto current staging (which already picked up the wolfssl patch previously included in this pull request).

@zimbatm
Copy link
Member

zimbatm commented Feb 26, 2016

I know it's been a long time. Would you mind making another rebase ?

@mcmtroffaes mcmtroffaes force-pushed the feature/curl-bootstrap-https branch from 7d1be85 to 1b4d5e7 Compare February 27, 2016 09:53
@mcmtroffaes
Copy link
Contributor Author

Yes, it was no longer merging without conflicts, that's now resolved. A fresh rebase pushed against the latest upstream. I have not tested after rebase as I don't have the proper resources at this moment, I will be able to test it on Monday.

I was hoping for some feedback from @wkennington on what else is needed from #8081, in particular whether or not a similar patch needs to be applied to pkgs/stdenv/linux/make-bootstrap-tools-cross.nix. If so, then as the two files duplicate a lot, maybe we can also think of ways to reduce that duplication.

Finally, #8081 has another nice feature: copying the required libraries by inspecting the binaries directly. I think that can be handled and discussed in a separate pull request as it does not pertain to https support for curl specifically.

@globin
Copy link
Member

globin commented Feb 27, 2016

We shouldn't enable SSLv3 support!

@vcunat
Copy link
Member

vcunat commented Feb 27, 2016

  • cross: I think we can leave cross-compiling bootstrap without https support for now. The duplication might be good to reduce, but let this PR not depend on that. WKennington has stopped contributing here and rolls his own fork; I don't expect him to react much anymore :-(
  • SSLv3: we only do it for fetchurl which already knows the hashes of the results, and it's only for a few packages used during bootstrapping, so I think such details don't matter at all.

@edolstra
Copy link
Member

I would rather use Nix's builtin fetchurl function. That would allow us to get rid of curl in the bootstrap altogether. I'll see if I can get that to work in the next few days.

@vcunat
Copy link
Member

vcunat commented Feb 27, 2016

OK, closing this then.

@vcunat vcunat closed this Feb 27, 2016
@mcmtroffaes
Copy link
Contributor Author

Looking forward to seeing the improved fetchurl!

For history's sake, just noting here that upstream curl has been patched in the mean time to compile against a vanilla version of wolfssl, that is without SSLv3 support and in fact also without get_peer_certificate:

curl/curl@1ff3a07#diff-d7cd75d19525a9177c7b19826b3cb84a

curl/curl@151da51#diff-d7cd75d19525a9177c7b19826b3cb84a

That will make it easier to use curl with wolfssl if ever desired in the future.

@mcmtroffaes mcmtroffaes deleted the feature/curl-bootstrap-https branch April 26, 2016 08:39
adrianpk added a commit to adrianpk/nixpkgs that referenced this pull request May 31, 2024
Picked from NixOS#11287.

(cherry picked from commit b5e06b0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0.kind: enhancement Add something new or improve an existing system. 8.has: package (new) This PR adds a new package 8.has: package (update) This PR updates a package to a newer version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants