support pr-loading gssapi libraries for negotiateauth

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: cneberg, Assigned: cneberg)

Tracking

Trunk
x86
Other
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Assignee)

Description

14 years ago
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.
(Assignee)

Comment 1

14 years ago
Posted 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
(Assignee)

Comment 2

14 years ago
Posted 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?
(Assignee)

Updated

14 years ago
Attachment #184237 - Attachment is obsolete: true
(Assignee)

Comment 3

14 years ago
Posted 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
(Assignee)

Comment 4

14 years ago
Note to self: gss_indicate_mechs shouldn't be called if initGssFun fails
Spelling: Unbuntu->Ubuntu
(Assignee)

Comment 5

14 years ago
Posted 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)

Comment 6

14 years ago
How many GSSAPI implementations have you tested?
(Assignee)

Comment 7

14 years ago
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

14 years ago
Wyllys: can you help test Solaris?

Comment 9

14 years ago
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 :)
(Assignee)

Comment 10

14 years ago
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)
(Assignee)

Comment 11

14 years ago
Posted 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)

Comment 12

14 years ago
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

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

Comment 14

14 years ago
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-

Updated

14 years ago
Attachment #185667 - Flags: review?(darin) → review+

Updated

14 years ago
Attachment #185668 - Flags: review?(darin) → review+
(Assignee)

Comment 15

14 years ago
Posted 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.
(Assignee)

Updated

14 years ago
Attachment #184900 - Attachment is obsolete: true
Attachment #185992 - Flags: review?(darin)
(Assignee)

Updated

14 years ago
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?
(Assignee)

Comment 18

14 years ago
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

14 years ago
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?

Updated

14 years ago
Attachment #185993 - Flags: review?(darin) → review+

Updated

14 years ago
Attachment #185992 - Flags: approval1.8b3?
Attachment #185992 - Flags: approval1.8b3+
Attachment #185992 - Flags: approval-aviary1.1a2?
Attachment #185992 - Flags: approval-aviary1.1a2+
(Assignee)

Comment 20

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

Comment 21

14 years ago
Yes, I think we should now compile this module statically.
(Assignee)

Comment 22

14 years ago
Allow static compile everything else is the same as patch 185992
Attachment #185992 - Attachment is obsolete: true
Attachment #186649 - Flags: review?(darin)
(Assignee)

Comment 23

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

Comment 24

14 years ago
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?

Updated

14 years ago
Attachment #186649 - Flags: approval1.8b3?
Attachment #186649 - Flags: approval1.8b3+
Attachment #186649 - Flags: approval-aviary1.1a2?

Comment 25

14 years ago
fixed-on-trunk
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 26

14 years ago
This caused bug 299305.

Updated

14 years ago
Blocks: 299305
(Assignee)

Comment 27

14 years ago
This also caused Bug 301030 
You need to log in before you can comment on or make changes to this bug.