Closed
Bug 303160
Opened 19 years ago
Closed 19 years ago
Add support for GSSAPI/Kerberos Authentication to IMAP/SMTP/POP3
Categories
(MailNews Core :: Networking: IMAP, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: simon, Assigned: Bienvenu)
References
Details
(Keywords: fixed1.8)
Attachments
(3 files, 6 obsolete files)
26.48 KB,
patch
|
Bienvenu
:
review+
mscott
:
superreview+
|
Details | Diff | Splinter Review |
10.43 KB,
patch
|
darin.moz
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
111.79 KB,
patch
|
Details | Diff | Splinter Review |
Add support for doing GSSAPI SASL based authentication to mailnews' IMAP implementation.
Reporter | ||
Comment 1•19 years ago
|
||
David and I have been thrashing this out over email, and in a number of other bugs for a weeks or so now. Creating this bug so there's somewhere to hang the patch. Bug #280792 and Bug #302209 contains some discussion of the changes that are necessary to the NegotiateAuth module to enable this functionality. This patch also contains these changes to NegotiateAuth - should these be split off into a seperate bug?
Reporter | ||
Comment 2•19 years ago
|
||
Changing the bug summary to make this more general - I've now got patches to add GSSAPI to POP3 and SMTP, as well as to IMAP. I'm going to have to bring up some services to test these against, which I won't be able to do until early next week, so just adding this as a placeholder to say that the work has been done.
Assignee | ||
Comment 3•19 years ago
|
||
I've already got a patch workingto add it to POP and someone working on SMTP (but not done, afaik)
Reporter | ||
Comment 4•19 years ago
|
||
There's a problem with the attached patch interoperating against Cyrus SASL. I'm working on a fix.
Assignee | ||
Comment 5•19 years ago
|
||
yes, Cyrus doesn't work for me either...let me know when you get a patch and I can try it.
Reporter | ||
Comment 6•19 years ago
|
||
This patch is still in a rough form - I haven't incorporated the changes (and renaming) of negotiate-auth being done by cneberg, nor have I preffed the choice of GSSAPI/SSPI module. However, it does fix the issues with interoperating with Cyrus SASL. The first few issues are ones which will need fixing in the negotiate-auth code: *) We need to be able to set the MUTUAL_AUTH flag in GSSAPI, as well as in SSPI *) init_sec_context producing a NULL token is not an error state. The other issues were: *) SASL should probably do MUTUAL_AUTH by default (2222 is very unclear, but its good practice) *) Servers using Cyrus-SASL will send an empty message to the client if accept_sec_context() returns COMPLETE with an empty output_token. In this case, we need to send a garbage token back to the server, so that the server can reply and advertise its security layers to us. I think that this is an error in Cyrus-SASL, but the language in 2222 is so loose we should probably cater for this case anyway. *) If the client COMPLETEs with an empty output buffer, then we need to send an empty buffer to the server, so it can do the security layer advertisment *) Authzid's are not NULL terminated. I'm going to do some work tomorrow on integrating cneberg's patches into this, and producing a diff against them. Would you like to use GSSAPI, rather than 'Gssapi' in general? (Should I alter all of the method names, as well as the filenames)
Attachment #191427 -
Attachment is obsolete: true
Reporter | ||
Comment 7•19 years ago
|
||
d'Oh - forgot about new files in the last attachment. Here's another, with them intact. I still need to move the SaslGssapi stuff out of its own extension, and back in to negotiateauth.
Attachment #192025 -
Attachment is obsolete: true
Reporter | ||
Comment 8•19 years ago
|
||
Here's a patch for the mailnews portion of the code to add GSSAPI support to POP3, IMAP and SMTP. This will do nothing without the presence of a sasl-gssapi module, and so could be committed independently of cneberg's negotiate-auth reorganisation.
Attachment #192026 -
Attachment is obsolete: true
Attachment #192083 -
Flags: review?(bienvenu)
Reporter | ||
Comment 9•19 years ago
|
||
This patch extends cneberg's auth changes (patch in bug #280792) to add the sasl-gssapi mechanism.
Assignee | ||
Comment 10•19 years ago
|
||
Comment on attachment 192083 [details] [diff] [review] Patch for the mailnews portion of the code thx a lot for doing all this! this part is my fault, but we should change the comment here. I think we don't need to check m_authModule here - if rv is success, m_authModule has to be non-null. + // if this fails, then it means that we cannot do negotiate auth. + if (NS_FAILED(rv) || !m_authModule) + return rv; + rv = m_runningURL->GetSmtpServer(getter_AddRefs(smtpServer)); + if (NS_FAILED(rv)) return NS_ERROR_FAILURE; + + rv = smtpServer->GetUsername(getter_Copies(userName)); + if (NS_FAILED(rv)) return NS_ERROR_FAILURE; + rv = smtpServer->GetHostname(getter_Copies(hostName)); + if (NS_FAILED(rv)) return NS_ERROR_FAILURE; In general, we much prefer NS_ENSURE_SUCCESS(rv, rv); here, instead of turning every failure into NS_ERROR_FAILURE. + rv = DoGSSAPIStep1(service.get(), userName, resp); + if (NS_FAILED(rv)) + command.Append("*"); + else + command.Append(resp); Out of curiousity, why do we send "*" when gssapistep1 failed, for all protocols? I assume this is part of seamlessly dealing with not having the auth module installed, but it looks like we continue on trying to do the gssapi auth. also, this could be something like: command.Append(NS_SUCCEEDED(DoGSSAPIStep1(service.get(), userName, resp)) ? resp.get : ""); I'll try this patch out...
Reporter | ||
Comment 11•19 years ago
|
||
(In reply to comment #10) > (From update of attachment 192083 [details] [diff] [review] [edit]) > thx a lot for doing all this! That's OK. I've made all of your suggested changes to my local tree. > Out of curiousity, why do we send "*" when gssapistep1 failed, for all > protocols? I assume this is part of seamlessly dealing with not having the auth > module installed, but it looks like we continue on trying to do the gssapi > auth. We only have to send '*' if we've already told the server we're trying GSSAPI. In this case the server will reply with an 'authentication failed' error, which the client traps in the same way as for any authentication failure, disables GSSAPI, and moves on to whichever mechanism is next. In the SMTP case, because we send the step 1 payload with the 'AUTH GSSAPI', we don't need to send the AUTH GSSAPI at all, and can move seamlessly on to the next mechanism. I'll take a look at implementing this.
Assignee | ||
Comment 12•19 years ago
|
||
OK, so "*" is just something that will fail - we might want to add a comment to that effect. for imap and pop3, maybe we could avoid trying AUTH=GSSAPI if DoGSSAPIStep1 fails, i.e., try it first, before sending AUTH=GSSAPI - is that too convoluted?
Reporter | ||
Comment 13•19 years ago
|
||
(In reply to comment #12) > OK, so "*" is just something that will fail - we might want to add a comment to > that effect. '*' is the defined failure message in SMTP, IMAP and POP (ie - the RFCs say that this is what you must send in order to terminate an authentication attempt) > for imap and pop3, maybe we could avoid trying AUTH=GSSAPI if DoGSSAPIStep1 > fails, i.e., try it first, before sending AUTH=GSSAPI - is that too convoluted? It could be, yes. You either end up calling Step1 twice (once to check that you're allowed to do so, and again to initiate the authentication), or you need to cache the returned value across a server response. This is relatively easy to do in the IMAP code, but more complex in the POP case.
Assignee | ||
Comment 14•19 years ago
|
||
re "*", ok, thx. It should be fairly easy to cache the response in the pop3 protocol object itself, in an nsCString...I defer to you re what the right thing to do is - I worry that there will be a fair number of users out there who won't have things configured correctly re gssapi/sspi/kfw, etc, or even have nothing configured, and try to connect to a server that advertises GSSAPI.
Reporter | ||
Comment 15•19 years ago
|
||
This revised patch replaces all of the if NS_FAILED(rv) stuff with NS_ENSURE_SUCCESS(rv,rv) It also only sends an AUTH GSSAPI to the server if a call to DoGSSAPIStep1 succeeds - this leads to somewhat convoluted logic, but reduces the number of server round-trips with client platforms that don't support GSSAPI.
Attachment #192083 -
Attachment is obsolete: true
Attachment #192142 -
Flags: review?(bienvenu)
Assignee | ||
Comment 16•19 years ago
|
||
Comment on attachment 192142 [details] [diff] [review] Revised patch for mailnews great, thx! + nsCString m_GSSAPICache; this should probably have a more meaningful name, like m_GSSAPIFirstToken. I can make that change when I get ready to check this in, if that's ok...
Attachment #192142 -
Flags: review?(bienvenu) → review+
Reporter | ||
Comment 17•19 years ago
|
||
Thats fine - thanks. Let me know if you need anything else!
Assignee | ||
Comment 18•19 years ago
|
||
We need reviews for the new sasl stuff, right? nsAuthSaslGSSAPI.cpp, in https://bugzilla.mozilla.org/attachment.cgi?id=192086 - or is that rolled into an other patch/file? Do you want me to review that attachment as is?
Reporter | ||
Comment 19•19 years ago
|
||
(In reply to comment #18) > We need reviews for the new sasl stuff, right? Yes, we do. > nsAuthSaslGSSAPI.cpp, in > https://bugzilla.mozilla.org/attachment.cgi?id=192086 - or is that rolled into > an other patch/file? Do you want me to review that attachment as is? I was waiting until the dust in bug #280792 settles :) At the moment the choice of which GSSAPI implementation to use isn't preffed, as I was waiting to see what decision is made about the pref for negotiateauth. There's also some additional changes to the GSSAPI module itself (to stop it from rejecting zero length server return packets, to set mCOMPLETE off when Reset() is called, and to implement mutual authentication)
Assignee | ||
Comment 20•19 years ago
|
||
Comment on attachment 192142 [details] [diff] [review] Revised patch for mailnews the one thing that prevents this from compiling independently of all the other changes is NS_SUCCESS_AUTH_FINISHED, which is defined in nsIAuthModule.idl.
Attachment #192142 -
Flags: superreview?(mscott)
Reporter | ||
Comment 21•19 years ago
|
||
Here's an updated version of the auth module changes, now with support for selecting between KfW and SSPI using the same switch as cneberg's stuff. This patch should apply cleanly, after applying https://bugzilla.mozilla.org/attachment.cgi?id=192364 (from the other bug...) Do we need to ask Darin to review these changes too?
Attachment #192086 -
Attachment is obsolete: true
Assignee | ||
Comment 22•19 years ago
|
||
Comment on attachment 192366 [details] [diff] [review] Revised sasl-auth patch Yes, in the interest of trying to get it in today, I think Darin should review - Christopher has seen the code and I think is OK with the approach.
Attachment #192366 -
Flags: superreview?(bienvenu)
Attachment #192366 -
Flags: review?(darin)
Reporter | ||
Comment 23•19 years ago
|
||
There's a stupid typo in attachment #192366 [details] [diff] [review] , when its choosing whether to use GSSAPI or SSPI - the fix is as follows: --- nsAuthSaslGSSAPI.cpp.orig 2005-08-11 17:56:49.000000000 +0100 +++ nsAuthSaslGSSAPI.cpp 2005-08-11 17:50:23.000000000 +0100 @@ -82,7 +82,7 @@ if (prefs) { PRBool val; rv = prefs->GetBoolPref(kNegotiateAuthSSPI, &val); - if (rv && val) + if (NS_SUCCEEDED(rv) && val) contractID = NS_AUTH_MODULE_CONTRACTID_PREFIX "kerb-sspi"; } Would you like a respun patch?
Assignee | ||
Comment 24•19 years ago
|
||
no thanks, I can make that change myself, thx.
Comment 25•19 years ago
|
||
Comment on attachment 192142 [details] [diff] [review] Revised patch for mailnews > inBufLen = (len / 4)*3 + ((len % 4 == 3)?2:0) + ((len % 4 == 2)?1:0); some minor white space, add some spaces before and after the ? and : > response.Adopt((char *)nsMemory::Clone("",1)); space after comma Why are we skipping the first five bytes in the response line here: if (responseLine.Find("GSSAPI", PR_TRUE, 5) >= 0) is that to skip the AUTH= part? Maybe we should have a small comment there. To be consisten with the rest of the auth code in nsSmtpProtocol, should we be using terms like SMTP_SEND_AUTH_GSSAPI_STEP1 instead of _FIRST? Ditto for method names like AuthGSSAPIFirst? i.e AuthGSSAPIStep0, AuthGSSAPIStep1 instead of First and Step. In nsImapProtocol, do we need a special gssrv value when calling DoGSSAPIStep1? Can't we just re-use rv here since if it's an error we're gonna return anyway? Seems like there could be some rv consolidation in that routine in general. I think you can ditch the curly braces here: + if (m_useSecAuth && GetServerStateParser().GetCapabilityFlag() & kHasAuthGssApiCapability) + { + clientSucceeded = NS_SUCCEEDED(AuthLogin(userName, password, kHasAuthGssApiCapability)); + }
Attachment #192142 -
Flags: superreview?(mscott) → superreview+
Comment 26•19 years ago
|
||
Comment on attachment 192366 [details] [diff] [review] Revised sasl-auth patch Hrm... maybe "Sasl" should be "SASL"... I'm also not sure why the module is named "SASL-GSSAPI" when in fact it could be "SASL-SSPI" on windows. Perhaps it should just be nsAuthSASL? And, just drop the GSSAPI from the class name? I'd also recommend renaming mGssapiModule to mInnerModule.
Attachment #192366 -
Flags: review?(darin) → review-
Reporter | ||
Comment 27•19 years ago
|
||
(In reply to comment #26) > (From update of attachment 192366 [details] [diff] [review] [edit]) > Hrm... maybe "Sasl" should be "SASL"... I'm also not sure why the module is > named "SASL-GSSAPI" when in fact it could be "SASL-SSPI" on windows. Because, on the wire, you're doing the GSSAPI SASL mechanism, regardless of which implementation is being used on the host machine. See RFC2222. The problem is that 'GSSAPI' defines both an API, and a method for encoding mechanism information on the wire. SSPI doesn't do the API part of this, but it can be used to provide on-the-wire encoding which is GSSAPI compatible. So, the on-the-wire SASL protocol remains GSSAPI, whichever API implementation you use. > Perhaps > it should just be nsAuthSASL? And, just drop the GSSAPI from the class name? Okay. > I'd also recommend renaming mGssapiModule to mInnerModule. What's the best way to do this - respin the patch and ask for another review?
Reporter | ||
Comment 28•19 years ago
|
||
(In reply to comment #25) > Why are we skipping the first five bytes in the response line here: > > if (responseLine.Find("GSSAPI", PR_TRUE, 5) >= 0) > > is that to skip the AUTH= part? Maybe we should have a small comment there. I believe so, yes. All of the other authentication mechanism checks in that section of code have the same usage. > To be consisten with the rest of the auth code in nsSmtpProtocol, should we be > using terms like > SMTP_SEND_AUTH_GSSAPI_STEP1 instead of _FIRST? Ditto for method names like > AuthGSSAPIFirst? i.e AuthGSSAPIStep0, AuthGSSAPIStep1 instead of First and > Step. The problem is that GSSAPI doesn't have a predetermined number of steps. You do 'First', then call 'Step' repeatedly until the server succeeds or fails the authentication. > In nsImapProtocol, do we need a special gssrv value when calling DoGSSAPIStep1? Yes. We need to know what the return code from step1 was when we go into the while loop around step2. Step1 can return NS_SUCCESS_AUTH_FINISHED, in which case we never need to call Step2, and can just return once we've sent Step1's data to the server. > Can't we just re-use rv here since if it's an error we're gonna return anyway? > Seems like there could be some rv consolidation in that routine in general. The control flow in that routine is a touch convoluted, because we call Step1 before sending the "AUTH GSSAPI" - we do this so, if the user doesn't have any Kerberos credentials (or, they've not even got Kerberos installed), we don't do an additional round trip to the server. This means, though, that we need to store the results of doing the Step1 call over the "authenticate GSSAPI" SendData(), and so need a different rv variable.
Reporter | ||
Comment 29•19 years ago
|
||
I've revised this patch to take into account Darin's comments: *) We're now called nsAuthSASL *) mGSSAPIModule is now mInnerModule I've kept the name as sasl-gssapi - in this case GSSAPI is the name of the SASL mechanism we're implementing, not of the API we're coding to. I can also see potential for writing other SASL mechanisms as AuthModules (so we could have sasl-digestmd5, for instance)
Attachment #192366 -
Attachment is obsolete: true
Attachment #192409 -
Flags: superreview?(bienvenu)
Attachment #192409 -
Flags: review?(darin)
Assignee | ||
Updated•19 years ago
|
Attachment #192409 -
Flags: superreview?(bienvenu) → superreview+
Assignee | ||
Comment 30•19 years ago
|
||
for posterity's sake, this is the cumulative patch for all the changes, if I've done my diffs right.
Comment 31•19 years ago
|
||
Comment on attachment 192409 [details] [diff] [review] Re-revised sasl-auth patch >diff -uN extensions/auth.orig/Makefile.in extensions/auth/Makefile.in > CPPSRCS += nsAuthModuleGSSAPI.cpp >+CPPSRCS += nsAuthSASL.cpp It's usually preferred to write the above like this: CPPSRCS += \ nsAuthModuleGSSAPI.cpp \ nsAuthSASL.cpp \ $(NULL) >+++ extensions/auth/nsAuthSASL.cpp 2005-08-11 20:41:10.000000000 +0100 >+ NS_CopyUnicodeToNative(mUsername, userbuf); >+ messageLen = strlen(userbuf.get())+4+1; You can replace |strlen()| here with userbuf.Lenght(), like so: messageLen = userbuf.Length() + 4 + 1; >+ message = (char *)nsMemory::Alloc(messageLen); >+ message[0] = 0x01; // No security layer What if the memory allocation fails? You should check for a NULL value returned from nsMemory::Alloc, and you should bail out if that happens. >+ // Userbuf should not be NULL terminated, so trim the trailing NULL >+ // when wrapping the message >+ rv = mInnerModule->Wrap((void *) message, messageLen-1, PR_FALSE, >+ outToken, outTokenLen); >+ nsMemory::Free(message); >+ Reset(); // All done >+ return NS_SUCCESS_AUTH_FINISHED; >+ } else { What if the Wrap method returns a non-success code? You seem to capture the return value, but you don't do anything with it :-( Also |else| after |return| is non-sequitor. Just remove the else, and reduce overall indenting. PRUint32 *outTokenLen) >+{ >+ return NS_ERROR_NOT_IMPLEMENTED; >+} nit: indentation should be 4 white spaces >+++ extensions/auth/nsAuthSASL.h 2005-08-11 20:36:18.000000000 +0100 >+#endif /* nsAuthSASL_h__ */ >+ nit: kill blank line at the end of this file r=darin with these points resolved
Attachment #192409 -
Flags: review?(darin) → review+
Assignee | ||
Comment 32•19 years ago
|
||
thx, Darin, I can address those comments in my tree (that would probably be easier than trying to apply a new patch :-) )
Assignee | ||
Comment 33•19 years ago
|
||
Simon, + { "nsAuthSASL", + NS_AUTHSASL_CID, + NS_HTTP_AUTHENTICATOR_CONTRACTID_PREFIX "sasl-gssapi", + nsAuthSASLConstructor } should this be NS_AUTH_MODULE_CONTRACTID_PREFIX instead of NS_HTTP_AUTHENTICATOR_CONTRACT_PREFIX? As I understand it, this is an auth module...and the code in mailnews/base/util/nsMsgProtocol.cpp tries to create NS_AUTH_MODULE_CONTRACTID_PREFIX "sasl-gssapi".
Reporter | ||
Comment 34•19 years ago
|
||
(In reply to comment #33) > should this be NS_AUTH_MODULE_CONTRACTID_PREFIX instead of > NS_HTTP_AUTHENTICATOR_CONTRACT_PREFIX? Yes - it should be. I think this only worked in my tree because I hadn't done a clean build and so still had an older version registered.
Assignee | ||
Comment 35•19 years ago
|
||
Comment on attachment 192083 [details] [diff] [review] Patch for the mailnews portion of the code clearing obsolete request
Attachment #192083 -
Flags: review?(bienvenu)
Assignee | ||
Comment 36•19 years ago
|
||
fixed on trunk, thx for everyone's help!
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 38•19 years ago
|
||
*** Bug 28200 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 39•19 years ago
|
||
*** Bug 274929 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 40•19 years ago
|
||
Comment on attachment 192366 [details] [diff] [review] Revised sasl-auth patch clearing request.
Attachment #192366 -
Flags: superreview?(bienvenu)
Updated•15 years ago
|
Summary: Add support for GSSAPI Authentication to IMAP → Add support for GSSAPI Authentication to IMAP/SMTP/POP3
Updated•15 years ago
|
Product: Core → MailNews Core
Updated•5 years ago
|
OS: Windows XP → All
Summary: Add support for GSSAPI Authentication to IMAP/SMTP/POP3 → Add support for GSSAPI/Kerberos Authentication to IMAP/SMTP/POP3
You need to log in
before you can comment on or make changes to this bug.
Description
•