go-ldap / ldap

Basic LDAP v3 functionality for the GO programming language.
Other
2.19k stars 352 forks source link

Attributes retrieve are case sensitive #129

Closed memorais closed 5 years ago

memorais commented 6 years ago

If we use GetAttributeValue function in a search to retrieve an specific attribute, the attribute name is currently case sensitive in this API.

According to LDAP specifications attribute retrieval must be case insensitive. For exemple, the attribute givenName and givenname are the same attribute, therefore if a client ask for "givenname" and LDAP definitions is set to "givenName", the API must be case insensitive returning the attribute and its value to the client, if present of course.

liggitt commented 6 years ago

According to LDAP specifications attribute retrieval must be case insensitive.

Can you provide a reference for that?

memorais commented 6 years ago

https://tools.ietf.org/html/rfc4511#section-4.5.1.8 "LDAPString values of this field are constrained to the following Augmented Backus-Naur Form (ABNF) [RFC4234]: ..."

https://tools.ietf.org/html/rfc4234#section-2.1 "Rule names are case-insensitive"

This behavior could be found in other wide used ldap clients like "ldapsearch" from OpenLDAP for example.

johnweldon commented 6 years ago

The relevant link is https://tools.ietf.org/html/rfc4512#section-2.5 I think.

johnweldon commented 6 years ago

Actually, none of these links seem conclusive. When using the attribute name we're using an alias for the OID (which doesn't have case), and rfc 4512 doesn't seem to clarify whether matching on the alias should be case sensitive. It's clear that the attribute name (alias) itself is just a utf-8 string.

memorais commented 6 years ago

IMHO it does not make sense not returning attributes based on their sensitiveness. If searches like '(givenname=*)' will work on server, no matter if LDAP definition is 'givenName' or 'givenname', this should work at the same manner for returned attributes.

liggitt commented 6 years ago

does that mean the server can return multiple attributes?

Givenname=foo
givenName=bar
GIVENNAME=baz

what should the behavior of getAttribute("givenname") be in that circumstance?

memorais commented 6 years ago

The server will return based on the schema definition, in this case it will return:

givenName=foo givenName=bar givenName=baz

Since this attribute is multi-valued, getAttribute("givenname") should be an array of all 3 givenName values.

liggitt commented 6 years ago

can a schema define multiple attributes that differ only by case?

if it is actually impossible to get a response like https://github.com/go-ldap/ldap/issues/129#issuecomment-326393033 from a server, I'm not necessarily opposed to something like https://github.com/go-ldap/ldap/pull/111, but I'm very wary of compressing a value space when it has been unclear to me whether the case insensitivity applies to specific uses of attributes (like DN formation and the search operation) or globally

memorais commented 6 years ago

No, it can't.

From LDAP point of view it applies globally. What we should take care is the value of attributes because of matching rules. Some attributes may have matching rules defined to be case sensitive. For example, java.schema from OpenLDAP have the attribute 'javaClassName' which have an equality rule 'caseExactMatch'. This have implications on how clients handle searches and values retrieval.

vetinari commented 6 years ago

@liggitt can we then merge #111? With the merge of master which fixes the race condition it passes... and what about #54 (sorry for the hijack ;-))

johnweldon commented 6 years ago

I still haven't found information in any spec to support this statement:

According to LDAP specifications attribute retrieval must be case insensitive.

However, going with the principle of least surprise, if we can document at least a few mainstream tools or libraries that default to case-insensitive attribute matching and searching, then I'd be open to having an exported package variable to toggle this feature on or off, and we can even default to having it enabled, as long as consumers can revert to case-sensitive behaviour as needed.

liggitt commented 6 years ago

a parallel set of case insensitive functions could make sense as well

memorais commented 6 years ago

@johnweldon And you found any information in spec which states that attribute names should be case sensitive?

One of the most used clients (ldapsearch from OpenLDAP as I said before) don't rely on case sensitiveness of the attribute names. In fact I never used an LDAP client which behaves like this API in this particular subject. You can also check tools like Apache Directory Studio [1] which is also widely used.

[1] - http://directory.apache.org/studio/

liggitt commented 6 years ago

And you found any information in spec which states that attribute names should be case sensitive?

For a low-level library, preserving fidelity of returned information is extremely important. Since it is possible for an LDAP server to return a set of attributes like the following, this library should not silently compress those differences:

Givenname=foo
givenName=bar
GIVENNAME=baz

I'd be amenable to adding GetAttributeValueIgnoreCase, etc, with clear documentation of behavior given an example like that

vetinari commented 6 years ago

@johnweldon, @liggitt IMO RFC 4512 / Section 2.5 does explicitly state that attribute names are case insensitive:

An attribute description is represented by the ABNF:

 attributedescription = attributetype options
 attributetype = oid
 options = *( SEMI option )
 option = 1*keychar

where \<attributetype> identifies the attribute type and each \<option> identifies an attribute option. Both \<attributetype> and \<option> productions are case insensitive.

The oid part is defined at end of section 1.4 (Common ABNF Productions):

Where either an object identifier or a short name may be specified, the following production is used:

 oid = descr / numericoid
johnweldon commented 6 years ago

@vetinari I think you're right; some quotes from 1.4 of RFC 4512: (emphasis mine)

Short names, also known as descriptors, are used as more readable aliases for object identifiers. Short names are case insensitive and conform to the ABNF:

  descr = keystring

Where either an object identifier or a short name may be specified, the following production is used:

  oid = descr / numericoid
memorais commented 6 years ago

Right, this should be the default behavior of the API.

johnweldon commented 6 years ago

I'm inclined to still approach this as @liggitt suggests - with parallel functions that are case insensitive. We shouldn't be changing the behaviour of the established API IMO.

vetinari commented 6 years ago

I've added now GetEqualFoldAttribute* in #111, but with switching to v3 we could use those as defaults and the old ones as e.g. GetAttributeValueCaseSensitive.

liggitt commented 6 years ago

with switching to v3 we could use those as defaults and the old ones as e.g. GetAttributeValueCaseSensitive.

I'd prefer to keep any value transformation done by this library explicit. If the comparison is ignoring case, the method name should make that clear