Allow setting git opt set sll cert locations option in libgit2 #879

imbuedhope · 2019-02-24T19:30:11Z

Closes #876

Adds support for the following

import pygit2
pygit2.settings.ssl_cert_file = '/path/to/file'
pygit2.settings.ssl_cert_dir = '/path/to/folder'

Which is the equivalent of the following in C

git_libgit2_opts(GIT_OPT_SET_SSL_CERT_LOCATIONS, '/path/to/file', NULL);
git_libgit2_opts(GIT_OPT_SET_SSL_CERT_LOCATIONS, NULL, '/path/to/folder');

Also adds support for directly loading the settings from the ssl module like follows

import pygit2
import ssl
pygit2.settings._ssl_cert_locations = ssl.get_default_verify_paths()

All the new properties that were added to pygit2.settings.Settings are write only, since it is write only in libgit2.

In addition, pygit2 also attempts sets _ssl_cert_locations based on the ssl module on import. This probably won't work on Windows until the wheels are changed to support this in the libgit2 being shipped with it.

webknjaz · 2019-03-01T13:12:00Z

Oh, I missed this PR...

P.S. s/sll/ssl/ in title

webknjaz

I've pointed out a few anti-pythonic approaches which should be fixed: pygit2 is a Python wrapper and as such, it shouldn't 100% repeat C-library principles on the high level.

webknjaz · 2019-03-01T14:01:53Z

pygit2/__init__.py

@@ -28,6 +28,9 @@
 # Import from the future
 from __future__ import absolute_import

+# Import from core python modules
+from ssl import get_default_verify_paths as default_ssl_verify_paths


Please don't rename this callable. It's an action, not a constant. So I must start with a verb. get. Renaming it to constant/var-style is harmful and misleading for readability.

Suggested change

from ssl import get_default_verify_paths as default_ssl_verify_paths

from ssl import get_default_verify_paths

webknjaz · 2019-03-01T14:05:05Z

pygit2/__init__.py

+try:
+    # try to set default ssl cert locations
+    settings._ssl_cert_locations = default_ssl_verify_paths()
+except:


You shouldn't catch all exceptions. Silencing everything makes any related bugs nearly impossible to locate and is harmful therefore. It's important to always fail fast and loudly.
ssl.get_default_verify_paths() shouldn't raise any exceptions anyway.

Suggested change

except:

webknjaz · 2019-03-01T14:07:53Z

pygit2/settings.py

+        """
+        return option(GIT_OPT_SET_SSL_CERT_LOCATIONS, value, None)
+
+    ssl_cert_file = property(fset=__set_ssl_cert_file)


It is an ancient way of creating properties. Always use it as a decorator instead.

Also, we should have a getter for this.

There is no get support for the ssl cert paths in libgit2. It makes sense to have it added to libgit2 first.

You can not use property as decorator if you don't have a setter since the first param in the __init__ for property is fget.

But you can track it on the Python side because it's needed to properly set the other value.

you can still use a decorator:

@property def attr(self): """A proper docstring.""" raise AttributeError @attr.setter def attr(self, value): # set val

cc #879 (comment)

So here's why we need to track the values we set earlier. Semantically, these setters are assigned with the values for what their names mean.
End-users of the library are Pythonistas who always expect the assignment behavior of the attribute to change exactly the thing its name refers to.
So resetting another setting would be misleading and is a side-effect.
That's why we need to track values which we set earlier for one connected setting to use them when setting the other.
Of course, it's not necessary to expose them for reading right now. But it's a least we can do to preserve consistent UX when setting one of TSL cert location sub-options.
As for setting both sub-options, it should be an explicit method because semantically setting a tuple to attribute to side-effect two values is weird.

Digging around, it looks like ultimately libgit2 makes a call to SSL_CTX_load_verify_locations when built with Openssl and the man page for it seems to describe the following behavior.

When looking up CA certificates, the OpenSSL library will first search the certificates in CAfile, then those in CApath.

Nice, that's what I thought...

webknjaz · 2019-03-01T14:10:49Z

pygit2/settings.py

+    def __set_ssl_cert_file(self, value):
+        """Set the ssl cert file. The value cannot be read.
+        """
+        return option(GIT_OPT_SET_SSL_CERT_LOCATIONS, value, None)


Setters don't return values.

When setting one value we shouldn't reset another. If a user wants to do this they can update it to None explicitly.

webknjaz · 2019-03-01T14:14:25Z

pygit2/settings.py

@@ -101,4 +101,30 @@ def cache_object_limit(self, object_type, value):
        """
        return option(GIT_OPT_SET_CACHE_OBJECT_LIMIT, object_type, value)

+    def __set_ssl_cert_file(self, value):


Suggested change

def __set_ssl_cert_file(self, value):

@property

def ssl_cert_file(self):

return self._ssl_cert_file

@ssl_cert_file.setter

def ssl_cert_file(self, value):

"""Set the ssl cert file."""

option(GIT_OPT_SET_SSL_CERT_LOCATIONS, value, self.ssl_cert_dir)

self._ssl_cert_file = value

webknjaz · 2019-03-01T14:16:07Z

pygit2/settings.py

+
+    ssl_cert_dir = property(fset=__set_ssl_cert_dir)
+
+    def __set_ssl_cert_locations(self, locations):


This one must be a method. Using property here is odd.

I went with properties because all other supported settings are implemented as property. It does not make sense to break consistency.

As I wrote earlier, we should go for Pythonic principles. And if they are not respected in some places it doesn't mean that you should follow the lead. When you write in C you don't follow Assembly code-style just because it's an underlying somewhere, do you?

I'll look at the issues this week-end.
But generally speaking, from PEP-8 itself:
Consistency with this style guide is important. Consistency within a project is more important.

Though we don't have a project style guide. Well, will check this week-end.

The whole paragraph:
A style guide is about consistency. Consistency with this style guide is important. Consistency within a project is more important. Consistency within one module or function is the most important.

By the way, thank you both for the awesome work you're doing!

This rule doesn't apply here. Since all other attributes correspond to one value. With two values it doesn't make sense. Setting two values by assigning a tuple to an attribute is ridiculous in Python.

And also when writing Python it doesn't have to look like another language https://www.youtube.com/watch?v=wf-BqAjZb8M. It's fine in the low-level wrapper but not in a public interface.

And I wasn't referring to just PEP8. It's quite a common sense/knowledge. PEP8 is a code style guide but I'm talking about more generic approaches which are probably not formalized explicitly but every Pythonista knows that when they see code looking like Java in .py file something isn't quite right.

webknjaz · 2019-03-01T19:52:51Z

pygit2/__init__.py

+
+try:
+    # try to set default ssl cert locations
+    settings._ssl_cert_locations = default_ssl_verify_paths()


I think this belongs at Settings.__init__(). It is a good practice to have a ready-to-use object after the initializer run.
Extra initializers are popular in C++, not Python. This semantics is untypical for Pythonic projects.

webknjaz · 2019-03-01T19:54:23Z

I still think that setting one value shouldn't reset another one.

jdavid · 2019-03-02T09:37:31Z

Finally looked at the api. I would replace both setters by a single method:

set_ssl_cert_locations(...)

webknjaz · 2019-03-02T09:50:38Z

I'd keep properties for individual suboptions. They are native to Python and is what a typical Pythonista would expect. It better communicates the intent as in "make this setting use that value".

jdavid · 2019-03-02T10:42:34Z

I'd rather not have 2 ways to do the same thing. It's good for the users to have 1 way, so they don't have to stop thinking which one they should use, and code looks the same whoever writes it.

It's also good for the developers/maintainers not to increase the burden: our limited resources are the main constraint. Aiming for feature completeness, api consistency, and quality documentation is already ambitious. So I'd keep it simple and stick to the 1 way principle (even if the API feels a bit low level, it's not awkward either).

The settings.py file is a good example of the inconsistency we have. It already mixes properties and methods. And a couple of methods even lack the verb at the beginning. caching and cache_max_size should be setters, and cache_object_limit should be renamed to set_cache_object_limit

Then the settings.py file would be all getter/setters except for the 2 not straightforward cases where we would have methods. The advantages: consistency, simplicity, 1 way.

webknjaz · 2019-03-02T10:46:17Z

Yes, there should be one way of doing things. Except for the cases where that way cannot be used. So are you okay with property-based getters/setters then?

jdavid · 2019-03-02T10:58:59Z

Could you summarize the API you propose for ssl cert locations?

webknjaz · 2019-03-02T11:00:00Z

I'll send it as a PR on top of this branch

webknjaz · 2019-03-02T11:06:20Z

Side note: after looking at Settings class I think that this use-case might be a good fit for using attrs library. But it's a topic for a separate discussion.

webknjaz · 2019-03-02T11:30:11Z

@jdavid #884

Usage:: import pygit2 pygit2.settings.ssl_cert_file = '/path/to/file' pygit2.settings.ssl_cert_dir = '/path/to/folder' del pygit2.settings.ssl_cert_file pygit2.settings.set_ssl_cert_locations( '/path/to/new/file', '/path/to/new/folder', ) Co-authored-by: Sriram Raghu <[email protected]> Closes libgit2#876 Superseeds and closes libgit2#879

rcoup · 2020-02-24T13:13:08Z

Example if you're using certifi:

import certifi
import pygit2
pygit2.settings.ssl_cert_file = certifi.where()

imbuedhope added 3 commits February 24, 2019 13:24

added hooks for git_libgit2_opts GIT_OPT_SET_SSL_CERT_LOCATIONS in C

553f74e

added support for setting ssl cert locations in pygit2.settings

333c7ba

set ssl cert location by default based on the ssl module

ecc2d91

webknjaz suggested changes Mar 1, 2019

View reviewed changes

webknjaz reviewed Mar 1, 2019

View reviewed changes

webknjaz mentioned this pull request Mar 2, 2019

Expose setting TLS file and dir lookup locations via settings module #884

Merged

jdavid closed this in #884 Mar 7, 2019

imbuedhope deleted the 876-Allow-setting-GIT_OPT_SET_SLL_CERT_LOCATIONS-option-in-libgit2 branch March 7, 2019 22:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow setting git opt set sll cert locations option in libgit2 #879

Allow setting git opt set sll cert locations option in libgit2 #879

imbuedhope commented Feb 24, 2019

webknjaz commented Mar 1, 2019

webknjaz left a comment •

edited

Loading

webknjaz Mar 1, 2019

webknjaz Mar 1, 2019

webknjaz Mar 1, 2019

webknjaz Mar 1, 2019

imbuedhope Mar 1, 2019

webknjaz Mar 1, 2019

webknjaz Mar 1, 2019

webknjaz Mar 1, 2019

imbuedhope Mar 1, 2019

webknjaz Mar 1, 2019

webknjaz Mar 1, 2019

webknjaz Mar 1, 2019

webknjaz Mar 1, 2019

webknjaz Mar 1, 2019

imbuedhope Mar 1, 2019

webknjaz Mar 1, 2019

jdavid Mar 1, 2019

jdavid Mar 1, 2019

jdavid Mar 1, 2019

webknjaz Mar 1, 2019

webknjaz Mar 1, 2019

webknjaz Mar 1, 2019

webknjaz commented Mar 1, 2019

jdavid commented Mar 2, 2019

webknjaz commented Mar 2, 2019

jdavid commented Mar 2, 2019

webknjaz commented Mar 2, 2019

jdavid commented Mar 2, 2019

webknjaz commented Mar 2, 2019

webknjaz commented Mar 2, 2019

webknjaz commented Mar 2, 2019

rcoup commented Feb 24, 2020

	from ssl import get_default_verify_paths as default_ssl_verify_paths
	from ssl import get_default_verify_paths

-    def __set_ssl_cert_file(self, value):
+    @property
+    def ssl_cert_file(self):
+        return self._ssl_cert_file
+    @ssl_cert_file.setter
+    def ssl_cert_file(self, value):
+        """Set the ssl cert file."""
+        option(GIT_OPT_SET_SSL_CERT_LOCATIONS, value, self.ssl_cert_dir)
+        self._ssl_cert_file = value


		ssl_cert_dir = property(fset=__set_ssl_cert_dir)

		def __set_ssl_cert_locations(self, locations):

Allow setting git opt set sll cert locations option in libgit2 #879

Allow setting git opt set sll cert locations option in libgit2 #879

Conversation

imbuedhope commented Feb 24, 2019

webknjaz commented Mar 1, 2019

webknjaz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webknjaz commented Mar 1, 2019

jdavid commented Mar 2, 2019

webknjaz commented Mar 2, 2019

jdavid commented Mar 2, 2019

webknjaz commented Mar 2, 2019

jdavid commented Mar 2, 2019

webknjaz commented Mar 2, 2019

webknjaz commented Mar 2, 2019

webknjaz commented Mar 2, 2019

rcoup commented Feb 24, 2020

webknjaz left a comment •

edited

Loading