Fwd: [PATCHES] simple gpo patches fixes

[David Mulder]

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.
No. This 'path' variable is defined in apply_gp(). It is always set to
'realm_name/Policies/{GPO-GUID}'.
>> +    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.
Thanks. I'll look for an alternative.
>> +        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.
Ok, added.
>> +		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)