Skip to content

Comments

gnutls: respect NIX_SSL_CERT_FILE, remove 3.5.10 on darwin#58611

Merged
vcunat merged 2 commits intoNixOS:stagingfrom
LnL7:darwin-gnutls
May 1, 2019
Merged

gnutls: respect NIX_SSL_CERT_FILE, remove 3.5.10 on darwin#58611
vcunat merged 2 commits intoNixOS:stagingfrom
LnL7:darwin-gnutls

Conversation

@LnL7
Copy link
Member

@LnL7 LnL7 commented Mar 31, 2019

Motivation for this change

Nix packages are expected to honor NIX_SSL_CERT_FILE and this removes the
dependency on the framework while bootstrapping the stdenv.

Fixes #58481

/cc @vcunat

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@GrahamcOfBorg GrahamcOfBorg added the 6.topic: darwin Running or building packages on Darwin label Mar 31, 2019
@vcunat
Copy link
Member

vcunat commented Mar 31, 2019

You missed #58513, right?

@vcunat
Copy link
Member

vcunat commented Apr 9, 2019

@LnL7: what's your thought on this? Any arguments against using the other approach? (not marked as WIP)

@LnL7
Copy link
Member Author

LnL7 commented Apr 9, 2019

It might not be warranted but I still have similar concerns as @copumpkin on d6454e6. This functionality won't work on nixos either (/etc/ssl is readonly) and introducing frameworks could have some undesirable side effects or issues like NixOS/nix#2523.

I marked it wip because I only tested the bootstrap build, not the full stdenv yet.

@vcunat
Copy link
Member

vcunat commented Apr 10, 2019

This functionality won't work on nixos either (/etc/ssl is readonly)

That contradicts my current understanding. "At work" we do successfully use gnutls system trust store on NixOS (in knot-resolver) for verifying TLS servers. I'm not aware of any functionality writing the system trust list (there might be), but reading it seems far more important anyway.

@LnL7
Copy link
Member Author

LnL7 commented Apr 10, 2019

That's my point, add_system_trust is probably not used anywhere. And if it is it's probably not an important feature that will fail.

@vcunat
Copy link
Member

vcunat commented Apr 10, 2019

Then I believe you misunderstood the purpose of the add_system_trust() function; it's precisely what we use.

/**
 * gnutls_x509_trust_list_add_system_trust:
 * @list: The structure of the list
 * @tl_flags: GNUTLS_TL_*
 * @tl_vflags: gnutls_certificate_verify_flags if flags specifies GNUTLS_TL_VERIFY_CRL
 *
 * This function adds the system's default trusted certificate
 * authorities to the trusted list. Note that on unsupported systems
 * this function returns %GNUTLS_E_UNIMPLEMENTED_FEATURE.
 *
 * This function implies the flag %GNUTLS_TL_NO_DUPLICATES.
 *
 * Returns: The number of added elements or a negative error code on error.
 *
 * Since: 3.1
 **/
int
gnutls_x509_trust_list_add_system_trust(gnutls_x509_trust_list_t list,
					unsigned int tl_flags,
					unsigned int tl_vflags)
{
	return add_system_trust(list, tl_flags|GNUTLS_TL_NO_DUPLICATES, tl_vflags);
}

@vcunat
Copy link
Member

vcunat commented Apr 10, 2019

BTW, our gnutls currently does not have a patch to honor NIX_SSL_CERT_FILE :-) The only thing it has is:

lib.optional stdenv.isLinux "--with-default-trust-store-file=/etc/ssl/certs/ca-certificates.crt"

@vcunat
Copy link
Member

vcunat commented Apr 10, 2019

So I guess your approach may work if we supplement it by honoring that variable – I suppose that's how other SSL stuff works on nixpkgs darwin? (I can't really judge the risk in using the system libraries for this. My NixOS doesn't define the variable, so I suppose the /etc/ssl fallback needs to be there anyway.)

@LnL7
Copy link
Member Author

LnL7 commented Apr 10, 2019

I have no idea how I came to the conclusion that it was mutating the trust store. While this patch is basically a revert of c0eb46d3463cd21b3f822ac377ff37f067f66b8d on 3.6.x, implementing NIX_SSL_CERT_FILE instead is probably the way to go then.

@vcunat
Copy link
Member

vcunat commented Apr 22, 2019

Rebased (atop current Hydra's staging-next so testers have some binaries):

  • fix trivial conflict with 3.6.6 -> 3.6.7
  • don't apply the patch on non-Darwin (forces autotools), so it now builds for me on Linux

@vcunat
Copy link
Member

vcunat commented Apr 22, 2019

I implemented the $NIX_SSL_CERT_FILE patch. I'm quite confident it will all be OK, but I can't test Darwin. I did test it overrides the store on x86_64-linux (in my case it's useful for tests running in nix builds).
@GrahamcOfBorg build gnutls

@vcunat vcunat changed the title [WIP] gnutls: remove 3.5.10 on darwin gnutls: respect NIX_SSL_CERT_FILE, remove 3.5.10 on darwin Apr 22, 2019
LnL7 and others added 2 commits April 22, 2019 16:43
Nix packages are expected to honor NIX_SSL_CERT_FILE and this removes the
dependency on the framework while bootstrapping the stdenv.
(+ nitpick changes from vcunat)

The patch is based on https://gitlab.com/gnutls/gnutls/commit/c0eb46d3463cd21b3f822ac377ff37f067f66b8d
The patch should work fine, regardless of the Darwin patch being applied.
@LnL7
Copy link
Member Author

LnL7 commented Apr 22, 2019

Thanks! That seems to work as expected, I also updated the patch to be an actual revert of the upstream commit for clarity.

@vcunat
Copy link
Member

vcunat commented Apr 22, 2019

OK. On 3.6.7 grep 'CoreFoundation/\|Security/' only finds this file, some m4+configure stuff and ./gl/tests/*, so it will probably be OK, if it builds for you.

@grahamc
Copy link
Member

grahamc commented Apr 22, 2019

@GrahamcOfBorg eval

(a problem was fixed on staging)

@ofborg ofborg bot requested review from edolstra, fpletz and lovek323 April 22, 2019 20:19
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 501-1000 This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Apr 22, 2019
@vcunat vcunat merged commit 39c2b64 into NixOS:staging May 1, 2019
vcunat added a commit that referenced this pull request May 1, 2019
... and remove 3.5.10 on darwin (into staging branch)
@LnL7 LnL7 deleted the darwin-gnutls branch May 1, 2019 21:12
@vcunat
Copy link
Member

vcunat commented May 9, 2019

Closely related: #61179

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

Labels

6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 501-1000 This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants