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.

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.

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.

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.

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.

(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.

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.

These are all of the supporting files out side of extentions/auth made as a real cvs diff.

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.

Thunderbird has platform-specific prefs - see http://lxr.mozilla.org/mozilla/source/mail/app/profile/all-thunderbird.js#449

(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! ;)

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?

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

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.

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.

change name of extension...

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.

Most nits addressed

(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!

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.

fixed on trunk

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

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.