228704 - libldap and the LDAP tools should support SASL/Digest

Status: RESOLVED FIXED

(Directory :: LDAP C SDK, defect)

Description

[Mark Smith [:mcs]]

The LDAP command line tools should support SASL/Digest and possibly other SASL
mechanisms (currently only SASL/EXTERNAL is supported for cert. based
authentication).  Doing this will likely involve incorporating a general SASL
client framework.

It would also be good to look at OpenLDAP and Sun's interface for this feature,
e.g., which command line option is used to indicate SASL/Digest.

Comment 1

[nkinder]

Attachment: CVS DIffs

These changes implement SASL support in the ldapsdk libraries as well as the command-line tools.  Most of this code was contributed by Sun, but there were certain adjustments that had to be made for the merge to be clean in addition to adding other SASL related functionality, addressing porting issues, and bug fixes.  There are also 4 new source files which will be attached to this bug separately.

I have tested building and running the command-line tools with these changes on Fedora Core 5 32-bit, RHEL4 x86, RHEL4 x86_64, Solaris 2.9 32-bit sparc, Solaris 2.9 64-bit sparc, HP-UX 11i PA 64-bit, and HP-UX 11i IA64 64-bit.  Compiling with SASL support is optional, and is defined at configure time using the "--with-sasl", "--with-sasl-inc", and "--with-sasl-lib" configure options.  Using the SASL options requires Cyrus SASL v2.  My testing was done with Cyrus SASL v2.1.21.  I verified that the CRAM-MD5, DIGEST-MD5, and GSSAPI (using Kerberos tickets) authentication methods worked against Fedora Directory Server v1.0.2 on all of the above platforms.  The only notable exception is the GSSAPI mechanism does not yet work on the HP-UX platforms.  GSSAPI is not recognized as a valid mechanism in those cases.  I will address this in a separate bug.

I would like to have a cursory review before checking this into the "sun_merge_branch_20060523" branch.  Once the code is checked in there, we should do a more intensive review before checking it into the trunk.

Comment 2

[nkinder]

Attachment: New source file (sasl.c)

Comment 3

[nkinder]

Attachment: New source file (ldaptool-sasl.c)

Comment 4

[nkinder]

Attachment: New source file (ldaptool-sasl.h)

Comment 5

[nkinder]

Attachment: New source file (sasl.m4)

Comment 6

[Rich Megginson]

What about Windows and Mac?

Comment 7

[Mark Smith [:mcs]]

Adding Mike Dautermann to the CC list.

Strange as it may sound, I do not have superreview privileges.  We need an sr before committing to the trunk though, and we can ask people like Dan Mosedale when we are ready.

It is great to see progress on SASL support.

So far I have only skimmed the code.  There are a lot of questions.  Here are some:

a) Does the API exactly match OpenLDAP's?  We should make sure it does.  Same
   goes for the command line tool options if possible (there may be so much
   divergence there it is impossible to do something compatible).
b) I am suprised to see changes in liblber as well as libldap.  Actually,
   it looks like most of the liblber changes are just adding fields to
   data structures.  If possible, we should confine the changes to one layer.
c) If we add a file named sasl.c we end up with saslbind.c and sasl.c.  Maybe
   the SASL I/O layer could be placed in a new file named saslio.c and the
   new bind code could be added to saslbind.c?
d) What is the meaning of DONT_DELAY_SASL_INIT?
e) What are the pthread-related changes in libldap/open.c?  Just other changes
   from the the Sun merge branch (sun_merge_branch_20060523)?
f) Any reason to commit this to sun_merge_branch_20060523?  Why not go
   directly to the trunk?

Comment 8

[Mark Smith [:mcs]]

Updated the component and summary.

Comment 9

[Mark Smith [:mcs]]

Also see bug 308118 (When binding to an LDAP server, Thunderbird should attempt GSSAPI (Kerberos) Authentication if available)

Comment 10

[Rich Megginson]

The function saslSetParam() writes to saslarg, which is optarg set by getopt().  I don't know if this is safe or portable.  It would be better to use strncasecmp().  Are the arguments required to be case insensitive?  If not, use strncmp() instead, which should be portable - I don't know if strncasecmp() is supported by all operating systems.

ldap/libraries/libldap/Makefile.in - should sasl.c be compiled even if sasl is not enabled?

ldap/libraries/libldap/open.c - Does nsldapi_init_mutex need to be a global instead of a static?

ldap/libraries/libldap/saslbind.c - this code seems wrong:
+        if ( cctrlp && clientctrls ) {
+                *clientctrls = NSLDAPI_MALLOC( sizeof( LDAPControl ) );
+                SAFEMEMCPY( clientctrls, cctrlp, sizeof( LDAPControl ) );
+        }
What is it trying to do here?  Why can't we just do *clientctrls = cctrlp and let the client free them?

It is ok to pass NULL to a free() function, so we don't need to have checks like if (ptr) free(ptr) anymore.

In ldaptool-sasl.c - I think there is a problem with getpass() on HP-UX - Ulf submitted a fix for this a while ago - check the other places in the code where we get the password from the user.

Comment 11

[nkinder]

To address Rich's question from Comment#6:

I have not tried to make this work on Windows or Mac OS X yet.  There is certainly some work that would be necessary to make it work on those platforms.

Comment 12

[nkinder]

To address Mark's questions from Comment#7:

a.)  The API does exactly match the API that OpenLDAP uses for ldap_sasl_interactive_bind_s().  The command-line options do not match due to the existing divergence (ex. - OpenLDAP uses -Y to specify the sasl mechanism, while we use -Y for proxied authentication).  We are matching the options that Sun has used for SASL, which is to specify multiple "-o" options to set sasl options (ex. "-o 'mech=GSSAPI' -o 'authzid=someuser'").

b.)  I agree that we should move the liblber changes to libldap, and it should be easy to do as well.  I'm not really sure why it was put in liblber in the first place, but I'll go ahead and move it over to libldap.

c.)  This is a good idea, and it should be fairly easy to do.  I will work on changing this.

d.)  The DONT_DELAY_SASL_INIT option was from Sun's contributed code, so I'm not sure what the original need for it was.  If this option is not defined, the sasl library is initialized when ldap_sasl_interactive_bind_s() is called.  If the option is defined, then we initialize the sasl library when nsldapi_initialize_defaults() is called.  I don't know why this would be necessary (and I was not defining this option during my tests), but I'm adding Anton to the CC list to see if he can shed some light on this.  If the option is not needed, I can remove the code that initialized the sasl library in nsldapi_initialize_defaults().

e.)  Yes, these changes were just other changes from the Sun contributed code.  I decided to merge that code over while I was merging the nearby sasl code as well since it looks to protect the initilization of the defaults with a mutex.  Perhaps Anton could elaborate on why this mutex was necessary?

f.)  The "sun_merge_branch_20060523" branch was created as a copy of the trunk by Rich for the purpose of merging the Sun contributed code which is located in "mozilla/directory/suncsdk".  The idea was to use the branch as an intermediate for the merge work.  It doesn't have to be done this way, and I'm open to suggestions on how we should deal with the merge process.

Comment 13

[Rich Megginson]

To elaborate on the merge branch: I thought it would be a good idea to have a branch into which we could put changes, checkout, build, and test from without polluting the trunk.

Comment 14

[Mark Smith [:mcs]]

(In reply to comment #13)
> To elaborate on the merge branch: I thought it would be a good idea to have a
> branch into which we could put changes, checkout, build, and test from without
> polluting the trunk.

OK, I understand now.  I think I just forgot the purpose for the branch.  Carry on....

Comment 15

[Anton Bobrov]

(In reply to comment #12)

> b.)  I agree that we should move the liblber changes to libldap, and it should
> be easy to do as well.  I'm not really sure why it was put in liblber in the
> first place, but I'll go ahead and move it over to libldap.

having ber layer exposed to that stuff aint nice but i believe there were
reasons it has been implemented like that. it would be real nice if you
can come up with implementation that will keep ber layer clean but if you
cannot easily do that, i'd say keep it like as it is, at least it works.

> d.)  The DONT_DELAY_SASL_INIT option was from Sun's contributed code, so I'm
> not sure what the original need for it was.  If this option is not defined, 

my best guess its there to let us build with SASL support but unless any
given app actually does SASL it can get away without having SASL libs
around. cuts on dependency without cutting on the feature and if you need
the feature you can just throw the dependency in and voila it works dunno
if its really needed these days but i remember people were witching about
it among the lines: "i dont use SASL why i should keep SASL libs around?!
". i think its more critical for ISVs who are not keen on shipping deps
that their apps dont really use or need.

> e.)  Yes, these changes were just other changes from the Sun contributed code. 
> I decided to merge that code over while I was merging the nearby sasl code as
> well since it looks to protect the initilization of the defaults with a mutex. 

because you dont wanna have a race if initializing defaults from two diff
threads.

Comment 16

[nkinder]

Attachment: Revised Diffs

These revised diffs (and the revised versions of the new source files that I will attach after this) address nearly all of the issues brought up in the reviews.  Below is a description of the changes since the last version of the diffs:

- The new SASL bind code has been moved into saslbind.c.  The new SASL I/O code is now in a new source file named saslio.c.

- The DONT_DELAY_SASL_INIT build option has been removed.  This option controlled when the initialization of the Cyrus SASL library would take place.  We are initializing the Cyrus SASL library when ldap_sasl_interactive_bind_s() is called.

- We no longer modify optarg in saslSetParam().  The previous code was taking an argument in the form of "attr=value" and writing '\0' in the position of the '=' character to get two strings.  We are now using strncasecmp() instead since modifying optarg is not safe.  This method is portable since we define strncasecmp to call other native functions (in portable.h) on operating systems that don't have support for strncasecmp().

- The Makefile for libldap has been changed so that we only compile saslio.c if SASL support is enabled.

- In open.c, the scope of nslapi_init_mutex was changed from global to static.

- We no longer bother checking if a pointer is valid before calling free functions on them in the new code.

- In ldaptool-sasl.c, e no longer use getpass() on HP-UX in since it has an 8 character limit.  Instead of copying the 40-some lines of code that are necessary to get a password from a user on HP-UX from elsewhere in the ldaptool code, I created a new ldaptool_getpass() function in common.c.  This function is basically just a big ifdef to get the passwords appropriately on the different platforms that we support.  I made sure that the command-line tools use this new function anywhere they prompt for a password from the user.

The below issues still need to be addressed, but I would like some input on how we should address them:

- There are still a few changes in liblber.  We need to store pointers to a SASL context (among a few other SASL related things) that is strongly associated with a connection.  It makes sense that it should be part of the sockbuf, which is in the liblber code.  Is this ok, or is there somewhere else in libldap that we could put it? 

- In saslbind.c, I removed the code that was copying controls returned by the server into clientctrls.  This code was leaking memory in addition to just being plain wrong.  Basically, we have no way to retrieve a bind response control (such as the password policy response) when you are using ldap_interactive_sasl_bind_s().  This seems to be a limitation of the LDAP API (which OpenLDAP also has).  To fetch a bind response control, you need to access the operation result.  To do this for a bind operation, you need to call an asynchronous bind function to get the msgid, then pass that to ldap_result() to get a pointer to the result.  You would then pass the result to ldap_parse_result() to fill in a pointer to an array of controls.  The synchronous bind functions have no way of getting a pointer to the result.  They simply get the LDAP result code returned to them.  There is no asynchronous version of ldap_interactive_sasl_bind_s() since it needs to interact with the user.  Should we introduce a new function to get a pointer to the result (or possibly just a function to fetch the controls sent by the server?

Comment 17

[nkinder]

Attachment: New source file (saslio.c)

Comment 18

[nkinder]

Attachment: New source file (ldaptool-sasl.c)

Comment 19

[Rich Megginson]

In nsldapi_sasl_do_bind(), scred could be allocated even if scred->bv_len == 0, so don't check for len.  Also, you should set scred == NULL at the top of the loop, then just use ber_bvfree(scred) to free it without checking in the error cases e.g.
if (rc != LDAP_SUCCESS) {
    ber_bvfree(scred);
    return( rc );
}

Other than that, looks good.

Comment 20

[Anton Bobrov]

(In reply to comment #16)
> - There are still a few changes in liblber.  We need to store pointers to a
> SASL context (among a few other SASL related things) that is strongly
> associated with a connection.  It makes sense that it should be part of the
> sockbuf, which is in the liblber code.  Is this ok, or is there somewhere else
> in libldap that we could put it?

while i agree with Mark it aint clean i think its ok to leave it like that
for now. if somebody can come up with better solution that can keep liblber
clean of that stuff then we should implement such solution. i dont have any
thing to offer today so unless somebody else does i think we should stick
with the current implementation.

> - In saslbind.c, I removed the code that was copying controls returned by the
> server into clientctrls.  This code was leaking memory in addition to just
> being plain wrong.  Basically, we have no way to retrieve a bind response
> control (such as the password policy response) when you are using
> ldap_interactive_sasl_bind_s().  This seems to be a limitation of the LDAP API
> (which OpenLDAP also has).  To fetch a bind response control, you need to
> access the operation result.  To do this for a bind operation, you need to call
> an asynchronous bind function to get the msgid, then pass that to ldap_result()
> to get a pointer to the result.  You would then pass the result to
> ldap_parse_result() to fill in a pointer to an array of controls.  The
> synchronous bind functions have no way of getting a pointer to the result. 
> They simply get the LDAP result code returned to them.  There is no
> asynchronous version of ldap_interactive_sasl_bind_s() since it needs to
> interact with the user.  Should we introduce a new function to get a pointer to
> the result (or possibly just a function to fetch the controls sent by the
> server?

this bad hack is on my hands. we had a bug filed on that i had to fix pronto.
sorry about that. it is important for us to have some sort of solution there 
which will work for ldap tools. the bug has been filed against -E option to 
ask the server to expose the ldap bind identity, which worked for simple bind 
but not for sasl bind. if this is broken we have a regression. one way or the 
other i believe we gonna have to fix it anyway.

Comment 21

[nkinder]

Attachment: Revised Diffs

These new diffs address the issues that Rich raised in comment 19.

Comment 22

[Rich Megginson]

One question about the new diffs:
+                if( rc == LDAP_SUCCESS && saslrc == SASL_OK ) {
+                        /* we're done, no need to step */
+                        if( scred ) {
+                                /* but server provided us with data! */
+                                ber_bvfree( scred );
+                                LDAP_SET_LDERRNO( ld, LDAP_LOCAL_ERROR,
+                                                  NULL, NULL );
+                                return( LDAP_LOCAL_ERROR );
+                        }
+                        break;
+                }

Does this mean you only want to return an error if the server returned scred?  In that case, what actually happened?  Did the server think we were using a different mechanism?  Is there a more appropriate error code or message in this case?

Regarding the problem of the controls: I think we have to leave ldap_interactive_sasl_bind_s as it is, to be compatible with other programs.  I think we need a new function e.g. ldap_interactive_sasl_bind_s_ext (s for synchronous and ext for extension) that will allow controls to be passed back.  This new function would take a parameter LDAPControl ***clientctrls that would allow the controls from the server to be passed back to the client.  This new function would have to call ldap_sasl_bind/ldap_result/ldap_parse_sasl_bind_result/ldap_parse_result in order to get all of the data returned, including the controls.  I'm not sure what we should do about the controls returned at each step - perhaps add them to the list of controls to return?  Or just take the controls from the last step?  As a side note: the only way to get the controls returned by the server is to use the async op functions and ldap_parse_result - all of the ldap_<operation> functions that accept a clientctrls parameter do not and cannot use this to return the controls to the caller.  Another approach would be to pass in a callback function to ldap_interactive_sasl_bind_s_ext - this function would be passed in the LDAPControl** returned by each step in the sasl bind, or *** to allow the function to free the controls.

With respect to the saslcontext/sockbuf: We already support prio and sslio without having to change the ber layer.  Can't we use that same mechanism?

Comment 23

[nkinder]

(In reply to comment #22)
> One question about the new diffs:
> +                if( rc == LDAP_SUCCESS && saslrc == SASL_OK ) {
> +                        /* we're done, no need to step */
> +                        if( scred ) {
> +                                /* but server provided us with data! */
> +                                ber_bvfree( scred );
> +                                LDAP_SET_LDERRNO( ld, LDAP_LOCAL_ERROR,
> +                                                  NULL, NULL );
> +                                return( LDAP_LOCAL_ERROR );
> +                        }
> +                        break;
> +                }
> 
> Does this mean you only want to return an error if the server returned scred? 
> In that case, what actually happened?  Did the server think we were using a
> different mechanism?  Is there a more appropriate error code or message in this
> case?
> 

This would indicate a failure in the SASL handshake.  Basically, when the handshake is completed, the client side of the SASL library will set saslrc to SASL_OK.  When the handshake is complete, the server side of the SASL library should not send a challenge since it's job is complete.

This code is checking if another challenge was sent from the server.  This situation indicates some sort of synch failure between the client and server side of the SASL library.  None of the LDAP result codes seem to fit very well, so one of the more generic codes needs to be used.  Either LDAP_LOCAL_ERROR or LDAP_OPERATIONS_ERROR seem to be most appropriate.  Maybe it would be best to set some more specific error text when this happens stating that this is a SASL handshake failure.

> Regarding the problem of the controls: I think we have to leave
> ldap_interactive_sasl_bind_s as it is, to be compatible with other programs. 

 I agree.  I don't want to change the defined API.

> I think we need a new function e.g. ldap_interactive_sasl_bind_s_ext (s for
> synchronous and ext for extension) that will allow controls to be passed back. 
> This new function would take a parameter LDAPControl ***clientctrls that would
> allow the controls from the server to be passed back to the client.  This new
> function would have to call
> ldap_sasl_bind/ldap_result/ldap_parse_sasl_bind_result/ldap_parse_result in
> order to get all of the data returned, including the controls.  I'm not sure
> what we should do about the controls returned at each step - perhaps add them
> to the list of controls to return?  Or just take the controls from the last
> step?  

I haven't seen any controls returned during the intermediate steps.  Controls only seem to be returned by the server once the SASL negotiation is complete.

> As a side note: the only way to get the controls returned by the server
> is to use the async op functions and ldap_parse_result - all of the
> ldap_<operation> functions that accept a clientctrls parameter do not and
> cannot use this to return the controls to the caller.  Another approach would
> be to pass in a callback function to ldap_interactive_sasl_bind_s_ext - this
> function would be passed in the LDAPControl** returned by each step in the sasl
> bind, or *** to allow the function to free the controls.
> 
> With respect to the saslcontext/sockbuf: We already support prio and sslio
> without having to change the ber layer.  Can't we use that same mechanism?
> 

We can probably do something similar to the way we support SSL.  We could stash a pointer to our SASL session info (context + other necessary info) in the soinfo_appdata member of the PRLDAPSocketInfo struct.  This appears to be what we do for SSL.

Comment 24

[nkinder]

Attachment: Revised Diffs

This revision of the diffs implements an ldap_interactive_sasl_bind_ext_s() function that allows the caller to pass in a pointer to an array of LDAPControl structs.  This pointer will receive the list of controls sent by the server.  This new function is now being used for the LDAP command-line tools.  We may want to go with a different approach and implement an ansynchronus version of ldap_interactive_sasl_bind() instead to be more consistent with the other API functions.

I have also improved the error message returned from the error case that Rich pointed out in comment 22.

Comment 25

[nkinder]

Attachment: CVS Commit Message

The previously attached diffs have been checked into the "sun_merge_branch_20060523" branch.

There are still a few issues to be addressed before we can merge this code to the trunk.  We still need to move the SASL struct elements out of the sockbuf in liblber and instead use the soinfo_appdata member of the PRLDAPSocketInfo struct to store a pointer to our SASL context data.  This will isolate the SASL changes to libldap.  We may also want to create an asynchronous version of ldap_interactive_sasl_bind_s() to allow us to get at the results and the returned controls instead of my current ldap_interactive_sasl_bind_ext_s() implementation.  This would make the API more consistent in it's usage.

Comment 26

[Anton Bobrov]

(In reply to comment #25)

> the trunk.  We still need to move the SASL struct elements out of the sockbuf
> in liblber and instead use the soinfo_appdata member of the PRLDAPSocketInfo
> struct to store a pointer to our SASL context data.  This will isolate the SASL
> changes to libldap.

i'm not sure i understand the use of PRLDAPSocketInfo struct and associated
NSPR involvement here. does it mean to use SASL one will have to use NSPR
as well ?

> We may also want to create an asynchronous version of
> ldap_interactive_sasl_bind_s() to allow us to get at the results and the
> returned controls instead of my current ldap_interactive_sasl_bind_ext_s()
> implementation.  This would make the API more consistent in it's usage.

thanx alot for taking care of this one for now!

Comment 27

[Mark Smith [:mcs]]

Excellent work.  Just a few comments:

- Earlier I overlooked the change in behavior of the -o and -B options to ldapsearch.  I am sure that change will annoy someone and may break some scripts.  Should we use another letter instead?  On the other hand, LDIF is pretty pervasive these days.  Is -o used by OpenLDAP for SASL options as well?

- Are you sure that free() is NULL-safe on all platforms?  I am not sure.

- I do not think SASL should depend on the libprldap layer.  Some application developers will want to layer SASL over the top of the native OS socket calls.  And some will want to layer SASL over the top of NSPR.

- The SASL API (created by OpenLDAP developers?) does not seem like it is designed at all to allow for non-blocking or asynchronous use.  Is that a requirement or something we do not need to worry about?  Most modern applications are likely multithreaded anyway (so blocking on one thread is OK).

Comment 28

[Rich Megginson]

> - Earlier I overlooked the change in behavior of the -o and -B options to
> ldapsearch.  I am sure that change will annoy someone and may break some
> scripts.  Should we use another letter instead?  On the other hand, LDIF
> is pretty pervasive these days.  Is -o used by OpenLDAP for SASL options
> as well?

I don't think -B will break any of our scripts, so I think that change is ok.  OpenLDAP uses -O (capital o) for sasl options.  We already use -O for maximum number of referral hops to traverse.  So it's close, but not the same as openldap.

> - Are you sure that free() is NULL-safe on all platforms?  I am not sure.

According to Wan-Teh and others, yes, free() is NULL safe on all of the platforms supported by NSPR/NSS.

> - I do not think SASL should depend on the libprldap layer.  Some
> application developers will want to layer SASL over the top of the
> native OS socket calls.
> And some will want to layer SASL over the top of NSPR.

I don't think Nathan meant that we should use the PRLDAPSocketInfo.  I think he meant we should do something similar.  The ldap/ber code already has a mechanism by which you can layer I/O functions.  We should use that same mechanism to layer the sasl I/O functions in order to minimize or eliminate sasl code in liblber.

> - The SASL API (created by OpenLDAP developers?) does not seem like
> it is designed at all to allow for non-blocking or asynchronous use.
> Is that a requirement or something we do not need to worry about?
> Most modern applications are likely multithreaded anyway
> (so blocking on one thread is OK).

I think the main requirement for a ldap_sasl_interactive_bind (no _s) function is so that the caller can retrieve the controls sent by the server.  But it would be nice if we could use it in multi-threaded applications also.

Comment 29

[Anton Bobrov]

(In reply to comment #28)

+1 for options to support the last comment made by Rich.

Comment 30

[Rich Megginson]

Hmm - looks like we can't really duplicate the semantics of the async ldap_op functions with interactive sasl bind.  One reason is that in order to check the status of the call to ldap_sasl_bind() we have to call ldap_result, which violates those semantics.  So if we're going to violate those semantics anyway, perhaps we should have a function ldap_sasl_interactive_bind() which takes a callback function as an argument - this function is called with the msgid returned by ldap_sasl_bind() at each step.  Or????  I still think the ldap_sasl_interactive_bind_ext_s() is useful to return the controls to the caller.

Comment 31

[Mark Smith [:mcs]]

I think we need to decide what the requirement are with respect to SASL bind.  Is there a real need for an async. flavor of ldap_sasl_interactive_bind_s()?  If not, let's just go with the ldap_sasl_interactive_bind_ext_s() solution.  If an async. API is needed so that applications can control when and how results are received, it seems to me like you would need a set of functions, perhaps something like:

ldap_sasl_interactive_bind_start()
ldap_sasl_interactive_bind_step()

Then the application would need to call the _step() function as many times as necessary.

Comment 32

[nkinder]

(In reply to comment #28)
> > - Earlier I overlooked the change in behavior of the -o and -B options to
> > ldapsearch.  I am sure that change will annoy someone and may break some
> > scripts.  Should we use another letter instead?  On the other hand, LDIF
> > is pretty pervasive these days.  Is -o used by OpenLDAP for SASL options
> > as well?
> 
> I don't think -B will break any of our scripts, so I think that change is ok. 
> OpenLDAP uses -O (capital o) for sasl options.  We already use -O for maximum
> number of referral hops to traverse.  So it's close, but not the same as
> openldap.

OpenLDAP uses -O for SASL security properties.  Each basic SASL option has it's own option in OpenLDAP's tools, all of which we use for other functionality already in our tools.  Here's what they use:

  -Y (SASL mechanism)
  -X (authzid)
  -U (authid)
  -R (realm)
  -O (secprops)

Our proposed implementation uses -o for all of these.  An example would be:

  ./ldapsearch -o "mech=DIGEST-MD5" -o "authzid=tuser" -o "authid=tuser" -b "" -s base "objectclass=*"

(In reply to comment #31)
> I think we need to decide what the requirement are with respect to SASL bind. 
> Is there a real need for an async. flavor of ldap_sasl_interactive_bind_s()? 
> If not, let's just go with the ldap_sasl_interactive_bind_ext_s() solution. 

There isn't a real need for an async. version of ldap_sasl_interactive_bind_s() at this time.  The main requirement (aside from supporting the various SASL mechanisms) is to have the ability to get controls back from the server.  The ldap_sasl_interactive_bind_ext_s() implementation seems to deal with this nicely.

Comment 33

[Rich Megginson]

Attachment: Support flags, minor bug fixes

1) I made several changes to the code to make it more compatible with the openldap implementation.  This involved fully supporting the 3 different modes - automatic, interactive, and quiet.  The default is automatic, which means use defaults for everything possible, and prompt the user for the rest (usually just password).  One tricky thing is that we want to use a default of "" (the empty string) if no other value is available.  This does some special magic inside cyrus-sasl-gssapi to automatically retrieve the username (authzid) from the Kerberos tgt which is almost always what you want to do.  If this is not desirable, then the user can always specify -o authzid=realname or just use -o flags=interactive.  We need to be able to use mode quiet (no prompts) if we ever want to use sasl for server-to-server, for example.
These diffs also clean up the code a bit (consistent indentation), a couple of minor errors, and a memory leak in nsldapi_sasl_close (I ran this through valgrind).  I also checked in a new configure flag --enable-clu so that you don't have to do make BUILCCLU=1 anymore.  This diff also fixes a bug with the --with-sasl option to make sure it adds -lsasl2 to the link line.  I also increased the buffer size in LDAPDebug and made sure the buffer was null terminated properly.
I eliminated the configure diffs because they were too big :-(

Comment 34

[nkinder]

The proposed changes look good.

Has any headway been made on moving the SASL changes out of liblber?

Comment 35

[Anton Bobrov]

there is one more thing i wanted to adrress: libsasl naming. is it possible
to add a configure flag to allow for different libsasl name ? currently its
hardcoded '-lsasl2' and i'm fine with keeping default but standard libsasl
that ships with Solaris for example has diff libsasl name which is just
'libsasl.so' there, probably because Solaris for one is very strict on lib
naming. also i think on older RedHat releases its libsasl.so as well and 2
has been added relatively recently tho it seems they both part of the same
package but i dunno what the diff between them exactly apart from assuming
old/new version which i guess could also mean they are incompatible. would
be nice to have an option to override default name using configure thingie.

Comment 36

[Rich Megginson]

Attachment: minor changes to previous diffs

Re: Comment #34 - I just started looking at it.  I'd like to get these changes in first before I start hacking the code.

Re: Comment #35 - Yes.  These new diffs introduce some changes to sasl.m4.  What we do now is first look for -lsasl2.  If not found, look for -lsasl.  It uses the default path (i.e. /usr/lib) for the search unless the user gives an explicit path with --with-sasl=path or --with-sasl-lib=path.  I think this should work for Solaris and most other operating systems unless they have /usr/lib/libsasl2.so that should not be used instead of /usr/lib/libsasl.so

Comment 37

[Anton Bobrov]

thanx Rich, that sure does the trick!

(In reply to comment #36)
> Re: Comment #35 - Yes.  These new diffs introduce some changes to sasl.m4. 
> What we do now is first look for -lsasl2.  If not found, look for -lsasl.  It
> uses the default path (i.e. /usr/lib) for the search unless the user gives an
> explicit path with --with-sasl=path or --with-sasl-lib=path.  I think this
> should work for Solaris and most other operating systems unless they have
> /usr/lib/libsasl2.so that should not be used instead of /usr/lib/libsasl.so
 

Comment 38

[nkinder]

The new changes in the attachment in comment 36 look fine.  That should work across all supported platforms.  I can't think of a case where we'd want to link with -lsasl instead of -lsasl2 where both libraries reside in the same directory.

Comment 39

[Rich Megginson]

So I've been looking at the saslio stuff.  The first problem I see is that you have to call sasl_client_new to get the sasl ctx.  You have to do this before you call sasl_client_start to begin the bind stuff, because it requires a valid ctx.  In order to call sasl_client_new, you have to have the host (FQDN if using kerberos).  In our current code, you can only get the host _after_ a successful connection attempt.  So the code does this weird thing to call nsldapi_open_ldap_defconn() to force the connect, get the ld->ld_defhost, call sasl_client_new with the host to get the context, and stuff the context in the sb for later use.

Perhaps we could call nsldapi_open_ldap_defconn() and get the connected host without having to hook into the connect functions, i.e. just grab ld->ld_defhost in nsldapi_do_sasl_bind and call sasl_client_new there?  Or is it possible to call sasl_client_new without a host argument and change it later?  I'm afraid I don't know the sasl api very well.

The next problem is that the ldap io stuff is not a "pushable" stack like the prio functions are.  When the saslio functions are set, we have to first get the current functions and context arg and save them (in the saslio context arg probably), so that we can call them from the saslio functions.  The code already does something like this - it saves the lber functions and calls them for the "system" read/write/etc. functions.  Unless they are not available, then it just calls system read/write/etc.  I think we could do the same thing with the existing mechanism without having to add ld_sasl_io_fns to LDAP* and sb_sasl_fns to Sockbuf*, by saving the old values in a socket context.

Comment 40

[Anton Bobrov]

(In reply to comment #39)
> So I've been looking at the saslio stuff.  The first problem I see is that you
> have to call sasl_client_new to get the sasl ctx.  You have to do this before
> you call sasl_client_start to begin the bind stuff, because it requires a valid
> ctx.  In order to call sasl_client_new, you have to have the host (FQDN if
> using kerberos).  In our current code, you can only get the host _after_ a
> successful connection attempt.  So the code does this weird thing to call
> nsldapi_open_ldap_defconn() to force the connect, get the ld->ld_defhost, call
> sasl_client_new with the host to get the context, and stuff the context in the
> sb for later use.

i'm not sure why you think its weird. you need to make sure that there is a
connection in connected state exist otherwise whats the point having sasl
context there ? what it will buy us having sasl context but no connection ?

> Perhaps we could call nsldapi_open_ldap_defconn() and get the connected host
> without having to hook into the connect functions, i.e. just grab
> ld->ld_defhost in nsldapi_do_sasl_bind and call sasl_client_new there?  Or is
> it possible to call sasl_client_new without a host argument and change it
> later?  I'm afraid I don't know the sasl api very well.

if the serverFQDN is null you gonna get SASL_BADPARAM from sasl_client_new().
each sasl context is associated with only one connection/session at a time.
 
> The next problem is that the ldap io stuff is not a "pushable" stack like the
> prio functions are.  When the saslio functions are set, we have to first get
> the current functions and context arg and save them (in the saslio context arg
> probably), so that we can call them from the saslio functions.  The code
> already does something like this - it saves the lber functions and calls them
> for the "system" read/write/etc. functions.  Unless they are not available,
> then it just calls system read/write/etc.  I think we could do the same thing
> with the existing mechanism without having to add ld_sasl_io_fns to LDAP* and
> sb_sasl_fns to Sockbuf*, by saving the old values in a socket context.

i think, if i understand the problem correctly here, we can push sb_sasl_*
members of struct sockbuf up libldap layer, say by aggregating them under
a single struct for sasl context and linking it in under struct ldap_conn.
i/o callbacks stack works pretty good and i dont see any reason/s changing
it right now unless of course id miss something in this discussion earlier 
.

Comment 41

[Rich Megginson]

The sasl context is associated with a connection, however, the call to sasl_client_new only needs the FQDN e.g. this snippet of nsldapi_sasl_open():
        saslrc = sasl_client_new( "ldap", host,
                NULL, NULL, /* iplocalport, ipremoteport - use defaults */
                NULL, 0, &ctx );
The host must be the FQDN (especially in order for gssapi to work).  All I'm asking is - do we have to call nsldapi_sasl_open from the low level nsldapi_connect_to_host()?  Or can we call it anytime, anywhere after sasl_client_init and before sasl_client_start()?  It's weird in that it works differently than other LDAP operation code - usually the connection is opened during the operation request phase, if it is not already opened.  It just seems weird to have all of this low level connection/socket code in the high level LDAP operation code.  However, in this case, I can see where that won't work, because it seems that the only way to get the FQDN is to actually open a connection.  Once I commit the other changes, I'll experiment with this.

Comment 42

[Rich Megginson]

Checking in mozilla/directory/c-sdk/configure.in;
/cvsroot/mozilla/directory/c-sdk/configure.in,v  <--  configure.in
new revision: 5.50.2.2; previous revision: 5.50.2.1
done
Checking in mozilla/directory/c-sdk/configure;
/cvsroot/mozilla/directory/c-sdk/configure,v  <--  configure
new revision: 5.51.2.2; previous revision: 5.51.2.1
done
Checking in mozilla/directory/c-sdk/config/autoconf.mk.in;
/cvsroot/mozilla/directory/c-sdk/config/autoconf.mk.in,v  <--  autoconf.mk.in
new revision: 5.10.2.1; previous revision: 5.10
done
Checking in mozilla/directory/c-sdk/config/autoconf/sasl.m4;
/cvsroot/mozilla/directory/c-sdk/config/autoconf/Attic/sasl.m4,v  <--  sasl.m4
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in mozilla/directory/c-sdk/ldap/clients/tools/Makefile.in;
/cvsroot/mozilla/directory/c-sdk/ldap/clients/tools/Makefile.in,v  <--  Makefile.in
new revision: 5.14.2.2; previous revision: 5.14.2.1
done
Checking in mozilla/directory/c-sdk/ldap/clients/tools/common.c;
/cvsroot/mozilla/directory/c-sdk/ldap/clients/tools/common.c,v  <--  common.c
new revision: 5.17.8.2; previous revision: 5.17.8.1
done
Checking in mozilla/directory/c-sdk/ldap/clients/tools/ldaptool-sasl.c;
/cvsroot/mozilla/directory/c-sdk/ldap/clients/tools/Attic/ldaptool-sasl.c,v  <--  ldaptool-sasl.c
new revision: 1.2.2.2; previous revision: 1.2.2.1
done
Checking in mozilla/directory/c-sdk/ldap/clients/tools/ldaptool-sasl.h;
/cvsroot/mozilla/directory/c-sdk/ldap/clients/tools/Attic/ldaptool-sasl.h,v  <--  ldaptool-sasl.h
new revision: 1.2.2.2; previous revision: 1.2.2.1
done
Checking in mozilla/directory/c-sdk/ldap/include/ldap-extension.h;
/cvsroot/mozilla/directory/c-sdk/ldap/include/ldap-extension.h,v  <--  ldap-extension.h
new revision: 5.4.8.2; previous revision: 5.4.8.1
done
Checking in mozilla/directory/c-sdk/ldap/include/ldaplog.h;
/cvsroot/mozilla/directory/c-sdk/ldap/include/ldaplog.h,v  <--  ldaplog.h
new revision: 5.2.8.1; previous revision: 5.2
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/saslbind.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/saslbind.c,v  <--  saslbind.c
new revision: 5.3.2.2; previous revision: 5.3.2.1
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/saslio.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/Attic/saslio.c,v  <--  saslio.c
new revision: 1.1.2.2; previous revision: 1.1.2.1
done

Anton, can you try the new configure on Solaris (sparc and x86 if possible) to see if it correctly finds and links with the sasl libs?

Comment 43

[nkinder]

(In reply to comment #41)
> The sasl context is associated with a connection, however, the call to
> sasl_client_new only needs the FQDN e.g. this snippet of nsldapi_sasl_open():
>         saslrc = sasl_client_new( "ldap", host,
>                 NULL, NULL, /* iplocalport, ipremoteport - use defaults */
>                 NULL, 0, &ctx );
> The host must be the FQDN (especially in order for gssapi to work).  All I'm
> asking is - do we have to call nsldapi_sasl_open from the low level
> nsldapi_connect_to_host()?  Or can we call it anytime, anywhere after
> sasl_client_init and before sasl_client_start()?

As you've noticed, SASL itself never deals with the actual connection since it's protocol independent.  You can indeed call sasl_client_new() from anywhere you want as long as the SASL library is initialized by calling sasl_client_init() first.

Comment 44

[Rich Megginson]

Also, correct me if I'm wrong, but we only need the sasl read/write/close if the server and client have negotiated using an encryption layer (SASL_SSF has a non-NULL/non-zero value).  And we only need the sb_sasl_ctx in the sockbuf in that case.  So I think I can move the sockbuf->sb_sasl_ctx initialization to nsldapi_sasl_install.

Comment 45

[nkinder]

(In reply to comment #44)
> Also, correct me if I'm wrong, but we only need the sasl read/write/close if
> the server and client have negotiated using an encryption layer (SASL_SSF has 
> a non-NULL/non-zero value).  

Yes, this is correct.

> And we only need the sb_sasl_ctx in the sockbuf in
> that case.  So I think I can move the sockbuf->sb_sasl_ctx initialization to
> nsldapi_sasl_install.
> 

This is not correct.  The sb_sasl_ctx is of type sasl_conn_t.  This SASL context is used to keep track of where the client is in the authentication handshake.  It is needed for all SASL mechanisms, regardless of whether an encryption layer is used or not.

Comment 46

[Rich Megginson]

Right.  We need the sasl_conn_t* in order to call sasl_client_start, step, etc.  However, the current code has a Sockbuf.sb_sasl_ctx argument that points to this sasl_conn_t*.  I don't think we need that Sockbuf.sb_sasl_ctx unless we are using sasl encryption.

Comment 47

[nkinder]

(In reply to comment #46)
> Right.  We need the sasl_conn_t* in order to call sasl_client_start, step, etc.
>  However, the current code has a Sockbuf.sb_sasl_ctx argument that points to
> this sasl_conn_t*.  I don't think we need that Sockbuf.sb_sasl_ctx unless we
> are using sasl encryption.
> 

We do things like the following in nsldapi_sasl_do_bind():

    /* expect context to be initialized when connection is open */
    ctx = (sasl_conn_t *)ld->ld_defconn->lconn_sb->sb_sasl_ctx;

This happens in all SASL cases.  That's not to say that we can't stash the pointer to the context elsewhere though.

Comment 48

[Anton Bobrov]

    (In reply to comment #42)

    > Anton, can you try the new configure on Solaris (sparc and x86 if possible) to
    > see if it correctly finds and links with the sasl libs?

    doesnt work for me. regardless if i use default [ --with-sasl ] or specific
    location [ --with-sasl=path_to_sasl ] configure gives this :

    checking for sasl_client_init in -lsasl2... (cached) no
    checking for sasl_client_init in -lsasl... (cached) no

    or this :

    checking for sasl_client_init in -lsasl2... no
    checking for sasl_client_init in -lsasl... no

    if i remove the cache file which i think is there by default.

    on a side note there is a problem with generated configure 
    script if the shell is anything but bash :

    checking for --with-sasl... using /share/builds/integration/sasl20/2.19_20060613/SunOS5.10_i86pc_DBG.OBJ
    ./configure: test: argument expected

Comment 49

[Mark Smith [:mcs]]

Comment on attachment 232792 [details]
Support flags, minor bug fixes


>2511a2512,2524
>>     } else if (!strncasecmp(saslarg, "flags", argnamelen)) {
>>          int len = strlen(attr);
>>          if (len && !strncasecmp(attr, "automatic", len)) {
>>             sasl_flags = LDAP_SASL_AUTOMATIC;
>>          } else if (len && !strncasecmp(attr, "interactive", len)) {
>>             sasl_flags = LDAP_SASL_INTERACTIVE;
>>          } else if (len && !strncasecmp(attr, "quiet", len)) {
>>             sasl_flags = LDAP_SASL_QUIET;
>>          } else {
>>             fprintf(stderr, "Invalid SASL flags value [%s]: must be one of "
>>                     "automatic, interactive, or quiet\n", attr);
>>             return (-1);

Please provide context diffs next time (cvs diff -U ...).

The above code allows any shortened form of the flag name to be used, e.g., flags=a.   Is that what we want?


>> 	if (len > 0) { /* user typed in something - use it */
>> 		if (interact->result) {
>> 			free(interact->result);
>> 		}
>> 		interact->result = strdup(newvalue);
>> 		memset(newvalue, '\0', len);
>> /*        ldap_memfree( newvalue );*/

Delete the commented out line above?

>465a493,498
>>                             if ( scred->bv_len == 0 ) { /* MS AD sends back empty screds */
>>                                 LDAPDebug(LDAP_DEBUG_ANY,
>>                                           "SASL BIND complete - ignoring empty credential response\n",
>>                                           0, 0, 0);

Are you sure you want the above message logged at the ANY level?  Maybe change to TRACE?

Comment 50

[Anton Bobrov]

Rich, just a wee comment to make sure we are all clear on this. when you
install sasl context and en/decode routines all data exchange between
client and server is wrapped with special sasl header and then payload
on the tail even when you send everything in clear without encryption. i
do not remember exact details of what header and especially tail stuffed
with but its not important in this case. anyway, all the details are in
related RFC if they are of interest here.

Comment 51

[Rich Megginson]

Anton - the test to find the sasl libs uses the c compiler to compile a test program that references the symbol sasl_client_init (which I assume exists in every sasl library that we are compatible with).  I'm not sure why this would fail.  You can do the same thing - take a look at the configure script - search for sasl_client_init(); - you can create a test program just like it is doing and then do cc -o testprog testprog.c -lsasl2.

For the sh problem - I'm not sure which test it is complaining about - unfortunately I cannot reproduce the problem on my machine - not even sh --posix.  Can you run sh -xv configure ...args... so I can perhaps pinpoint where the problem is?

Comment 52

[Rich Megginson]

Re: Comment #49:
> Please provide context diffs next time (cvs diff -U ...).

Sorry about that.

> The above code allows any shortened form of the flag name to be used, e.g.,
> flags=a.   Is that what we want?

Yes, I think so.  That way you can pass in -o flags=I which is similar to openldap -I and -o flags=Q (openldap -Q).  I think it makes sense to allow the shortest unique initial string (which in this case is the first letter).  Newbies may want to use -o flags=quiet to make their intentions clear.  But it's ok with me if you want to just make it use single letters for the flags.

>> 	if (len > 0) { /* user typed in something - use it */
>> 		if (interact->result) {
>> 			free(interact->result);
>> 		}
>> 		interact->result = strdup(newvalue);
>> 		memset(newvalue, '\0', len);
>> /*        ldap_memfree( newvalue );*/

> Delete the commented out line above?

Ok.

>465a493,498
>>                             if ( scred->bv_len == 0 ) { /* MS AD sends back empty screds */
>>                                 LDAPDebug(LDAP_DEBUG_ANY,
>>                                           "SASL BIND complete - ignoring empty credential response\n",
>>                                           0, 0, 0);

> Are you sure you want the above message logged at the ANY level?  Maybe
> change to TRACE?

I did it this way to be compatible with openldap, which uses LDAP_DEBUG_ANY.

Comment 53

[Mark Smith [:mcs]]

(In reply to comment #52)
> Yes, I think so.  That way you can pass in -o flags=I which is similar to
> openldap -I and -o flags=Q (openldap -Q).  I think it makes sense to allow the
> shortest unique initial string (which in this case is the first letter). 
> Newbies may want to use -o flags=quiet to make their intentions clear.  But
> it's ok with me if you want to just make it use single letters for the flags.

I am OK with what you have.  Is the comparison case insensitive?  Might as well make it so....


> > Are you sure you want the above message logged at the ANY level?  Maybe
> > change to TRACE?
> 
> I did it this way to be compatible with openldap, which uses LDAP_DEBUG_ANY.

OK.  If it becomes annoying we can always change it to TRACE.

-Mark

Comment 54

[Rich Megginson]

Re: Comment #50:
> Rich, just a wee comment to make sure we are all clear on this. when you
> install sasl context and en/decode routines all data exchange between
> client and server is wrapped with special sasl header and then payload
> on the tail

Correct.

> even when you send everything in clear without encryption. i

Huh?  In the current code, the install of the en/decode routines only happens when the client and server have negotiated encryption - that is, after the bind steps have been successfully completed, the client looks in the context to see if encryption has been negotiated:
        saslrc = sasl_getprop( ctx, SASL_SSF, (const void **) &ssf );
        if( saslrc == SASL_OK ) {
                if( ssf && *ssf ) {
                        LDAPDebug(LDAP_DEBUG_TRACE,
                                "SASL install encryption, for SSF: %lu\n",
                                (unsigned long) *ssf, 0, 0 );
                        nsldapi_sasl_install( ld, ctx );
                }
        }
That's what I mean by encryption.  So I suppose if ssf is set to some low value which means "send everything in clear" then all reads/writes would still go through sasl_encode/sasl_decode even though there was nothing actually encrypted.  In this case, since the client and server negotiated this protocol, they know how to read/write it.

I'm really talking about the case where ssf above is NULL or 0.  In this case, I don't think we need to have the sockbuf.sb_sasl_ctx set since it is not used.   We still need to keep the ctx around somewhere so that ldap_get_option will work for the sasl related options.

> do not remember exact details of what header and especially tail stuffed
> with but its not important in this case. anyway, all the details are in
> related RFC if they are of interest here.

Comment 55

[Rich Megginson]

Re: Comment #53:

> I am OK with what you have.  Is the comparison case insensitive?  Might as
> well make it so....

Yes, it is case insensitive - strncasecmp.

>> > Are you sure you want the above message logged at the ANY level?  Maybe
>> > change to TRACE?
>> 
>> I did it this way to be compatible with openldap, which uses LDAP_DEBUG_ANY.

> OK.  If it becomes annoying we can always change it to TRACE.

Ok.

Comment 56

[Anton Bobrov]

(In reply to comment #54)

> I'm really talking about the case where ssf above is NULL or 0.  In this case,
> I don't think we need to have the sockbuf.sb_sasl_ctx set since it is not used.
>   We still need to keep the ctx around somewhere so that ldap_get_option will
> work for the sasl related options.

well theoretically if no security layer has been negotiated you are can get rid
of that context with sasl_dispose() and then shutdown sasl with sasl_done() but
i do not think there is any benefit of doing either our case or is there ? 

Comment 57

[Rich Megginson]

Re: Comment #56:
>> I'm really talking about the case where ssf above is NULL or 0.  In this case,
>> I don't think we need to have the sockbuf.sb_sasl_ctx set since it is not used.
>>   We still need to keep the ctx around somewhere so that ldap_get_option will
>> work for the sasl related options.
>
>well theoretically if no security layer has been negotiated you are can get rid
>of that context with sasl_dispose() and then shutdown sasl with sasl_done() but
>i do not think there is any benefit of doing either our case or is there ? 

We have to keep the context around for the lifetime of the session because the user may want to get information from it via ldap_get_option().  So we still need it, we just don't need it associated with the sockbuf layer.

Comment 58

[Anton Bobrov]

(In reply to comment #57)
> Re: Comment #56:
> We have to keep the context around for the lifetime of the session because the
> user may want to get information from it via ldap_get_option().  So we still
> need it, we just don't need it associated with the sockbuf layer.
 
yep, and i believe i did address this in Comment #40, we can push it up there.

re configure stuff, i'm gonna change some stuff to allow for bulding with Sun
Studio 10/11 compilers/tools [which currently fails] and try to address lsasl
build issues there as well, hopefully tomorrow.

Comment 59

[Rich Megginson]

I can't figure out a place to store the sasl data.  I initially thought I could store the context in the ldap_x_ext_io_fns.lextiof_session_arg field.  But this won't work if using prldap because prldap already uses this.  I could proxy it, but that would mean some ugly, complicated code.  I think the simpler approach is this, and matches closely with what openldap does:
1) Leave the global options (mech, realm, authcid, authzid, secprops) in struct ldap, enclosed in an #ifdef
2) Remove ld_sasl_io_fns from struct ldap (see below)
3) Put the sasl context in the LDAPConn structure enclosed in an #ifdef (it is associated with a connection, after all - openldap always uses ld->ld_defconn to get it) e.g. sasl_conn_t *lconn_sasl_ctx;
4) Add code to nsldapi_free_connection() to do sasl_dispose of lconn_sasl_ctx
5) Create a data structure for the sasl options that are currently in lber - ibuf, iptr, ilen and remove them from struct sockbuf
6) When installing the sasl codecs, use the extio functions to override read, write, poll, and close - instead of storing the sb* in the socketarg in the lber_ext_io_fn, store our structure, which contains our sasl options, our saved iofns from the layer we are replacing, and a pointer to the lconn_sasl_ctx.

Much of the rest of the code in saslio.c can be reused.  I note that openldap takes a similar approach - they have a private data structure in lber to avoid having sasl specific (or tls for that matter) code in lber.

This should at least minimize coupling to sasl without making the code too heinously complex.

Comment 60

[Anton Bobrov]

freshly added LDAPDebug in ldaptool-sasl.c kills the build with :

Undefined                       first referenced
 symbol                             in file
ldap_debug                          ldaptool-sasl.o
ld: fatal: Symbol referencing errors. No output written to bin/ldapdelete

i dont think LDAPDebug was ever intended for any use outside lib.

Comment 61

[Rich Megginson]

Attachment: Remove sasl specific data from lber layer

This implements support for the items from Comment #59.  In addition, I added LDAPToolDebug so that we could trace ldaptool code.  The only tricky part was nsldapi_sasl_poll - please read the comments.

Comment 62

[Anton Bobrov]

Attachment: configure patch

this one should address the following issues with configure :

- sasl test c script fails to build on Solaris thus failing
  lsasl flags.
- configure fails on Solaris due to oldstyle builtin 'test'.
- tools build fails with SunStudio 10/11 due to obsolete CXX 
  options.

plus a minor change to allow for Solaris 10 x86-64 64b build 
when using SunStudio 10/11 toolchain.

the funny thing about these problems is that if we were to
use autoconf 2.60 none of these fixes apart from x86-64 opts
would be needed as all required magic would have been there
in configure script by default but it is my understanding we
cannot switch from 2.13 due to bug 104642 tho our wad is not
affected and technically we can switch straight away unless
the rest of mozilla wanna reconf themselves in their builds.

thanx to Rich for telling me how to autoconf this correctly.

Comment 63

[Rich Megginson]

Re: Comment #62
Looks good.  Go ahead and commit it.  I think we should wait a while on moving off of autoconf-2.13 - I think the mozilla client guys are going to have to switch before too long.

Comment 64

[Anton Bobrov]

(In reply to comment #61)
> LDAPToolDebug so that we could trace ldaptool code.  The only tricky part was

we already have ldaptool_verbose [ -v/-vv ] or it doesnt fit the bill in this
case and this is something end users arent supposed to see unless debugging ? 

> nsldapi_sasl_poll - please read the comments.

just a question/thought here. it seems that the cornerstone is that we have 
to handle two layers [ sasl / pr ] in this case and thats kinda makes it 
complicated to clear lber layer from sasl pieces that dont belong there and 
you done it but as you note yourself its tricky. so the question i have is 
this: will replacing struct lber_x_ext_io_fns sb_ext_io_fns with linked list 
of these structures [ say de/allocated and linked dynamically depending on 
how many layers we need ] instead, would help to get rid of that trickery or
at least somewhat minimize it ie make it less complicated, more transparent 
and maybe more flexible if we ever need to squeeze in any additional layers?

Comment 65

[Anton Bobrov]

Checking in mozilla/directory/c-sdk/config/autoconf/sasl.m4;
/cvsroot/mozilla/directory/c-sdk/config/autoconf/Attic/sasl.m4,v  <--  sasl.m4
new revision: 1.1.2.3; previous revision: 1.1.2.2
done
Checking in mozilla/directory/c-sdk/config/autoconf/nspr.m4;
/cvsroot/mozilla/directory/c-sdk/config/autoconf/nspr.m4,v  <--  nspr.m4
new revision: 5.3.2.1; previous revision: 5.3
done
Checking in mozilla/directory/c-sdk/config/autoconf/nss.m4;
/cvsroot/mozilla/directory/c-sdk/config/autoconf/nss.m4,v  <--  nss.m4
new revision: 5.3.8.1; previous revision: 5.3
done
Checking in mozilla/directory/c-sdk/configure.in;
/cvsroot/mozilla/directory/c-sdk/configure.in,v  <--  configure.in
new revision: 5.50.2.3; previous revision: 5.50.2.2
done
Checking in mozilla/directory/c-sdk/configure;
/cvsroot/mozilla/directory/c-sdk/configure,v  <--  configure
new revision: 5.51.2.3; previous revision: 5.51.2.2
done

Comment 66

[Rich Megginson]

> we already have ldaptool_verbose [ -v/-vv ] or it doesnt fit the bill in this
> case and this is something end users arent supposed to see unless debugging ? 

I think the stuff I'm using LDAPToolDebug for is only really useful for debugging - it's not something an end user would usually care to see.

> will replacing struct lber_x_ext_io_fns sb_ext_io_fns with linked list 
> of these structures [ say de/allocated and linked dynamically depending on 
> how many layers we need ] instead, would help to get rid of that trickery or
> at least somewhat minimize it ie make it less complicated, more transparent 
> and maybe more flexible if we ever need to squeeze in any additional layers?

Definitely.  The LDAP code is designed to support only 1 layer at a time, that's why we have to jump through hoops to make sasl work with and without prldap.  If we were to implement a stack of IO layers like nsprio that would make it really easy and flexible.  In fact, it would probably be pretty simple to implement support for sasl as a layer on top of prldap - mostly just a copy/paste of libssldap.

Comment 67

[Rich Megginson]

Ok.  Nathan and I managed to get this code working with DIGEST-MD5 and GSSAPI on linux, solaris, and hpux.  Can we commit this?  We have a lot of pending changes on our plates.

Comment 68

[Anton Bobrov]

Rich, if you are talking about the last proposed patch i would say i'm fine
with it as long as it works. tho it seems to me that we just exchanged one
set of complications for the other and i would much prefer to have  a stack
approach being implemented instead but i do understand that you cant rework
the thing over and over again due to resource constraints. i would support
Nathan's initial changes as they are [ and i said that from the beggining ]
because while Sun implementation trashed liblber with stuff that aint spose
to be there it was pretty straightforward and proven to work implementation
but if there is a very strong desire to keep liblber clean of that stuff
then i'm fine with your implementation because you did test it and it works
and i trust you on that.
  



(In reply to comment #67)
> Ok.  Nathan and I managed to get this code working with DIGEST-MD5 and GSSAPI
> on linux, solaris, and hpux.  Can we commit this?  We have a lot of pending
> changes on our plates.
> 

Comment 69

[Rich Megginson]

One of the problems with both implementations is that they change the ABI since LDAP and LDAPConn are now a different size.  So this is just additional hassle to make sure we recompile everything and keep the old and new libraries completely separated.  I tried and tried to figure out a way to prevent this, but I could find no way to avoid changing either LDAP or LDAPConn without making the code far uglier.  But the changes only affect code that compiles with SASL, and that code would have had to be recompiled and relinked anyway in order to use SASL, so it's probably no big deal.  I did, however, manage to avoid changing the size of Sockbuf, which is perhaps a small consolation since it is compiled into libldap.  We are going to be testing these changes pretty heavily in the next couple of months, and if we find any problems, we can just revert back to the Sun implementation.

I too would rather have a model which has a stack of layers that you can push.  OpenLDAP lber is implemented this way, and they do push sasl and tls layers (and other I/O types) using this mechanism.  I think maybe for the next major revision of mozldap we should implement a layered model.  I don't what Sun's schedule is like, but we (RH) just can't afford to do this right now.

Comment 70

[Rich Megginson]

I'm going to go ahead and commit this to the merge branch so that we can begin merging in the other Sun code into these commits, and test.

Comment 71

[Rich Megginson]

Checking in mozilla/directory/c-sdk/configure.in;
/cvsroot/mozilla/directory/c-sdk/configure.in,v  <--  configure.in
new revision: 5.50.2.4; previous revision: 5.50.2.3
done
Checking in mozilla/directory/c-sdk/configure;
/cvsroot/mozilla/directory/c-sdk/configure,v  <--  configure
new revision: 5.51.2.4; previous revision: 5.51.2.3
done
Checking in mozilla/directory/c-sdk/ldap/clients/tools/ldaptool-sasl.c;
/cvsroot/mozilla/directory/c-sdk/ldap/clients/tools/Attic/ldaptool-sasl.c,v  <--  ldaptool-sasl.c
new revision: 1.2.2.3; previous revision: 1.2.2.2
done
Checking in mozilla/directory/c-sdk/ldap/clients/tools/ldaptool.h;
/cvsroot/mozilla/directory/c-sdk/ldap/clients/tools/ldaptool.h,v  <--  ldaptool.h
new revision: 5.7.8.2; previous revision: 5.7.8.1
done
Checking in mozilla/directory/c-sdk/ldap/libraries/liblber/lber-int.h;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/liblber/lber-int.h,v  <--  lber-int.h
new revision: 5.3.8.2; previous revision: 5.3.8.1
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/getoption.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/getoption.c,v  <--  getoption.c
new revision: 5.4.8.2; previous revision: 5.4.8.1
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/ldap-int.h;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/ldap-int.h,v  <--  ldap-int.h
new revision: 5.6.2.2; previous revision: 5.6.2.1
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/os-ip.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/os-ip.c,v  <--  os-ip.c
new revision: 5.9.8.2; previous revision: 5.9.8.1
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/request.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/request.c,v  <--  request.c
new revision: 5.5.8.1; previous revision: 5.5
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/saslbind.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/saslbind.c,v  <--  saslbind.c
new revision: 5.3.2.3; previous revision: 5.3.2.2
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/saslio.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/Attic/saslio.c,v  <--  saslio.c
new revision: 1.1.2.3; previous revision: 1.1.2.2
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/setoption.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/setoption.c,v  <--  setoption.c
new revision: 5.5.8.2; previous revision: 5.5.8.1
done
RCS file: /cvsroot/mozilla/directory/c-sdk/ldap/examples/Attic/nsprsasl.c,v
done
Checking in mozilla/directory/c-sdk/ldap/examples/nsprsasl.c;
/cvsroot/mozilla/directory/c-sdk/ldap/examples/Attic/nsprsasl.c,v  <--  nsprsasl.c
new revision: 1.1.2.1; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/directory/c-sdk/ldap/examples/Attic/nsprsaslsearch.c,v
done
Checking in mozilla/directory/c-sdk/ldap/examples/nsprsaslsearch.c;
/cvsroot/mozilla/directory/c-sdk/ldap/examples/Attic/nsprsaslsearch.c,v  <--  nsprsaslsearch.c
new revision: 1.1.2.1; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/directory/c-sdk/ldap/examples/Attic/sasl.c,v
done
Checking in mozilla/directory/c-sdk/ldap/examples/sasl.c;
/cvsroot/mozilla/directory/c-sdk/ldap/examples/Attic/sasl.c,v  <--  sasl.c
new revision: 1.1.2.1; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/directory/c-sdk/ldap/examples/Attic/saslsearch.c,v
done
Checking in mozilla/directory/c-sdk/ldap/examples/saslsearch.c;
/cvsroot/mozilla/directory/c-sdk/ldap/examples/Attic/saslsearch.c,v  <--  saslsearch.c
new revision: 1.1.2.1; previous revision: 1.1
done
Checking in mozilla/directory/c-sdk/ldap/examples/Makefile;
/cvsroot/mozilla/directory/c-sdk/ldap/examples/Makefile,v  <--  Makefile
new revision: 5.2.8.1; previous revision: 5.2
done

Comment 72

[nkinder]

An additional change is needed for the SDK to build properly without SASL support.  The nsldapi_sasl_bind_s() function is currently inside of an "ifdef LDAP_SASLIO_HOOKS" block.  This function is used by the old ldap_sasl_bind_s() function, so it should not be ifdef'd out when not building with SASL enabled.  

Here's the fix:

Index: saslbind.c
===================================================================
RCS file: /cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/saslbind.c,v
retrieving revision 5.3.2.3
diff -u -4 -t -r5.3.2.3 saslbind.c
--- saslbind.c  17 Aug 2006 14:44:39 -0000      5.3.2.3
+++ saslbind.c  17 Aug 2006 16:17:27 -0000
@@ -320,8 +320,9 @@

         ldap_charray_free( props );
         return( LDAP_SUCCESS );
 }
+#endif /* LDAP_SASLIO_HOOKS */

 static int
 nsldapi_sasl_bind_s(
     LDAP                *ld,
@@ -370,8 +371,9 @@

         return( ldap_result2error( ld, result, 1 ) );
 }

+#ifdef LDAP_SASLIO_HOOKS
 static int
 nsldapi_sasl_do_bind( LDAP *ld, const char *dn,
         const char *mechs, unsigned flags,
         LDAP_SASL_INTERACT_PROC *callback, void *defaults,

Comment 73

[Mark Smith [:mcs]]

(In reply to comment #72)
> An additional change is needed for the SDK to build properly without SASL
> support.  The nsldapi_sasl_bind_s() function is currently inside of an "ifdef
> LDAP_SASLIO_HOOKS" block.  This function is used by the old ldap_sasl_bind_s()
> function, so it should not be ifdef'd out when not building with SASL enabled. 

Looks fine to me.

Comment 74

[nkinder]

Checked in the fix proposed in comment 72 to the merge branch.

Checking in saslbind.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/saslbind.c,v  <--  saslbind.c
new revision: 5.3.2.4; previous revision: 5.3.2.3
done

Comment 75

[Anton Bobrov]

folks, i just tried Windows build with SASL enabled and it doesnt work. first
there is unknown type uint32_t in the code [ should be ulong on Windows ] and
build scripts / Makefiles do not cater for Windows build either, as they are.
 

Comment 76

[Rich Megginson]

(In reply to comment #75)
> folks, i just tried Windows build with SASL enabled and it doesnt work. first
> there is unknown type uint32_t in the code [ should be ulong on Windows ] and
> build scripts / Makefiles do not cater for Windows build either, as they are.

Which SASL did you use?  The cygwin one?  I was hoping there would be some resolution to the LDAP SASL on Windows problem within the Mozilla clients (address book), but I have not seen any more progress on that effort.  I would like to be able to use the native SASL support on Windows, if any.  I know Windows supports Kerberos natively, so I assume there's some way to use that via GSSAPI.  I doubt Windows natively supports Digest-MD5, but maybe they do, I just don't know.

Comment 77

[Anton Bobrov]

(In reply to comment #76)

we use inhouse builds of Cyrus SASL. on Windows there is only one plugin we
have with our build and its digestmd5 but i would like to have that working
as it used to anyway. afaik if you try building Cyrus SASL on Windows there
is a bunch of other plugins you can enable [ some are beta/experimental 
state on Windows ]. gssapi is there and kerberos is doable i think tho you 
are likely to tweak few things in libSASL build to make it work. i've never
tried any of that myself because we are pretty much wired to QA'ed inhouse
SASL builds and there seem to be no notion of bringing more plugins for Win
right now.

Comment 78

[Rich Megginson]

So is it that your pre-merge SASL code worked with your in-house Cyrus SASL build on Windows, and post-merge, it doesn't work?  Do you have any idea about what broke?

I'll try building Cyrus SASL natively on Windows and see what happens.

Comment 79

[Anton Bobrov]

(In reply to comment #78)
> So is it that your pre-merge SASL code worked with your in-house Cyrus SASL
> build on Windows, and post-merge, it doesn't work?  Do you have any idea about
> what broke?

yeah, i actually trying to make a patch but cant seem to find a clean way to do
it [ more below ]. regarding what got broken i did identify the following :

- http://lxr.mozilla.org/mozilla/source/directory/c-sdk/ldap/libraries/libldap/saslio.c#124
there is a cast to uint32_t which doesnt exist on Doze, should be ulong, ifdef.

- http://lxr.mozilla.org/mozilla/source/directory/c-sdk/build.mk#157 should be  changed as well.

- http://lxr.mozilla.org/mozilla/source/directory/c-sdk/config/autoconf/sasl.m4#97 doesnt know how to check for libsasl on Doze.

this last one is especially nasty. i cant seem to find a clean way for configure to check if we have good and proper sasl32.lib. AC_CHECK_LIB doesnt work on Doze, AC_CHECK_FILE is not cross compliant or so autoconf complains. i really dunno what kinda check should be placed in there.

Comment 80

[Rich Megginson]

I think I have a fix for the last one - you have to hack LIBS and LDFLAGS to make AC_CHECK_LIB work.  The hack is not too bad.  I'm testing it right now - will post a diff later today.

Comment 81

[Rich Megginson]

Attachment: Windows SASL - list of files modified

Comment 82

[Rich Megginson]

Attachment: Windows SASL - diffs to fix

The biggest change was to hack sasl.m4 to make AC_CHECK_LIB work on Windows.  I had to override LIBS to be able to pass in /link /LIBPATH:c:/path/to/sasl libsasl.lib.
Another big change was to implement getlogin() on Windows, required by ldaptool-sasl.c.  The implementation is pretty simple.
There were several places in the Makefiles that assumed Windows meant no support for SASL, so I just changed them, and added some support for SASL_LIBS in some places where it was missing.

Comment 83

[Anton Bobrov]

Rich, great stuff, thanx a bunch! i just tested the patch and it works good
for me apart from one thing: the lib name in my case is sasl32.lib and not
libsasl[2].lib i think the later is purely Cygwin name tho i might be quite
mistaken. i would also rather have _WINDOWS defines there too, like this :

#if defined( _WINDOWS ) || defined( _WIN32 )

not sure if its required as it seems to work just fine as it is on W2K but
many places in the code check it like that instead.

Comment 84

[Mark Smith [:mcs]]

Comment on attachment 252483 [details] [diff] [review]
Windows SASL - diffs to fix

OK.

Comment 85

[Rich Megginson]

Attachment: Diffs to address Anton's concerns

These diffs change the configure code to first look for sasl32.lib on Windows, then libsasl2.lib and libsasl.lib.  This doesn't support cygwin sasl - I tested this with the sasl source straight from CMU and just built it using the NTMakefiles they supply, and used the cmd shell and nmake to build.  It builds libsasl.lib/dll if you do it that way.

Comment 86

[Anton Bobrov]

thanx alot Rich! you are absolutely right, i just checked Cyrus SASL code i have
here [ 2.1.21 ] and indeed it is libsasl.lib by default but i do appreciate you
making this change to accommodate our somewhat custom as it seems needs re sasl.
 

Comment 87

[Rich Megginson]

Committed.

Checking in mozilla/directory/c-sdk/build.mk;
/cvsroot/mozilla/directory/c-sdk/build.mk,v  <--  build.mk
new revision: 5.36; previous revision: 5.35
done
Checking in mozilla/directory/c-sdk/configure;
/cvsroot/mozilla/directory/c-sdk/configure,v  <--  configure
new revision: 5.60; previous revision: 5.59
done
Checking in mozilla/directory/c-sdk/config/autoconf/sasl.m4;
/cvsroot/mozilla/directory/c-sdk/config/autoconf/sasl.m4,v  <--  sasl.m4
new revision: 5.4; previous revision: 5.3
done
Checking in mozilla/directory/c-sdk/ldap/clients/tools/Makefile.in;
/cvsroot/mozilla/directory/c-sdk/ldap/clients/tools/Makefile.in,v  <--  Makefile
.in
new revision: 5.16; previous revision: 5.15
done
Checking in mozilla/directory/c-sdk/ldap/clients/tools/ldaptool-sasl.c;
/cvsroot/mozilla/directory/c-sdk/ldap/clients/tools/ldaptool-sasl.c,v  <--  ldap
tool-sasl.c
new revision: 5.2; previous revision: 5.1
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/Makefile.in;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/Makefile.in,v  <--  Make
file.in
new revision: 5.21; previous revision: 5.20
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/saslio.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/saslio.c,v  <--  saslio.
c
new revision: 5.2; previous revision: 5.1
done

Comment 88

[Anton Bobrov]

Mark, i think its safe to close this one now.

Comment 89

[Mark Banner (:standard8)]

Comment on attachment 232806 [details]
minor changes to previous diffs

Clearing old review requests that appear to be no longer valid. If they are, please move the patches to open bugs and re-request review there.