Closed Bug 303160 Opened 15 years ago Closed 15 years ago

Add support for GSSAPI/Kerberos Authentication to IMAP/SMTP/POP3

Categories

(MailNews Core :: Networking: IMAP, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: simon, Assigned: Bienvenu)

References

Details

(Keywords: fixed1.8)

Attachments

(3 files, 6 obsolete files)

Add support for doing GSSAPI SASL based authentication to mailnews' IMAP
implementation.
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?
Depends on: 280792
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.
I've already got a patch  workingto add it to POP and someone working on SMTP
(but not done, afaik)
There's a problem with the attached patch interoperating against Cyrus SASL. I'm
working on a fix.
yes, Cyrus doesn't work for me either...let me know when you get a patch and I
can try it.
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
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
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)
This patch extends cneberg's auth changes (patch in bug #280792) to add the
sasl-gssapi mechanism.
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...
(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.
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?
(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.
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.
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)
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+
Thats fine - thanks. Let me know if you need anything else!
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?
(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)
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)
Attached patch Revised sasl-auth patch (obsolete) — Splinter Review
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
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)
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?
no thanks, I can make that change myself, thx.
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 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-
(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?
(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.
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)
Attachment #192409 - Flags: superreview?(bienvenu) → superreview+
Attached patch cumulative patchSplinter Review
for posterity's sake, this is the cumulative patch for all the changes, if I've
done my diffs right.
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+
thx, Darin, I can address those comments in my tree (that would probably be
easier than trying to apply a new patch :-) )
Blocks: 274929
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".
(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.

Comment on attachment 192083 [details] [diff] [review]
Patch for the mailnews portion of the code

clearing obsolete request
Attachment #192083 - Flags: review?(bienvenu)
fixed on trunk, thx for everyone's help!
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
fix checked into 1.5, thx everybody!
Keywords: fixed1.8
*** Bug 28200 has been marked as a duplicate of this bug. ***
*** Bug 274929 has been marked as a duplicate of this bug. ***
Comment on attachment 192366 [details] [diff] [review]
Revised sasl-auth patch

clearing request.
Attachment #192366 - Flags: superreview?(bienvenu)
Summary: Add support for GSSAPI Authentication to IMAP → Add support for GSSAPI Authentication to IMAP/SMTP/POP3
Product: Core → MailNews Core
Duplicate of this bug: 359289
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.