Skip to content

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

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions pygit2/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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


# Low level API
from _pygit2 import *

Expand Down Expand Up @@ -267,3 +270,9 @@ def clone_repository(
return Repository._from_c(crepo[0], owned=True)

settings = Settings()

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

Choose a reason for hiding this comment

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

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.

except:
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

pass
28 changes: 27 additions & 1 deletion pygit2/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
from _pygit2 import GIT_OPT_GET_CACHED_MEMORY
from _pygit2 import GIT_OPT_ENABLE_CACHING
from _pygit2 import GIT_OPT_SET_CACHE_MAX_SIZE

from _pygit2 import GIT_OPT_SET_SSL_CERT_LOCATIONS

class SearchPathList(object):

Expand Down Expand Up @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

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

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

"""Set the ssl cert file. The value cannot be read.
"""
return option(GIT_OPT_SET_SSL_CERT_LOCATIONS, value, None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Setters don't return values.

Copy link
Contributor

Choose a reason for hiding this comment

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

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


ssl_cert_file = property(fset=__set_ssl_cert_file)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we should have a getter for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

you can still use a decorator:

@property
def attr(self):
    """A proper docstring."""
    raise AttributeError

@attr.setter
def attr(self, value):
    # set val

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, that's what I thought...


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

ssl_cert_dir = property(fset=__set_ssl_cert_dir)

def __set_ssl_cert_locations(self, locations):
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

"""
Passes both file_path and dir_path to libgit2.

locations must be a list/tuple type with the the file path
at index 0 and the directory path at index 1

The values set cannot be read.
"""
return option(GIT_OPT_SET_SSL_CERT_LOCATIONS, locations[0], locations[1])

_ssl_cert_locations = property(fset=__set_ssl_cert_locations)

30 changes: 30 additions & 0 deletions src/options.c
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,36 @@ option(PyObject *self, PyObject *args)
return tup;
}

case GIT_OPT_SET_SSL_CERT_LOCATIONS:
{
PyObject *py_file, *py_dir;
const char *file_path, *dir_path;
int err;

py_file = PyTuple_GetItem(args, 1);
py_dir = PyTuple_GetItem(args, 2);

/* py_file and py_dir are only valid if they are strings */
if (PyUnicode_Check(py_file) || PyBytes_Check(py_file)) {
file_path = py_str_to_c_str(py_file, Py_FileSystemDefaultEncoding);
} else {
file_path = NULL;
}

if (PyUnicode_Check(py_dir) || PyBytes_Check(py_dir)) {
dir_path = py_str_to_c_str(py_dir, Py_FileSystemDefaultEncoding);
} else {
dir_path = NULL;
}

err = git_libgit2_opts(GIT_OPT_SET_SSL_CERT_LOCATIONS, file_path, dir_path);

if (err < 0)
return Error_set(err);

Py_RETURN_NONE;
}

}

PyErr_SetString(PyExc_ValueError, "unknown/unsupported option value");
Expand Down
1 change: 1 addition & 0 deletions src/pygit2.c
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,7 @@ moduleinit(PyObject* m)
ADD_CONSTANT_INT(m, GIT_OPT_GET_CACHED_MEMORY);
ADD_CONSTANT_INT(m, GIT_OPT_ENABLE_CACHING);
ADD_CONSTANT_INT(m, GIT_OPT_SET_CACHE_MAX_SIZE);
ADD_CONSTANT_INT(m, GIT_OPT_SET_SSL_CERT_LOCATIONS);

/* Errors */
GitError = PyErr_NewException("_pygit2.GitError", NULL, NULL);
Expand Down