347033 - Client cert auth doesn't work with startTLS

Status: RESOLVED FIXED

(Directory :: LDAP Tools, defect)

Description

[Rich Megginson]

If the command line tools have been built with svrcore support, using -ZZ and -N does not work - no combination of -W and -I will pass the password to unlock the key db.

Comment 1

[Rich Megginson]

Attachment: diffs for fix

Elsewhere in the code it tests if (isZZ || secure) to mean that it has to perform SSL related initialization.  This was just one case that was missed.

Comment 2

[Mark Smith [:mcs]]

Comment on attachment 231780 [details]
diffs for fix

OK.  I looked and do not see any other places we missed.

Comment 3

[Rich Megginson]

Checking in common.c;
/cvsroot/mozilla/directory/c-sdk/ldap/clients/tools/common.c,v  <--  common.c
new revision: 5.18; previous revision: 5.17
done

Comment 4

[Rich Megginson]

It doesn't look like it was ever designed to work.  Client auth is only enabled if  the LDAPSSLSessionInfo * has the lssei_client_auth field set to TRUE.  This is set in ldapssl_enable_clientauth.  This works fine for the non-starttls case because the field is not checked until ldapssl_connect() which normally occurs during the initial connection in ldap_sasl_bind.  In the starttls case, the check for lssei_client_auth is done _before_ ldapssl_enable_clientauth is called.  The way ldapssl_enable_clientauth() is written, it requires a valid LDAP* structure with all of the SSL routines already enabled, which can only happen after ldapssl_init or ldap_start_tls_s.

One approach would be to use ldap_set_option (or ldapssl_set_option?) to set the certname and keypassword in the ld before calling ldap_start_tls_s.  Then ldaptls_setup or ldaptls_complete could call ldapssl_enable_clientauth with the properly initialized ld, certname, and keypassword parameters.

Comment 5

[Mark Smith [:mcs]]

Your suggested approach sounds OK (is that what Sun and OpenLDAP do?)  But it would be nice if ldapssl_enable_clientauth() could be changed so it did not require that SSL already be enabled.  Looking at the code, it may be possible to change ldapssl_enable_clientauth() and ldaptls_setup() to check to see if the session info structure (LDAPSSLSessionInfo) has been allocated already and to allocate one if not.

Comment 6

[Rich Megginson]

I think that is the more desirable approach - make ldapssl_enable_clientauth() able to be called before ldap_start_tls_s().  I think the Sun SDK has the same problem - the test to see if sseip->lssei_client_auth is true in ldaptls_complete() comes before the call to ldapssl_enable_clientauth(), and ldapssl_enable_clientauth() has the same problem ours does.

It looks like OpenLDAP may work - it takes the approach of storing everything via ldap_set_option() so the private key is available at any time during the handshake.

Comment 7

[Anton Bobrov]

Rich, as discussed earlier Sun SDK dropped svrcore while ago and we never
got around replacing it. now that RedHat opensourced svrcore we can use
it again so i cannot really add any valuable comments here on svrcore use
in this case.

regarding ldapssl_enable_clientauth before ldap_start_tls_s. did you look
at the ldaptool_ldap_init of common.c in Sun SDK. thats exactly how its
done there. i added start tls support there while ago so i'm kinda rusty
on details but it worked just fine and passed all the tests. did you find
it doesnt work in this case ? maybe i didnt gasp the problem described in
this bug just yet so let me know and will have a better look at it then.

(In reply to comment #6)
> I think that is the more desirable approach - make ldapssl_enable_clientauth()
> able to be called before ldap_start_tls_s().  I think the Sun SDK has the same
> problem - the test to see if sseip->lssei_client_auth is true in
> ldaptls_complete() comes before the call to ldapssl_enable_clientauth(), and
> ldapssl_enable_clientauth() has the same problem ours does.
> 
> It looks like OpenLDAP may work - it takes the approach of storing everything
> via ldap_set_option() so the private key is available at any time during the
> handshake.
> 

Comment 8

[Rich Megginson]

Thanks Anton.  Ok, I see that.  I can just grab the sun code.

Comment 9

[Rich Megginson]

Attachment: enable clientauth for starttls

This allows ldapssl_enable_clientauth to be called before ldap_start_tls_s.  If the session info has not been set, it will allocate it and set it in the pr layer.  A similar change is made to ldaptls_setup.  It checks for an existing session info and just uses it instead of creating a new one every time, and allocates a new one if there isn't an existing one.

The Sun version had some additional code to change the ldapssl_connect function to accept an additional clientauth flag, but this appears to be unused in their code, and our code otherwise looks the same.

With these fixes, I was able to successfully use client auth both with and without starttls using ldapsearch.

Comment 10

[Mark Smith [:mcs]]

Comment on attachment 231956 [details]
enable clientauth for starttls


>-    /*
>-     * Store session info. for later retrieval.
>-     */
>+
>+    memset( &sei, 0, sizeof(sei));
>     sei.seinfo_size = PRLDAP_SESSIONINFO_SIZE;
>-    sei.seinfo_appdata = (void *)ssip;
>-    if (LDAP_SUCCESS != (rc = prldap_set_session_info( ld, NULL, &sei ))) {
>-	ldapssl_free_session_info( &ssip );
>-	return( rc );
>+    if ( prldap_get_session_info( ld, NULL, &sei ) == LDAP_SUCCESS ) {
>+	ssip = (LDAPSSLSessionInfo *)sei.seinfo_appdata;
>+    } else {
>+	return( -1 );
>+    }

We need to return an LDAP error code here instead of -1 (just return the value returned by prldap_get_session_info).

Everything else looks okay to me.

Comment 11

[Rich Megginson]

Attachment: new diffs - new return codes

I revised the return codes to always return an LDAP error code instead of -1.  I found one place in common.c that expected return code < 0 meant error, so I changed it to != LDAP_SUCCESS instead.  All of the other places I checked used a non-zero return code for error.

Comment 12

[Mark Smith [:mcs]]

Comment on attachment 231978 [details]
new diffs - new return codes

I do not think we should change the return values for ldapssl_enable_clientauth() (existing application code expects it to return 0 upon success and -1 upon error).

Comment 13

[Rich Megginson]

Attachment: better diffs

Ok.  Changed the return codes back for ldapssl_enable_clientauth.

Comment 14

[Mark Smith [:mcs]]

Comment on attachment 232064 [details]
better diffs

Looks good now.  Thanks.

Comment 15

[Rich Megginson]

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.19; previous revision: 5.18
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libssldap/ldapsinit.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libssldap/ldapsinit.c,v  <--  ldapsinit.c
new revision: 5.14; previous revision: 5.13
done