Fwd: [PATCHES] simple gpo patches fixes

[David Mulder]

Douglas could you take a look at the current set of patches on:

https://github.com/samba-team/samba/pull/154

These patches delegate all the work for downloading the cache to the
check_refresh_gpo_list() function in libgpo (which was written
previously for this very purpose). So, there are no longer file
permission issues, etc, being handled in the python code (I believe
Garming was concerned about this).

On 04/05/2018 05:57 PM, Douglas Bagnall wrote:
> On 30/03/18 07:44, David Mulder via samba-technical wrote:
>> I've broken down that patch into sizable chunks. I've pushed them to the
>> git repo: https://github.com/samba-team/samba/pull/154
> Good. You can add my reviewed-by to all of them except the last one
> where I've got a question or two (these questions may be stupid: it
> would be good if the other reviewer knows *something* about GPO).
>
>> +def gpo_version(lp, conn, path):
>> +    # gpo.gpo_get_sysvol_gpt_version() reads the GPT.INI from a local file.
>> +    # If we don't have a sysvol path locally (if we're not a kdc), then
>> +    # store the file locally here so it can be read on a client.
>> +    sysvol = lp.get("path", "sysvol")
>> +    if sysvol:
>> +        local_path = os.path.join(sysvol, path, 'GPT.INI')
> Is there ever going to be a chance that 'path' here could contain "../"
> or start with "/"? That would lead to the wrong result.
>
> The existing use looks OK, but I feel a bit nervous about having a
> function around that opens files for writing without checking the
> path.
>
>> +    else:
>> +        gpt_path = lp.cache_path(os.path.join('gpt', path))
>> +        local_path = os.path.join(gpt_path, 'GPT.INI')
>> +        if not os.path.exists(os.path.dirname(local_path)):
>> +            os.makedirs(os.path.dirname(local_path), 0o700)
>> +        data = conn.loadfile(os.path.join(path, 'GPT.INI').replace('/', '\\'))
>> +        encoding = chardet.detect(data)
>                       ^^^^^^^
> BTW, AFAICT, chardet is not a standard Python module, and is not listed
> as a dependency anywhere in Samba -- it just happens to be on every
> machine we've tried. This problem wasn't introduced with this
> patchset, but it should be fixed.
>
>> +        open(local_path, 'w').write(data.decode(encoding['encoding']))
>> +    return int(gpo.gpo_get_sysvol_gpt_version(os.path.dirname(local_path))[1])
>> +
>> --- a/source4/param/pyparam.c
>> +++ b/source4/param/pyparam.c
>> +static PyObject *py_cache_path(PyObject *self, PyObject *args)
>> +{
>> +	struct loadparm_context *lp_ctx = PyLoadparmContext_AsLoadparmContext(self);
>> +	char *name, *path;
>> +	PyObject *ret;
>> +
>> +	if (!PyArg_ParseTuple(args, "s", &name)) {
>> +		return NULL;
>> +	}
>> +
>> +	path = lpcfg_cache_path(NULL, lp_ctx, name);
>> +	if (!path) {
> It would be good to have something like
>
> 		PyErr_Format(PyExc_RuntimeError,
>                              "Unable to access cache %s",
> 			     name);	
>
> in here before the return NULL, so that python has an explanation
> (using RuntimeError is not great, but it is what we do). The
> PyArg_ParseTuple() case is different because it sets an exception
> itself.
>
>> +		return NULL;
>> +	}
>> +	ret = PyStr_FromString(path);
>> +	talloc_free(path);
>> +
>> +	return ret;
>> +}
>>  
>>> Also, in Python 3, is a string what you want? It means the path has to
>>> be utf-8 -- is that something we know? I don't know what we normally
>>> do here.
>> None of this code works in python3 yet, afaik. The libgpo python code
>> was only just barely ported to python3.
> OK.
>
> cheers,
> Douglas
>

-- 
David Mulder
SUSE Labs Software Engineer - Samba
dmulder at suse.com
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)