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

Call SSL_CTX_load_verify_locations by default when initializing an SSL_CTX #632

Closed
reaperhulk opened this issue Jun 7, 2017 · 6 comments · Fixed by #633
Closed

Call SSL_CTX_load_verify_locations by default when initializing an SSL_CTX #632

reaperhulk opened this issue Jun 7, 2017 · 6 comments · Fixed by #633

Comments

@reaperhulk
Copy link
Member

reaperhulk commented Jun 7, 2017

cryptography is planning to ship a manylinux1 wheel soon(ish). Unfortunately, the default verify location for SSL_CTX is defined during compile and different linux distributions choose different locations. This means that pyOpenSSL will experience issues with finding certificate trust stores on many distributions unless we make a change. To (generally) preserve the existing behavior it has been proposed that we write some code that iterates over a set of possible paths and finds the cert store. This would mimic the existing behavior of Go (root_linux.go).

To a first approximation this PR would add a method call in SSL.Context.__init__ that finds the root store and then calls SSL_CTX_load_verify_locations on the context.

Update: The original proposal here is nowhere near what has actually been implemented. See #633 for details.

cc @alex @hynek @Lukasa @njsmith @tiran to see if anybody has objections to this approach.

@Lukasa
Copy link
Member

Lukasa commented Jun 7, 2017

Please don't auto-find the system roots. That behaviour would make it impossible to have a Context that does not trust the system cert store, a very valid use-case for scoping trust.

I'm fine with that being done, but why not do it in set_default_verify_paths?

@reaperhulk
Copy link
Member Author

@Lukasa we definitely don't want to prevent that. I'm unsure how to use set_default_verify_paths though. Does it take arguments outside of reading env vars?

@Lukasa
Copy link
Member

Lukasa commented Jun 7, 2017

To be clear, all I'm suggesting is that you don't do this when __init__ is called. I was suggesting an alternative function to hide your magic in.

@reaperhulk
Copy link
Member Author

@Lukasa ah, I understand. Now I am wondering if we can replicate the behavior of the set_default_paths so that everything behaves the same though. Maybe I should ask the remedial question first though: on linux with pyOpenSSL right now are any trust roots available by default on the SSL_CTX? I've been assuming that the answer is "the system roots are available".

Anyway...

SSL_CTX_set_default_verify_paths calls X509_STORE_set_default_paths on ctx->cert_store.

X509_STORE_set_default_paths does the following:

  int X509_STORE_set_default_paths(X509_STORE *ctx)
  {
      X509_LOOKUP *lookup;

      lookup = X509_STORE_add_lookup(ctx, X509_LOOKUP_file());
      if (lookup == NULL)
          return (0);
      X509_LOOKUP_load_file(lookup, NULL, X509_FILETYPE_DEFAULT);

      lookup = X509_STORE_add_lookup(ctx, X509_LOOKUP_hash_dir());
      if (lookup == NULL)
          return (0);
      X509_LOOKUP_add_dir(lookup, NULL, X509_FILETYPE_DEFAULT);

      /* clear any errors */
      ERR_clear_error();

      return (1);
  }

Frankly I'm either too tired or too stupid to entirely puzzle out what's happening there. It gets an X509_LOOKUP *, adds it to the X509_STORE, then loads a NULL file (which presumably triggers it loading whatever the compiled default is?).

We can get the X509_STORE * from an SSL_CTX * with SSL_CTX_get_cert_store, and all the methods here are actually in public headers so if we can sort out whether we can alter the "default" then we

@Lukasa
Copy link
Member

Lukasa commented Jun 7, 2017

I think your post tailed off there @reaperhulk. ;) But yeah, that basically seems to just be loading the compiled defaults.

@reaperhulk
Copy link
Member Author

Yeah okay, with sleep and actually paying attention I have sorted out all my confusion. PR inbound.

@alex alex closed this as completed in #633 Jun 29, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants