ipaldap: handle binary encoding option transparently

[frasertweedale]

This patchset addresses
https://fedorahosted.org/freeipa/ticket/6529. I'm publishing it for
discussion and review but it should not be hastily merged because
there are compatibility implications for older server versions
(discussed in https://fedorahosted.org/freeipa/ticket/6530).

Per RFC 4523, particular attribute syntaxes require use of the
';binary' attribute option. Attributes using these syntaxes include
'userCertificate' and 'cACertificate'. Our handling of this requirement is
inconsistent with no library support (i.e. it was up to individual plugin
authors to "do the right thing".

Also, because 389 DS currently does not always use this encoding option
(which is a defect), whether you need to read the
'userCertificate' or 'userCertificate;binary' attribute - or both - is
often unclear. But technically, these both refer to the same attribute,
because the 'binary' option does not specify an attribute subtype.

This commit implements proper handling of the binary encoding option within
ipaldap. Plugin code should now always use an attribute description
without the binary encoding option, and allow ipaldap to automatically
add it when sending attribute to the server, and strip it when reading
attributes in search results.

[HonzaCholasta]

ipaldap is not the proper place to handle this - it implements a (almost) generic LDAP client, not a 389 DS client, and as such should not contain any 389 DS specific hacks. The premise is that the server gets exactly what the user of ipaldap requested, and the user gets exactly what the server returned.

The binary transfer option is currently handled in code of the affected commands. The next layer below is baseldap, which is where the handling should be moved to make it generic for all commands.

Also note that the real bug in 389 DS is that it defines the attribute types to use octet string syntax, rather than the certificate syntax as defined in RFC 4523. It actually behaves correctly, not enforcing the binary transfer option on attribute types with octet string syntax.

[frasertweedale]

@jcholast I disagree. If ipaldap is a generic LDAP client, it should obey the RFCs and always transfer the relevant attributes (userCertificate, cACertificate, etc) with the ;binary encoding option, and it should expect to see it when reading the relevant attributes from the server. IMO ipaldap should handle this transparently because it is part of the LDAP protocol. There is no 389DS-specific hack in my proposed change (but I'm curious about what part of it you feel is).

This would also avoid inconsistent handling of relevant attributes between different plugins, which is the situation we currently have. But apart from the inconsisency (which is a nusiance) we have a bigger problem - in several plugins we specifically try to read userCertificate, but a RFC 4522 compliant server (which 389DS is not now, but hopefully one day will be) will always return userCertificate;binary. So, our current code breaks if/when that happens. Furthermore, other RFC 4522-compliant programs that correctly use the ;binary transfer encoding option to, e.g. write certificates to user entries, will cause those certificates to be unreadable by current IPA plugin code. This is not good enough.

Also note that the real bug in 389 DS is that it defines the attribute types to use octet string syntax, rather than the certificate syntax as defined in RFC 4523. It actually behaves correctly, not enforcing the binary transfer option on attribute types with octet string syntax.

389DS does not behave correctly; it's treatment of ;binary is wrong in several ways, apart from the incorrect attribute syntax for relevant attributes.

[HonzaCholasta]

If ipaldap is a generic LDAP client, it should obey the RFCs and always transfer the relevant attributes (userCertificate, cACertificate, etc) with the ;binary encoding option, and it should expect to see it when reading the relevant attributes from the server.

No, it should respect whatever is defined on the server, otherwise it's not a generic LDAP client. If the server does something wrong, it has to be fixed there, on the server. The goal of ipaldap is not to make buggy or non-LDAPv3 (e.g. AD) servers look like they are LDAPv3-compliant, the goal is to interpret attributes according to the server-defined schema.

IMO ipaldap should handle this transparently because it is part of the LDAP protocol.

Nowhere in the RFCs is it mandated that a compliant client cannot request the attributes without the option, nor that it must not accept the attributes without the option in server responses. If this was true, it would have to be fixed in OpenLDAP libs anyway, not in ipaldap.

There is no 389DS-specific hack in my proposed change (but I'm curious about what part of it you feel is).

The part where you implicitly add the binary transfer option to attribute names (although not mandated on clients by any RFC) without knowing how the attribute types are defined on the server (although mandated only on attribute types with the certificate syntax by RFC 4523) .

This would also avoid inconsistent handling of relevant attributes between different plugins, which is the situation we currently have.

This is because of historical reasons (the original implementation of host and service plugins used userCertificate instead of userCertificate;binary) and will have to stay this way at least until all of the buggy 389 DS / IPA releases go out of support.

But apart from the inconsisency (which is a nusiance) we have a bigger problem - in several plugins we specifically try to read userCertificate, but a RFC 4522 compliant server (which 389DS is not now, but hopefully one day will be) will always return userCertificate;binary. So, our current code breaks if/when that happens. Furthermore, other RFC 4522-compliant programs that correctly use the ;binary transfer encoding option to, e.g. write certificates to user entries, will cause those certificates to be unreadable by current IPA plugin code. This is not good enough.

We can easily fix the plugins to read from userCertificate;binary in addition to userCertificate. We have to continue to write to userCertificate only though, because of backward compatibility with older servers.

389DS does not behave correctly; it's treatment of ;binary is wrong in several ways, apart from the incorrect attribute syntax for relevant attributes.

Not enforcing ;binary on attribute types with octet string syntax is correct. I was not trying to imply anything else.

[frasertweedale]

OK, let's just fix all the plugins / other routines that deal with the relevant attributes to explicitly read both userCertificate and userCertificate;binary and concat the results. I think there is a lot more we could and should do to improve usability w.r.t. these attributes but it will do for now. Closing this PR.