Closed Bug 308118 Opened 19 years ago Closed 16 years ago

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

Categories

(MailNews Core :: LDAP Integration, enhancement)

Product:
MailNews Core
Component:
LDAP Integration
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
Thunderbird 3.0b3

People

(Reporter: dautermann, Assigned: simon)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 8 obsolete files)

LDAP GSSAPI patch updated to latest Hg tip
49.12 KB, patch
dmosedale
: review+
dmosedale
: superreview+
Details | Diff | Splinter Review
thunderbird ldap bind dump
3.74 KB, application/octet-stream
Details
ldap admin bind dump
10.88 KB, application/octet-stream
Details
User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6 Build Identifier: Thunderbird version 1.6a1 (20050908) On an LDAP directory server, there is usually public info and some priviledged info (e.g. internal phone numbers, physical location, etc.) that is only accessible to users who can authenticate (bind) to the server. Instead of saving passwords locally, one option is to use Kerberos tickets to bind to LDAP servers that allow GSSAPI authentication. Reproducible: Always Steps to Reproduce: 1. Go to "Address Book" in Thunderbird. 2. Under the "File" menu, there's a submenu attached to the "New" menu item. Click "LDAP directory". 3. You can choose one of two LDAP servers: Name: U-Penn Hostname: directory.upenn.edu Base DN: ou=People,dc=upenn,dc=edu Port number: 389 Bind DN: ___________ blank, a.f.a.i.k. once bound, the Bind DN is the same as the Base DN plus the addition of "uid=<myloginname>". But I believe this would be done programatically. Name: U-Michigan Hostname: ldap.itd.umich.edu Base DN: ou=people,dc=umich,dc=edu Port number: 389 Bind DN: _________ (leave blank) 4. If (and only if) the LDAP servers admit that they can handle GSSAPI authentication, attempt to log in using Kerberos credentials. If credentials don't exist (or are expired) on the user's machine, the Kerberos Ticket Manager will prompt the user for the password to generate the credentials. 5. Then the user is bound. voila! Expected Results: this is an enhancement request. I'm working on the LDAP modifications right now (it's nice to see the stuff under c-sdk/ldap seem to be a direct lift from OpenLDAP).
Version: unspecified → Trunk
Product: Thunderbird → Core
Component: Security → MailNews: LDAP Integration
> it's nice to see the stuff under c-sdk/ldap seem to be a > direct lift from OpenLDAP The Mozilla and OpenLDAP core libldap code share a common origin: both are based on the University of Michigan LDAP 3.3 release (circa 1996). But the two LDAP implementations have diverged a lot since then. Also be careful about reuse of OpenLDAP code (I am not sure if their license is compatible with those used by Mozilla or not).
Assignee: dveditz → dautermann
Status: UNCONFIRMED → NEW
Ever confirmed: true
The main trick here is going to be figuring out exactly where the GSSAPI code ought to live. I believe the LDAP C SDK supports SASL, and if the SASL-GSSAPI support works, my suspicion is that exposing that functionality is going to be the easiest way to go. Even better would be if we can do so by implementing nsIAuthModule and not having to invent a new interface. Whether this is really the most practical approach, I don't know, as I have experience with none of SASL, GSSAPI, nor nsIAuthModule. I'd be interested to hear thoughts from Darin, David, MCS, or anyone else on this issue./
I don't have experience with the ldap sdk authentication code, but nsIAuthModule is pretty trivial to use, from mozilla c++ code. But, GSSAPI involves several round trips, usually, which means that the ldap xpcom code can't simply preflight the data and pass it into the ldap cdk - you pass data to the server, it sends data back, you calculate your response based on that data, etc. nsIAuthModule handles all that. Cc'ing Simon, in case he has some experience with the LDAP CDK.
The LDAP C SDK (libldap) provides a way to send SASL data inside LDAP bind (authentication) requests and a way to receive SASL response data. That is about all it provides for SASL currently, but it should be enough to layer any SASL scheme on top.
I haven't looked in any detail at the LDAP SDK bind implementation, or at the RFC describing the SASL profile for LDAP. However, its worth nothing that, unlike most other SASL mechanisms, GSSAPI requires multiple rounds between client and server. The process looks something like this (this is for Kerberos, with mutual auth requested, but without delegation) C->S: GSSAPI packet S->C: GSSAPI response C->S: empty packet S->C: gss_wrap()ed packet containing list of acceptable security layers C->S: gss_wrap()ed packet containing selected security layer, and desired authorization identity. SASL negotiation isn't complete until the server has received this final message. The GSSAPI implementation of nsIAuthModule will handle all of this for you - the application just needs to take care of shipping payloads to and from nsIAuthModule. There's no support for security layers as yet - the implementation will always negotiate no security layer.
Any news about this issue? I just tried Thunderbird 1.5, but it seems to not support Kerberos for LDAP. regards Hadmut
I believe Michael is working on this
There's some debugging stuff in here that I have to remove still (e.g. some of the DNS stuff for a machine that wasn't resolving nicely) and a tiny bit of non-used stuff (e.g. the "DoWeUseGSSAPI" function), but this is a first pass at a patch for the problem. I still have to find out if I can get away with not using the nsIAuthModule for this solution.
I realise that this is a first cut of the code, but I have a couple of concerns. Firstly, this adds a requirement on Cyrus SASL to the build. Given that the code doesn't use Cyrus for any of the SASL support in mailnews (which uses, variously, nsIAuthModule and some internal implementations of DIGEST-MD5, CRAM-MD5 and plain), personally I think it would be better to avoid this depenency here. The AuthModule GSSAPI implementation should do all you need here, in any case, and with less complex callback requirements. Secondly, I'm concerned by the direct calls into the Kerberos library, which breaks through both the GSSAPI and SASL abstraction layers. You shouldn't provide either a authcid or authzid when doing a GSSAPI LDAP bind, unless those are explicitly different from those which can be infered by the server from the GSSAPI identity. Feel free to contact me directly - I'd especially be interested in what the problems you found with the nsIAuthModule implementation were.
According to what I've heard (scratching my head here), both Macintosh and Linux distributions come with Cyrus SASL libraries included now. This obviously wouldn't work on a Windows machine that didn't have Cyrus intalled. I do intend to do a pass of this with nsIAuthModule support and hope to do it very quickly (after a long planned 10 day vacation I'll be starting up tonight). Another concern I had with using the nsIAuthModule is that I'd *like* to programatically retrieve the username that the Kerberos user is authenticated as, instead of depending on the user to add the "uid=XXXXX" bit into the bind dn text field. The nsIAuthModule wouldn't allow me to get the principal or figure out the authenticated name.
It's kind of hard to generalize about Linux distributions as there are so many of them, with such different sets of bundled packages. I'll give you that the major players (RedHat, Debian, SuSe) all pretty much require that you install a version of Cyrus SASL in order to make it work as a server. On the Mac, Apple does ship with the Cyrus SASL libraries, but doesn't (currrently) seem to bundle the headers required to build against whichever version their shipping. My concern isn't really limited to particular distribution issues, rather just the fact that Thunderbird already has three protocols (IMAP, POP3, SMTP) with SASL support, including GSSAPI, without adding an external library dependency. It seems a shame to add an additional major dependency just to include LDAP support. In response to your second point - I don't think that you can portably infer the bind DN in this way. For example, some sites may have DNs of the form "cn=Simon Wilkinson, ou=..." rather than using uid DNs. The correct way to do this is to allow the server to infer the bind DN based on the SASL identity being asserted by the client. OpenLDAP uses sasl-regexp to acheive this - other servers should have similar mechanisms. The mechanism you're currently using to do this breaks through multiple abstraction layers. In addition to creating dependencies upon Kerberos, it will create depenencies upon whichever Kerberos library it is compiled against, as none of the Kerberos vendors have API (let alone ABI) compatible libraries. Sorry to be down on these two things - I think having this code is a really good thing, and will increase Thunderbird's deployability in many corporate environments.
(In reply to comment #11) > My concern isn't really limited to particular distribution issues, rather just > the fact that Thunderbird already has three protocols (IMAP, POP3, SMTP) with > SASL support, including GSSAPI, without adding an external library dependency. > It seems a shame to add an additional major dependency just to > include LDAP support. This is a reasonable point of view for Thunderbird. There are other (non-Mozilla) projects that use the LDAP C SDK that can't use nsIAuthModule. That argues for an LDAP C SDK that provides the hooks to use either SASL implementation. Since this bug is about TBird, the focus should be on what is best for TBird. I don't know anything about this history of nsIAuthModule, e.g., why the Cyrus SASL code was not used inside an XPCOM wrapper. > In response to your second point - I don't think that you can portably infer > the bind DN in this way. I agree 100% on that point. Either the user needs to configure the bind DN or the server needs to determine it (preferrably the 2nd thing).
What if we create our own nsIAuthModule implementation for non-Mozilla clients? I don't think we would have to go the full XPCOM route, we could just make our implementation look like the XPCOM nsIAuthModule to the ldap code.
One option is to have the library return control to the caller at each step in the SASL handshake. So you could have int ldap_sasl_bind_start(LDAP *ld, const char **mechlist, char **mechanism); /* Start establishes an anonymous bind with the server, extracts supportedSASLMechanisms from * the rootDSE, and selects a mutually supported mechanism from the list in mechlist, returning it * to the caller in mechanism */ int ldap_sasl_bind_continue(LDAP *ld, const char *dn, const char *mechanism, char *intok, char **outtok); /* Start carries out the SASL handshake with the server, sending the token in intok, and returning * the servers response in outtok */ You'd need to add a couple of return codes to mark the SASL complete and continue events. In this way, Thunderbird could use the above mechanism, and make use of its internal SASL code. The library could also contain its own ldap_sasl_bind_interactive() which would use the above framework and Cyrus SASL - that would be optional built so it was available to third parties, but didn't add additional dependencies for Thunderbird.
Also see bug 228704 (libldap and the LDAP tools should support SASL/Digest) which is based on contributions from Sun and Red Hat.
(In reply to comment #14) > One option is to have the library return control to the caller at each step in > the SASL handshake. The ldap library does return control to the caller at each step of the SASL handshake already. Essentially, you would need to create the initial SASL data in Thunderbird, and pass it to ldap_sasl_bind_s() as the "cred" parameter. The LDAP server will return LDAP_SASL_BIND_IN_PROGRESS if it needs another authentication step with it's challenge returned as "servercred". You would continue this challenge/response scenario until ldap_sasl_bind_s() returns LDAP_SUCCESS. To deal with the encryption of the payloads, you could just swap in your own IO function pointers that handle the encryption and pass the payload the original IO functions. I have implemented an ldap_sasl_interactive_bind_s() function that does use Cyrus SASL, but support for it is optionally compiled in, so there will be no problem building the LDAP C SDK without any extra dependencies for Thunderbird. For details on this work, see the bug that Mark referenced above in comment 15.
I'm attaching a proof of concept GSSAPI patch which uses some of the ideas discussed earlier in the bug - we use the existing ldap_sasl_bind() calls in the C-SDK, and do the GSSAPI handshake using the same nsIAuthModule code as used for all of the rest of Thunderbird's SASL. At this moment, I'm interested in getting some architecture review from people more familiar with the LDAP XPCOM stuff (and with XPCOM in general). I've tried to abstract the SASL side of things as far as possible - so nsLDAPOperation gains a SaslBind function which takes a nsIAuthModule pointer - that AuthModule could implement any SASL mechanism. nsLDAPOperation also needs a SASL step helper function - this function would ideally be private between it, and nsLDAPConnection, but for now it's in the IDL too. Things still to do: * Fetch the rootDSE from the server and see if we should be trying GSSAPI at all * Fallback from GSSAPI to simple binds if GSSAPI fails * Change the CID for the changed interface * Work out whether the contract ID should also change (comments welcome!) Any and all feedback is highly welcome! Patch is against the current trunk.
Simon, thx for working on this! A few overall comments - There probably needs to be a way for the user to turn off even attempting gssapi auth. I'd prefer not dropping/converting error codes if possible. So, maybe just return rv instead of NS_ERROR_UNEXPECTED in the two instances of code like this: + case NS_ERROR_UNEXPECTED: + case NS_ERROR_ILLEGAL_VALUE: + default: + (void *)ldap_abandon_ext(mConnectionHandle, mMsgID, 0, 0); + return NS_ERROR_UNEXPECTED; + } Why noscript here? + [noscript] void saslStep([const] in string token, in unsigned long tokenLen); + (void *)ldap_abandon_ext(mConnectionHandle, mMsgID, 0, 0); why wouldn't that just be (void) ldap_abandon_ext...? Are we just trying to silence warnings about dropping return values? this can just be void saslStep(in string token, in unsigned log tokenLen); can't it? I can't speak to the horribleness of the change to nsLDAPConnection.cpp, or the general approach, really. It doesn't seem that bad to me...
(In reply to comment #17) > nsLDAPOperation also needs a SASL step helper function - this function would > ideally be private between it, and nsLDAPConnection, but for now it's in the > IDL too. Thanks immensely for doing work on this! I added this patch to a clean tree and tried it out. My remote LDAP server spewed out these errors in the log: Feb 25 19:22:01 myhost slapd[13563]: [ID 848112 local4.debug] conn=1 fd=9 ACCEPT from IP=217.194.71.178:51177 (IP=128.91.3.43:389) Feb 25 19:22:01 myhost slapd[13563]: [ID 347666 local4.debug] conn=1 op=0 BIND dn="ou=pennpeople,dc=upenn,dc=edu" method=163 Feb 25 19:22:01 myhost slapd[13563]: [ID 347666 local4.debug] conn=1 op=1 BIND dn="ou=pennpeople,dc=upenn,dc=edu" method=163 Feb 25 19:22:01 myhost slapd[13563]: [ID 347666 local4.debug] conn=1 op=2 BIND dn="ou=pennpeople,dc=upenn,dc=edu" method=163 Feb 25 19:22:01 myhost slapd[13563]: [ID 668004 local4.debug] SASL [conn=1] Failure: Inappropriate authentication Feb 25 19:22:01 myhost slapd[13563]: [ID 217296 local4.debug] conn=1 op=2 RESULT tag=97 err=50 text=SASL(-14): authorization failure: Inappropriate authentication Feb 25 19:24:16 myhost slapd[13563]: [ID 952275 local4.debug] conn=1 fd=9 closed In my T-Bird address book, my LDAP server's property tab has the dn and the bind dn matching what you see in the log snippet up above. Should I change either of these properties to get further? I would be happy to work on a helper function and cleaning up the "things still to do" list, which would certainly help push this along. All I gotta do at this point is get authenticated. :-)
From the error you're getting, I suspect that the LDAP server doesn't like the fact that you're trying to assert a bind DN - try leaving this out, and see if things work.
(In reply to comment #20) > I suspect that the LDAP server doesn't like the > fact that you're trying to assert a bind DN You're right. That was it. I'm authenticating on my server now. I'll see if I can contribute some resolutions to the to-do list up in comment # 17.
Here's what I think needs doing... *) Before binding, we should be searching the rootDSE. We need to be careful about errors here, which should cause us to fallback to a simple bind, and finally to a protocol version 2 bind. *) We should only try GSSAPI if the rootDSE says that it's supported *) The code should be generalised so that its used for all binds, not just those in classes inheriting from ListenerBase *) There should probably be some UI to control which authentication methods we use. There's also some bugs to iron out. In particular, when I'm running with my patch and do an LDAP search, Thunderbird won't quit from the menu, and has to be killed from the shell. I suspect we're leaving something lying around that we shouldn't be, but I haven't had a chance to go looking yet.
(In reply to comment #22) > *) Before binding, we should be searching the rootDSE. We need to be careful > about errors here, which should cause us to fallback to a simple bind, and > finally to a protocol version 2 bind. I'm not sure about falling back to a version 2 bind - at the moment Thunderbird uses the ldap_2.servers.xxx.protocolVersion pref (or something similar to that) to work out which protocol version to use. We should ensure that we still respect that pref even if we do allow a full back.
Here is Simon's patch with a couple enhancements: 1) a new "DoesServerHandleGSSAPI" function checks the rootDSE to see if it supports GSSAPI 2) I added code to fall into SimpleBind if SaslBind failed or if SaslStep failed. 3) I took out a totally unused variable named mOperation from the nsAbLDAPListenerBase class. Also, I spent a few hours trying to isolate down where the hang-on-quit problem was coming in. The hang-on-quit issue seems to happen when SaslStep is called twice, not when SaslStep is called one time only. On the Macintosh platform, the i386 version of T-Bird with this patch applied exits okay; PowerPC versions need to be killed from the shell (if a search using this patch is done).
Attachment #227079 - Attachment is obsolete: true
1) Turns out my last attachment (259687) won't compile off the top of trunk. So I converted the original patch's "translateBindError" calls to the new (in trunk) TranslateLDAPErrorToNSError calls. 2) I also put in the code for nsLDAPOperation::DoesServerHandleGSSAPI XPCom function, which somehow escaped making it in the last patch. 3) I built a "release" version of the app with this new patch and the Darwin/i386 version also hung when quitting from the menu, so whatever is still loose in the patch (a thread?) affects both Macintosh architectures.
Attachment #259687 - Attachment is obsolete: true
(In reply to comment #25) > Created an attachment (id=259717) [details] > an update to the update that will compile better > 2) I also put in the code for > nsLDAPOperation::DoesServerHandleGSSAPI XPCom function, which somehow escaped > making it in the last patch. The associated proposed new LDAP API function doesn't work as it stands now, presumably that's a stub for a function that will actually do the work? The libutil/Makefile is not used anymore and should probably be CVS removed. I don't think a function like 'ldap_gssapi_capable()' is required to be added to the LDAP API. ldap_sasl_bind() will return a very specific error code if the server does not support the requested SASL mechanism - LDAP_STRONG_AUTH_NOT_SUPPORTED (aka LDAP_AUTH_METHOD_NOT_SUPPORTED - deprecated). The LDAP RFCs refer to this as authMethodNotSupported - see http://www.ietf.org/rfc/rfc4511.txt for details. > 3) I built a "release" version of the app with > this new patch and the Darwin/i386 version also hung when quitting from the > menu, so whatever is still loose in the patch (a thread?) affects both > Macintosh architectures. >
(In reply to comment #24) > Created an attachment (id=259687) [details] > updated patch for GSSAPI LDAP binding... I think it'd be nice to have some UI controls eventually - IMHO the user should be able to select to "always use GSSAPI binding" or "try GSSAPI if available" or "don't try GSSAPI". > 3) I took out a totally unused variable named mOperation from the > nsAbLDAPListenerBase class. Don't worry about doing that, my patch in bug 316170 already does that as well as some other tidying up. > Also, I spent a few hours trying to isolate down where the hang-on-quit problem > was coming in. That is most likely because you're not freeing something you should be, so your reference counts are wrong and its probably causing two objects to hold on to each other. I thought at first it was due to the nsIAuthModule, but I'm not sure at the moment, and can't see anything wrong yet. Have you got your Add & Remove Pending Operations correct? Here's some general comments on the patch, as you'll need to tidy them up before being able to commit the patch and I don't know if you know about them or not: You'll need to update the uuid for nsILDAPOperation. + [noscript] void saslStep([const] in string token, in unsigned long tokenLen); Why the [noscript]? I also don't see the point in making token a const when you cast the const away in the SaslStep function. All indentation should be using spaces (no tabs). Normally 2-space indentation should be used for new code, though 4-space is acceptable where its fitting into existing functions. I'm not sure of the standard for the c-sdk, but things like: ber_bvfree( creds ); and if( 0 == err ) should be ber_bvfree(creds); and if (0 == err) in the c++ code. + // make sure the connection knows where to call back once the messages + // for this operation start coming in + rv = NS_STATIC_CAST(nsLDAPConnection *, + NS_STATIC_CAST(nsILDAPConnection *, + mConnection.get()))->AddPendingOperation(this); + switch (rv) { + case NS_OK: + break; + + // note that the return value of ldap_abandon_ext() is ignored, as + // there's nothing useful to do with it + + case NS_ERROR_OUT_OF_MEMORY: + (void *)ldap_abandon_ext(mConnectionHandle, mMsgID, 0, 0); + return NS_ERROR_OUT_OF_MEMORY; + break; + + case NS_ERROR_UNEXPECTED: + case NS_ERROR_ILLEGAL_VALUE: + default: + (void *)ldap_abandon_ext(mConnectionHandle, mMsgID, 0, 0); + return NS_ERROR_UNEXPECTED; + } + + return NS_OK; How about just: rv = NS_STATIC_CAST... if (NS_FAILED(rv)) (void *)ldap_abandon_ext(mConnectionHandle, mMsgID, 0, 0); return rv; + int err; + + err = ldap_gssapi_capable(mConnectionHandle); + if( 0 == err ) + { + return NS_OK; + } + return NS_ERROR_UNEXPECTED; You could avoid the temporary variable, by just doing "if (ldap_gssapi_capable(mConnectionHandle) == 0)", you could make it even simpler as well: return ldap_gssapi_capable(mConnectionHandle) == 0 ? NS_OK : NS_ERROR_UNEXPECTED; + int capabErr; Doesn't seem to be used. + // Let's try a GSSAPI Bind */ + + rv2 = ldapOperation->DoesServerHandleGSSAPI(); + if( rv2 == NS_OK ) if (NS_SUCCEEDED(ldapOperation->DoesServerHandleGSSAPI()) would be better here, then you could avoid the rv2 variable as well.
One(In reply to comment #25) > Created an attachment (id=259717) [details] One other comment: + void doesServerHandleGSSAPI(); Unless you need to rework how this is called/check per Rich's comment, I think this would be better as: readonly attribute boolean gSSAPICapable; Doing it your way doesn't feel right.
I haven't had much time to work on this, or to review the latest patch. However, some general points. I think it is important to determine whether the server supports GSSAPI before we try and do it. On some platforms (Mac OS X, Kerberos for Windows) starting a GSSAPI handshake will prompt the user for credentials if they don't already have them. In order to avoid user confusion, therefore, you only want to try GSSAPI if you know the server supports it (and, perhaps, if you have been configured to do so). That's why the supportedSASLMechanisms attribute is provided in the rootDSE. I don't think, however, that the SDK should be providing something of the form 'ldap_gssapi_capable'. We may want to extend our SASL support to include other mechanisms (all those already implemented for Thunderbird would be a good starting point) - so a more general interface is required. I'm not sure if this interface need be in the SDK - as all of the building blocks to access the RootDSE are available, without having to add methods here. ldap_gssapi_capable appears to be incomplete as currently implemented - it never checks to see if GSSAPI is in the list of supportedSASLMechanisms. I'm also unsure whether you can do a search without doing a bind at all, or whether you need to do an anonymous bind before accessing the DSE. Perhaps someone with the LDAP RFCs to hand can answer this one. SaslStep was originally [noscript] when it took its arguments in a different form. It is, however, an implementation detail - and ideally would be a private function shared only with it's caller. I wasn't sure how to implement this with COM, hence it appearing in a public interface.
Hi guys... Something appears to have changed in the trunk over the past couple weeks that ends up breaking Simon's patch. I had an archived build from 3/27 that GSSAPI authenticates and retrieves LDAP records just fine. To make sure it wasn't something I might have introduced in my coding, I did a fresh checkout of the trunk a couple days ago, updated it to current just now and applied Simon's patch by itself (without my soon-to-be-attached-here mods & enhancements) and ldap_sasl_bind is failing with a LDAP_CONNECT_ERROR. Is this same thing happening for you guys?
Just a quick note that I'm now working on this again, with a view to completing the implementation (Michael - I've taken the bug from you, I hope that's OK) I've found the leak. rawMsg wasn't garbage collecting properly - moving the nsCOMPtr assignment to before the SaslStep call solved the problem. I suspect that the version 3 => version 2 bind fallback on which the SaslStep call was modelled will have had the same problem. My intention now is to complete the user interface changes to permit GSSAPI selection when configuring an LDAP datasource.
Assignee: dautermann → simon
I've attached a patch which implements Kerberos authentication for LDAP, using the nsIAuthModule interface. The changes are as follows: *) directory/xpcom: Add saslBind and saslStep methods to the nsILDAPOperation interface. Implement SASL support in nsLDAPConnection and nsLDAPOperation *) extensions/auth: Make nsAuthGSSAPI and nsAuthSASL use IMPL_THREADSAFE, as the LDAP code initialises, and uses, them from multiple threads. Add the 'ldap' service name to the list which are considered 'mail' tasks, so Mac OS X will prompt if your credentials are expired. *) mail (and mailnews): Add a user interface to support selection of authentication type. In the Advanced section of the 'Add Directory" dialog there is now a "Login method" option which can currently be either 'Simple' or 'Kerberos'. Setting it to Kerberos enables this code. The code has been tested with auto complete, normal searching and replication. It's also designed to be extensible - any other SASL mechanism which is implemented as an nsIAuthModule could be easily integrated. I realise that with the changes to 3 different areas of the code, I'll probably need multiple reviewers. Could someone more familar with the review process please steer me through it? Thanks, Simon.
Attachment #255514 - Attachment is obsolete: true
Attachment #259717 - Attachment is obsolete: true
Attachment #284012 - Flags: review?(bugzilla)
(In reply to comment #32) > I realise that with the changes to 3 different areas of the code, I'll probably > need multiple reviewers. Could someone more familar with the review process > please steer me through it? In the first instance, I'll do, as I can cover at least two of those areas. I'll need to have a look in the patch in some more detail, but here's a few comments just from a quick look at it: + void saslBind(in string service, in string mechanism, in nsIAuthModule authModule); Its better if the strings used here were both ACString. We generally prefer the A(C)String types. The saslStep function I think can stay as it is as its not worth doing the conversion to and from in that case. +NS_IMETHODIMP +nsLDAPOperation::SaslStep(const char *token, PRUint32 tokenLen ) ... +NS_IMETHODIMP +nsLDAPOperation::SaslBind(const char *service, const char *mechanism, nsIAuthModule *authModule) Can these two be the other way round please to match the order in the header? Drop the space after tokenLen. Also, please 2-space indent these new functions. For new functions/code I generally prefer 2-space indentation, but if you're modifying old functions, its sometimes better to stick with 4. Index: mail/components/compose/content/MsgComposeCommands.js Can you also do an update to the mailnews/compose/resources/content/ version for SeaMonkey please? If you don't want to, we'll have to get someone else to do it (or me) for submission at the same time as your patch as otherwise SeaMonkey will be broken. Index: mail/locales/en-US/chrome/messenger/addressbook/pref-directory-add.dtd The same applies here (except the equivalent file is in suite/locales/en-US/chrome/mailnews/pref/pref-directory-add.dtd) and in this case the breakage would be more obvious. /** + * The SASL mechanism to use to authenticate to the LDAP server + */ + attribute ACString saslMechanism; It would be useful to document the value(s) we currently support, and how to disable it. +NS_IMETHODIMP nsAbLDAPDirectory::SetSaslMechanism(const nsACString &aSaslMechanism) +{ + // XXX We should cancel any existing LDAP connections here and + // be ready to re-initialise them with the new auth details. + return SetStringValue("auth.saslmech", aSaslMechanism); I don't think you need the XXX comment here, as we're already doing the check in nsAbLDAPDirectoryQuery. Like I said before, I still need to take a bit more of a detailed look at it - there's still a few things I'd like to check up upon. Thanks for working on the patch. btw reassigning QA contact to default for this component (which I track).
QA Contact: thunderbird → ldap-integration
I've made all of these changes (including modifying the two additional files for SeaMonkey). I'll leave uploading another diff until you've had a chance to take a more detailed look at the current one.
Attached patch Updated patch (obsolete) — — Splinter Review
I'll be offline for a few days, so I'm just uploading the current version of this patch. It includes corrections for all of the issues raised so far.
Attachment #284012 - Attachment is obsolete: true
Attachment #284446 - Flags: review?(bugzilla)
Attachment #284012 - Flags: review?(bugzilla)
Comment on attachment 284446 [details] [diff] [review] Updated patch Ok, I've taken a better look at the patch now. Unfortunately, its still not quite working right. My LDAP server doesn't support GSSAPI, so if I startup address book and have a GSSAPI connection defined and do a query, I get: ###!!! ASSERTION: nsAbLDAPMessageBase::OnLDAPInit(): failed to perform GSSAPI bind: 'Error', file /mozilla/ldap/mozilla/mailnews/addrbook/src/nsAbLDAPListenerBase.cpp, line 312 which is expected from looking at the code. Unfortunately it hangs on shutdown. Probably due to a reference count cycle. This could be because you've changed the location of rawMsg. It works fine if I cancel a normal authenticated connection (the same function handles that). So r- until we resolve that unfortunately. Having said that, the patch looks really good, and the comments below are mainly just nits. + * Asyncrhonously perform a SASL bind against the LDAP server Asyncrhonously -> Asynchronously + void saslStep([const] in string token, in unsigned long tokenLen); You don't need the const in here. + ldap_parse_sasl_bind_result( + loop->mRawConn->mConnectionHandle, + msgHandle, &creds, 0); I'd prefer these arguments to be 2-space indented from the ldap_ line. + rv = mAuthModule->GetNextToken((void *)nsnull, 0, (void **)&creds.bv_val, + &credlen); You don't need to cast nsnull to void*. + NS_ConvertUTF8toUTF16(bindName.get()).get(), nsnull); You shouldn't need to .get() on the bindName. +<!ENTITY saslMechanism.label "Login method: "> +<!ENTITY saslMechanism.accesskey "l"> +<!ENTITY saslOff.label "Simple"> +<!ENTITY saslGSSAPI.label "Kerberos"> + I think Kerberos (GSSAPI) may be clearer to some folks. Not sure about the simple, but I expect that's the best we can do. + mSaslMechanism(EmptyCString()), You don't need to do this, an ns*String will default to empty. + nsCAutoString host; + rv = mDirectoryUrl->GetAsciiHost(host); + NS_ENSURE_SUCCESS(rv, rv); + + nsCAutoString service("ldap@"); + service.Append(host); Drop the service string bit and do: host.Insert(NS_LITERAL_CSTRING("ldap"), 0);
Attachment #284446 - Flags: review?(bugzilla) → review-
Found this problem. As far as I can see, we end up with a reference cycle as soon as mOperation->Init() is called. In normal usage this is broken by the call to Clear() when a callback is received for an incoming message. However, in the GSSAPI case idenitified above, we're erroring out before we even bother performing an LDAP operation (the nsIAuthModule can't get GSSAPI credentials, and so returns an error code to the GetNextToken call in SaslBind). This means that we exit from the operation without ever breaking the circular reference. Adding a call to Clear() from all of the SaslBind failure cases that don't result in data being sent solves the problem. The same problem exists if SimpleBind fails, too, although it's much hard to provoke one of the failure cases in that function. New patch to follow ...
Attached patch Patch, now without hangs (obsolete) — — Splinter Review
This patch solves the hang-on-exit problem when GSSAPI authentication fails, as well as addressing the other review comments. It needs the patch in bug #400360, in order to fix a hang-on-exit problem when using the autocomplete code, which appears unrelated to these changes. For reference, the circular reference here was: Listener -> Operation -> Listener
Attachment #284446 - Attachment is obsolete: true
Attachment #285461 - Flags: review?(bugzilla)
Comment on attachment 285461 [details] [diff] [review] Patch, now without hangs + <label value="&saslMechanism.label;" control="saslMechanism"/> + <menulist id="saslMechanism"> + <menupopup> + <menuitem id="Simple" value="" + label="&saslOff.label;"/> + <menuitem id="GSSAPI" value="GSSAPI" + label="&saslGSSAPI.label;"/> + </menupopup> You missed adding the access key for this. Something else has just occurred to me, if the user uses GSSAPI to connect to the server, doesn't that negate the need for BindDN? If so, we should set the BindDN to read-only when the user has GSSAPI selected. Which may also mean we want to put the selection on the general tab rather than the advanced tab.
At the moment the bindName gets passed into the SASL layer as the username to use to contact the server. In the GSSAPI case, this means that this is asserted as the authorization identity to connect to. In the long run this is probably not what we want. When other SASL mechanisms are provided (this is going to come from the refactoring work I'm doing with the mailnews SASL mechs, which will give us CRAM-MD5, LOGIN, PLAIN and NTLM, at least, with DIGEST-MD5 to follow for all protocols), we'll probably want to prompt for a username, rather than a bind DN. That's going to involve rather more UI thought.
Status: NEW → ASSIGNED
Comment on attachment 285461 [details] [diff] [review] Patch, now without hangs r=me for the directory/ mailnews/ and mail/ parts. You'll need to get review from someone for the extensions/auth part, and you'll also need sr for the whole lot (probably David Bienvenu).
Attachment #285461 - Flags: review?(bugzilla) → review+
(In reply to comment #41) > (From update of attachment 285461 [details] [diff] [review]) > r=me for the directory/ mailnews/ and mail/ parts. Opps, that is, r=me as long as you fix the access key problems (comment 39).
Comment on attachment 285461 [details] [diff] [review] Patch, now without hangs the extensions/auth part looks ok to me (cneberg says it's fine with him for me to review that part)
Depends on: 428482
At Standard8's suggestion, the extensions/auth portions of this patch are now in bug #428482, as they'll require approval.
This patch unbitrots the earlier version (changes calls using sPrefs to use getPrefs() in the composer window), adds in the requested accesskeys, and removes the extensions/auth changes which have now been committed.
Attachment #285461 - Attachment is obsolete: true
Attachment #315548 - Flags: superreview?
Attachment #315548 - Flags: superreview? → superreview?(dmose)
Product: Core → MailNews Core
Dan could you please review patch?
Flags: wanted-thunderbird3?
Yes, sorry for the delay. I'll try to get to it in the next couple of weeks.
Dan, any news on review for this patch? It will be great if it can make into beta2. Thanks
Flags: wanted-thunderbird3?
Apologies again for letting this sit. At this point, I don't expect to get to this until sometime after beta2 ships. Feel free to ping me again at that point.
Flags: wanted-thunderbird3+
Dan, as we know beta2 has been shipped yesterday. So I asking you again for review of patch.
Thanks for the ping; it's on my radar. I hope to get to this sometime in the next 2-3 weeks.
This had bitrotted slightly, updated to fix (sorry for the long delay, Simon). Now to review...
Attachment #315548 - Attachment is obsolete: true
Attachment #368333 - Flags: superreview?(dmose)
Attachment #368333 - Flags: review+
Attachment #315548 - Flags: superreview?(dmose)
Comment on attachment 368333 [details] [diff] [review] LDAP GSSAPI patch updated to latest Hg tip sr=dmose; looks good. I'll land this later today.
Attachment #368333 - Flags: superreview?(dmose) → superreview+
Changeset 73e75f355c99 pushed. Thanks muchly for both the patch and your patience, Simon!
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b3
Thanks Simon, and Dan for review. I've got one crash when enabled gssapi, can't reproduce it for now. bp-c9db6b27-117d-410a-9bf4-9f3372090320 I also dump traffic between server to check what's going on, why are we sending second bind request w/o credentials such packet is malformed.
Status: RESOLVED → VERIFIED
Can you clarify what you mean about the malformed packet? Are you sure you're not just seeing the second step in the SASL handshake? A copy of the packet dump, or a text output of the same would be very helpful here. As to the stack trace - that looks like its crashing when it's trying to build an address book card after performing an LDAP search. That's quite a bit after the bind has completed. S.
Attached file ldap admin bind dump —
ldap admin bind packet dump for comparison
What I'm seeing here is ldap admin is sending one packet for bind, while Thunderbird sending 3 packets. And second of it is malformed as seen in wireshark.
The two packet traces are doing different SASL mechanisms - the first, GSSAPI and the second GSS-SPNEGO. Needless to say, the SASL exchange differs between these two mechanisms. The GSSAPI negotiation is correct - the empty packet is because the GSSAPI part of the negotiation is complete, but the server must initiate the security layer negotiation, so the client has to send the server an empty packet to force it to begin that stage. From RFC4752: If the last call to GSS_Init_sec_context returned an output_token, then the client responds with the output_token, otherwise the client responds with no data. S.
And we are not using GSS-SPNEGO because its Windows dependent, right? If so that make sense.

Twelve years later, Microsoft Windows can enforce Channel Binding over LDAP when using SASL GSSAPI. RFC 4752 says this (Channel Binding over SASL GSSAPI) is invalid, but the Windows AD (that can be implented also by OpenLDAP) does it anyway.

And we are not using GSS-SPNEGO because its Windows dependent, right?

GSSAPI is a SASL mechanism. GSS-SPNEGO is an HTTP Authentication mechanism (thus not SASL), but otherwise my understanding is that both SASL GSSAPI and GSS-SPNEGO are very similar. In particular there is no non-Microsoft alternative to GSS-SPNEGO.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: