Closed Bug 280792 Opened 20 years ago Closed 19 years ago

Include Optional Support for MIT Kerberos for Windows

Categories

(Core :: Networking, enhancement)

Product:
Core
Component:
Networking
Platform:
x86
Windows NT
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: cneberg, Assigned: darin.moz)

References

Details

(Keywords: fixed1.8)

Attachments

(2 files, 12 obsolete files)

updated per review
74.66 KB, patch
Details | Diff | Splinter Review
fix build bustage when pr logging turned off
811 bytes, patch
cneberg
: review+
mscott
: superreview+
Details | Diff | Splinter Review 
Several applications like Vandyke Secure CRT allow the user to choose on Windows
when they use gss-api Kerberos authentication whether they use the Windows SSPI
or MIT Kerberos at runtime through configuration.  I'm interested in Mozilla
supporting this option as well. Would a sufficient number of people find this
useful to include it? We should of course keep the default to SSPI for Windows
platforms which support it.
In order to support  this, the host would have to already have the MIT
Kerberos-For-Windows packages already installed.

I think there is a very tiny percentage of sites that would find this useful.
I don't really know if this could be a run-time option, it would most likely
have to be compiled at build time which makes it even less attractive.

I really don't see what the functional benefit would be.  SSPI is integrated in
Windows and is wire-compatible with GSSAPI applications.  Where is the benefit
to the end user of having mozilla use GSSAPI on Windows instead of SSPI?


> I don't really know if this could be a run-time option, it would most likely
> have to be compiled at build time which makes it even less attractive. 

I think it could be a run time option without too much trouble.  A lot of UNIX
machines don't have Kerberos installed and that does not stop them from loading
Mozilla, if Mozilla can dynamically load their kerb libs, they are used, if not
then they are not used.  With the dynamic loading ability of components in
Mozilla I think we could support any number of nsIAuthModule modules at run time.

> I really don't see what the functional benefit would be.  SSPI is integrated in
> Windows and is wire-compatible with GSSAPI applications.  Where is the benefit
> to the end user of having mozilla use GSSAPI on Windows instead of SSPI?

Benefits

1.  If a Site has an MIT KDC, then they likely still use it for many of their
servers.  Maybe email, and HTTP keys.  Despite Microsoft's literature about
cross cell trust with MIT.  If you login into the Windows domain, you can't get
a key from the MIT KDC because the MIT KDC is not registed in the Global Catalog
and so no referals exist for you to actually request the ticket from the MIT KDC
 (and their is no domain realm mapping file to do this for the SSPI).  It's
possible that this bug will (or has recently been fixed) but it existed 6 months
ago.  (Note that a user can get tickets from the MIT KDC and AD if they use
his/her MIT username@MIT_REALM and password to log in, but why force users to do
this?)

2.  Sites have existing shared keys between their MIT KDC and another site can
take advantage of them to authenticate to other sites.  Many MIT installations
don't take advantage of transitive trust.  But their is no way to get a key for
the service at the remote site because of the bug I discussed in benefit 1. (the
MIT KDC is not in the global catalog so you can't get service tickets from it at
all).  (This again can be fixed by logging into the MIT KDC w/ a MIT
username/password, but it also requires a patch to MIT to support referrals).

3.  It could be made to work on Win 98 and NT 4.0 which don't support the
Kerberos SSPI.
After talking with some of the MIT KRB5 developers, I have been "educated" a bit
more about KfW (as I knew that I would be when I posted my original response).

I agree this would be a useful feature if the choice to choose between the SSPI
or GSSAPI interface were possible at run-time (not compile time).  

If the KfW GSSAPI interfaces are the same as the existing Unix GSSAPI interfaces
(and they should be since GSSAPI is a standard interface), then I would think
very little code shuold have to be written, it seems like it would mostly
involve changes to the configuration and build environment.  New code to detect
which interface to use would have to be added, though.


Christopher, how's the work on this going? Are you writing code to automatically
detect if gssapi is installed on windows?
(In reply to comment #4)
> Christopher, how's the work on this going? Are you writing code to automatically
> detect if gssapi is installed on windows?

I won't have time to work on this till Saturday.  I will most likely
be adding sections of your patch and mine together.  Running the
functions after they were prloaded in my old version would crash the browser. 
I'm assuming your more windows friendly function declarations  will fix
this issue (I hope).

>>Are you writing code to automatically detect if gssapi is installed
on windows?
My current thinking was no. I think windows sspi should always be the default. 
Just because a gssapi32.dll exists doesn't necessarly
mean its configured, working, and has credentials. I was going to let
the user specify whether to use it in about:config.
Old link from MIT kerberos list discussing this bug 
http://mailman.mit.edu/pipermail/kerberos/2005-February/007141.html
>I'm assuming your more windows friendly function declarations  will fix
>this issue (I hope).

I'm fairly confident that they will.

If you need anything from me, please let me know. I'm happy to test a patch on
windows for IMAP, for example. Or help Simon in some way, perhaps with the xpcom
factory mechanism...

> If you need anything from me, please let me know. I'm happy to test a patch on
> windows for IMAP, for example. Or help Simon in some way, perhaps with the xpcom
> factory mechanism...

What IMAP Server supports gssapi? Is there any hope of it working with MS Exchange?

In what bug is the patch (or is it already in the code somewhere?) to support
using this new auth module we are creating in mailnews?
UW and Cyrus support it. I don't know about MS Exchange but this link implies
that it does, at least for POP3/SMTP:

http://www.msexchange.org/tutorials/Telnet-Exchange2003-POP3-SMTP-Troubleshooting.html

I'll attach a patch that gets the imap mail code to do auth GSSAPI. It does it,
however, by using the negotiate auth module with the patch that Simon came up
with as an initial hack. I'll just attach the whole patch for good measure.
Attached patch work in progress (obsolete) — — Splinter Review 
this includes the new stand-alone gssapi.h file
Makefile.in and configure.in changes still need to be done. But this run's in
windows and it can use GSSAPI or SSPI depending on the value of
network.negotiate-auth.sspi. 

I'll also still be adding the Wrap and UnWrap functions.
In order to get the SASL stuff going, I've implemented Wrap() and Unwrap() for
GSSAPI. I'll pull the code out from the bigger SASL patch, and attach it to this
bug.
Attached file gssapi.h (obsolete) — 
Forgot gssapi.h from previous patch.  This is the one Bienvenu gave me. I
question tri licensing someone elses header file.  Maybe we should use the
license block MIT uses for their header gssapi.h header file.

/*
 * Copyright 1993 by OpenVision Technologies, Inc.
 * 
 * Permission to use, copy, modify, distribute, and sell this software
 * and its documentation for any purpose is hereby granted without fee,
 * provided that the above copyright notice appears in all copies and
 * that both that copyright notice and this permission notice appear in
 * supporting documentation, and that the name of OpenVision not be used
 * in advertising or publicity pertaining to distribution of the software
 * without specific, written prior permission. OpenVision makes no
 * representations about the suitability of this software for any
 * purpose.  It is provided "as is" without express or implied warranty.
 * 
 * OPENVISION DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
 * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
 * EVENT SHALL OPENVISION BE LIABLE FOR ANY SPECIAL, INDIRECT OR
 * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF
 * USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR
 * OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 * PERFORMANCE OF THIS SOFTWARE.
 */
This patch adds support for doing Wrap() and Unwrap() in nsIAuthModule, and
includes an implementation for GSSAPI. It also adds a NS_SUCCESS_AUTH_COMPLETE
return code to allow GetNextToken to signal when a handshake has completed.
I'm not sure what to do with the licensing issue. I agree that tri-licensing
gave me qualms. As I understand it, the header file was taken from the rfc,
which says the following: 

"C-language GSS-API implementations should include a copy of the following
header-file."

The copyright on the RFC - http://www.faqs.org/rfcs/rfc2744.html is Copyright
The Internet Society (2000)

And that copyright reads in part:

Copyright (C) The Internet Society (2000).  All Rights Reserved.

   This document and translations of it may be copied and furnished to
   others, and derivative works that comment on or otherwise explain it
   or assist in its implementation may be prepared, copied, published
   and distributed, in whole or in part, without restriction of any
   kind, provided that the above copyright notice and this paragraph are
   included on all such copies and derivative works.  However, this
   document itself may not be modified in any way, such as by removing
   the copyright notice or references to the Internet Society or other
   Internet organizations, except as needed for the purpose of
   developing Internet standards in which case the procedures for
   copyrights defined in the Internet Standards process must be
   followed, or as required to translate it into languages other than
   English.

I assume the copyright applies to the appendix as well. Is pulling out the
header file "a derivitive work"? In which case, we'd need to put this copyright
in the file. I don't know if OpenVision got some special dispensation (is that
the same OpenVision that merged with Veritas/Symantec?). And how it got into the
MIT code is an other mystery to me. After a little googling, it looks like
OpenVision developed the draft RFC's and then the final version has the ISOC
copyright...so, if that's the case, Christopher is right and we should probably
use the same copyright. Gerv, you've had a lot of experience in this licensing
area, what do you suggest?



Simon and Christopher, thanks for all the work...

Christoper, re your patch:

this looks like my crap, and thus wrong, at least the c:\inc\krb5\gssapi stuff.

Index: Makefile.in
===================================================================
RCS file: /cvsroot/mozilla/extensions/negotiateauth/Makefile.in,v
retrieving revision 1.8
diff -r1.8 Makefile.in
47a48,49
> #undef USE_SSPI
> GSSAPI_INCLUDES = -Ic:\inc\krb5\gssapi
49a52
> USE_GSSAPI = 1
73a77,79
> LOCAL_INCLUDES += -Ic:/inc/krb5/gssapi 
> 
> 
75c81,82
< LOCAL_INCLUDES	= -DUSE_GSSAPI $(GSSAPI_INCLUDES)
---
> LOCAL_INCLUDES += -Ic:\inc\krb5\gssapi -Ic:\inc\krb5
> LOCAL_INCLUDES	+= -DUSE_GSSAPI $(GSSAPI_INCLUDES)
80c87
< LOCAL_INCLUDES	= -DUSE_SSPI
---
> LOCAL_INCLUDES	+= -DUSE_SSPI


I've been talking privately with Simon about where his SaslGssapi module should
live. He has tentatively made it its own extension, living in
extensions/saslgssapi. My impression was that code would live in the
negotiateauth extension but it's up to Darin and Christoper. Simon's code
creates an instance of the negotiateauth module to do the heavy lifting so it
can live anywhere.

Once we decide where it lives, we need to merge these patches...

When describing how the mailnews code would do security layers, Simon brought up
the idea of auth module wrapper nsIInput and Output streams, that would know how
to wrap/unwrap before passing the data along the underlying streams. We could 
make these standalone classes, in which case they would need to get passed the
auth module and the underlying stream in the factory constructor, e.g., nsresult
NS_NewAuthModuleWrappedInputStream(nsIInputStream **aWrapperStream,
nsIAuthModule *aAuthModule, nsIInputStream *aStreamToWrap);

Or we could roll this functionality into nsIAuthModule itself, by adding methods
like this:

nsIInputStream getWrappedInputStream(in nsIInputStream streamToWrap);
nsIOutputStream getWrappedOutputStream(in nsIOutputStream streamToWrap);

We might not even need the Wrap and Unwrap methods, if the saslgssapi code lived
in the same module as the negotiateauth code (so it could call the negotiate
auth code wrap/unwrap functions directly), and we could convince ourselves that
all other callers of wrap/unwrap really want stream wrappers. The mailnews code
just wants stream wrappers, but it doesn't really care because our reads and
writes are done from basically three places, so it's pretty easy to change. So
whatever you guys want is fine with me.
Do stream wrappers make sense?  I don't know if the GSSAPI specifies how the
data is marshalled into tokens.  That work needs to be done before the wrap or
unwrap is called.  Is marshalling protocol specific?  If so I don't know how we
could make a generic stream handler that would work everywhere.  Sorry if these
are dumb questions you've moved out of my area of expertise.

I don't care if your SASL Implementation lives inside negotiateauth or not.
Logically it makes sense, the only down side is a tiny code size increase.
darin's call on that.

There are a few changes which still need made to the patch.  I'm not sure if
they need added to nsIAuthModule interface or just new class constructors.  The
down side of custom constructors is that you will have to treat SSPI and GSS
different because they are now different classes and I'd like to hide the
differences whenever possible. Maybe nsIAuthmodule just a new init (or
constructor) method which takes an int to pass flags.

1. Let the caller pass in an arg which specifies options
eg Negotiate or Kerberos

2. GSSAPI Options
    Mutual authentication, Privacy, Integrity
GSSAPI doesn't specify marshalling, but SASL does (or, more correctly, it
specifies maximum sizes of buffers which can be passed out of the GSSAPI layer)
Marshalling is not protocol specific - it occurs below the protocol level.
Generic stream handlers are a fairly common means of implementing SASL security
layers.

Implementing security layers correctly also requires access to the
'gss_wrap_size_limit()' function.

Re your point (1), it would be nice to be able to control whether or not we get
the SPNEGO or Kerberos mechanism when using SSPI, as you've noted. 

Re point (2) - Can the GSSAPI options not be controlled through the existing
nsIAuthModule flags?

is any code besides mailnews going to care about the wrap/unwrap stuff?
(In reply to comment #19)
> is any code besides mailnews going to care about the wrap/unwrap stuff?

No not currently.
In that case, it's probably OK to worry about the wrap/unwrap stream stuff after
the basic stuff lands. Mail&news can have its own solution, and then that can be
rolled into the core stuff if desired. 
(In reply to comment #21)
> In that case, it's probably OK to worry about the wrap/unwrap stream stuff after
> the basic stuff lands. Mail&news can have its own solution, and then that can be
> rolled into the core stuff if desired. 

I think this is an argument for keeping the SASL stuff in a seperate module from
negotiate auth - any stream code we add is going to be SASL specific.

Should I open a new bug to hold the SASL patch?
>I think this is an argument for keeping the SASL stuff in a seperate module from
>negotiate auth - any stream code we add is going to be SASL specific.

I'm not sure that's true. If "not currently" means at some point in the future,
it  might or will be required by the browser, then there's still a argument for
keeping it in the same module. The stream code is going to be fairly
lightweight, I would think - maybe on the order of 50 lines of code? And if I
just do it in the core mailnews code for now, then that argument goes away,
doesn't it?

I think the overhead of having an extra dll/so outweigh the advantages of
keeping the code separate - but again, this is Darin's call. I'm not even sure
if Firefox ships with negotiateauth.dll, but Thunderbird definitely would ship
with both, if only because the sasl code needs the negotiate auth code. (Firefox
does a static build, so it's hard to tell without looking through the config files)

Yes, I guess a new bug is the way to go for the sasl/gssapi patch. 

Bug #303160 contains the complete GSSAPI SASL patch for those who are interested.

What's the best thing to do about the required changes  (Wrap, Unwrap, exit
status) to NegotiateAuth? Can the attachment above be reviewed, or should I open
a new bug for those too?

My thought is that Christoper's changes should go first, and then we can adapt
the sasl changes. For review, I think we just need a patch with the correct
license (and I'm OK with doing what Christoper proposed) and the makefile.in
changes cleaned up a bit.
OK, talked to Darin, and this is what we're proposing:

put these things in the same module, but rename the cvs directory as:

extensions/auth 

nsNegotiateAuthGSSAPI.cpp should be renamed nsAuthModuleGSSAPI.cpp

We're not sure about the new SASL stuff. If it's using GSSAPI, perhaps it should
live in nsAuthModuleGSSAPI.cpp with the other stuff. Or, nsAuthModuleSASL.cpp,
if that makes more sense to Christopher and/or Simon.

Does this make sense? I can take care of creating the new directories and
getting Chase to do the cvs magic...



I can try to get Chase to maintain the cvs history when moving+renaming files.
(In reply to comment #26)
> OK, talked to Darin, and this is what we're proposing:
> 
> put these things in the same module, but rename the cvs directory as:
> 
> extensions/auth 

Fine

> 
> nsNegotiateAuthGSSAPI.cpp should be renamed nsAuthModuleGSSAPI.cpp
> 
> We're not sure about the new SASL stuff. If it's using GSSAPI, perhaps it should
> live in nsAuthModuleGSSAPI.cpp with the other stuff. Or, nsAuthModuleSASL.cpp,
> if that makes more sense to Christopher and/or Simon.

nsAuthModuleSASL.cpp makes more sense to me because you don't want to duplicate
code for SSPI and GSSAPI.

> 
> Does this make sense? I can take care of creating the new directories and
> getting Chase to do the cvs magic...
> 
Yes.  Sounds fine.
Attached patch latest untested patch (obsolete) — — Splinter Review 
This is the latest work I've done on the gssapi.  It contains the Wrap and
Unwrap implementations Simon wrote for mit and I attempted some for SSPI as
well. I have no way to test the SSPI wrap and unwrap so it probably doesn't
work as is and could use some tweaking. This patch is untested, its late and I
just wanted to put it up here for safekeeping.
(In reply to comment #27)

Can I suggest that we fit 'GSSAPI' into the SASL module name, just in case we
want to create modules implementing other SASL mechanisms at a later date -
something like nsAuthModuleSaslGssapi.cpp ?

(In reply to comment #28)

Similarly, I can't test the SSPI stuff. What we need is someone with an Active
Directory setup, and a non-Exchange IMAP server which is a member of that domain.

However, here are some comments on the patch, based on the MSDN documentation,
and other information on the 'net.

In Wrap() you're returning the contents of the SECBUFFER_PADDING block - I
believe that you should be returning the concatenated contents of all three blocks.

In GetNextToken() - the return code NS_OK should be returned when
SEC_I_CONTINUE_NEEDED is returned. NS_SUCCESS_AUTH_FINISHED should be returned
when InitializeSecurityContext gives SEC_E_OK.
Attached file newest version (obsolete) — 
Since I don't know how to do the cvs fun to rename files for the time being I
just attached a tar ball of the new auth directory. I should have a chance
tomorrow evening to try it out on Linux and make sure it still works there.

1.  Renamed files
2.  GSSAPI and SSPI both work on windows for HTTP Negotiate auth
3.  Addressed Simon's remarks on SSPI and Wrap and getNextToken
4.  network.negotiate-auth.sspi true/false determines if the SSPI is used.  I'd
like suggestions for a better name.

Can everyone start looking this over? I'll ask for a formal review tomorrow
evening after I test it on Linux and have time to re-post the configure.in file
and other supporting files which live outside of the auth directory.
... provided that the above copyright notice appears in all copies and
 * that both that copyright notice and this permission notice appear in
 * supporting documentation ...

How is this part of the copyright notice from the gssapi.h file and openvision
to be fulfilled? Especially the supporting documentation part.
AFAIK, we don't have any supporting documentation for this authentication module
but I think a link to the notice in the header file would be sufficient.  I
couldn't find any references to OpenVision in the MIT Kerberos distribution
documentation, other than the copyright notice in the header file.
I'll try it on windows today.

If I can't get any of Chase's time, how crucial is the cvs history for these files?
c:/tbird1.5/mozilla/extensions/auth/nsAuthSSPI.cpp(373) : error C2065: 'SECBUFFE
R_STREAM' : undeclared identifier
c:/tbird1.5/mozilla/extensions/auth/nsAuthSSPI.cpp(445) : error C2065: 'SECBUFFE
R_PADDING' : undeclared identifier
make[1]: *** [nsAuthSSPI.obj] Error 2

My VC 6.0 copy of sspi.h doesn't include declarations for these. It has
SECBUFFER_TOKEN, SECBUFFER_DATA, etc, but not the two above.
that probably has to do with not having the latest ms header files for
something...but I'd hate to force all our developers who build negotiate auth to
have to update their sdk. I added the following to nsAuthSSPI.h:

#ifndef SECBUFFER_PADDING
#define SECBUFFER_PADDING 9
#endif

#ifndef SECBUFFER_STREAM
#define SECBUFFER_STREAM 10
#endif


Have you changed security/manager/ssl/src/nsNTLMAuthModule.cpp? I.e., is that
one of the files changed outside the extensions/auth directory? It needs wrap
and unwrap methods. If you haven't done this, I can attach a patch. 
in extensions/auth/nsAuthGSSAPI.cpp, I had to change these two lines so it would
work on windows, by adding KRB_CCONV:

typedef OM_uint32 (KRB_CCONV *gss_wrap_type)(

typedef OM_uint32(KRB_CCONV *gss_unwrap_type)(

In my tree, I've removed KRB_CCONV completely and replaced it with GSS_CALLCONV,
which is defined by gssapi.h.

Once I made those changes, added Simon's SaslGssapi.cpp to Makefile.in and
registered it appropriately in nsAuthFactory.cpp, and used the new contract-id,
"negotiate-gss" in nsSaslGssapi::Init, and modified the mailnews code to use
"saslgssapi", I was able to authenticate against an imap server with
auth=gssapi. (now, am I supposed to make the mailnews code check a pref and use
"sspi" instead of "saslgssapi" by default?)

POP3's not working for some reason, but that might be some other change in my tree.
(In reply to comment #31)
The copyright notice on gssapi.h appears equivalent to a 2 clause BSD licence. A
gssapi.h with that notice attached is distributed as part of MIT Kerberos, and
as part of the Solaris operating system.

(In reply to comment #36)
The latest tarball doesn't contain patches for the NTLM code - however there are
suitable patches in the two earlier attachments.

(In reply to comment #37)
The mailnews code should never use 'sspi' directly - as SSPI doesn't implement
SASL. The SaslGssapi module should check a pref and decide whether to load 
negotiate-gss or kerb-sspi, in the same way as the HttpNegotiateAuth code does.

>The copyright notice on gssapi.h appears equivalent to a 2 clause BSD licence. A
>gssapi.h with that notice attached is distributed as part of MIT Kerberos, and
>as part of the Solaris operating system.

Does the copyright notice appear anywhere in those two distributions besides in
the gssapi.h? I.e., are we OK with just leaving it in gssapi.h?

Do you want to change SaslGssapi.cpp to check a pref or should I?

I think we should be consistent about capitalizing GSSAPI in the class names and
file names - is SaslGSSAPI.* OK with you?
>>The SaslGssapi module should check a pref and decide whether to load 
>>negotiate-gss or kerb-sspi, in the same way as the HttpNegotiateAuth code does.

I think you would want to use kerb-gss not negotiate-gss for mail.  The reason
is negotiate-gss is designed to use SPNEGO instead of kerberos if it can find
it. It fails back to kerberos if it can't. Sun has (or will have) SPNEGO in
their kerberos libraries. kerb-gss always uses gssapi kerberos and I remember
you saying GSSAPI in sasl really just means kerberos.
(In reply to comment #40)
> I think you would want to use kerb-gss not negotiate-gss for mail.  The reason
> is negotiate-gss is designed to use SPNEGO instead of kerberos if it can find
> it. It fails back to kerberos if it can't. Sun has (or will have) SPNEGO in
> their kerberos libraries. kerb-gss always uses gssapi kerberos and I remember
> you saying GSSAPI in sasl really just means kerberos.

Just to clarify - Solaris 10 has a GSSAPI SPNEGO module, it is quite separate
from the GSSAPI Kerberos mechanism.   By design, the negotiateauth module will 
check for the set of available GSSAPI mechanisms that the system provides, if 
SPNEGO is found, it is used, otherwise it tries to use Kerberos.

Also, w.r.t SASL, you are correct.  The SASL mechanism naming is pretty
confusing.  "GSSAPI" actually means use SASL w/GSSAPI+KRB5.  The SASL mechanism 
name for SASL-SPNEGO is "GSS-SPNEGO".  See
http://www.ietf.org/internet-drafts/draft-ietf-sasl-gssapi-02.txt for more info
on that whole mess.
(In reply to comment #39)
> Do you want to change SaslGssapi.cpp to check a pref or should I?

I'll do it, if you like

cneberg: Do you think there would be mileage in making the pref something more
general, so that both negotiateauth and mailnews could check the same pref? 

Something like network.gssapi.use-sspi ?
 
> I think we should be consistent about capitalizing GSSAPI in the class names 
> and file names - is SaslGSSAPI.* OK with you?

That's fine. I'll update the mailnews SASL stuff in bug #303160.
Blocks: 303160
(In reply to comment #42)
> (In reply to comment #39)
> > Do you want to change SaslGssapi.cpp to check a pref or should I?
> 
> I'll do it, if you like
> 
> cneberg: Do you think there would be mileage in making the pref something more
> general, so that both negotiateauth and mailnews could check the same pref? 
> 
> Something like network.gssapi.use-sspi ?

Sure I can make it more general.  Does mozilla have any other platform specific
prefs?  It also feels strange to have a pref for what should be the default
behavior for that platform rather than having a pref to enable the alternative
item.  Especially if these items ever get a GUI.

Maybe 

network.win32.use-gssapi defaulted to false.

I'm not sure which is better, but I'll set it to network.gssapi.use-sspi for the
patch for review which I'm submitting in a minute.
Attached patch new auth module (obsolete) — — Splinter Review 
This is the new auth module. It's not a diff against CVS because I can't figure
out how to do a CVS diff against new files without checkin rights.  Not that I
should need checkin rights but I was going to try a cvs add followed by a cvs
diff but the cvs add fails even though I had no intention of trying a cvs
commit.
Attachment #191826 - Flags: review?(darin)
Attached patch supporting changes (obsolete) — — Splinter Review 
These are all of the supporting files out side of extentions/auth made as a
real cvs diff.
Attachment #190967 - Attachment is obsolete: true
Attachment #191192 - Attachment is obsolete: true
Attachment #191193 - Attachment is obsolete: true
Attachment #191194 - Attachment is obsolete: true
Attachment #191447 - Attachment is obsolete: true
Attachment #191568 - Attachment is obsolete: true
Attachment #191827 - Flags: review?(darin)
I'm attaching this to help with the review.  I renamed most of the files back
to their old names and did a diff so its easier to see what has changed.
(In reply to comment #43)

> Maybe 
> 
> network.win32.use-gssapi defaulted to false.
> 


I think this is better because it indicates that it is a Win32 parameter only
and thus should not confuse non-Windows users.  It should also be obvious to
the casual reader that this is a windows-only parameter.

Also, saying "network.gssapi.use-sspi" is confusing since SSPI and GSSAPI are 2
different things entirely and mixing them together in this way is a little
misleading and confusing.




(In reply to comment #48)
> > network.win32.use-gssapi defaulted to false.

> Also, saying "network.gssapi.use-sspi" is confusing since SSPI and GSSAPI are 2
> different things entirely and mixing them together in this way is a little
> misleading and confusing.

I don't really care that much about what these prefs are called, provided their
never exposed to the user.

However, my concern is that having 'use-gssapi' set to false would indicate to
me that the client's _not_ going to do SASL-GSSAPI. Whatever the programming
differences between SSPI and GSSAPI, negotiate-auth and SASL's use of SSPI is to
provide something that is indistinguishable from GSSAPI on-the-wire - saying
they are "2 different things entirely" isn't really correct in this case.

Perhaps the difference between SSPI and GSSAPI isn't what we should be stressing
(it is, after all, an implementation detail). The key differences are whether
we're using Microsoft or MIT's Kerberos implementation, and whether we're using
credentials directly from the LSA, or through KfW.

(In reply to comment #49)
>
> I don't really care that much about what these prefs are called, provided their
> never exposed to the user.

I guess the only way they are exposed is thru the "about:config" screen.
However, that is also the only way to change the current negotiateauth
preferences, so whatever is decided, it will be visible to the
users who care about the feature.  At least until someone comes
up with a GUI configuration panel for this feature or adds it to the
security preferences screen (Preferences->Advanced +security)

BTW - is there a bug to add some of this to one of the more visible
preferences screens instead of just being in about:config?

>
> Perhaps the difference between SSPI and GSSAPI isn't what we should be stressing
> (it is, after all, an implementation detail). The key differences are whether
> we're using Microsoft or MIT's Kerberos implementation, and whether we're using
> credentials directly from the LSA, or through KfW.
> 

Yeah, good point.  I agree with Christopher that the default should be
to use SSPI.  So, the question is how to describe the non-SSPI case.
Perhaps it should be:  network.win32.auth.sspi - (true/false)
The default setting being "True".  "False" means use KfW/GSSAPI.

I don't want to get too far into this since you all did the majority of the
work on this feature, I'm just adding my 2 cents.



If a person upgrades and has an existing pref file, do they inherit new values
from all.js for attributes not in their current config file?  If not the current
patch is wrong.  It assumes a missing preference is equal to a false value, that
means the default for upgraders in the current patch would be to use GSSAPI
rather than SSPI on windows.

So the pref should be something where the default is false. So that a missing
pref item and false mean the same thing.
>If a person upgrades and has an existing pref file, do they inherit new values
>from all.js for attributes not in their current config file?

Yes, they get the default value from all.js. In fact, we never write out prefs
whose value is the default value, so upgrading and having the default value look
exactly the same in prefs.js. Assuming I'm understanding your question correctly.

If a pref is given a default value in a prefs file like all.js, getting the pref
value will not fail even if it's not in prefs.js.

But the pref only seems to be defined for windows, but still accessed on
non-windows platforms, assuming I'm getting enough context in the diffs. In that
case TestBoolPref will return false, and the code will do the right thing, at
least as it is in the patch.
(In reply to comment #52)

Thanks for clearing that up.


Here's the CVS magic Chase is planning on doing. Does this still look correct?
The only name change is nsNegotiateAuthGSSAPI.cpp becomes
auth/nsAuthModuleGSSAPI.cpp. Are we also planning on changing the name of the
factory .cpp/h files from nsNegotiateAuthFactory to nsAuthFactory?

   1.  Backup the cvsroot repository.
         1. rsync -av /data/cvs/jail/cvsroot ~root/cvsroot.backup.20050807 
   2. From a trunk working copy, add the extensions/auth directory.
         1. cd mozilla/extensions/
         2. cvs add auth 
   3. In the repository, copy files from extensions/negotiateauth to
extensions/auth.
         1. cd cvsroot/mozilla/extensions/
         2. /opt/cvsmgmt/copy-cvs-file.pl negotiateauth/Makefile.in,v auth/
         3. /opt/cvsmgmt/copy-cvs-file.pl
negotiateauth/nsHttpNegotiateAuth.cpp,v auth/
         4. /opt/cvsmgmt/copy-cvs-file.pl negotiateauth/nsHttpNegotiateAuth.h,v
auth/
         5. /opt/cvsmgmt/copy-cvs-file.pl
negotiateauth/nsNegotiateAuthFactory.cpp,v auth/
         6. /opt/cvsmgmt/copy-cvs-file.pl
negotiateauth/nsNegotiateAuthGSSAPI.cpp,v auth/nsAuthModuleGSSAPI.cpp,v
         7. /opt/cvsmgmt/copy-cvs-file.pl
negotiateauth/nsNegotiateAuthGSSAPI.h,v auth/
         8. /opt/cvsmgmt/copy-cvs-file.pl negotiateauth/nsNegotiateAuth.h,v auth/
         9. /opt/cvsmgmt/copy-cvs-file.pl
negotiateauth/nsNegotiateAuthSSPI.cpp,v auth/
        10. /opt/cvsmgmt/copy-cvs-file.pl negotiateauth/nsNegotiateAuthSSPI.h,v
auth/ 



I don't completely understand the licensing situation here - there's a lot to
read. Can someone summarise?

Some background info: unfortunately, the copyright message on RFCs makes them
non-free software. If the only place we can obtain the relevant file is by
getting the text out of the RFC, that will present difficulties.

Gerv
Gerv: We want to include the 'gssapi.h' header file, so that we can build GSSAPI
support into negotiate-auth without needing the GSSAPI libraries present at
build time (the GSSAPI code is then dynamically loaded).

The 'gssapi.h' header file is available from a number of sources:
  *) RFC 2344 (under an ISOC copyright)
  *) The MIT Kerberos distribution (under an OpenVision copyright using similar,
but not identical language to two-clause BSD)
  *) The Heimdal Kerberos distribution (under a three clause BSD copyright)

The file which is currently up for inclusion is the MIT Kerberos one, whose
header reads:

/*
 * Copyright 1993 by OpenVision Technologies, Inc.
 *
 * Permission to use, copy, modify, distribute, and sell this software
 * and its documentation for any purpose is hereby granted without fee,
 * provided that the above copyright notice appears in all copies and
 * that both that copyright notice and this permission notice appear in
 * supporting documentation, and that the name of OpenVision not be used
 * in advertising or publicity pertaining to distribution of the software
 * without specific, written prior permission. OpenVision makes no
 * representations about the suitability of this software for any
 * purpose.  It is provided "as is" without express or implied warranty.
 *
 * OPENVISION DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
 * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
 * EVENT SHALL OPENVISION BE LIABLE FOR ANY SPECIAL, INDIRECT OR
 * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF
 * USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR
 * OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 * PERFORMANCE OF THIS SOFTWARE.
 */


(In reply to comment #56)
> Gerv: We want to include the 'gssapi.h' header file, so that we can build GSSAPI
> support into negotiate-auth without needing the GSSAPI libraries present at
> build time (the GSSAPI code is then dynamically loaded).

For Solaris (8, 9, 10, etc), gssapi.h is a standard header file
(/usr/include/gssapi) as is the gssapi library.  If the build is 
being done on Solaris, it would be best to use the native headers 
rather than the included copy.




Comment on attachment 191827 [details] [diff] [review]
supporting changes

Whenever you add, remove, or modify methods on an interface, you MUST change
the interface's uuid property in the header.  Please use the uuidgen utility to
generate a new uuid.

Also, please document the wrap and unwrap methods.  I for one do not understand
what they are used for or why you are adding them here.  Since they are not
implemented for NTLM, it seems like you should document that.  You should
document the exception that is thrown when they cannot be used.  What happens
when Wrap/Unwrap is used with sys-ntlm?

You should mention NS_SUCCESS_AUTH_FINISHED in the documentation for
getNextToken as well.

In all.js, "intead" is a mispelling.

Horray for configure.in simplification! ;)
Attachment #191827 - Flags: review?(darin) → review-
Comment on attachment 191826 [details] [diff] [review]
new auth module

>diff -N /tmp/auth/gssapi.h auth/gssapi.h
...
>> /* vim:set ts=4 sw=4 sts=4 et cindent: */
>> /* ***** BEGIN LICENSE BLOCK *****
>>  * Copyright 1993 by OpenVision Technologies, Inc.
>>  * 
>>  * Permission to use, copy, modify, distribute, and sell this software
>>  * and its documentation for any purpose is hereby granted without fee,
>>  * provided that the above copyright notice appears in all copies and
>>  * that both that copyright notice and this permission notice appear in
>>  * supporting documentation, and that the name of OpenVision not be used
>>  * in advertising or publicity pertaining to distribution of the software
>>  * without specific, written prior permission. OpenVision makes no
>>  * representations about the suitability of this software for any
>>  * purpose.  It is provided "as is" without express or implied warranty.
>>  * 
>>  * OPENVISION DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
>>  * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
>>  * EVENT SHALL OPENVISION BE LIABLE FOR ANY SPECIAL, INDIRECT OR
>>  * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF
>>  * USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR
>>  * OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
>>  * PERFORMANCE OF THIS SOFTWARE.
>>  ****** END LICENSE BLOCK ***** */

I think that someone at the Mozilla Foundation (Gerv?) needs to approve of
the OpenVision license agreement.  It looks fine to me, but I'm not a lawyer.


>diff -N /tmp/auth/nsAuthFactory.cpp auth/nsAuthFactory.cpp

>>   nsNegotiateAuthSSPI *auth = new nsNegotiateAuthSSPI("NTLM");

I'm not so sure if I like the package strings.	It seems
like an enumeration type might be better...


>diff -N /tmp/auth/nsAuthGSSAPI.cpp auth/nsAuthGSSAPI.cpp

>> nsNegotiateAuth::nsNegotiateAuth(const char * package)
>>     : mServiceFlags(REQ_DEFAULT),
>>       mPackage(package)
>> {
>>     OM_uint32 minstat;
>>     OM_uint32 majstat;
>>     gss_OID_set mech_set;
>>     gss_OID item;
>>     unsigned int i;
>>     static gss_OID_desc gss_krb5_mech_oid_desc =
>>         { 9, (void *) "\x2a\x86\x48\x86\xf7\x12\x01\x02\x02" };
>>     static gss_OID_desc gss_spnego_mech_oid_desc =
>>         { 6, (void *) "\x2b\x06\x01\x05\x05\x02" };
>> 
>>     LOG(("entering nsNegotiateAuth::nsNegotiateAuth()\n"));
>> 
>>     if (!gssFunInit && NS_FAILED(gssInit()))
>>         return;
>> 
>>     mCtx = GSS_C_NO_CONTEXT;
>>     mMechOID = &gss_krb5_mech_oid_desc;
>> 
>>     if (!strcmp(mPackage, "Kerberos"))
>>         return;

should there be some comment here explaining why a package
name of "Kerberos" is not acceptable?  Why even bother calling
gssInit() if you're just going to return early?

mPackage doesn't seem to be used for anything else in this
class, so why is it a member variable?


>> NS_IMETHODIMP nsNegotiateAuth::Unwrap(const void *inToken,

convention is to put NS_IMETHODIMP on a line by itself.


>>     *outTokenLen = output_token.length;
>>     *outToken = nsMemory::Clone(output_token.value, output_token.length);
>>     gss_release_buffer_ptr(&minor_status, &output_token);

Is it possible for output_token.length to be zero at this point?
If so, are we happy cloning an empty buffer?


>>     *outTokenLen = output_token.length;
>>     *outToken = nsMemory::Clone(output_token.value, output_token.length);
>>     gss_release_buffer_ptr(&minor_status, &output_token);

same questions apply here.


It seems a little odd to me that this is nsAuthGSSAPI, but the class
is nsNegotiateAuth.  Shouldn't the class name correspond to the file
name (since we are renaming things w/ this patch)?


>diff -N /tmp/auth/nsAuth.h auth/nsAuth.h

>> #ifndef nsNegotiateAuth_h__
>> #define nsNegotiateAuth_h__

Make these include blocks correspond to the name of the file.

>> #if defined( MOZ_LOGGING)

nit: can you fix the whitespace here while you're at it?


>diff -N /tmp/auth/nsAuthSSPI.cpp auth/nsAuthSSPI.cpp

>> nsNegotiateAuthSSPI::nsNegotiateAuthSSPI(const char * package)

Should this be nsAuthSSPI?  or is this the best name of the class?
Should the other be nsNegotiateAuthGSS(API)?

>> nsNegotiateAuthSSPI::Unwrap(const void *inToken,
...
>>     ib[0].cbBuffer = inTokenLen;
>>     ib[0].pvBuffer = nsMemory::Alloc(ib[0].cbBuffer);
...
>>    if (SEC_SUCCESS(rc))  
>>    {
>>        *outToken = ib[1].pvBuffer;
>>        *outTokenLen = ib[1].cbBuffer;
>>    }

In the failure case, don't you need to free the memory
buffer?


>>     ib[0].pvBuffer = nsMemory::Alloc(sizes.cbSecurityTrailer);
>> 
>>     if (!ib[0].pvBuffer)
>>         return NS_ERROR_OUT_OF_MEMORY;
>> 
>>     // APP Data
>>     ib[1].BufferType = SECBUFFER_DATA;
>>     ib[1].pvBuffer = nsMemory::Alloc(inTokenLen);
>>     ib[1].cbBuffer = inTokenLen;
>>     
>>     if (!ib[1].pvBuffer)
>>         return NS_ERROR_OUT_OF_MEMORY;

ditto... doesn't this failure case leak ib[0].pvBuffer?


>>     ib[2].pvBuffer = nsMemory::Alloc(ib[2].cbBuffer);
>> 
>>     if (!ib[2].pvBuffer)
>>         return NS_ERROR_OUT_OF_MEMORY;

more potential memory leaking


>> 
>>     rc = (sspi->EncryptMessage)(&mCtxt,
>>           confidential ? 0 : KERB_WRAP_NO_ENCRYPT,
>>          &ibd, 0);
>> 
>>     if (SEC_SUCCESS(rc))
>>     {
>>        int len  = ib[0].cbBuffer + ib[1].cbBuffer + ib[2].cbBuffer;
>> 
>>         *outToken = nsMemory::Alloc(len);
>> 
>>         if (!*outToken)
>>             return NS_ERROR_OUT_OF_MEMORY;

And, again... maybe it would help to write a small class that
will call nsMemory::Free from its destructor that you could
use to manage all these early returns.


>diff -N /tmp/auth/nsHttpNegotiateAuth.cpp auth/nsHttpNegotiateAuth.cpp

>> static const char kNegotiateAuthSSPI[] = "network.gssapi.use-sspi";

The name of this pref seems funny to me.  Why is "use-sspi" a child
of "network.gssapi"... isn't SSPI parallel to GSSAPI?  Shouldn't it be
like "network.negotiate-auth.use-sspi"?


>>     if (TestBoolPref(kNegotiateAuthSSPI)) {
>>         LOG(("  using negotiate-sspi \n"));
>>         rv = CallCreateInstance(NS_AUTH_MODULE_CONTRACTID_PREFIX "negotiate-sspi", &module);
>>     }
>>     else 
>>     {
>>         LOG(("  using negotiate-gss \n"));
>>         rv = CallCreateInstance(NS_AUTH_MODULE_CONTRACTID_PREFIX "negotiate-gss", &module);
>>     }

It might be better to reduce codesize slightly by only calling
CreateInstance once, like so:

       const char *contractID;
       if (TestBoolPref(kNegotiateAuthSSPI)) {
	   LOG(("  using negotiate-sspi\n"));
	   contractID = NS_AUTH_MODULE_CONTRACTID_PREFIX "negotiate-sspi";
       }
       else {
	   LOG(("  using negotiate-gss\n"));
	   contractID = NS_AUTH_MODULE_CONTRACTID_PREFIX "negotiate-gss";
       }
       rv = CallCreateInstance(contractID, &module);



>>     if (NS_FAILED(rv))
>>     {

Please keep bracket style consistent.


>diff -N /tmp/auth/nsHttpNegotiateAuth.h auth/nsHttpNegotiateAuth.h

>> // The nsGssapiAuth class provides responses for the GSS-API Negotiate method

can you fix the documentation here to mention the correct class name?
Attachment #191826 - Flags: review?(darin) → review-
The updated instructions.  I'm going to pull the trigger on this very soon if
someone doesn't stand up and say stop.

   1.  Backup the cvsroot repository.
         1. rsync -av /data/cvs/jail/cvsroot ~root/cvsroot.backup.20050807 
   2. From a trunk working copy, add the extensions/auth directory.
         1. cd mozilla/extensions/
         2. mkdir auth
         3. cvs add auth 
   3. In the repository, copy files from extensions/negotiateauth to
extensions/auth.
         1. cd cvsroot/mozilla/extensions/
         2. /opt/cvsmgmt/copy-cvs-file.pl negotiateauth/Makefile.in,v auth/
         3. /opt/cvsmgmt/copy-cvs-file.pl
negotiateauth/nsHttpNegotiateAuth.cpp,v auth/
         4. /opt/cvsmgmt/copy-cvs-file.pl negotiateauth/nsHttpNegotiateAuth.h,v
auth/
         5. /opt/cvsmgmt/copy-cvs-file.pl
negotiateauth/nsNegotiateAuthFactory.cpp,v auth/nsAuthFactory.cpp,v
         6. /opt/cvsmgmt/copy-cvs-file.pl
negotiateauth/nsNegotiateAuthGSSAPI.cpp,v auth/nsAuthModuleGSSAPI.cpp,v
         7. /opt/cvsmgmt/copy-cvs-file.pl
negotiateauth/nsNegotiateAuthGSSAPI.h,v auth/nsAuthModuleGSSAPI.h,v
         8. /opt/cvsmgmt/copy-cvs-file.pl negotiateauth/nsNegotiateAuth.h,v
auth/nsAuth.h,v
         9. /opt/cvsmgmt/copy-cvs-file.pl
negotiateauth/nsNegotiateAuthSSPI.cpp,v auth/nsAuthSSPI.cpp,v
        10. /opt/cvsmgmt/copy-cvs-file.pl negotiateauth/nsNegotiateAuthSSPI.h,v
auth/nsAuthSSPI.h,v 

> Also, please document the wrap and unwrap methods. 

cneberg: proposed text for Wrap():

/** 
 * Once a security context has been established through calls to GetNextToken()
 * it may be used to protect data exchanged between client and server. Calls
 * to Wrap() are used to protect items of data to be sent to the server.
 * 
 * @param aInToken
 *        A buffer containing the data to be sent to the server
 * @param aInTokenLength
 *        The length of the input token
 * @param confidential
 *        If set to true, Wrap() will encrypt the data, otherwise data will
 *        just be integrity protected (checksummed)
 * @param aOutToken
 *        A buffer containing the resulting data to be sent to the server
 * @param aOutTokenLength
 *        The length of the output token buffer
 *
 * Wrap() may return NS_ERROR_NOT_IMPLEMENTED, if the underlying authentication
 * mechanism does not support security layers.
 */

And, for Unwrap()

/** 
 * Unwrap() is used to unpack, decrypt, and verify the checksums on data
 * returned by a server when security layers are in use.
 * 
 * @param aInToken
 *        A buffer containing the data received from the server
 * @param aInTokenLength
 *        The length of the input token
 * @param aOutToken
 *        A buffer containing the plaintext data from the server
 * @param aOutTokenLength
 *        The length of the output token buffer
 *
 * Unwrap() may return NS_ERROR_NOT_IMPLEMENTED, if the underlying  
 * authentication mechanism does not support security layers.
 */
> should there be some comment here explaining why a package
> name of "Kerberos" is not acceptable?  Why even bother calling
> gssInit() if you're just going to return early?

It's not rejecting the mechanism - just defaulting to using the Kerberos OID.
 
> Is it possible for output_token.length to be zero at this point?
> If so, are we happy cloning an empty buffer?

It's not possible for output_token.length to be zero in the Wrap() case.

It could conceivably be zero in the Unwrap() case, although this would require
the server to be sending us a zero length payload.

The check in GetNextToken for a zero length payload should be removed - it isn't
an error for get_init_sec_context to return a zero length payload (and, in the
Kerberos case, it always will where mutual auth is being done).

(In reply to comment #60)
> The updated instructions.  I'm going to pull the trigger on this very soon if
> someone doesn't stand up and say stop.

Changes made.  Please verify extensions/auth is how you want it and let me know
thumbs up/thumbs down.
Sorry about the bug spam! 

Another few changes to nsAuthGSSAPI.cpp that I feel should be included...

mComplete = PR_FALSE;

should be included in the constructor and in the Reset() method.

In GetNextToken()

    if (mServiceFlags & REQ_MUTUAL_AUTH)
        req_flags |= GSS_C_MUTUAL_FLAG;

should be added after the other mServiceFlags code (otherwise, GSSAPI can't do
MUTUAL authentication - the equivalent code is already present in the SSPI module)

The code which returns an error if (output_token.length == 0) should be removed
- its valid for GSSAPI to return a zero length output token, especially when the
client completes before the server.

(In reply to comment #57) 
> For Solaris (8, 9, 10, etc), gssapi.h is a standard header file
> (/usr/include/gssapi) as is the gssapi library.  If the build is 
> being done on Solaris, it would be best to use the native headers 
> rather than the included copy.

If we use our own gssapi.h header file we fix several open bugs, simplify
configure.in, and building mozilla.  Do you believe there will more work for us
in the long run to include our own version?  If so, why?
(In reply to comment #65)
> (In reply to comment #57) 
> > For Solaris (8, 9, 10, etc), gssapi.h is a standard header file
> > (/usr/include/gssapi) as is the gssapi library.  If the build is 
> > being done on Solaris, it would be best to use the native headers 
> > rather than the included copy.
> 
> If we use our own gssapi.h header file we fix several open bugs, simplify
> configure.in, and building mozilla.  Do you believe there will more work for us
> in the long run to include our own version?  If so, why?

After looking closer at the Solaris gssapi.h and the new Mozilla gssapi.h, I
don't see a big problem with the Mozilla one.  My concern was that if a vendor
had added new features to the header that would not be tracked by the Mozilla
gssapi.h, then those features would be lost.

The IETF KITTEN WG is currently working on quite a few significant changes to
GSSAPI that will likely result in changes to this header in the future.

It's not definitely clear that Mozilla will need to change in the future. Since
the usage in this case is pretty limited and should not be affected by future
updates to the spec.

thx, Wyllys. Using our own is a big win for our build configuration. Gerv, can
you look at the license we've put in that file and see if it's acceptable, or if
one of the other licenses it's released under is acceptable?
(In reply to comment #60)
> negotiateauth/nsNegotiateAuthGSSAPI.cpp,v auth/nsAuthModuleGSSAPI.cpp,v
>          7. /opt/cvsmgmt/copy-cvs-file.pl
> negotiateauth/nsNegotiateAuthGSSAPI.h,v auth/nsAuthModuleGSSAPI.h,v

GSSAPI adds the word Module in the middle of the name but the SSPI version below
doesn't.  Sorry, that I didn't catch this earlier. If we fix this please leave
out the word Module for nsAuthGSSAPI.cpp and .h.

9. /opt/cvsmgmt/copy-cvs-file.pl
negotiateauth/nsNegotiateAuthSSPI.cpp,v auth/nsAuthSSPI.cpp,v
        10. /opt/cvsmgmt/copy-cvs-file.pl negotiateauth/nsNegotiateAuthSSPI.h,v
auth/nsAuthSSPI.h,v
Attached patch updated per comments (obsolete) — — Splinter Review 
I've tried to update this to all of the comments.  Unfortuneatly my windows
build environment is now broken, so I couldn't test the windows changes.  I'll
try to get back to this tomorrow evening. Since we are so time contrained if
someone else has time to get to this before I do go ahead.
The licence in comment #56 is fine. We will need to include a copy of the text
in about:licence (along with several other similar texts) when I get around to
sorting that out.

Gerv
cneberg's last set of changes, with some typos preventing them from working
under Linux fixed, and the GSSAPI header file included.
Attachment #191826 - Attachment is obsolete: true
Attachment #191827 - Attachment is obsolete: true
Attachment #191831 - Attachment is obsolete: true
Attachment #192337 - Attachment is obsolete: true
Attachment #192364 - Flags: review?(darin)
my vc 6.0 compiler is unhappy about the "ptype" stuff in nsAuthSSPI.cpp,h
(In reply to comment #72)
> my vc 6.0 compiler is unhappy about the "ptype" stuff in nsAuthSSPI.cpp,h

I don't have working vc compiler to compile the code with on windows.   I might
be able to get it working this evening.
I had to make a few changes to get nsAuthSSPI.cpp to compile:

ptype -> pType (which matches the declaration)

SecBuffers -> secBuffers, again to match the declaration:

+    SecBufferDesc ibd;
+    secBuffers bufs;
+    SecPkgContext_Sizes sizes;

and then, I changed this code:

+    ibd.cBuffers = 3;
+    ibd.pBuffers = bufs.ib; /*****/
+    ibd.ulVersion = SECBUFFER_VERSION;


it used to be ibd.pBuffers = ib;, but ib isn't defined in the ::Wrap method, and
I think the intent was to use bufs.ib, not ib.
I'm sorry, I won't be able to review the latest patches here until tomorrow...
(In reply to comment #74)

You are correct in your fixes. Unfortunatly my build enviroment is still broken.
Also we need to include a modified allmakefiles.sh.
Attached patch allmakefiles.sh change (obsolete) — — Splinter Review 
change name of extension...
Attachment #192510 - Flags: superreview?(mscott)
Attachment #192510 - Flags: superreview?(mscott) → superreview+
Comment on attachment 192364 [details] [diff] [review]
Fixed version of auth module patches

>Index: extensions/auth/nsAuth.h

>+/* types of packages */
>+enum pType {Kerberos = 0, Negotiate, NTLM};
>+const char *const pTypeName [] = {"Kerberos", "Negotiate", "NTLM"};

How about this instead:

enum {
    ePackageType_Kerberos,
    ePackageType_Negotiate,
    ePackageType_NTLM,
};

Or,

enum {
    PACKAGE_TYPE_KERBEROS,
    PACKAGE_TYPE_NEGOTIATE,
    PACKAGE_TYPE_NTLM
};

Also, the mapping from type to string is only needed by nsAuthSSPI.cpp,
and in fact the mapping is probably SSPI specific.  I think you should 
move this pTypeName array into that file.  Moreover, I think you'd have
a bug if more than two CPP files tried to use pTypeName since it is not
declared static.



>Index: extensions/auth/nsAuthModuleGSSAPI.cpp

Why is this file called nsAuthModuleGSSAPI.cpp when the others are 
called nsAuthSSPI.cpp and nsAuthSASL.cpp?  It seems to me that this
file should be called nsAuthGSSAPI.cpp for consistency.  Sorry to be
such a pest about naming conventions, but I think it helps make the
code more friendly to new-comers.


>+typedef OM_uint32 (GSS_CALLCONV *  gss_display_status_type)(

Why don't you #define GSS_USE_FUNCTION_POINTERS before #include'ing
gssapi.h so that you can avoid these redundant typedefs here?


>+            "gssapi32"

Should this be #ifdef XP_WIN ??


>+        errorStr.Append((char *) status1_string.value, status1_string.length);
...
>+        errorStr.Append((const char *) status2_string.value, status2_string.length);

Nit: be consistent with the use of |const| in the casting


>+    {
>+      for (i=0; i<mech_set->count; i++) {

Indentation is 4 white space characters; please keep the code
consistent.


>Index: extensions/auth/nsAuthSSPI.cpp

>+   if (SEC_SUCCESS(rc))  
>+   {
>+       *outToken = ib[1].pvBuffer;
>+       *outTokenLen = ib[1].cbBuffer;
>+   }
>+   else
>+   {
>+       nsMemory::Free(ib[1].pvBuffer);
>+   }

Please use a style for brackets that is consistent with existing
code.  For example, I believe the above code would be better
formatted like this:

   if (SEC_SUCCESS(rc)) {
       *outToken = ib[1].pvBuffer;
       *outTokenLen = ib[1].cbBuffer;
   }
   else
       nsMemory::Free(ib[1].pvBuffer);


>+   nsMemory::Free(ib[0].pvBuffer);
>+
>+   return rc;

nit: indentation is 4 white spaces; please be consistent


>+// utility class used to free memory on exit
>+class secBuffers
>+{
>+public:
>+
>+    SecBuffer ib[3];
>+
>+    secBuffers() {memset(&ib, 0, sizeof(ib));}

add some whitespace around memset call please


>+
>+   ~secBuffers()
>+    {
>+       if (ib[0].pvBuffer)
>+        nsMemory::Free(ib[0].pvBuffer);

fix indentation


>+    if (!SEC_SUCCESS(rc))  
>+    {
>+        return rc;
>+    }

drop brackets for consistency with style of existing code


>+    if (SEC_SUCCESS(rc))
>+    {
>+       int len  = bufs.ib[0].cbBuffer + bufs.ib[1].cbBuffer + bufs.ib[2].cbBuffer;
>+
>+        *outToken = nsMemory::Alloc(len);

fix indentation and use of brackets


>Index: extensions/auth/nsHttpNegotiateAuth.cpp

>     if (NS_FAILED(rv))
>+    {
>+        LOG(("  Failed to load Negotiate Module \n"));
>         return rv;
>+    }

Again, please use consistent bracket style


>Index: netwerk/base/public/nsIAuthModule.idl

>     void getNextToken([const] in voidPtr  aInToken,
>-                      in unsigned long    aInTokenLength,
>-                      out voidPtr         aOutToken,
>-                      out unsigned long   aOutTokenLength);
>+                          in unsigned long    aInTokenLength,
>+                          out voidPtr         aOutToken,
>+                          out unsigned long   aOutTokenLength);

This indentation change seems bogus to me.  please revert.


>+    void wrap([const] in voidPtr aInToken,
>+                  in unsigned long   aInTokenLength,
>+                  in boolean         confidential, 
>+                  out voidPtr        aOutToken,
>+                  out unsigned long  aOutTokenLength);

fix indentation


r=darin with these issues resolved.
Attachment #192364 - Flags: review?(darin) → review+
Most nits addressed
Attachment #192364 - Attachment is obsolete: true
Attachment #192510 - Attachment is obsolete: true
(In reply to comment #78)

> Why is this file called nsAuthModuleGSSAPI.cpp when the others are 
> called nsAuthSSPI.cpp and nsAuthSASL.cpp? 

Not fixed yet.  Do we need chase to fix this so we don't loose cvs history?

> 
> >+typedef OM_uint32 (GSS_CALLCONV *  gss_display_status_type)(
> 
> Why don't you #define GSS_USE_FUNCTION_POINTERS before #include'ing
> gssapi.h so that you can avoid these redundant typedefs here?
> 

Wow, because I didn't notice it. fixed.

I folded in the allmakefiles.in patch.

I added 2 lines which will force the auth lib to be included in packaging for
windows and unix instead of negotiateauth for installers.

bienvenu can you have a quick look and make sure I didn't do anything that would
stop it from compiling in windows?  I've tested in on Linux.

>Do we need chase to fix this so we don't loose cvs history?
Yes, Chase or someone on the sysadmin list will need to do this. I'll make that
happen.

When I roll these changes into my tree, I'll make sure everything compiles and
runs on Windows.

thx again for all your help, Christopher!
Depends on: 304849
bug 304849 filed for cvs rename issue. I will integrate the latest changes for
this bug and bug 303160 into my tree tomorrow and try to get it checked into the
trunk.
No longer depends on: 304849
Depends on: 304849
fixed on trunk
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Depends on: 305305

        Attachment #193311 -
        Flags: superreview?(mscott)

    
  

        Attachment #193311 -
        Flags: review?(cneberg)

    
    

    
  
browser/installer/unix/packages-static still has the old library name in two places

It looks like cvs copied files (comment 54) in mozilla/extensions/negotiateauth/
haven't been cvs removed.  They should be.  People will get confused.

    
    

    
  
Comment on attachment 193311 [details] [diff] [review]
fix build bustage when pr logging turned off

Could you fix the packages-static file with this check in to?

Thanks

        Attachment #193311 -
        Flags: review?(cneberg) → review+

    
    

    
  
both fixes checked in.

    
    

    
  
What are the chances of getting branch approval for this and the sasl patch?

    
    

    
  
I've asked for permission but I haven't had any response.

    
  

        Attachment #193311 -
        Flags: superreview?(mscott) → superreview+

    
  
Keywords: fixed1.8

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: