295109 - support pr-loading gssapi libraries for negotiateauth

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Christopher Nebergall]

I'm working on a patch to support dynamically loading a gssapi library so MIT,
heimdal, and Sun Kerberos implementations can all be supported from mozilla.org
binaries. It will also allow a user to manually select their kerberos library if
they choose.

Comment 1

[Christopher Nebergall]

Attachment: early version

Early Version 

Todo:
1.  Move prloading into constructor and set an initialized variable to check
for in init
2.  Can I make the library and function variables static members of the
negotiateauth class?
3.  Close Library (in destructor?)
4.  Check for various other kerberos libraries besides MIT
5.  create pref to let user set path to desired kerberos library

Comment 2

[Christopher Nebergall]

Attachment: updated

Almost complete.   
Missing configure.in and final makefile.in changes.

At the very least we don't need the path to the GSS Libs any more but we do
want the path to the header files.

How standard is the gssapi.h header file?  Could we just put a copy in the
negotiateauth directory so there are no external build time requirements? 

I'm not sure what to do about GSS_C_NT_HOSTBASED_SERVICE vs 
gss_nt_service_name, because one of them should now be selected at runtime. 
Could I try loading gss_nt_service_name from the gssapi library and use it only
if it is found?  I'm not sure.	It also may be defined in the header file
instead.  Where is one used rather than the other?

Comment 3

[Christopher Nebergall]

Attachment: updated with Mac OS X change

Forgot mac OS X specific change.  If we aren't running the native libs don't
use the Mac specific calls to check if we have tgt.

Comment 4

[Christopher Nebergall]

Note to self: gss_indicate_mechs shouldn't be called if initGssFun fails
Spelling: Unbuntu->Ubuntu

Comment 5

[Christopher Nebergall]

Attachment: updated

configure.in and all.js changes to come in separate patches

Comment 6

[Darin Fisher]

How many GSSAPI implementations have you tested?

Comment 7

[Christopher Nebergall]

On Unbuntu Linux
MIT 1.2.8
MIT 1.3.6-1  - Unbuntu Package
Heimdal 0.6.3-7Unbuntu

I could use some help testing in other OS's.

Comment 8

[Darin Fisher]

Wyllys: can you help test Solaris?

Comment 9

[Wyllys Ingersoll]

Yes, I will try and test on Solaris 10 and report back here.  Its been a while
since I downloaded and built my own Mozilla code, but I'll give it a shot :)

Comment 10

[Christopher Nebergall]

Attachment: configure.in changes

promised configure.in changes.	The pervious patch actually works fine without
these changes.	I attempted to remove all traces of us looking for or testing
gssapi libraries, and just tried to find the header files instead. This would
have the downside of not allowing us to check the sanity of the gssapi header
files with a test compile during the configure step.  The up side would be that
fixing bugs with hemdial keeping its header files and libraries in separte
locations would be much easier to fix.

Comment 11

[Christopher Nebergall]

Attachment: all.js diff

Small patch to add an empty pref to all.js for loading gssapi libs.

Comment 12

[Wyllys Ingersoll]

I patched the latest CVS code with your patches (all of them) and compiled it on 
Solaris 10 (SPARC).   It worked great.  I was able to specify the Solaris
GSSAPI library (/usr/lib/libgss.so.1) and successfully access a KRB5 
protected page.  Then, I restarted the browser and switched the setting 
to use the MIT KRB5 library (/usr/local/lib/libgssapi_krb5.so) and that 
also worked.

Nice job, Chris.

Comment 13

[Darin Fisher]

I plan to review these patches tomorrow.  Thanks for testing Solaris, Wyllys.

Comment 14

[Darin Fisher]

Comment on attachment 184900 [details] [diff] [review]
updated

>Index: nsNegotiateAuthGSSAPI.cpp

>+//We define GSS_C_NT_HOSTBASED_SERVICE explicitly since it may be referenced by
>+//by a different name depending on the implementation of gss but always
>+//has the same value

nit: please insert a space between comment and text.


>+static gss_OID_desc gss_c_nt_hostbased_service = {10, (void *)"\x2a\x86\x48\x86\xf7\x12\x01\x02\x01\x04"};

nit: please honor 80 character line width.


>+static const char * funcStr[] = {"gss_display_status", "gss_init_sec_context", 
>+                    "gss_indicate_mechs", "gss_release_oid_set",
>+                    "gss_delete_sec_context", "gss_import_name",
>+                    "gss_release_buffer", "gss_release_name"};

nit: please format like this instead

  static const char * funcStr[] = {
    "gss_display_status",
    "gss_init_sec_context", 
    "gss_indicate_mechs",
    "gss_release_oid_set",
    "gss_delete_sec_context",
    "gss_import_name",
    "gss_release_buffer",
    "gss_release_name"
  };


>+#define funcItems 8

nit: use NS_ARRAY_LENGTH(funcStr) so you don't have to hardcode this value.


nit: maybe prefix funcStr and funcItems with "gss" for consistency?


>+#define _pr_gss_display_status ((gss_display_status_type)*gssFunPtr[0])
>+#define _pr_gss_init_sec_context ((gss_init_sec_context_type)*gssFunPtr[1])
>+#define _pr_gss_indicate_mechs ((gss_indicate_mechs_type)*gssFunPtr[2])
>+#define _pr_gss_release_oid_set ((gss_release_oid_set_type)*gssFunPtr[3])
>+#define _pr_gss_delete_sec_context ((gss_delete_sec_context_type)*gssFunPtr[4])
>+#define _pr_gss_import_name ((gss_import_name_type)*gssFunPtr[5])
>+#define _pr_gss_release_buffer ((gss_release_buffer_type)*gssFunPtr[6])
>+#define _pr_gss_release_name ((gss_release_name_type)*gssFunPtr[7])

nit: the "_pr" prefix is reserved for use inside the NSPR implementation.


>+static NS_IMETHODIMP
>+initGssFun()

nit: gssInit ?


>+  LOG(("entering loadFuncPtrs()\n"));

nit: comment seems out of date.


>+  char * gssLib = NULL;

nit: space before or after "*" but not both please :)


>+  if (prefs && NS_SUCCEEDED(prefs->GetCharPref(kNegotiateAuthGssLib, &gssLib)) 
>+      && gssLib && *gssLib) {

when calling GetCharPref from C++, it is best to use nsXPIDLCString like so:

  nsXPIDLCString str;
  prefs->GetCharPref("blah", getter_Copies(str));
  if (!str.IsEmpty()) {
    ...
    DoStuff(str.get());
  }

Using this class makes it so that you don't have to explicitly
free the memory buffer when you are done with it.


>+  else {
>+
>+    char *libName = NULL;
>+
>+     // SUN
>+     libName = PR_GetLibraryName(NULL, "gss");
>+     LOG(("Attempting to load %s Sun Library\n",libName));
>+     mGssLib = PR_LoadLibrary(libName);

nit: please stick to consistent indentation (2 white spaces)


>+
>+     // MIT
>+     if (!mGssLib) {
>+        libName = PR_GetLibraryName(NULL, "gssapi_krb5");
>+        LOG(("Attempting to load %s MIT Library\n",libName));
>+        mGssLib = PR_LoadLibrary(libName);
>+     }
>+
>+     // Heimdal 
>+     if (!mGssLib) {
>+        libName = PR_GetLibraryName(NULL, "gssapi");
>+        LOG(("Attempting to load %s Heimdal Library\n",libName));
>+        mGssLib = PR_LoadLibrary(libName);
>+     }

don't you need to free the memory buffer returned by PR_GetLibraryName?

also, this code might be better as a loop:

  const char *libnames[] = {
    "gss",
    "gssapi_krb5",
    "gssapi"
  };
  for (int i = 0; i < NS_ARRAY_LENGTH(libnames); ++i) {
    char *libname = PR_GetLibraryName(NULL, libnames[i]);
    if (libname) {
      mGssLib = PR_LoadLibrary(libname);
      PR_Free(libname);
    }
  }


> #if defined(XP_MACOSX)
>     // Suppress Kerberos prompts to get credentials.  See bug 240643.
>+    // We can only use Mac OS X specific kerb functions if we are using the native lib
>     KLBoolean found;
>+    if (gssNativeLib && (KLCacheHasValidTickets(NULL, kerberosVersion_V5, &found, NULL, NULL) != klNoErr || !found))

Hmm... our searching could find a non-native lib though, right?

Comment 15

[Christopher Nebergall]

Attachment: updated per comments

Updated per review.  Having to have a special pref just to fix an anomaly with
the OS X kerberos implementation is a bummer.

Comment 16

[Christopher Nebergall]

Attachment: updated all.js with new pref

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

so why is it a problem to call that function if the impl is nonnative?

Comment 18

[Christopher Nebergall]

I was afraid that the system call KLCacheHasValidTickets may end up checking a
different or incompatible credentials cache than what would be used by the
non-native gss implementation. 

The discussion at the link below states that most mac apps don't support
KRB5CCNAME to specify the cache location and it sounds as if it may not be
possible to force the system calls to use a non memory cache. No other gss
implemenation currently supports memory caches on OS X.

http://mailman.mit.edu/pipermail/krbdev/2002/000720.html

Comment 19

[Darin Fisher]

Comment on attachment 185992 [details] [diff] [review]
updated per comments

There are some formatting inconsistencies, but otherwise this
patch looks great to me.  r=darin

I'll fixup the format before checking this in.

Requesting approval since this will help make our GSSAPI code
work on more platforms.  I'd like to get this tested in the
next Firefox alpha if possible.

Comment 20

[Christopher Nebergall]

Darin, do you want this module to have the option of being complied statically
now?  The current Makefile.in still forbids it.

Comment 21

[Darin Fisher]

Yes, I think we should now compile this module statically.

Comment 22

[Christopher Nebergall]

Attachment: only changes are to makefile.in to allow static compile

Allow static compile everything else is the same as patch 185992

Comment 23

[Christopher Nebergall]

Darin are you waiting on me to get an sr?  If so who would be a good person to ask?

Comment 24

[Darin Fisher]

Comment on attachment 186649 [details] [diff] [review]
only changes are to makefile.in to allow static compile

looks good.  sorry for the delay.  i've been swamped.

requesting approval for Deer Park Alpha 2.  we want to maximize coverage for
this fix as it should impact a fairly small number of users, so it is important
to give ourselves enough time to gather possible reports of regressions.

Comment 25

[Darin Fisher]

fixed-on-trunk

Comment 26

[Mark Mentovai]

This caused bug 299305.

Comment 27

[Christopher Nebergall]

This also caused Bug 301030