Closed Bug 295109 Opened 19 years ago Closed 19 years ago

support pr-loading gssapi libraries for negotiateauth

Categories

(Core :: Networking, defect)

x86
Other
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: cneberg, Assigned: cneberg)

References

Details

Attachments

(3 files, 6 obsolete files)

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.
Attached patch early version (obsolete) — Splinter Review
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
Attached patch updated (obsolete) — Splinter Review
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?
Attachment #184237 - Attachment is obsolete: true
Attached patch updated with Mac OS X change (obsolete) — Splinter Review
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.
Attachment #184292 - Attachment is obsolete: true
Note to self: gss_indicate_mechs shouldn't be called if initGssFun fails
Spelling: Unbuntu->Ubuntu
Attached patch updated (obsolete) — Splinter Review
configure.in and all.js changes to come in separate patches
Attachment #184297 - Attachment is obsolete: true
Attachment #184900 - Flags: review?(darin)
How many GSSAPI implementations have you tested?
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.
Wyllys: can you help test Solaris?
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 :)
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.
Attachment #185667 - Flags: review?(darin)
Attached patch all.js diff (obsolete) — Splinter Review
Small patch to add an empty pref to all.js for loading gssapi libs.
Attachment #185668 - Flags: review?(darin)
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.




I plan to review these patches tomorrow.  Thanks for testing Solaris, Wyllys.
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?
Attachment #184900 - Flags: review?(darin) → review-
Attachment #185667 - Flags: review?(darin) → review+
Attachment #185668 - Flags: review?(darin) → review+
Attached patch updated per comments (obsolete) — Splinter Review
Updated per review.  Having to have a special pref just to fix an anomaly with
the OS X kerberos implementation is a bummer.
Attachment #184900 - Attachment is obsolete: true
Attachment #185992 - Flags: review?(darin)
Attachment #185668 - Attachment is obsolete: true
Attachment #185993 - Flags: review?(darin)
so why is it a problem to call that function if the impl is nonnative?
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 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.
Attachment #185992 - Flags: review?(darin)
Attachment #185992 - Flags: review+
Attachment #185992 - Flags: approval1.8b3?
Attachment #185992 - Flags: approval-aviary1.1a2?
Attachment #185993 - Flags: review?(darin) → review+
Attachment #185992 - Flags: approval1.8b3?
Attachment #185992 - Flags: approval1.8b3+
Attachment #185992 - Flags: approval-aviary1.1a2?
Attachment #185992 - Flags: approval-aviary1.1a2+
Darin, do you want this module to have the option of being complied statically
now?  The current Makefile.in still forbids it.
Yes, I think we should now compile this module statically.
Allow static compile everything else is the same as patch 185992
Attachment #185992 - Attachment is obsolete: true
Attachment #186649 - Flags: review?(darin)
Darin are you waiting on me to get an sr?  If so who would be a good person to ask?
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.
Attachment #186649 - Flags: review?(darin)
Attachment #186649 - Flags: review+
Attachment #186649 - Flags: approval1.8b3?
Attachment #186649 - Flags: approval-aviary1.1a2?
Attachment #186649 - Flags: approval1.8b3?
Attachment #186649 - Flags: approval1.8b3+
Attachment #186649 - Flags: approval-aviary1.1a2?
fixed-on-trunk
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
This caused bug 299305.
Blocks: 299305
This also caused Bug 301030 
You need to log in before you can comment on or make changes to this bug.