[PATCH] Port of samba.security Python module

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Port of samba.security Python module

Samba - samba-technical mailing list
Hello.

I've ported another Python C extension to Python 3 compatible form.
AFAIK there is only one usage of samba.security - found in
python/samba/netcmd/gpo.py on line 407. Usage of this module looks very
specific and complicated because it needs token from user session and
unpacked NDR objects as arguments and samba.security is not used widely
so I am not sure how to write tests for this function. Any pointers
would be appreciated..

Have a nice day.

Lumír


0001-python-Port-samba.security-to-Python-3-compatible-fo.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Port of samba.security Python module

Samba - samba-technical mailing list
Hello.

Is there any problem with this patch? I've tried to rebase it to the
latest master and there is no conflict.

If the problem is that the module is unstested, could you please help me
to write some test?

Thank you and have a nice day.

Lumír


On 05/22/2017 04:24 PM, Lumir Balhar via samba-technical wrote:

> Hello.
>
> I've ported another Python C extension to Python 3 compatible form.
> AFAIK there is only one usage of samba.security - found in
> python/samba/netcmd/gpo.py on line 407. Usage of this module looks
> very specific and complicated because it needs token from user session
> and unpacked NDR objects as arguments and samba.security is not used
> widely so I am not sure how to write tests for this function. Any
> pointers would be appreciated..
>
> Have a nice day.
>
> Lumír
>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Port of samba.security Python module

Samba - samba-technical mailing list
On Mon, 2017-08-07 at 16:48 +0200, Lumir Balhar via samba-technical
wrote:
> Hello.
>
> Is there any problem with this patch? I've tried to rebase it to the 
> latest master and there is no conflict.
>
> If the problem is that the module is unstested, could you please help
> me 
> to write some test?

We do always need more tests :-)

It doesn't look like a complex conversion however.  How about creating
an SD from SSDL, then testing it against a token (even an empty one).
Add that to the python/samba/tests/security.py testsuite.

That should confirm the C code still gets hit by the python code.

Thanks,

Andrew Bartlett
--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT  
https://catalyst.net.nz/services/samba





Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Port of samba.security Python module

Samba - samba-technical mailing list
On 08/08/2017 06:38 AM, Andrew Bartlett wrote:

> On Mon, 2017-08-07 at 16:48 +0200, Lumir Balhar via samba-technical
> wrote:
>> Hello.
>>
>> Is there any problem with this patch? I've tried to rebase it to the
>> latest master and there is no conflict.
>>
>> If the problem is that the module is unstested, could you please help
>> me
>> to write some test?
> We do always need more tests :-)
>
> It doesn't look like a complex conversion however.  How about creating
> an SD from SSDL, then testing it against a token (even an empty one).
> Add that to the python/samba/tests/security.py testsuite.
>
> That should confirm the C code still gets hit by the python code.
>
> Thanks,
>
> Andrew Bartlett
Hello.

Thank you for the hint. I've created tests as you suggested with an
empty token and same descriptor as other tests are using in
samba.test.security.

Unfortunately, I cannot use assertRaises() as a context manager for
testing exceptions because it is not supported in Python 2.6.

Please, let me know if this is ok.

Thank you and have a nice day.

Lumír

0003-python-Enable-execution-of-samba.tests.security-with.patch (1K) Download Attachment
0002-python-Add-tests-for-check_access-function-from-samb.patch (1K) Download Attachment
0001-python-Port-samba.security-to-Python-3-compatible-fo.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Port of samba.security Python module

Samba - samba-technical mailing list
Hi Lumir,

> +
> +class CheckAccessTests(samba.tests.TestCase):
> +
> +    def test_check_access(self):
> +        desc = security.descriptor.from_sddl("O:AOG:DAD:(A;;RPWPCCDCLCSWRCWDWOGA;;;S-1-0-0)", security.dom_sid("S-2-0-0"))
> +        token = security.token()
> +
> +        self.assertEqual(access_check(desc, token, 0), 0)
> +
> +        params = (
> +            (-1, -1073741727, 'A required privilege is not held by the client.'),
> +            (1, -1073741790, '{Access Denied} A process has requested access to an object but has not been granted those access rights.')
> +        )
Can you use string constants for the integer values?

I guess you can use
security.SEC_FLAG_SYSTEM_SECURITY/ntstatus.NT_STATUS_PRIVILEGE_NOT_HELD
and
security.SEC_STD_READ_CONTROL/ntstatus.NT_STATUS_ACCESS_DENIED

And I guess checking the status code is enough, we don't
need to assert on the error message.

metze




signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Port of samba.security Python module

Samba - samba-technical mailing list
On 08/08/2017 10:47 AM, Stefan Metzmacher wrote:

> Hi Lumir,
>
>> +
>> +class CheckAccessTests(samba.tests.TestCase):
>> +
>> +    def test_check_access(self):
>> +        desc = security.descriptor.from_sddl("O:AOG:DAD:(A;;RPWPCCDCLCSWRCWDWOGA;;;S-1-0-0)", security.dom_sid("S-2-0-0"))
>> +        token = security.token()
>> +
>> +        self.assertEqual(access_check(desc, token, 0), 0)
>> +
>> +        params = (
>> +            (-1, -1073741727, 'A required privilege is not held by the client.'),
>> +            (1, -1073741790, '{Access Denied} A process has requested access to an object but has not been granted those access rights.')
>> +        )
> Can you use string constants for the integer values?
>
> I guess you can use
> security.SEC_FLAG_SYSTEM_SECURITY/ntstatus.NT_STATUS_PRIVILEGE_NOT_HELD
> and
> security.SEC_STD_READ_CONTROL/ntstatus.NT_STATUS_ACCESS_DENIED
>
> And I guess checking the status code is enough, we don't
> need to assert on the error message.
>
> metze
>
>
>
Thank you for the review. Yes, I can use constants instead of integers -
good idea - but samba.ntstatus module is not ready for Python 3 yet. It
is not a big problem because I am working on samba.ntstatus and
samba.werror modules right now so I am gonna send another patchset today
and when it will be merged I'll fix these test.

Thank you one more time and have a nice day.
Lumír

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Port of samba.security Python module

Samba - samba-technical mailing list
On 08/08/2017 11:47 AM, Lumir Balhar via samba-technical wrote:

> On 08/08/2017 10:47 AM, Stefan Metzmacher wrote:
>> Hi Lumir,
>>
>>> +
>>> +class CheckAccessTests(samba.tests.TestCase):
>>> +
>>> +    def test_check_access(self):
>>> +        desc =
>>> security.descriptor.from_sddl("O:AOG:DAD:(A;;RPWPCCDCLCSWRCWDWOGA;;;S-1-0-0)",
>>> security.dom_sid("S-2-0-0"))
>>> +        token = security.token()
>>> +
>>> +        self.assertEqual(access_check(desc, token, 0), 0)
>>> +
>>> +        params = (
>>> +            (-1, -1073741727, 'A required privilege is not held by
>>> the client.'),
>>> +            (1, -1073741790, '{Access Denied} A process has
>>> requested access to an object but has not been granted those access
>>> rights.')
>>> +        )
>> Can you use string constants for the integer values?
>>
>> I guess you can use
>> security.SEC_FLAG_SYSTEM_SECURITY/ntstatus.NT_STATUS_PRIVILEGE_NOT_HELD
>> and
>> security.SEC_STD_READ_CONTROL/ntstatus.NT_STATUS_ACCESS_DENIED
>>
>> And I guess checking the status code is enough, we don't
>> need to assert on the error message.
>>
>> metze
>>
>>
>>
> Thank you for the review. Yes, I can use constants instead of integers
> - good idea - but samba.ntstatus module is not ready for Python 3 yet.
> It is not a big problem because I am working on samba.ntstatus and
> samba.werror modules right now so I am gonna send another patchset
> today and when it will be merged I'll fix these test.
>
> Thank you one more time and have a nice day.
> Lumír
>
Hello.

Because samba.ntstatus module is now available for Python 3 in master, I
tried your suggestion and I replaced integers values with constants from
samba.dcerpc.security and samba.ntstatus.

Replaced arguments SEC_FLAG_SYSTEM_SECURITY and SEC_STD_READ_CONTROL are
working well but exceptions contain different error numbers than
NT_STATUS_PRIVILEGE_NOT_HELD and NT_STATUS_ACCESS_DENIED and I cannot
find the right ones.

NT_STATUS_PRIVILEGE_NOT_HELD = 3221225569 but exception contains -1073741727
NT_STATUS_ACCESS_DENIED = 3221225506 but exception contains -1073741790

Please, where do I find the right constants?

Thank you and have a nice day.
Lumír

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Port of samba.security Python module

Samba - samba-technical mailing list
On Thursday, 24 August 2017 13:21:27 CEST Lumir Balhar via samba-technical
wrote:

> On 08/08/2017 11:47 AM, Lumir Balhar via samba-technical wrote:
> > On 08/08/2017 10:47 AM, Stefan Metzmacher wrote:
> >> Hi Lumir,
> >>
> >>> +
> >>> +class CheckAccessTests(samba.tests.TestCase):
> >>> +
> >>> +    def test_check_access(self):
> >>> +        desc =
> >>> security.descriptor.from_sddl("O:AOG:DAD:(A;;RPWPCCDCLCSWRCWDWOGA;;;S-1-
> >>> 0-0)", security.dom_sid("S-2-0-0"))
> >>> +        token = security.token()
> >>> +
> >>> +        self.assertEqual(access_check(desc, token, 0), 0)
> >>> +
> >>> +        params = (
> >>> +            (-1, -1073741727, 'A required privilege is not held by
> >>> the client.'),
> >>> +            (1, -1073741790, '{Access Denied} A process has
> >>> requested access to an object but has not been granted those access
> >>> rights.')
> >>> +        )
> >>
> >> Can you use string constants for the integer values?
> >>
> >> I guess you can use
> >> security.SEC_FLAG_SYSTEM_SECURITY/ntstatus.NT_STATUS_PRIVILEGE_NOT_HELD
> >> and
> >> security.SEC_STD_READ_CONTROL/ntstatus.NT_STATUS_ACCESS_DENIED
> >>
> >> And I guess checking the status code is enough, we don't
> >> need to assert on the error message.
> >>
> >> metze
> >
> > Thank you for the review. Yes, I can use constants instead of integers
> > - good idea - but samba.ntstatus module is not ready for Python 3 yet.
> > It is not a big problem because I am working on samba.ntstatus and
> > samba.werror modules right now so I am gonna send another patchset
> > today and when it will be merged I'll fix these test.
> >
> > Thank you one more time and have a nice day.
> > Lumír
>
> Hello.
>
> Because samba.ntstatus module is now available for Python 3 in master, I
> tried your suggestion and I replaced integers values with constants from
> samba.dcerpc.security and samba.ntstatus.
>
> Replaced arguments SEC_FLAG_SYSTEM_SECURITY and SEC_STD_READ_CONTROL are
> working well but exceptions contain different error numbers than
> NT_STATUS_PRIVILEGE_NOT_HELD and NT_STATUS_ACCESS_DENIED and I cannot
> find the right ones.
>
> NT_STATUS_PRIVILEGE_NOT_HELD = 3221225569 but exception contains -1073741727
> NT_STATUS_ACCESS_DENIED = 3221225506 but exception contains -1073741790

Sounds like the bug. The exception seems to return an int where it should
return an unsigned int??

i2c -1073741727
18446744072635809889 0xFFFFFFFFC0000061 01777777777770000000141

#define NT_STATUS_PRIVILEGE_NOT_HELD NT_STATUS(0xc0000061)


        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Port of samba.security Python module

Samba - samba-technical mailing list
On Thu, 2017-08-24 at 14:22 +0200, Andreas Schneider wrote:

> On Thursday, 24 August 2017 13:21:27 CEST Lumir Balhar via samba-technical
> wrote:
> >
> > Hello.
> >
> > Because samba.ntstatus module is now available for Python 3 in master, I
> > tried your suggestion and I replaced integers values with constants from
> > samba.dcerpc.security and samba.ntstatus.
> >
> > Replaced arguments SEC_FLAG_SYSTEM_SECURITY and SEC_STD_READ_CONTROL are
> > working well but exceptions contain different error numbers than
> > NT_STATUS_PRIVILEGE_NOT_HELD and NT_STATUS_ACCESS_DENIED and I cannot
> > find the right ones.
> >
> > NT_STATUS_PRIVILEGE_NOT_HELD = 3221225569 but exception contains -1073741727
> > NT_STATUS_ACCESS_DENIED = 3221225506 but exception contains -1073741790
>
> Sounds like the bug. The exception seems to return an int where it should
> return an unsigned int??
>
> i2c -1073741727
> 18446744072635809889 0xFFFFFFFFC0000061 01777777777770000000141
>
> #define NT_STATUS_PRIVILEGE_NOT_HELD NT_STATUS(0xc0000061)
>

I think:

#define PyErr_FromNTSTATUS(status) Py_BuildValue("(i,s)", NT_STATUS_V(status), discard_const_p(char, get_friendly_nt_error_msg(status)))
 
in pyerrors.h is wrong.

We probably want an unsigned int, not a signed int, for the first
constant.  We first saw this in some other patches that Gary did, where
he ended up casting via a C type casting lib to make it work, but
didn't dig into it properly.

Thanks,

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Port of samba.security Python module

Samba - samba-technical mailing list
On 08/27/2017 09:08 AM, Andrew Bartlett wrote:

> On Thu, 2017-08-24 at 14:22 +0200, Andreas Schneider wrote:
>> On Thursday, 24 August 2017 13:21:27 CEST Lumir Balhar via samba-technical
>> wrote:
>>> Hello.
>>>
>>> Because samba.ntstatus module is now available for Python 3 in master, I
>>> tried your suggestion and I replaced integers values with constants from
>>> samba.dcerpc.security and samba.ntstatus.
>>>
>>> Replaced arguments SEC_FLAG_SYSTEM_SECURITY and SEC_STD_READ_CONTROL are
>>> working well but exceptions contain different error numbers than
>>> NT_STATUS_PRIVILEGE_NOT_HELD and NT_STATUS_ACCESS_DENIED and I cannot
>>> find the right ones.
>>>
>>> NT_STATUS_PRIVILEGE_NOT_HELD = 3221225569 but exception contains -1073741727
>>> NT_STATUS_ACCESS_DENIED = 3221225506 but exception contains -1073741790
>> Sounds like the bug. The exception seems to return an int where it should
>> return an unsigned int??
>>
>> i2c -1073741727
>> 18446744072635809889 0xFFFFFFFFC0000061 01777777777770000000141
>>
>> #define NT_STATUS_PRIVILEGE_NOT_HELD NT_STATUS(0xc0000061)
>>
> I think:
>
> #define PyErr_FromNTSTATUS(status) Py_BuildValue("(i,s)", NT_STATUS_V(status), discard_const_p(char, get_friendly_nt_error_msg(status)))
>  
> in pyerrors.h is wrong.
>
> We probably want an unsigned int, not a signed int, for the first
> constant.  We first saw this in some other patches that Gary did, where
> he ended up casting via a C type casting lib to make it work, but
> didn't dig into it properly.
>
> Thanks,
>
> Andrew Bartlett
>
Hello.

Thanks a lot, Andrew. I fixed the bug you mentioned and now everything
is working.

Patchset rebased and attached.

Have a nice day.
Lumír

0004-python-Enable-execution-of-samba.tests.security-with.patch (1K) Download Attachment
0003-python-Fix-bad-type-in-conversion-of-NTSTATUS.patch (1K) Download Attachment
0002-python-Add-tests-for-check_access-function-from-samb.patch (1K) Download Attachment
0001-python-Port-samba.security-to-Python-3-compatible-fo.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Port of samba.security Python module

Samba - samba-technical mailing list
On Wednesday, 6 September 2017 09:33:52 CEST Lumir Balhar wrote:

> On 08/27/2017 09:08 AM, Andrew Bartlett wrote:
> > On Thu, 2017-08-24 at 14:22 +0200, Andreas Schneider wrote:
> >> On Thursday, 24 August 2017 13:21:27 CEST Lumir Balhar via
> >> samba-technical
> >>
> >> wrote:
> >>> Hello.
> >>>
> >>> Because samba.ntstatus module is now available for Python 3 in master, I
> >>> tried your suggestion and I replaced integers values with constants from
> >>> samba.dcerpc.security and samba.ntstatus.
> >>>
> >>> Replaced arguments SEC_FLAG_SYSTEM_SECURITY and SEC_STD_READ_CONTROL are
> >>> working well but exceptions contain different error numbers than
> >>> NT_STATUS_PRIVILEGE_NOT_HELD and NT_STATUS_ACCESS_DENIED and I cannot
> >>> find the right ones.
> >>>
> >>> NT_STATUS_PRIVILEGE_NOT_HELD = 3221225569 but exception contains
> >>> -1073741727 NT_STATUS_ACCESS_DENIED = 3221225506 but exception contains
> >>> -1073741790>>
> >> Sounds like the bug. The exception seems to return an int where it should
> >> return an unsigned int??
> >>
> >> i2c -1073741727
> >> 18446744072635809889 0xFFFFFFFFC0000061 01777777777770000000141
> >>
> >> #define NT_STATUS_PRIVILEGE_NOT_HELD NT_STATUS(0xc0000061)
> >
> > I think:
> >
> > #define PyErr_FromNTSTATUS(status) Py_BuildValue("(i,s)",
> > NT_STATUS_V(status), discard_const_p(char,
> > get_friendly_nt_error_msg(status)))
> >
> > in pyerrors.h is wrong.
> >
> > We probably want an unsigned int, not a signed int, for the first
> > constant.  We first saw this in some other patches that Gary did, where
> > he ended up casting via a C type casting lib to make it work, but
> > didn't dig into it properly.
> >
> > Thanks,
> >
> > Andrew Bartlett
>
> Hello.
>
> Thanks a lot, Andrew. I fixed the bug you mentioned and now everything
> is working.
>
> Patchset rebased and attached.


RB+

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Port of samba.security Python module

Samba - samba-technical mailing list
On Wed, 2017-09-06 at 10:11 +0200, Andreas Schneider wrote:

> On Wednesday, 6 September 2017 09:33:52 CEST Lumir Balhar wrote:
> >
> > Hello.
> >
> > Thanks a lot, Andrew. I fixed the bug you mentioned and now everything
> > is working.
> >
> > Patchset rebased and attached.
>
>
> RB+

Reviewed and pushed to autobuild!

Thanks for you patience here.  Are you waiting on anything else, other
than the ABI thing?

Thanks,

Andrew Bartlett


--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Port of samba.security Python module

Samba - samba-technical mailing list
On 09/06/2017 11:36 AM, Andrew Bartlett wrote:

> On Wed, 2017-09-06 at 10:11 +0200, Andreas Schneider wrote:
>> On Wednesday, 6 September 2017 09:33:52 CEST Lumir Balhar wrote:
>>> Hello.
>>>
>>> Thanks a lot, Andrew. I fixed the bug you mentioned and now everything
>>> is working.
>>>
>>> Patchset rebased and attached.
>>
>> RB+
> Reviewed and pushed to autobuild!
>
> Thanks for you patience here.  Are you waiting on anything else, other
Thank you for your help. I have a lot of work to do so some waiting is
not a big problem. I still cannot solve the issue with samba.netbios
[0]. Do you know anybody who can help me with this?

[0] https://lists.samba.org/archive/samba-technical/2017-August/122181.html
> than the ABI thing?
>
> Thanks,
>
> Andrew Bartlett
>
Thank you one more time. I think that everything is going well.

Have a nice day.
Lumír