User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/84.0.4147.125 Safari/537.36

Steps to reproduce:

Updated from Thunderbird 68.9 to 78.1.1

Actual results:

no results when searching in Addressbook which should connect to my Domaincontroller with SSL Port 636 (no errors or warnings)
If i change to port 389 without SSL, it works fine
my servers already supports ldaps connections and LDAP with SSL was tested with tool ldp .
LDAP with SSL also works fine with Thunderbird 68

Expected results:

searching in Addressbook should also work with LDAP over SSL, because in future Microsoft will reject connections without ldaps

It works for me on a server we frequently use for testing. Perhaps your server doesn't support the same TLS (SSL) version as Thunderbird 78? TLS 1.0 and 1.1 are now disabled by default.

We're now requiring TLS 1.2 or higher. It's possible to set security.tls.version.min to 0 to override this.
Or is the certificate self signed?

hi guys, thank you for answering:
setting security.tls.version.min to 0 did not solve my problem and LDAP-Version is 3 and 2
Powershell Get-ADRootDSE -Server [myfqdomain] = supportedLDAPVersion: {3, 2}

Certificate is from an Active-Directory-Integrated Certificate Authority which is self signed, but i think, many users already have this kind of certificates

I guess the LDAP version of bug 1590474.

Bug 528922 comment 46 may have a workaround

A few notes to myself:

• we'll be needing the securityInfo if the TLS negotiation fails, in order to get at the bad cert (so we can present the add-an-exception dialog box). The current nsLDAPSSLConnect() (in ldap/xpcom/src/nsLDAPSecurityGlue.cpp) io callback does receive the secInfo when connecting, but throws it away. Looks straightforward enough to stash a reference in the session_private data so we'll be able to get at it later on.

• the current prldap poll function causes an assert in the NSS layer, when a handshake errors. It seems the handshake errors are communicated by out-of-band/high priority data... and there's an assert that triggers because the ldap poll usage doesn't set the PR_EXCEPT flag when polling, which is when checking for out-of-band-data. There's a corresponding ldap flag (LDAP_X_POLLPRI) but it's never used in C-C code. Forcing PR_EXCEPT flag in prldap_poll seems to get it working, although this seems a little dodgy... should be set in one of the other layers, but I haven't found out where those flags are set up. It's pretty convoluted.

• the ldap polling FD-descriptors (which say which socket's you want to poll, and which events you're interested in) are set up as a growable array, and seem a little odd: they always end up sending in the whole array to the polling function, rather than just the ones in use. So as far as I can see, we only need one FD-descriptor (we're only using one socket), but ldap always sets up 5 to pass into PR_Poll(). PR_Poll() can cope with unused ones, so it's not a big problem... just overcomplicated and silly.

• soooooo many layers!

  1. NSPR (platform-neutral sockets)
  2. NSS (the security layer - TLS handshaking etc)
  3. libldap - the generic ldap library (+extensions api + a couple of support libraries: liblber etc)
  4. libprldap - monkey-patches libldap to use NSPR instead of the socket support built into libldap.
  5. Our LDAP TLS layer (ldap/xpcom/src/nsLDAPSecurityGlue.cpp) - monkey-patches libprldap to add TLS support
  6. our XPCOM-based C++ ldap stuff (lots of it :- )
  7. The UI code which calls the XPCOM

First part - a nice simple one.
This tweaks libprldap to pick up oob/priorty packets, which is how the NSS layer communicates its errors. With this, we start getting proper security error codes via PR_GetError().

Next layer up is nsLDAPSecurityGlue, which patches a TLS-capable socket on top of libprldap.
This patch keeps hold of the securityInfo object (via a horrific russian-doll set of userdata structs!), and adds a function to get it back at a later time. We'll need this to access a failing certificate later, when we want to give the user the option to add a security exception.

Not ready for review yet - I'll need to test it properly once parts for the other layers are in place.

There are two more layers to do:

• Our XPCOM LDAP code. Need to add an error callback to the listener interface. Should be pretty straightforward (Bug 1592449).
• The Addressbook layer. The complicated bit. Need to providing the error handling at the GUI level (showing the add-an-exception dialog etc). Need to investigate how (if!) the addressbook interfaces handle errors. nsIAbDirSearchListener doesn't currently seem to have any error-handling, which I thought would have been the place for it. In any case, even if there is already error handling, it probably won't be general enough to express security errors (with secInfo access), so there'll be some finessing required.

nsIAbDirSearchListener doesn't currently seem to have any error-handling, which I thought would have been the place for it. In any case, even if there is already error handling, it probably won't be general enough to express security errors (with secInfo access), so there'll be some finessing required.

You're supposed to call onSearchFinished with an error code, but I can see that doesn't really fit with certificate errors. Perhaps add another callback that can handle the data you need to get to the UI. Then you've got the problem that there's multiple places where this code could be called (actually ext-addressBook.js and AbAutoCompleteSearch.jsm shouldn't be hitting LDAP).

(In reply to Geoff Lankow (:darktrojan) from comment #9)

You're supposed to call onSearchFinished with an error code, but I can see that doesn't really fit with certificate errors. Perhaps add another callback that can handle the data you need to get to the UI.

Doh - for some reason I just saw long aResult, didn't see any nsresult and thought "oh, it doesn't have an error code...". Cool.

I guess the cheesy thing is just add the failed securityInfo as a new member of nsIAbLDAPDirectory, and for error-handling code to try and QI to get at it... (assuming the nsIAbDirSearchListener has access to the directory object).
But in theory there could be other network-based addressbook implementations, right? So might be worth making more general...

New version of the nsLDAPSecurityGlue layer patch. It needed a little tweaking, but seems to do the trick now (mainly jumping through lots of libprldap hoops to stash the securityInfo somewhere it can be retrieved from, when required).

Up into the XPCOM LDAP layer now. This patch adds an extra listener callback, OnLDAPError(), including the securityInfo, if the failure was due to an NSS error.
It also removes the error-reporting responsibilities from OnLDAPInit().

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/c412f7c2284a
Enable NSS error codes in libprldap. r=mkmelin

(In reply to Magnus Melin [:mkmelin] from comment #16)

should we make sure we got a listener before continuing?

Yes - good point! Patch tweaked.

should null check secInfo I think

No, it's valid for secInfo to be null. The error might be unrelated to the NSS layer (secInfo irrelevant), or it might be an plain text connection (secInfo non-existent).

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/f1b318b027a4
Provide access to securityInfo in secure LDAP connections. r=mkmelin
https://hg.mozilla.org/comm-central/rev/44e81d98aeb6
Add OnLDAPError callback to nsILDAPMessageListener to allow better handling of security (NSS) errors. r=mkmelin

Second-to-last layer: this modifies nsIAbDirSearchListener.onSearchFinished() to pass out an nsresult and secInfo.
The patch also removes the redundant nsIAbDirectoryQueryResultListener interface, which looks like it was being phased out - there was just a little C++ use, and it was still providing bespoke result codes to nsIAbDirSearchListener...

The next (last!) patch will be to actually show the add-an-exception dialog box (I added a little TODO stub marking the place in abView.js).
There are also uses for autocomplete, but I'm not sure we should be popping up dialogs there... anyone got any strong opinions on that?

Geoff: does the new signature for nsIAbDirSearchListener.onSearchFinished() seem OK to you? I decided to ditch the bespoke search result codes in favour of nsresult codes (using NS_ERROR_ABORT for users stopping a search). There was also an error message param, but it never seemed to be used, so I ditched it.

Oh, almost forgot: I learned my lesson and did a try build :-)
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=89ab6f74b7df181c54ef5f0a787ba350cdd84d5e
seems promising so far...

Comment on attachment 9182522 [details] [diff] [review]
[part four, checked in] 1659947-part-four-revamp-nsIAbDirSearchListener-onSearchFinished-1.patch

Yes, I'm all for removing arcane solutions like this when a generic one will do.

This was meant to be the nice simple one, but I'd forgotten the certificate exception needs the location ("host:port"). So the callbacks have an extra location param now. Yay.
While I was at it I also opted to specialise the secInfo param to nsITransportSecurityInfo, which saves a QI on the JS side and makes more sense anyway.

Anyway - all works fine for me now with this patch. When I look up addresses in the addressbook view, it prompts me to add an exception for the self-signed cert on my LDAP connection, and subsequent attempts work just fine.

Could do the same for address autocompletion... but seems a little jarring to yank the user out to a dialog box while they are in the middle of typing.

Try run here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7ab5dbb3fc3841f2d3317ce53eaae886c8805a61

just quick notes (sorry, been mostly away today):

(In reply to Magnus Melin [:mkmelin] from comment #27)

(Comment on attachment 9182847 [details] [diff] [review])
Is there some semi easy way for me to test this?

I installed slapd and enabled TLS connections, following the directions here:
https://devconnected.com/how-to-setup-openldap-server-on-debian-10/
It's a little... arcane. Setting the certificates involves modifying ldap entries.
I never got as far as actually installing any addresses - it was just the SSL connection I wanted.

However, I think you could use any secure server (IMAP, SMTP whatever) - it's only the connection part we're testing here. It doesn't actually need to have functioning LDAP.

My testing procedure was:

1. Go to preferences -> Composition -> Addressing
2. enable "Directory Server"
3. "Edit Directories" and add an LDAP server entry, localhost, SSL enabled (I left the Base DN and Bind DN blank - at least one would need to be filled in for a server with real data, but blank is fine for testing connection)
4. open up address book window (click on "Address Book" in main window toolbar.
5. select the LDAP directory we just added
6. type something into the search bar and hit return.

I think this should be hooked up at least under add/edit directories too.

You mean in the preferences window where you add/edit directory server details? I agree, but I don't think this dialog currently does any kind of validation... I'd probably break this out into a new bug.

Agreed for searching we probably shouldn't do it.

Sure, not if the server setup is verified during a validation step...
That said, I think the search should be better at communicating errors to the user anyway (at the moment, all errors are just ignored).

The ones I didn't add it to were the auto-completion ones - I think these are used in the composition window?
Again, if the validation is done, these probably shouldn't bother offering the user the add-an-exception dialog. But it would be nice if there was some indication of addressbook errors occuring... (like a statusbar message or something? a dialog box is probably overly-intrusive).

::: mailnews/addrbook/public/nsIAbDirSearchListener.idl
@@ +32,5 @@

*                 to obtain a failing certificate, to present the user an
*                 option to add it as a security exception (handy for
*                 LDAP servers with self-signed certs).

• • @param location If status is an NSS error code, this holds the location

• •             of the failed operation ("<host>:<port>").
    

should include scheme, no? e.g. ldap://foo.bar:123

Hmm. I'm not sure. I don't think so - at least that wasn't the case for SMTP and IMAP.
I think the host/port is all that is required to add the exception.
I have an idea https is included because the dialog box knows how to download certificates for https addresses. It'd be the other "raw TLS-from-the-very start" connections like ldaps:// and imaps:// , but wouldn't work for StartTLS-based setups where it had to know how each individual protocol issues the StartTLS command...
Kind of a moot point anyway, because in all our cases we already have the certificate - we don't need to download it separately.

::: mailnews/addrbook/src/nsAbDirectoryQuery.cpp
@@ +201,2 @@

rv = query(aDirectory, expression, listener, doSubDirectories, &resultLimit);

this rv is not checked now
Oops, yes! Will revise...

oneliner change - now checks the return code from the query() call.

Ah, looks like it broke windows builds. https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=319422076&repo=comm-central&lineNumber=13142

Backout: https://hg.mozilla.org/comm-central/rev/163ecfe068e1a26843c888a1331d1305748a44ef

Doh! Sorry about that. This should fix it.
I can't seem to get a windows try build running at the moment, so best hold off landing again until we get one running clean. I'll keep trying (bad pun intended).

Try build seems to be building now:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9ea4fdef1bc271a6be10d37d9c5905543913aa0e

Just another try run on top of the fix for Bug 1673205:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ea4a00c3b8ede5a1bb066c3a91cf204ca2116660

There's some orange there, but I don't think there's anything I could attribute to this patch, so I'm going to flag it for landing.

Pushed by thunderbird@calypsoblue.org:
https://hg.mozilla.org/comm-central/rev/e48d3c359bcd
Allow user to add exceptions for bad certs in addressbook LDAP lookup. r=mkmelin

Comment on attachment 9183286 [details] [diff] [review]
[checked in] 1659947-part-five-cert-exception-dialog-3.patch

[Approval Request Comment]
User impact if declined: can't use LDAP with self signed cert
Testing completed (on c-c, etc.): yes
Risk to taking this patch (and alternatives if risky): has been working find on trunk/beta

Approval for all the patches in this bug.

Comment on attachment 9183286 [details] [diff] [review]
[checked in] 1659947-part-five-cert-exception-dialog-3.patch

[Triage Comment]
Approved for esr78

These are the commits I uplifted:
[1/5] c412f7c2284a
[2/5] f1b318b027a4
[3/5] 44e81d98aeb6
[bustage fix] acc903633940
[bustage fix] b52839dad5d0
[4/5] d6cb53b01e4e
[5/5] e48d3c359bcd (second version of this patch)

Thunderbird 78.5.1:
https://hg.mozilla.org/releases/comm-esr78/rev/d3df83ff6192
https://hg.mozilla.org/releases/comm-esr78/rev/fa83ee050cd5
https://hg.mozilla.org/releases/comm-esr78/rev/42278ed43eb8
https://hg.mozilla.org/releases/comm-esr78/rev/19dbf3c49152
https://hg.mozilla.org/releases/comm-esr78/rev/64b43dc39cb8
https://hg.mozilla.org/releases/comm-esr78/rev/810a2b9c7e59
https://hg.mozilla.org/releases/comm-esr78/rev/55f0e8bf4db0

There's a problem with this uplift to esr78.

14:55:28     INFO -  /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ -std=gnu++17 -o nsAbLDAPDirectory.o -c  -I/builds/worker/workspace/obj-build/dist/stl_wrappers -I/builds/worker/workspace/obj-build/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -ftrivial-auto-var-init=pattern -DDEBUG=1 -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/comm/mailnews/addrbook/src -I/builds/worker/workspace/obj-build/comm/mailnews/addrbook/src -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/mozilla-config.h -Qunused-arguments -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Wunused-function -Wunused-variable -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-error=tautological-type-limit-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=return-std-move -Wno-error=atomic-alignment -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fno-aligned-new -fcrash-diagnostics-dir=/builds/worker/artifacts -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/obj-build/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -Os -fno-omit-frame-pointer -funwind-tables -Werror -fexperimental-new-pass-manager  -MD -MP -MF .deps/nsAbLDAPDirectory.o.pp   /builds/worker/checkouts/gecko/comm/mailnews/addrbook/src/nsAbLDAPDirectory.cpp
14:55:28    ERROR -  /builds/worker/checkouts/gecko/comm/mailnews/addrbook/src/nsAbLDAPDirectory.cpp:346:34: error: out-of-line definition of 'OnSearchFinished' does not match any declaration in 'nsAbLDAPDirectory'

It looks like Bug 1633996 may be a dependency here. I tried to uplift that and it compiled locally, but then had problems running unit tests.

Unexpected Results
------------------
comm/mailnews/addrbook/test/unit/test_vCard.js
  FAIL comm/mailnews/addrbook/test/unit/test_vCard.js - xpcshell return code: 0
  FAIL testVCardToAbCard - [testVCardToAbCard : 24] expected BirthDay - "2" == "3"
/home/rob/moz/uplift/obj-mozilla-esr78-linux64/_tests/xpcshell/comm/mailnews/addrbook/test/unit/test_vCard.js:check:24
/home/rob/moz/uplift/obj-mozilla-esr78-linux64/_tests/xpcshell/comm/mailnews/addrbook/test/unit/test_vCard.js:testVCardToAbCard:125
/home/rob/moz/uplift/mozilla-esr78/testing/xpcshell/head.js:_run_next_test/<:1618
/home/rob/moz/uplift/mozilla-esr78/testing/xpcshell/head.js:_run_next_test:1618
/home/rob/moz/uplift/mozilla-esr78/testing/xpcshell/head.js:run:777
/home/rob/moz/uplift/mozilla-esr78/testing/xpcshell/head.js:_do_main:248
/home/rob/moz/uplift/mozilla-esr78/testing/xpcshell/head.js:_execute_test:577
-e:null:1

Might want bug 1658438 as well?

Scratch that. The vcard test only fails locally for me. Uplifting bug 1633996 fixes the compile issue and tests pass on my try build.