Bug 8929 - correct "servicePrincipalName" handling
Summary: correct "servicePrincipalName" handling
Status: ASSIGNED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB (show other bugs)
Version: 4.13.3
Hardware: All All
: P5 critical (vote)
Target Milestone: ---
Assignee: Andrew Bartlett
QA Contact: samba4-qa@samba.org
URL:
Keywords:
Depends on: 7485
Blocks: 13361
  Show dependency treegraph
 
Reported: 2012-05-11 07:13 UTC by Matthias Dieter Wallnöfer
Modified: 2021-07-04 20:25 UTC (History)
8 users (show)

See Also:
mdw: review+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Dieter Wallnöfer 2012-05-11 07:13:46 UTC
"servicePrincipalName" should be treated case-insensitively in respect to matching. This means that "HOST/somename" is not allowed when "host/somename" is already there.

See thread "cannot rename windows xp machine in samba4" on samba-technical.

I really think that this needs to be fixed before s4 final.
Comment 1 Stefan Metzmacher 2012-10-22 14:42:16 UTC
This branch seems to have some patches for the problem...

s4:sam.py - "servicePrincipalName" - test for case-insensitiveness
https://gitweb.samba.org/?p=mdw/samba.git;a=shortlog;h=44353515af2

Andrew, Matthias any plan to get this to master/v4-0-test?
Comment 2 Matthias Dieter Wallnöfer 2012-10-22 20:08:42 UTC
They need some rework in order to pass the tests. I'm sorry but I cannot do it at the moment.

I paste you the explanations which Andrew gave me some time ago:
> Sadly this has required a fair bit of work, even to just get the obvious
> problems sorted.

> In particular, the error codes from the operator_fn() hook cannot be
> ignored, and we should not pass in a memory context to a function that
> does not itself return memory. 

> I've not had a chance to actually test these, and I'm a little worried
> about what would happen for the cases where we never match (element
> values containing a deleted DN).

> I've pushed my rework (compiles, untested so far) to my mdw-master
> branch.
Comment 3 Matthieu Patou 2012-11-06 08:39:30 UTC
Moving to 4.1 as I don't think it's critical for 4.0.0 fix can be made for 4.0.x
Comment 4 Matthias Dieter Wallnöfer 2013-05-17 13:12:22 UTC
Metze, please have a look at my updated branch: http://gitweb.samba.org/samba.git/?p=mdw/samba.git;a=log;h=refs/heads/ldb_schema.

I think I have found the issue which prevented this from working.
Comment 5 Andrew Bartlett 2013-06-27 08:51:29 UTC
the ldb_schema branch fails autobuild due to errors in the 'make test' of ldb.  That is the main reason this wasn't merged.
Comment 6 Matthieu Patou 2013-08-21 04:39:15 UTC
Matthias can you have a look at why your branch fails autobuid ?
Comment 7 Stefan Metzmacher 2013-08-29 08:05:59 UTC
(In reply to comment #6)
> Matthias can you have a look at why your branch fails autobuid ?

I think this will pass autobuild:

https://gitweb.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-mdw

Matthias and Andrew, can I add your review and push it to master?

Simo, can we also get your review?
Comment 8 Stefan Metzmacher 2013-08-29 10:27:25 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > Matthias can you have a look at why your branch fails autobuid ?
> 
> I think this will pass autobuild:
> 
> https://gitweb.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-mdw
> 
> Matthias and Andrew, can I add your review and push it to master?
> 
> Simo, can we also get your review?

https://gitweb.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=780f108588616b
passed autobuild twice for me.
Comment 9 Karolin Seeger 2013-08-30 08:29:31 UTC
Is this a blocker for 4.1.0 or 4.2?
Comment 10 Karolin Seeger 2013-08-30 09:47:27 UTC
(In reply to comment #9)
> Is this a blocker for 4.1.0 or 4.2?

Comment from Metze:

For the patches for https://bugzilla.samba.org/show_bug.cgi?id=8929
(correct "servicePrincipalName" handling)
I just need the permission from Andrew and Matthias to add their
sign-off to the patches. I already reviewed them, but I'd like to get
an ack from Simo.
See
https://gitweb.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=780f108588616b
Comment 11 Matthias Dieter Wallnöfer 2013-08-30 15:37:12 UTC
I am sorry that I have not answered earlier. The patches are perfectly okay.
Comment 12 Andrew Bartlett 2013-09-02 20:34:57 UTC
These tags may be added to https://gitweb.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=780f108588616b

Reviewed-by: Andrew Bartlett <abartlet@samba.org>
Signed-off-by: Andrew Bartlett <abartlet@samba.org>

If as indicated this all passes autobuild, it will be great to finally have this fixed in the tree (even if it does make some things slower).
Comment 13 Matthias Dieter Wallnöfer 2013-09-05 15:30:47 UTC
This should be the fixed up version of the last patch (test-generic.sh). Please review!

http://gitweb.samba.org/samba.git/?p=mdw/samba.git;a=commitdiff;h=118a2eee516e1bf8c4c7b799f00fab2f567ac58d
Comment 14 Karolin Seeger 2013-09-16 07:56:35 UTC
Please attach the patches and review flags to the bug report! Thanks!
Comment 15 Karolin Seeger 2013-09-20 07:12:59 UTC
ping
Comment 16 Stefan Metzmacher 2013-09-27 07:56:48 UTC
This gets larger than expected :-(

https://gitweb.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-mdw

Don't block 4.1.0 it's not a regression compared to 4.0.x
Comment 17 Karolin Seeger 2013-12-10 15:34:27 UTC
Any news on this one?
Comment 18 Clement Wong 2014-03-06 00:57:49 UTC
Guys, I run into a 8418 mismatch problem and it seems to be cause by this issue, any progress on this? Can it make it into 4.2?
This is really important for us.
I would love to help testing.
Comment 19 Matthias Dieter Wallnöfer 2015-12-28 20:58:00 UTC
@metze: any news here? Did you manage to do something?

I stumbled over my patches written some years ago and so I wonder if this is still a current issue. Or has it been solved differently?
Comment 20 Matthias Dieter Wallnöfer 2017-12-29 08:29:25 UTC
Hi folks,

since I still think that this is a very crucial point for Samba's directory implementation and I have seen no one intended to fix I have resumed the work on this issue.

The decision was also motivated by the recent schema work done by Andrew and his colleagues at Catalyst. 

@metze + @abartlet: Please find my updated branch on: https://git.samba.org/samba.git/?p=mdw/samba.git;a=log;h=refs/heads/ldb_schema.new

My idea would be to launch a private autobuild on this to see if it really works. How can I approach this?
Comment 21 Andrew Bartlett 2018-01-10 01:39:51 UTC
LDB tests fail with:

[==========] 44 test(s) run.
[==========] Running 2 test(s).
[ RUN      ] test_ldb_msg_find_duplicate_val
[  FAILED  ] test_ldb_msg_find_duplicate_val
[ RUN      ] test_ldb_msg_find_common_values
[  FAILED  ] test_ldb_msg_find_common_values
[==========] 2 test(s) run.
Comment 22 Andrew Bartlett 2018-03-12 05:57:34 UTC
Just to connect to dots, here is the associated mail thread for the most recent re-raising of this:

https://lists.samba.org/archive/samba-technical/2018-January/124878.html
Comment 23 Björn Jacke 2019-01-23 21:23:45 UTC
did the status of this bug change in the meantime?

I got a glance at a setup that got updated to 4.9.4, which had (preexisting) duplicate servicePrincipalName attributes. dbcheck did not find any error but ldbedit was complaining about indices pointing to different GUIDs. This got fixed manually then with the previous non guid indexing samba release.

This bug really needs a proper fix and dbcheck also needs to find those cases.
Comment 24 Andrew Bartlett 2019-01-23 23:17:10 UTC
(In reply to Björn Jacke from comment #23)
Sadly no.

I agree it would be great if the mythical 'someone' could find the time to work on this.  Now that we have public CI we are at least able to test the full implication of the patches more easily.

The big requirement placed on any patch will be lots of tests, both unit tests at the LDB layer and integration tests on the LDAP server.
Comment 25 Björn Jacke 2019-01-24 11:32:18 UTC
the point is that this was a "minor" issue until 4.7 this became critical issue with the introduction of the guid indexing. Anyone updating from 4.7 or earlier to 4.8 or later gets really into trouble by this bug now.
Comment 26 Andrew Bartlett 2019-01-25 04:23:03 UTC
(In reply to Björn Jacke from comment #25)
I'm confused as to why this is now critical, it is just a warning. 

The warning was put in by 
https://attachments.samba.org/attachment.cgi?id=14166 of bug 13335, which was a critical regression.
Comment 27 Matthias Dieter Wallnöfer 2021-07-04 20:25:59 UTC
Again I have rebased the patch, so if someone would like to test it (https://gitlab.com/samba-team/samba/-/merge_requests/698).