Closed
Bug 295109
Opened 20 years ago
Closed 20 years ago
support pr-loading gssapi libraries for negotiateauth
Categories
(Core :: Networking, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: cneberg, Assigned: cneberg)
References
Details
Attachments
(3 files, 6 obsolete files)
3.66 KB,
patch
|
darin.moz
:
review+
|
Details | Diff | Splinter Review |
965 bytes,
patch
|
darin.moz
:
review+
|
Details | Diff | Splinter Review |
11.73 KB,
patch
|
darin.moz
:
review+
benjamin
:
approval1.8b3+
|
Details | Diff | Splinter Review |
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•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
Attachment #184237 -
Attachment is obsolete: true
Assignee | ||
Comment 3•20 years ago
|
||
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•20 years ago
|
||
Note to self: gss_indicate_mechs shouldn't be called if initGssFun fails
Spelling: Unbuntu->Ubuntu
Assignee | ||
Comment 5•20 years ago
|
||
configure.in and all.js changes to come in separate patches
Attachment #184297 -
Attachment is obsolete: true
Attachment #184900 -
Flags: review?(darin)
Comment 6•20 years ago
|
||
How many GSSAPI implementations have you tested?
Assignee | ||
Comment 7•20 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•20 years ago
|
||
Wyllys: can you help test Solaris?
Comment 9•20 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•20 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•20 years ago
|
||
Small patch to add an empty pref to all.js for loading gssapi libs.
Attachment #185668 -
Flags: review?(darin)
Comment 12•20 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•20 years ago
|
||
I plan to review these patches tomorrow. Thanks for testing Solaris, Wyllys.
Comment 14•20 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•20 years ago
|
Attachment #185667 -
Flags: review?(darin) → review+
Updated•20 years ago
|
Attachment #185668 -
Flags: review?(darin) → review+
Assignee | ||
Comment 15•20 years ago
|
||
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•20 years ago
|
Attachment #184900 -
Attachment is obsolete: true
Attachment #185992 -
Flags: review?(darin)
Assignee | ||
Comment 16•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #185668 -
Attachment is obsolete: true
Attachment #185993 -
Flags: review?(darin)
Comment 17•20 years ago
|
||
so why is it a problem to call that function if the impl is nonnative?
Assignee | ||
Comment 18•20 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•20 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•20 years ago
|
Attachment #185993 -
Flags: review?(darin) → review+
Updated•20 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•20 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•20 years ago
|
||
Yes, I think we should now compile this module statically.
Assignee | ||
Comment 22•20 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•20 years ago
|
||
Darin are you waiting on me to get an sr? If so who would be a good person to ask?
Comment 24•20 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•20 years ago
|
Attachment #186649 -
Flags: approval1.8b3?
Attachment #186649 -
Flags: approval1.8b3+
Attachment #186649 -
Flags: approval-aviary1.1a2?
Comment 25•20 years ago
|
||
fixed-on-trunk
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 26•20 years ago
|
||
This caused bug 299305.
Assignee | ||
Comment 27•20 years ago
|
||
This also caused Bug 301030
You need to log in
before you can comment on or make changes to this bug.
Description
•