-
-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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
gnutls: respect NIX_SSL_CERT_FILE, remove 3.5.10 on darwin #58611
Conversation
You missed #58513, right? |
@LnL7: what's your thought on this? Any arguments against using the other approach? (not marked as WIP) |
It might not be warranted but I still have similar concerns as @copumpkin on d6454e6. This functionality won't work on nixos either ( I marked it wip because I only tested the bootstrap build, not the full stdenv yet. |
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. |
That's my point, |
Then I believe you misunderstood the purpose of the /**
* 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);
} |
BTW, our gnutls currently does not have a patch to honor lib.optional stdenv.isLinux "--with-default-trust-store-file=/etc/ssl/certs/ca-certificates.crt" |
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 |
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. |
Rebased (atop current Hydra's staging-next so testers have some binaries):
|
I implemented the |
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.
Thanks! That seems to work as expected, I also updated the patch to be an actual revert of the upstream commit for clarity. |
OK. On 3.6.7 |
@GrahamcOfBorg eval (a problem was fixed on staging) |
... and remove 3.5.10 on darwin (into staging branch)
Closely related: #61179 |
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
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)