Client cert auth doesn't work with startTLS

RESOLVED FIXED

Status

Directory
LDAP Tools
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Rich Megginson, Assigned: mcs)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
Created attachment 231780 [details]
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.
Attachment #231780 - Flags: review?(mcs)
(Assignee)

Comment 2

11 years ago
Comment on attachment 231780 [details]
diffs for fix

OK.  I looked and do not see any other places we missed.
Attachment #231780 - Flags: review?(mcs) → review+
(Reporter)

Comment 3

11 years ago
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Reporter)

Comment 4

11 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 5

11 years ago
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.
(Reporter)

Comment 6

11 years ago
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

11 years ago
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.
> 
(Reporter)

Comment 8

11 years ago
Thanks Anton.  Ok, I see that.  I can just grab the sun code.
(Reporter)

Comment 9

11 years ago
Created attachment 231956 [details]
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.
Attachment #231956 - Flags: review?(mcs)
(Assignee)

Comment 10

11 years ago
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.
Attachment #231956 - Flags: review?(mcs) → review-
(Reporter)

Comment 11

11 years ago
Created attachment 231978 [details]
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.
Attachment #231956 - Attachment is obsolete: true
Attachment #231978 - Flags: review?(mcs)
(Assignee)

Comment 12

11 years ago
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).
(Reporter)

Comment 13

11 years ago
Created attachment 232064 [details]
better diffs

Ok.  Changed the return codes back for ldapssl_enable_clientauth.
Attachment #231978 - Attachment is obsolete: true
Attachment #232064 - Flags: review?(mcs)
Attachment #231978 - Flags: review?(mcs)
(Assignee)

Comment 14

11 years ago
Comment on attachment 232064 [details]
better diffs

Looks good now.  Thanks.
Attachment #232064 - Flags: review?(mcs) → review+
(Reporter)

Comment 15

11 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.