Skip to content

Commit

Permalink
SDAP: Merge multiple instance of the same attribute
Browse files Browse the repository at this point in the history
Some environments may send multiple instances of one attribute with
the same name, instead of a single multi-valued attribute.  With the
current parsing code, this leads to the same attribute-name being
observed multiple times but with the value of the first attributed.

One such case are user-defined, multi-valued attributes provided from
Google Secure LDAP: given that SSH public keys will be stored as
user-defined, multi-valued attributes in the Google Workspace
directory, the LDAP response will contain multiple attributes named
'sshPublicKey' with one value each.

The root-cause of the values from the first attribute-instance being
observed every time the same attribute-name is seen is with the API
used:
 - ldap_first_attribute/ldap_next_attribute will iterate over the
   BER message statefully (i.e., recording their current position
   in the message and continuing from there), returning the attribute
   name multiple times
 - ldap_get_values/ldap_get_values_len starts from the start of the
   BER message and returns the first instance with the provided
   attribute name.

This change rewrites the parsing code in sdap_parse_entry (and makes
it behave similarily to how ldapsearch works): it reads the message
from start to finish using the 'ber'-variants (ldap_get_dn_ber,
ldap_get_attribute_ber) of the accessor functions, which ensures that
every encountered attribute is returned together with the values
associated with that instance of the attribute.  This now correctly
supports storing (and retrieving!)  multiple values for sshPublicKey
as a user-defined, multi-valued field in the Google Workspace
Directory.  Even though this effectively rewrites the sdap_parse_entry
function, it retains much of the original structure (although
simplifying it) and attempts to print the same (or at least similar)
trace and debug messages.

Using the 'ber'-variants of these functions has the added benefit of
explicitly returning the internal ldap-errno (i.e., no more calls to
ldap_set_option/ldap_get_option) and not allocating extra memory (for
the value-array) that then has to be freed explicitly.

Note that ldap_get_dn_ber and ldap_get_attribute_ber have been present
(and publicly exported) in libldap since approx. 2002-09 (see commit
6a903bc1e54ce7fcf5b2fd06a390b33706be0cb1 in openldap.git), but have
been undocumented until 2021 (at least ldap_get_attribute_ber has been
added to the man-pages, whereas ldap_get_dn_ber is still only
'documented' by its prototype in ldap.h).

Signed-off-by: Philipp Tomsich <[email protected]>
Reviewed-by: Christoph Müllner <[email protected]>
  • Loading branch information
ptomsich committed Jan 5, 2024
1 parent ea7de58 commit 53145f3
Show file tree
Hide file tree
Showing 4 changed files with 243 additions and 157 deletions.
4 changes: 4 additions & 0 deletions Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -3155,6 +3155,10 @@ sdap_tests_LDFLAGS = \
-Wl,-wrap,ldap_value_free_len \
-Wl,-wrap,ldap_first_attribute \
-Wl,-wrap,ldap_next_attribute \
-Wl,-wrap,ldap_get_dn_ber \
-Wl,-wrap,ldap_get_attribute_ber \
-Wl,-wrap,ber_memfree \
-Wl,-wrap,ber_free \
$(NULL)
sdap_tests_LDADD = \
$(CMOCKA_LIBS) \
Expand Down
4 changes: 3 additions & 1 deletion src/external/ldap.m4
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,9 @@ AC_CHECK_FUNCS([ldap_control_create ldap_init_fd \
ldap_create_deref_control_value \
ldap_parse_derefresponse_control \
ldap_derefresponse_free \
ldap_is_ldapc_url])
ldap_is_ldapc_url \
ldap_get_dn_ber \
ldap_get_attribute_ber])
AC_CHECK_MEMBERS([struct ldap_conncb.lc_arg],
[AC_RUN_IFELSE(
[AC_LANG_PROGRAM(
Expand Down
Loading

0 comments on commit 53145f3

Please sign in to comment.