153250 - need a way to set SSL options when using libssldap

Status: RESOLVED FIXED

(Directory :: LDAP C SDK, enhancement, P1)

Description

[Mark Smith [:mcs]]

Several people have asked for a way to set SSL options (as you can do with the
NSS call SSL_OptionSet). Currently there is no way to do it if you use the
libssldap layer. Now we (Netscape) have a need for this ASAP. I propose we add
two API calls:

/*
 * Set or get SSL options on an existing SSL-enabled LDAP session handle.
 * If ld is NULL, the default options used for all future LDAP SSL sessions
 * are the ones affected. The option values are specific to the underlying
 * SSL provider; see ssl.h within the Network Security Services (NSS)
 * distribution for the options supported by NSS (the default SSL provider).
 *
 * These functions should be called before any LDAP connections are created.
 * Both functions return 0 if all goes well.
 */
int LDAP_CALL ldapssl_option_set( LDAP *ld, PRInt32 option, PRBool on );
int LDAP_CALL ldapssl_option_get( LDAP *ld, PRInt32 option, PRBool *onp );

Comment 1

[Mark Smith [:mcs]]

Working on this.

Comment 2

[Mark Smith [:mcs]]

*** Bug 153249 has been marked as a duplicate of this bug. ***

Comment 3

[Mark Smith [:mcs]]

Attachment: proposed fix

The new code is fairly straightforward, although supporting ldapssl_option_set(
NULL, ... ) as well as ldapssl_option_set( ld, ... ) and preserving the NSS
defaults in the absence of any ldapssl_option_set() makes for a bit more code
than I expected. Not too bad though.

I also fixed a bug in common.c:ldaptool_print_lderror (LDAP command line tools)
where we did not check for SSL errors when the error code was "can't connect."
We were only checking on "server down."

I also fixed a bug in ldapsinit:do_ldapssl_connect() that sometimes caused
PR_Close() to be called twice on an SSL file descriptor when an error occurred
(once in do_ldapssl_connect() itself and once in the libprldap close function
that is called from do_ldapssl_connect()).

Comment 4

[Mark Smith [:mcs]]

Forgot to mention that I also updated the error string mapping tables in
libssldap (a few newer NSPR and NSS errors were missing).

Comment 5

[Mark Smith [:mcs]]

Attachment: revised fix: avoid NSPR types in the ldapssl_ API

Slight change of plans: we don't use NSPR types anywhere else in the ldapssl_
API, so I'd rather not start now. Replaced PRBool with an int. Otherwise
identical to the first patch.

Comment 6

[Rich Megginson]

Looks ok, but it would be nice to get another reviewer.

Comment 7

[Mark Smith [:mcs]]

I just found out about bug 135261:

  "SSL_REQUIRE_CERTIFICATE semantic troubles"
  http://bugzilla.mozilla.org/show_bug.cgi?id=153250

The proposed fix involves allowing 4 values for the SSL_REQUIRE_CERTIFICATE
option (and shoving them into a PRBool so there is no API change within NSS).
But my proposed ldapssl_option_get/set() implementation assumes all SSL options
can be represented in one bit. How inconvenient.

Comment 8

[Mark Smith [:mcs]]

Attachment: revised fix: store a PRBool value for each option

Same as the last patch, except for ldapsinit.c: we now store a complete PRBool
value for each option (same as the NSS API supports). This simplifies the code
as well (at the expense of taking up more space for each LDAPS session handle).

Comment 9

[Rich Megginson]

Ok.  Doesn't take that much more space to use an array than a bit vector.

Comment 10

[Mark Smith [:mcs]]

Attachment: revised fix: rename new ldapssl functions; add exports for MacOS and Windows

Comment 11

[Mark Smith [:mcs]]

I have been convinced to go with these function names:

ldapssl_set_option()
ldapssl_get_option()

I think these names are more consistent with those used for other libssldap and
libldap functions, e.g., ldap_set_option(). Building on Win32 now before I
commit this.

Comment 12

[Rich Megginson]

Ok.

Comment 13

[Mark Smith [:mcs]]

Fix committed to the trunk:

mozilla/directory/c-sdk/ldap/clients/tools/common.c
  new revision: 5.4; previous revision: 5.3
mozilla/directory/c-sdk/ldap/include/ldap_ssl.h
  new revision: 5.2; previous revision: 5.1
mozilla/directory/c-sdk/ldap/libraries/libldap_ssl.ex
  new revision: 5.2; previous revision: 5.1
mozilla/directory/c-sdk/ldap/libraries/libssldap/ldapsinit.c
  new revision: 5.4; previous revision: 5.3
mozilla/directory/c-sdk/ldap/libraries/libssldap/prerrstrs.h
  new revision: 5.1; previous revision: 5.0
mozilla/directory/c-sdk/ldap/libraries/libssldap/secerrstrs.h
  new revision: 5.1; previous revision: 5.0
mozilla/directory/c-sdk/ldap/libraries/libssldap/sslerrstrs.h
  new revision: 5.1; previous revision: 5.0
mozilla/directory/c-sdk/ldap/libraries/macintosh/LDAPSSLClient.exp
  new revision: 5.2; previous revision: 5.1
mozilla/directory/c-sdk/ldap/libraries/msdos/winsock/ldapssl.def
  new revision: 5.2; previous revision: 5.1
mozilla/directory/c-sdk/ldap/libraries/msdos/winsock/nsldapssl32.def
  new revision: 5.2; previous revision: 5.1
mozilla/directory/c-sdk/ldap/libraries/msdos/winsock/nssldap32.def
  new revision: 5.2; previous revision: 5.1
    Fix bug # 153250 - need a way to set SSL options when using libssldap.
        Added two new libssldap public functions: ldapssl_set_option() and
            ldapssl_get_option().
        Also fixed a bug in ldapsinit:do_ldapssl_connect() that sometimes
            caused PR_Close() to be called twice on an SSL file descriptor
            if an error occurred (once in do_ldapssl_connect() itself and
            once in the libprldap close function that is called from
            do_ldapssl_connect()).
        Also updated the NSPR and NSS "error code to string" mapping
            tables that are used by ldapssl_err2string().
        Also fixed a bug in common.c:ldaptool_print_lderror (LDAP command
            line tools) where we did not check for SSL errors when the
            error code was "can't connect."  We were only checking on
            "server down" errors.

Comment 14

[Frankie]

Spam for bug 129472