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: use default system trust storage if no other CA is set #331

Closed
wants to merge 1 commit into from
Closed

gnutls: use default system trust storage if no other CA is set #331

wants to merge 1 commit into from

Conversation

dangowrt
Copy link

Signed-off-by: Daniel Golle daniel@makrotopia.org

@dangowrt
Copy link
Author

resolves #330

@dangowrt dangowrt changed the title gnutls: use default system trust storage if not other CA is set gnutls: use default system trust storage if no other CA is set Jun 29, 2015
dangowrt added a commit to openwrt/packages that referenced this pull request Jun 29, 2015
If no explicit CA file is given, gnurl fails to setup HTTPS connections
as it doesn't looks for certificates in /etc/ssl/certs/ in any way.
Fix that by utilizing GnuTLS' gnutls_certificate_set_x509_system_trust
as a fall-back if neither CA file, CA path nor SRP is declared.

Reported upstream: curl/curl#330
Fix suggested upstream: curl/curl#331

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
@ghedo
Copy link
Contributor

ghedo commented Jun 30, 2015

To be honest, I don't think patch is really needed, and someone may even not want to set any default CA on purpose, and this patch would make it impossible (while other TLS backends do allow this). But it's really not up to me to decide.

As for the implementation, instead of having that big if condition, I'd use a flag set to true by default, and set to false if the trust is set by any other mean. Something like this:

  int set_system_trust = 1;

  [...]

  if(data->set.ssl.CAfile) {
    [...]
    set_system_trust = 0;
  }

#ifdef HAS_CAPATH
  if(data->set.ssl.CApath) {
    [...]
    set_system_trust = 0;
  }
#endif

#ifdef HAS_SYSTEM_TRUST
  if (set_system_trust) {
    <set system trust here>
  }
#endif

This way you'd also avoid that #ifdef mess. Also, I think that the system trust should be enabled even if SRP is used.

@bagder
Copy link
Member

bagder commented Jul 28, 2015

With the fix already merged I rather abstain from accepting this as it changes the behavior slightly and I don't think it is needed now. Thoughts?

@bagder bagder added the TLS label Jul 28, 2015
@bagder bagder self-assigned this Jul 28, 2015
@dangowrt
Copy link
Author

I agree that this is no longer needed if up-to-date gnuTLS is being used. From my side, I'm fine with this patch it as in places where I needed it I by now updated gnuTLS and back-ported 5a1614c where ever curl is still used in older versions.

@bagder bagder closed this Sep 13, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

3 participants