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

gnutls: set certificate location #8121

Closed
wants to merge 1 commit into from
Closed

Conversation

vcunat
Copy link
Member

@vcunat vcunat commented Jun 1, 2015

This is a fast fix; it might be best to use $SSL_CERT_FILE. Tested on vlc with youtube https URLs. Discussed also on #8118.

Any better ideas?

This is a fast fix; it might be best to use $SSL_CERT_FILE.
Tested on vlc with youtube https URLs.
Discussed also on NixOS#8118.
@vcunat vcunat added 0.kind: enhancement Add something new 0.kind: question Requests for a specific question to be answered 1.severity: mass-rebuild This PR causes a large number of packages to rebuild labels Jun 1, 2015
@vcunat vcunat added this to the 15.06 milestone Jun 1, 2015
@vcunat vcunat mentioned this pull request Jun 1, 2015
@vcunat
Copy link
Member Author

vcunat commented Jun 1, 2015

Forgot to link upstream docs.

@vcunat vcunat self-assigned this Jun 1, 2015
@nckx
Copy link
Member

nckx commented Jun 1, 2015

👍

(I guess $SSL_CERT_FILE would be... cleaner? Haven't thought through all the implications, though.)

@vcunat
Copy link
Member Author

vcunat commented Jun 1, 2015

I didn't want to poke now through the source and search what to patch, so I just did this quick fix. We use $SSL_CERT_FILE in openssl and curl, for example, so I guess the (security) implications of that were considered already.

@vcunat
Copy link
Member Author

vcunat commented Jun 2, 2015

/cc maintainers @edolstra, @wkennington.

@wkennington
Copy link
Contributor

I would prefer this to use the cacert package directly so we can guarantee this works on non-nixos systems. Ideally it wouldn't need to depend on the hash of the cacert package so that we could update cacerts without triggering a rebuild.

@vcunat
Copy link
Member Author

vcunat commented Jun 2, 2015

Is gnutls case different from openssl and other cert users? I would expect same or similar handling.

@wkennington
Copy link
Contributor

They should be doing this as well. Again, the biggest downside is that
cacert updates will now trigger mass rebuilds if you add this kind of
change to something like openssl (nss already handles this internally and
it's the source of cacerts).

Maybe it would be useful to move the current cacert implementation to
something called cacert-generator, and make all ssl libraries depend on a
static tarball created from this generator like how bootstrap tools works.
Then, have a test case which fails if the generated files are different
from the tarballd ones.

On Tue, Jun 2, 2015, 11:25 Vladimír Čunát [email protected] wrote:

Is gnutls case different from openssl and other cert users? I would expect
same or similar handling.


Reply to this email directly or view it on GitHub
#8121 (comment).

@wkennington
Copy link
Contributor

The problem with the tarball approach is that very few people can update it
since there isn't an ideal spot for just any committer to put tarballs.
This leads to cacert chain stagnation. I would hope that by adding a
blocking test case we could at least light a fire so that a committer will
attempt to put it out.

On Tue, Jun 2, 2015, 11:34 William Kennington [email protected]
wrote:

They should be doing this as well. Again, the biggest downside is that
cacert updates will now trigger mass rebuilds if you add this kind of
change to something like openssl (nss already handles this internally and
it's the source of cacerts).

Maybe it would be useful to move the current cacert implementation to
something called cacert-generator, and make all ssl libraries depend on a
static tarball created from this generator like how bootstrap tools works.
Then, have a test case which fails if the generated files are different
from the tarballd ones.

On Tue, Jun 2, 2015, 11:25 Vladimír Čunát [email protected]
wrote:

Is gnutls case different from openssl and other cert users? I would
expect same or similar handling.


Reply to this email directly or view it on GitHub
#8121 (comment).

@edolstra
Copy link
Member

edolstra commented Jun 2, 2015

If we do this, we should probably do it for OpenSSL as well, for consistency.

The main argument against it is impurity - a non-chroot build could end up using certificates in /etc. But that's probably not very likely.

@wkennington Depending on the certificate bundle directly is tempting but not a good idea from a security perspective, because it makes it hard to revoke a certificate system-wide (you'd have to make sure that you have no package with an obsolete certificate bundle in its closure).

@wkennington
Copy link
Contributor

You have the same problem with a bug in an ssl implementation, you still
have to replace packages which depend on it. This applies to any
application with a security vulnerability. CA certificates hardly seem like
a special case. Users need to patch their software as with any security
update.

If you look at the frequency of updates, openssl alone is on par with the
number of ca updates. Should we provide openssl at the system level too to
ensure it gets updates?

These default certificates should be only used in the fallback case. The
NixOS system or any other system can override this setting with an
environment variable which we already use in nixos. You could even apply it
to every service and make it overridable.

This also doesn't fix the issue of a system rollback rolling back a ca
bundle.

On Tue, Jun 2, 2015, 13:14 Eelco Dolstra [email protected] wrote:

If we do this, we should probably do it for OpenSSL as well, for
consistency.

The main argument against it is impurity - a non-chroot build could end up
using certificates in /etc. But that's probably not very likely.

@wkennington https://github.com/wkennington Depending on the
certificate bundle directly is tempting but not a good idea from a security
perspective, because it makes it hard to revoke a certificate system-wide
(you'd have to make sure that you have no package with an obsolete
certificate bundle in its closure).


Reply to this email directly or view it on GitHub
#8121 (comment).

@vcunat
Copy link
Member Author

vcunat commented Jun 3, 2015

As for non-chrooted builds... in our master gnutls detects CA file location at configure time. On Hydra it's chrooted, so it finds nothing and uses a default path not populated on NixOS (/etc/ssl/cert.pem, if I read the script correctly). Therefore, I did this quick PR to remove the impurity and to make it work at least on NixOS (and some other distros using the same path, e.g. Ubuntu).

Do I understand correctly that the only point of improvement against current openssl state ($SSL_CERT_FILE) would be to better support running on non-nixos? It seems easiest to patch the implementations to e.g. try a few common /etc locations at runtime if that variable isn't present. I can't see a benefit in rebuilding because of a CA change.

@vcunat
Copy link
Member Author

vcunat commented Jun 5, 2015

Thinking of the upcoming release: what about just merging this and leaving openssl as it is. We can make larger changes later, as I think just stabilizing and testing what's committed among various 15.06-bound branches will need more than enough work until the end of the month.

vcunat added a commit that referenced this pull request Jun 9, 2015
This is a fast fix; it might be best to use $SSL_CERT_FILE.
Tested on vlc with youtube https URLs.
Discussed also on #8118. Feel free to discuss further improvements on #8247.
@vcunat
Copy link
Member Author

vcunat commented Jun 9, 2015

Pushed to staging now. Further discussion on #8247.

@vcunat vcunat closed this Jun 9, 2015
@vcunat vcunat deleted the p/gnutls-cert branch June 9, 2015 14:49
@edolstra
Copy link
Member

edolstra commented Jun 9, 2015

I'm not in favor of pushing this to staging if we don't also do it for OpenSSL.

@vcunat
Copy link
Member Author

vcunat commented Jun 9, 2015

IMHO openssl had and still has even a better support than gnutls (it uses the env var).

@@ -13,6 +13,8 @@ stdenv.mkDerivation rec {
inherit src patches;

configureFlags = [
# FIXME: perhaps use $SSL_CERT_FILE instead
"--with-default-trust-store-file=/etc/ssl/certs/ca-certificates.crt"
Copy link
Member

Choose a reason for hiding this comment

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

How does this affect Darwin? That path doesn't exist.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I guess the configure flag should only be added on Linux? Without it, configure probes a few options, which might give a correct result, as darwin builds is are not chrooted.

Copy link
Member

Choose a reason for hiding this comment

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

darwin builds is are not chrooted.

They are in the painfully long-lived awesome branch with the new stdenv 😭

Copy link
Member

Choose a reason for hiding this comment

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

But yeah, this should probably be conditional behind .isLinux

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed.

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 0.kind: question Requests for a specific question to be answered 1.severity: mass-rebuild This PR causes a large number of packages to rebuild
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants