[PATCH] cracknames: add python test & fix issues (bug #12842)

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] cracknames: add python test & fix issues (bug #12842)

Samba - samba-technical mailing list
Hi all,

The attached patch #2 fixes the flapping test in rpc.cracknames, and the
first patch tests this fix as well as the implementation of user and
service principals.

Before this patch, we could potentially return two results from a
cracknames trying to map from an account name to a GUID, in the scenario
where a deleted user had the same account name as a non-deleted user
(this would happen with any applicable attribute). This was just due to
a check being the wrong way around; we should search in the recycled
partition when we're mapping _from_ a GUID, since there can only be one
object mapped from a GUID and it's possible that we're trying to find a
recycled object in this scenario.

The third patch adds support for getting user principals and service
principals from cracknames. While poking around in it, I found that
these two weren't implemented and that it would be trivial to do so.
Note that, since service principal is a multi-valued attribute, we check
if the result would be unique before returning. Windows also gives
DS_NAME_STATUS_NOT_UNIQUE if there is more than one result, but also
returns null whereas we appear to return the first result. In addition
to this, we don't support SID_OR_SID_HISTORY_NAME or DNS_DOMAIN_NAME. We
also don't support any of the formats in MS-DRSR 4.1.4.1.2. This could
be a potential future task, but seems like it would be non-trivial since
some of these aren't just for converting from one format to another.

Please review & push if applicable.

Thanks,
Bob


cracknames-patch (20K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] cracknames: add python test & fix issues (bug #12842)

Samba - samba-technical mailing list
Hi again,

Turns out that I actually broke the existing cracknames test with this;
oops. Attached is a patch which passes. The change is using a different
error code when it can't find the expected format.

Thanks,
Bob


On 06/07/17 13:23, Bob Campbell wrote:

> Hi all,
>
> The attached patch #2 fixes the flapping test in rpc.cracknames, and the
> first patch tests this fix as well as the implementation of user and
> service principals.
>
> Before this patch, we could potentially return two results from a
> cracknames trying to map from an account name to a GUID, in the scenario
> where a deleted user had the same account name as a non-deleted user
> (this would happen with any applicable attribute). This was just due to
> a check being the wrong way around; we should search in the recycled
> partition when we're mapping _from_ a GUID, since there can only be one
> object mapped from a GUID and it's possible that we're trying to find a
> recycled object in this scenario.
>
> The third patch adds support for getting user principals and service
> principals from cracknames. While poking around in it, I found that
> these two weren't implemented and that it would be trivial to do so.
> Note that, since service principal is a multi-valued attribute, we check
> if the result would be unique before returning. Windows also gives
> DS_NAME_STATUS_NOT_UNIQUE if there is more than one result, but also
> returns null whereas we appear to return the first result. In addition
> to this, we don't support SID_OR_SID_HISTORY_NAME or DNS_DOMAIN_NAME. We
> also don't support any of the formats in MS-DRSR 4.1.4.1.2. This could
> be a potential future task, but seems like it would be non-trivial since
> some of these aren't just for converting from one format to another.
>
> Please review & push if applicable.
>
> Thanks,
> Bob
>


cracknames-fix-ammended (20K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] cracknames: add python test & fix issues (bug #12842)

Samba - samba-technical mailing list
On Mon, 2017-07-10 at 13:31 +1200, Bob Campbell via samba-technical
wrote:
> Hi again,
>
> Turns out that I actually broke the existing cracknames test with this;
> oops. Attached is a patch which passes. The change is using a different
> error code when it can't find the expected format.
>
> Thanks,
> Bob

Thanks Bob.  Can you confirm you have tested both against Windows?

One nit, normally the BUG lines go with the signed-off-by etc, not
first in the commit message.  Other than that:

Reviewed-by: Andrew Bartlett <[hidden email]>

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
|  
Report Content as Inappropriate

Re: [PATCH] cracknames: add python test & fix issues (bug #12842)

Samba - samba-technical mailing list
On 10/07/17 17:18, Andrew Bartlett via samba-technical wrote:

> On Mon, 2017-07-10 at 13:31 +1200, Bob Campbell via samba-technical
> wrote:
>> Hi again,
>>
>> Turns out that I actually broke the existing cracknames test with this;
>> oops. Attached is a patch which passes. The change is using a different
>> error code when it can't find the expected format.
>>
>> Thanks,
>> Bob
> Thanks Bob.  Can you confirm you have tested both against Windows?
>
> One nit, normally the BUG lines go with the signed-off-by etc, not
> first in the commit message.  Other than that:
>
> Reviewed-by: Andrew Bartlett <[hidden email]>
>
> Thanks!
>
> Andrew Bartlett
Hi Andrew,

Both tests do pass against Windows (2012R2).

Thanks,
Bob


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] cracknames: add python test & fix issues (bug #12842)

Samba - samba-technical mailing list
Hi,

Because of the way it's being tested (which is fine), the 3rd patch is
actually required for backporting the 2nd patch in order to keep
everything tested. This means that BUG: should probably be on the third
patch also.

Otherwise:
Reviewed-by: Garming Sam <[hidden email]>

Also, just a note, we now have a knownfail.d directory for temporary
failures so as to avoid merge conflicts. Make a new file in the
directory with the test failure, and delete the failure afterwards.

Cheers,

Garming

On 2017-07-11 09:16, Bob Campbell via samba-technical wrote:

> On 10/07/17 17:18, Andrew Bartlett via samba-technical wrote:
>> On Mon, 2017-07-10 at 13:31 +1200, Bob Campbell via samba-technical
>> wrote:
>>> Hi again,
>>>
>>> Turns out that I actually broke the existing cracknames test with
>>> this;
>>> oops. Attached is a patch which passes. The change is using a
>>> different
>>> error code when it can't find the expected format.
>>>
>>> Thanks,
>>> Bob
>> Thanks Bob.  Can you confirm you have tested both against Windows?
>>
>> One nit, normally the BUG lines go with the signed-off-by etc, not
>> first in the commit message.  Other than that:
>>
>> Reviewed-by: Andrew Bartlett <[hidden email]>
>>
>> Thanks!
>>
>> Andrew Bartlett
> Hi Andrew,
>
> Both tests do pass against Windows (2012R2).
>
> Thanks,
> Bob

Loading...