Bug 6157 - LDAP: MultiValue uid: "attribute uid has 2 values, expected only one" "init_sam_from_ldap: No uid attribute found for this user!"
LDAP: MultiValue uid: "attribute uid has 2 values, expected only one" "init_s...
Status: RESOLVED FIXED
Product: Samba 3.2
Classification: Unclassified
Component: User & Group Accounts
3.2.8
Other Linux
: P3 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-04 06:30 UTC by Noël Köthe
Modified: 2010-01-19 04:44 UTC (History)
5 users (show)

See Also:


Attachments
patch (3.21 KB, patch)
2009-03-12 11:25 UTC, Volker Lendecke
no flags Details
patch (3.16 KB, patch)
2009-04-01 06:53 UTC, Volker Lendecke
no flags Details
patch for 3.3 (3.19 KB, patch)
2009-05-12 09:08 UTC, Volker Lendecke
no flags Details
New patch for v3-3 (3.09 KB, patch)
2010-01-12 07:40 UTC, Stefan Metzmacher
idra: review+
Details
Patch for v3-5 (3.37 KB, patch)
2010-01-13 19:33 UTC, Stefan Metzmacher
idra: review+
Details
Patch for v3-4 (3.37 KB, patch)
2010-01-13 19:34 UTC, Stefan Metzmacher
idra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Noël Köthe 2009-03-04 06:30:51 UTC
Hello,

we are using samba 3.0 with two uid: LDAP attribute (multivalue):

dn: uid=justauser,ou=Users,ou=company,dc=tld
...
uid: justauser
uid: justauser@something.company.tld
...

This is working fine with 3.0 but in our samba 3.2.8 tests we got the following error:

"attribute uid has 2 values, expected only one
init_sam_from_ldap: No uid attribute found for this user!"

uid is a multivalue an from the LDAP view this is OK and because of other applications using the LDAP its not possible to remove one of the uid's to have only one.:(

IMHO samba 3.2 should support uid multivalue like 3.0 or is there a reason I cannot see why this isn't allowed anymore?

Thank you.

Regards.

  Noèl
Comment 1 Noël Köthe 2009-03-04 06:32:15 UTC
Just as additional info:

http://lists.samba.org/archive/samba-technical/2008-September/061452.html

He was lucky he could remove additional uids.;)
Comment 2 Volker Lendecke 2009-03-09 07:04:18 UTC
Hmm. Okay. We now check that in lib/smbldap.c:313. The question is: How should we figure out the right value in the multivalue field? Samba 3.0 just used the first value that came back from ldapsearch, but this is not really deterministic.

Volker
Comment 3 Simo Sorce 2009-03-09 08:53:14 UTC
(In reply to comment #2)
> Hmm. Okay. We now check that in lib/smbldap.c:313. The question is: How should
> we figure out the right value in the multivalue field? Samba 3.0 just used the
> first value that came back from ldapsearch, but this is not really
> deterministic.

We don't.
We assume that uid is a single valued attribute (and so does a lot of other software).

We should just consider this scenario invalid, if Noèl wants to keep using this schema, IMHO, he has to come up with his own patch to samba that is able to pick an choose the right value for him.
Comment 4 Volker Lendecke 2009-03-09 08:54:29 UTC
Well, this used to work in 3.0, where we did not check. So you could call that a regression.

Volker
Comment 5 Simo Sorce 2009-03-09 08:58:06 UTC
(In reply to comment #4)
> Well, this used to work in 3.0, where we did not check. So you could call that
> a regression.

Personally I think it was a bug in 3.0.x, that we did not check. So from my POV we just fixed bad behavior.

Simo.
Comment 6 Volker Lendecke 2009-03-09 09:04:44 UTC
The LDAP RFCs seem to make "uid" a multi-value attribute. Why shouldn't we support that? For those who have a "safe" directory, this does not matter. The others get the non-deterministic behaviour?

Volker
Comment 7 Simo Sorce 2009-03-09 10:54:29 UTC
(In reply to comment #6)
> The LDAP RFCs seem to make "uid" a multi-value attribute. Why shouldn't we
> support that? For those who have a "safe" directory, this does not matter. The
> others get the non-deterministic behaviour?

Which one do you pick ?
Order is not defined in LDAP, so you might get any value as the first one.
That would make the behaviour unpredictable.

I personally think an unpredictable behaviour is worst than throwing an error if multiple uids are found.

Unless there is a way we can pick the right value somehow.
Comment 8 Volker Lendecke 2009-03-09 11:04:06 UTC
Use the shortest value? Or the longest? Or do an alphabetic sort and use the first one? Thinking about it, I like the "alphabetically smallest" one :-)

Volker
Comment 9 Volker Lendecke 2009-03-12 11:25:22 UTC
Created attachment 3990 [details]
patch

The attached patch compiles but is completely untested. It implements the idea to pick the alphabetically smallest value of the multiple ones in the "uid" attribute.

Noel, can you test that?

Simo, what do you think?

Thanks,

Volker
Comment 10 Simo Sorce 2009-03-12 13:13:08 UTC
(In reply to comment #9)
> Created an attachment (id=3990) [edit]
> patch
> 
> The attached patch compiles but is completely untested. It implements the idea
> to pick the alphabetically smallest value of the multiple ones in the "uid"
> attribute.
> 
> Noel, can you test that?
> 
> Simo, what do you think?

I am not sure that picking the alphabetically smallest value is always the right choice, but at least it is predictable.

I was thinking that another way (possibly more useful) could be to provide a new parameter were you can set a regular expression to match the value you want to use, and fall back to alphabetic sorting if no value or more than one value match.
This way I could have a regex like (.*)[^@]* and that would match only entries that do not contain the @ sign (the regex may not be correct, but you get the idea :-).
But I understand that implementing regex matching would take a bigger patch and new options, and I don't have time to implement it.

So as far as I am concerned alphabetic sorting is probably better than nothing for now.
Comment 11 Noël Köthe 2009-03-17 03:07:55 UTC
Thanks Volker.

I will test it today and report the results.
Comment 12 Volker Lendecke 2009-03-25 08:24:15 UTC
Noel, any updates here?

Volker
Comment 13 Noël Köthe 2009-03-31 10:24:30 UTC
Sorry for failing my own dates.:(

I tried to build 3.2.8 with your patch but got the following error:

Compiling lib/winbind_util.c
Compiling lib/smbldap.c
lib/smbldap.c: In function `smbldap_talloc_smallest_attribute':
lib/smbldap.c:355: error: too many arguments to function `pull_utf8_talloc'
lib/smbldap.c:367: error: too many arguments to function `pull_utf8_talloc'
The following command failed:
i386-linux-gcc -I. -I/tmp/buildd/samba-3.2.8/source  -O 
-D_SAMBA_BUILD_=3  -I/tmp/buildd/samba-3.2.8/source/iniparser/src 
-Iinclude -I./include  -I. -I. -I./lib/replace -I./lib/talloc 
-I./lib/tdb/include -I./libaddns -I./librpc -DHAVE_CONFIG_H 
-D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE -Iinclude 
-I./include -I. -I. -I./lib/replace -I./lib/talloc -I./lib/tdb/include 
-I./libaddns -I./librpc -I./popt -DLDAP_DEPRECATED   -I/include 
-I/tmp/buildd/samba-3.2.8/source/lib -D_SAMBA_BUILD_=3 -fPIC -c 
lib/smbldap.c -o lib/smbldap.o
make[1]: *** [lib/smbldap.o] Error 1
make[1]: Leaving directory `/tmp/buildd/samba-3.2.8/source'
make: *** [build-stamp] Error 2

Comment 14 Volker Lendecke 2009-04-01 06:53:47 UTC
Created attachment 4042 [details]
patch

Sorry. I've just built the attached new patch in a 3.2.8 successfully.

Volker
Comment 15 Noël Köthe 2009-04-15 08:43:05 UTC
Hello Volker,

thank you for the updated patch.
I tested it today with 3.2.8 and is working fine.

The users with two uid attributes can connect and use the server.

We will use this patch with 3.2.8 but integration into samba (3.3.x, no need from our side for 3.2) would be great.

Thank you.
Comment 16 Volker Lendecke 2009-05-12 09:08:23 UTC
Created attachment 4145 [details]
patch for 3.3

Is this a candidate for 3.3?

Volker
Comment 17 Volker Lendecke 2009-05-15 15:21:42 UTC
Will be in 3.4. Nobody was found who would vote this in for 3.3.

Sorry,

Volker
Comment 18 Karolin Seeger 2009-05-29 06:50:27 UTC
As this is a regression, I would vote for picking this patch for 3.3!
Comment 19 Karolin Seeger 2009-05-29 06:50:58 UTC
Jeremy, would you like to review the patch?

Thanks!
Comment 20 Jeremy Allison 2009-05-29 18:11:48 UTC
Ok, patch looks good - does exactly what it says.
Thanks,
Jeremy.
Comment 21 Karolin Seeger 2009-06-06 08:44:39 UTC
Pushed, will be included in 3.3.5.
Closing out bug report.

Thanks for reporting!
Comment 22 Stefan Metzmacher 2010-01-05 05:58:16 UTC
All LDAP-Server I've seen yet (AD and OpenLDAP 2.x.x) return the values
in the order they were stored.

There're Samba 3.0 setups which rely on the first uid being used.
As that's also what nss_ldap will return.

Should we add an option so select between "alphabetically smallest"
and "first" value. Or should be just restore the Samba 3.0.x behavior?
Comment 23 Stefan Metzmacher 2010-01-05 08:48:02 UTC
A discussion on IRC;

14:10 <@idra> metze, ldap does not guarantee any order, relying on the order of the attributes is just plain 
              wrong
14:14 < metze> what LDAP do you know that doesn't return the same order that's stored?
14:14 <@idra> metze, I don't know of specific examples, but you just rely on specific implementations behavior
14:15 <@idra> metze, however if nss_ldap always picks the first we should probably do the same otherwise 
              things could break
14:15 < blingme> is this for enumeration, or selection of a specific account?
14:15 < metze> I think both
14:16 < blingme> well, multi-valued or not should only affect enumeration
14:16 < blingme> for authn/authz of a specific account, the filter, and/or matched result extension should be 
                 enough?
14:17 < blingme> (but, I'm not familiar with samba internals)
14:17 < metze> we search for (uid=%s) which will match any uid of the account
14:18 < metze> and then use the first (in 3.0.x) uid of the result to fill in the account name of the user
14:19 < metze> so the user can login with any uid from the client, but samba only uses the primary uid
14:19 < metze> e.g. for %U substitutions
14:24 < blingme> would it make sense to check if the attribute name is an/the RDN ?
14:25 < blingme> (in the example from the user, this is the case)
14:34 < metze> maybe a good idea, I'll check
Comment 24 Stefan Metzmacher 2010-01-05 09:00:52 UTC
As far as I can see, nss_ldap just returns the first value.

I'd preferr to just do the same...



Comment 25 Andrew Bartlett 2010-01-06 19:08:13 UTC
My view is that we should be exactly as broken as nss_ldap (because it is common software paring), after the administrator sets an option.  (If we must support this at all). 

The e-mail address login thing is neat however, and I've had folks asking about it on IRC (migration from AD). 

But I wish we had stayed strict - it's just asking for trouble. 
Comment 26 Stefan Metzmacher 2010-01-12 07:40:28 UTC
Created attachment 5160 [details]
New patch for v3-3

Is that ok for v3-3?
Comment 27 Simo Sorce 2010-01-12 08:30:15 UTC
Looks good to me
Comment 28 Karolin Seeger 2010-01-13 06:17:17 UTC
Pushed Metze's patches to v3-3-test.
Re-assigning to Metze to add the same patch for v3-4-test.
Comment 29 Stefan Metzmacher 2010-01-13 19:33:17 UTC
Created attachment 5174 [details]
Patch for v3-5
Comment 30 Stefan Metzmacher 2010-01-13 19:34:43 UTC
Created attachment 5175 [details]
Patch for v3-4
Comment 31 Stefan Metzmacher 2010-01-19 04:37:21 UTC
Can we get that into 3.4.6 and 3.5.0rc2?
Comment 32 Karolin Seeger 2010-01-19 04:44:02 UTC
Yes, pushed to v3-5-test and v3-4-test.
Closing out bug report.

Thanks!