Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1736009:
Preliminary support for GSSAPI (Linux and Mac OS X). (Closed)
|
Created:
10 years, 8 months ago by ahendrickson Modified:
9 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, John Grabowski, Paweł Hajdan Jr., pam+watch_chromium.org, brettw-cc_chromium.org, darin-cc_chromium.org Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionPreliminary support for GSSAPI (Linux and Mac OS X).
Second CL.
BUG=33033
.
TEST=None.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=48945
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #Patch Set 8 : '' #
Total comments: 10
Patch Set 9 : '' #
Total comments: 31
Patch Set 10 : '' #Patch Set 11 : '' #Patch Set 12 : '' #
Total comments: 16
Patch Set 13 : '' #
Total comments: 6
Patch Set 14 : Fixed hang bug when GSSAPI lib is not present. #Patch Set 15 : Submitting from correct branch now . . . #Patch Set 16 : Cleanup. #
Total comments: 26
Patch Set 17 : More cleanup, and Native Library error message improvements. #
Total comments: 45
Patch Set 18 : Suggested changes. #Patch Set 19 : Removed Native Library changes. #Patch Set 20 : Removed remainder of Mac changes to Native Library. #
Total comments: 5
Patch Set 21 : Removed Native Library changes. #
Total comments: 20
Patch Set 22 : Bug and style fixes. #
Total comments: 9
Messages
Total messages: 27 (0 generated)
This is the smaller initial code.
Added a missing source file.
Sigh. Removed extraneous source file.
Quick first draft of comments. http://codereview.chromium.org/1736009/diff/61001/46009 File base/native_library_linux.cc (right): http://codereview.chromium.org/1736009/diff/61001/46009#newcode48 base/native_library_linux.cc:48: void* pv = dlsym(library, name); I'm not sure if you should be adding the LOG(ERROR) in this case. Some clients may actually expect that dlsym will return NULL (i.e. for an optional feature). Please go through all callers and see if this is the case. Also, you can probably DCHECK that name is not NULL - it seems like a programming mistake rather than a LOG(ERROR). http://codereview.chromium.org/1736009/diff/61001/46012 File net/http/http_auth_gssapi_posix.h (right): http://codereview.chromium.org/1736009/diff/61001/46012#newcode10 net/http/http_auth_gssapi_posix.h:10: #include "base/native_library.h" I don't think this is needed in the header file. http://codereview.chromium.org/1736009/diff/61001/46012#newcode16 net/http/http_auth_gssapi_posix.h:16: #include "net/http/http_auth.h" I don't think this is used either. http://codereview.chromium.org/1736009/diff/61001/46014 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/1736009/diff/61001/46014#newcode8 net/http/http_network_transaction.cc:8: #include <set> This shouldn't be part of this CL. Also - why is it needed? Is it an "include what you use" lint warning?
http://codereview.chromium.org/1736009/diff/61001/46009 File base/native_library_linux.cc (right): http://codereview.chromium.org/1736009/diff/61001/46009#newcode48 base/native_library_linux.cc:48: void* pv = dlsym(library, name); On 2010/05/07 18:09:17, cbentzel wrote: > I'm not sure if you should be adding the LOG(ERROR) in this case. Some clients > may actually expect that dlsym will return NULL (i.e. for an optional feature). > Please go through all callers and see if this is the case. > > Also, you can probably DCHECK that name is not NULL - it seems like a > programming mistake rather than a LOG(ERROR). Done. http://codereview.chromium.org/1736009/diff/61001/46012 File net/http/http_auth_gssapi_posix.h (right): http://codereview.chromium.org/1736009/diff/61001/46012#newcode10 net/http/http_auth_gssapi_posix.h:10: #include "base/native_library.h" On 2010/05/07 18:09:17, cbentzel wrote: > I don't think this is needed in the header file. Done. http://codereview.chromium.org/1736009/diff/61001/46012#newcode16 net/http/http_auth_gssapi_posix.h:16: #include "net/http/http_auth.h" On 2010/05/07 18:09:17, cbentzel wrote: > I don't think this is used either. We need this because we use HttpAuth::ChallengeTokenizer, and I can't seem to forward-declare it. http://codereview.chromium.org/1736009/diff/61001/46014 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/1736009/diff/61001/46014#newcode8 net/http/http_network_transaction.cc:8: #include <set> On 2010/05/07 18:09:17, cbentzel wrote: > This shouldn't be part of this CL. Also - why is it needed? Is it an "include > what you use" lint warning? OK, I'll remove it. It actually is from a lint warning, and brings this file closer to conformance with our coding style.
http://codereview.chromium.org/1736009/diff/61001/46014 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/1736009/diff/61001/46014#newcode8 net/http/http_network_transaction.cc:8: #include <set> So this is worth doing but should be part of a different CL (it doesn't really have anything to do with the current one).
http://codereview.chromium.org/1736009/diff/61001/46014 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/1736009/diff/61001/46014#newcode8 net/http/http_network_transaction.cc:8: #include <set> On 2010/05/07 18:51:31, cbentzel wrote: > So this is worth doing but should be part of a different CL (it doesn't really > have anything to do with the current one). Agreed.
Can you change this CL to assume that the third_party gssapi change will be approved?
Andy, Thanks for the CL! Does this have enough code to work? Please see my comments below. http://codereview.chromium.org/1736009/diff/66001/67001 File base/native_library_linux.cc (right): http://codereview.chromium.org/1736009/diff/66001/67001#newcode49 base/native_library_linux.cc:49: void* pv = dlsym(library, name); Nit: in general our style guide discourages variable names that are abbreviations, such as "pv", unless they're well-known. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming... http://codereview.chromium.org/1736009/diff/66001/67001#newcode52 base/native_library_linux.cc:52: LOG(WARNING) << "dlsym failed when finding function pointer for " << name Not being able to find a symbol may be expected in some cases. An example is to look up a function that exists in only the latest versions of the OS. So the symbol lookup is expected to fail when running on older versions of the OS. So it seems wrong to always log a warning message when dlsym fails. In fact, I believe this is similar to the reason why we make exceptions for libxul.so and libxpcom.so at lines 26-27 above -- some dlopen failures may be expected. It seems better to let the caller decide if it wants to log a message on dlopen/dlsym failure. http://codereview.chromium.org/1736009/diff/66001/67003 File net/http/http_auth_gssapi_posix.cc (right): http://codereview.chromium.org/1736009/diff/66001/67003#newcode41 net/http/http_auth_gssapi_posix.cc:41: static const char* kLibraryNames[]; Nit: since kLibraryNames is only used by the LoadSharedObject method, you can move kLibraryNames into that method. http://codereview.chromium.org/1736009/diff/66001/67003#newcode178 net/http/http_auth_gssapi_posix.cc:178: static const int kMaxDisplayIterations = 8; Nit: static is not necessary inside an unnamed namespace. Move this constant into DisplayCode because it's only used in that function. http://codereview.chromium.org/1736009/diff/66001/67003#newcode180 net/http/http_auth_gssapi_posix.cc:180: std::string DisplayCode(gss_display_status_type display_status, Instead of taking individual function pointers, these functions can just take a GSSAPILibrary* pointer. http://codereview.chromium.org/1736009/diff/66001/67003#newcode307 net/http/http_auth_gssapi_posix.cc:307: return true; Why do we return true here? Is it because the very first "WWW-Authenticate: Negotiate" header has no auth token? http://codereview.chromium.org/1736009/diff/66001/67003#newcode323 net/http/http_auth_gssapi_posix.cc:323: 0x2a, 0x86, 0x48, 0x86, 0xF7, 0x12, 0x01, 0x02, 0x01, 0x04}; Nit: put the closing curly brace '}' on the next line. Similarly for the closing curly brace '}' on line 326 below. kHostbasedServiceOidElements and kHostbasedServiceOidDesc should be moved into the unnamed namespace above or the GetNextSecurityToken method. I believe kHostbasedServiceOidDesc is equivalent to GSS_C_NT_HOSTBASED_SERVICE. Why don't you use GSS_C_NT_HOSTBASED_SERVICE? http://codereview.chromium.org/1736009/diff/66001/67003#newcode338 net/http/http_auth_gssapi_posix.cc:338: if (!IsFinalRound()) { This assumes that a Negotiate auth sequence consists of exactly two rounds, right? Ideally we should not depend on such an assumption, but this is fine. http://codereview.chromium.org/1736009/diff/66001/67003#newcode346 net/http/http_auth_gssapi_posix.cc:346: input_token.value = static_cast<void *>(const_cast<char *>( Remove static_cast<void *>. It is not necessary to cast a pointer to void*. Similarly for line 379 below. Since decoded_server_auth_token_ holds binary data, we should use .data() intead of .c_str() on the next line. http://codereview.chromium.org/1736009/diff/66001/67003#newcode352 net/http/http_auth_gssapi_posix.cc:352: ScopedBuffer sb_out_token(&output_token, library_->release_buffer()); Nit: the "sb" prefix is a little cryptic. Perhaps just use "scoped"? http://codereview.chromium.org/1736009/diff/66001/67003#newcode382 net/http/http_auth_gssapi_posix.cc:382: OM_uint32 minor_status_import_name = 0; Since you call one method at a time, you can reuse the minor_status/major_status variables for the next call, and therefore they don't need to have the _import_name or _sec_context suffix. http://codereview.chromium.org/1736009/diff/66001/67004 File net/http/http_auth_gssapi_posix.h (right): http://codereview.chromium.org/1736009/diff/66001/67004#newcode10 net/http/http_auth_gssapi_posix.h:10: //#include "base/native_library.h" Please remove this line. It's not necessary to undefine GSS_USE_FUNCTION_POINTERS at line 14 below. http://codereview.chromium.org/1736009/diff/66001/67004#newcode25 net/http/http_auth_gssapi_posix.h:25: class GSSAPILibrary { Nit: add a short comment to describe what this class is. (It's not hard to figure it out though.) API DESIGN: comparing this class with the SSPILibrary class in http_auth_sspi_win.h, I see a clear difference: this class's virtual methods return function pointers, where as SSPILibrary's virtual methods do actual work. I think we should eliminate this inconsistency. With the current GSSAPILibrary interface, it actually has double indirection -- virtual method + function pointer, which is not necessary. http://codereview.chromium.org/1736009/diff/66001/67004#newcode46 net/http/http_auth_gssapi_posix.h:46: const gss_OID gss_oid, This const is not necessary. gss_OID is a pointer type. I believe this const applies to the pointer rather than the object it points to. Nit: remove the blank line at line 63 below. http://codereview.chromium.org/1736009/diff/66001/67006 File net/net.gyp (right): http://codereview.chromium.org/1736009/diff/66001/67006#newcode326 net/net.gyp:326: 'http/http_auth_gssapi_posix.cc', Consider removing _posix from the file names. I see nothing in these files that can't be compiled on Windows. (I know you modeled the file names after http_auth_sspi_win.{h,cc}.)
Please note the bug number (33033) in the Description of this CL.
wtc: This won't work as is (the http_auth_handler_negotiate_posix will need to be written to use the http_auth_gssapi stuff). This will need to be merged with the net/third_party/gssapi change. I'll take a more detailed look after that's complete.
http://codereview.chromium.org/1736009/diff/66001/67001 File base/native_library_linux.cc (right): http://codereview.chromium.org/1736009/diff/66001/67001#newcode49 base/native_library_linux.cc:49: void* pv = dlsym(library, name); On 2010/05/18 23:59:19, wtc wrote: > Nit: in general our style guide discourages variable names > that are abbreviations, such as "pv", unless they're > well-known. See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming... Done. http://codereview.chromium.org/1736009/diff/66001/67001#newcode52 base/native_library_linux.cc:52: LOG(WARNING) << "dlsym failed when finding function pointer for " << name On 2010/05/18 23:59:19, wtc wrote: > Not being able to find a symbol may be expected in some > cases. An example is to look up a function that exists in > only the latest versions of the OS. So the symbol lookup is > expected to fail when running on older versions of the OS. > > So it seems wrong to always log a warning message when > dlsym fails. > > In fact, I believe this is similar to the reason why we > make exceptions for libxul.so and libxpcom.so at lines > 26-27 above -- some dlopen failures may be expected. > > It seems better to let the caller decide if it wants to > log a message on dlopen/dlsym failure. Added a function to retrieve the last error message. http://codereview.chromium.org/1736009/diff/66001/67003 File net/http/http_auth_gssapi_posix.cc (right): http://codereview.chromium.org/1736009/diff/66001/67003#newcode41 net/http/http_auth_gssapi_posix.cc:41: static const char* kLibraryNames[]; On 2010/05/18 23:59:19, wtc wrote: > Nit: since kLibraryNames is only used by the LoadSharedObject > method, you can move kLibraryNames into that method. Done. Moved to an internal method, as another function (for unit testing) uses it too. http://codereview.chromium.org/1736009/diff/66001/67003#newcode178 net/http/http_auth_gssapi_posix.cc:178: static const int kMaxDisplayIterations = 8; On 2010/05/18 23:59:19, wtc wrote: > Nit: static is not necessary inside an unnamed namespace. > > Move this constant into DisplayCode because it's only used > in that function. Done. http://codereview.chromium.org/1736009/diff/66001/67003#newcode180 net/http/http_auth_gssapi_posix.cc:180: std::string DisplayCode(gss_display_status_type display_status, On 2010/05/18 23:59:19, wtc wrote: > Instead of taking individual function pointers, these functions > can just take a GSSAPILibrary* pointer. Done. http://codereview.chromium.org/1736009/diff/66001/67003#newcode323 net/http/http_auth_gssapi_posix.cc:323: 0x2a, 0x86, 0x48, 0x86, 0xF7, 0x12, 0x01, 0x02, 0x01, 0x04}; On 2010/05/18 23:59:19, wtc wrote: > Nit: put the closing curly brace '}' on the next line. > > Similarly for the closing curly brace '}' on line 326 below. > > kHostbasedServiceOidElements and kHostbasedServiceOidDesc > should be moved into the unnamed namespace above or > the GetNextSecurityToken method. > > I believe kHostbasedServiceOidDesc is equivalent to > GSS_C_NT_HOSTBASED_SERVICE. Why don't you use > GSS_C_NT_HOSTBASED_SERVICE? Using GSS_C_NT_HOSTBASED_SERVICE instead. http://codereview.chromium.org/1736009/diff/66001/67003#newcode346 net/http/http_auth_gssapi_posix.cc:346: input_token.value = static_cast<void *>(const_cast<char *>( On 2010/05/18 23:59:19, wtc wrote: > Remove static_cast<void *>. It is not necessary to cast a > pointer to void*. > > Similarly for line 379 below. > > Since decoded_server_auth_token_ holds binary data, we should > use .data() intead of .c_str() on the next line. Done. http://codereview.chromium.org/1736009/diff/66001/67003#newcode352 net/http/http_auth_gssapi_posix.cc:352: ScopedBuffer sb_out_token(&output_token, library_->release_buffer()); On 2010/05/18 23:59:19, wtc wrote: > Nit: the "sb" prefix is a little cryptic. Perhaps just > use "scoped"? Done. http://codereview.chromium.org/1736009/diff/66001/67003#newcode382 net/http/http_auth_gssapi_posix.cc:382: OM_uint32 minor_status_import_name = 0; On 2010/05/18 23:59:19, wtc wrote: > Since you call one method at a time, you can reuse the > minor_status/major_status variables for the next call, > and therefore they don't need to have the _import_name > or _sec_context suffix. Done. http://codereview.chromium.org/1736009/diff/66001/67004 File net/http/http_auth_gssapi_posix.h (right): http://codereview.chromium.org/1736009/diff/66001/67004#newcode10 net/http/http_auth_gssapi_posix.h:10: //#include "base/native_library.h" On 2010/05/18 23:59:19, wtc wrote: > Please remove this line. > > It's not necessary to undefine GSS_USE_FUNCTION_POINTERS > at line 14 below. Done. http://codereview.chromium.org/1736009/diff/66001/67004#newcode25 net/http/http_auth_gssapi_posix.h:25: class GSSAPILibrary { On 2010/05/18 23:59:19, wtc wrote: > Nit: add a short comment to describe what this class is. > (It's not hard to figure it out though.) > > API DESIGN: comparing this class with the SSPILibrary class > in http_auth_sspi_win.h, I see a clear difference: this class's > virtual methods return function pointers, where as > SSPILibrary's virtual methods do actual work. > > I think we should eliminate this inconsistency. > With the current GSSAPILibrary interface, it actually > has double indirection -- virtual method + function pointer, > which is not necessary. Done. http://codereview.chromium.org/1736009/diff/66001/67006 File net/net.gyp (right): http://codereview.chromium.org/1736009/diff/66001/67006#newcode326 net/net.gyp:326: 'http/http_auth_gssapi_posix.cc', On 2010/05/18 23:59:19, wtc wrote: > Consider removing _posix from the file names. I see nothing > in these files that can't be compiled on Windows. > > (I know you modeled the file names after http_auth_sspi_win.{h,cc}.) I like this name because it *won't* be compiled on Windows.
http://codereview.chromium.org/1736009/diff/66001/67003 File net/http/http_auth_gssapi_posix.cc (right): http://codereview.chromium.org/1736009/diff/66001/67003#newcode41 net/http/http_auth_gssapi_posix.cc:41: static const char* kLibraryNames[]; On 2010/05/28 16:14:04, ahendrickson wrote: > > Moved to an internal method, as another function (for unit testing) uses it too. If it's used by multiple functions, it can be a static variable in the file scope. Would that simplify your code? Your new GetLibraryName function seems complicated, with the index range checking and internal state (the function-static 'index' and 'current_index' variables). http://codereview.chromium.org/1736009/diff/66001/67006 File net/net.gyp (right): http://codereview.chromium.org/1736009/diff/66001/67006#newcode326 net/net.gyp:326: 'http/http_auth_gssapi_posix.cc', On 2010/05/28 16:14:04, ahendrickson wrote: > > I like this name because it *won't* be compiled on Windows. I see. That may have been one reason Chris added _win to http_auth_sspi_win.{h,cc}. Just FYI: you can use the "sources!" variable in a GYP file to exclude some source files for a platform. http://codereview.chromium.org/1736009/diff/96001/97001 File base/native_library.h (right): http://codereview.chromium.org/1736009/diff/96001/97001#newcode69 base/native_library.h:69: string16 GetLibraryErrorMessage(); This function's name should contain "Loader", such as GetLibraryLoaderErrorMessage or GetNativeLibraryLoaderErrorMessage, to stress the fact that the error message is associated with the loader as opposed to some (unspecified) library. Please document this function. I'm not convinced that this function is useful, but I see no other way to get this info on Unix because of the <dlfcn.h> API. http://codereview.chromium.org/1736009/diff/96001/97003 File base/native_library_mac.mm (right): http://codereview.chromium.org/1736009/diff/96001/97003#newcode77 base/native_library_mac.mm:77: const char* last_err_message = dlerror(); The Mac implementation is a little risky because dlerror() can only be called when the last failed function used the <dlfcn.h> interface rather than the CFBundle interface. But it's impossible for the caller to tell.
Would it make sense to move the native library changes to a separate CL? http://codereview.chromium.org/1736009/diff/66001/67006 File net/net.gyp (right): http://codereview.chromium.org/1736009/diff/66001/67006#newcode326 net/net.gyp:326: 'http/http_auth_gssapi_posix.cc', On 2010/05/28 20:41:14, wtc wrote: > On 2010/05/28 16:14:04, ahendrickson wrote: > > > > I like this name because it *won't* be compiled on Windows. > > I see. That may have been one reason Chris added > _win to http_auth_sspi_win.{h,cc}. > > Just FYI: you can use the "sources!" variable in > a GYP file to exclude some source files for a > platform. Whereas the SSPI implementation definitely won't be built on non-Windows platforms, the GSSAPI implementation could be built on Windows. However, I have no problem with naming this _posix for now, and if behavior changes removing the _posix suffix. http://codereview.chromium.org/1736009/diff/96001/97005 File net/http/http_auth_gssapi_posix.cc (right): http://codereview.chromium.org/1736009/diff/96001/97005#newcode21 net/http/http_auth_gssapi_posix.cc:21: gss_OID_desc GSS_C_NT_USER_NAME_VAL = { Are any of these used at all? Do you need this due to gssapi.h? http://codereview.chromium.org/1736009/diff/96001/97005#newcode90 net/http/http_auth_gssapi_posix.cc:90: DCHECK(!initialized_); Should this immediately set initialized_ to true at the top instead of assuming the caller will? http://codereview.chromium.org/1736009/diff/96001/97006 File net/http/http_auth_gssapi_posix.h (right): http://codereview.chromium.org/1736009/diff/96001/97006#newcode32 net/http/http_auth_gssapi_posix.h:32: virtual gssapi::OM_uint32 import_name( You should separate out these methods and add a simple comment mention that these match the same named functions in the gssapi library. http://codereview.chromium.org/1736009/diff/96001/97006#newcode77 net/http/http_auth_gssapi_posix.h:77: class GSSAPISharedLibrary : public GSSAPILibrary { I think you could get away with putting this in the .cc file only. http://codereview.chromium.org/1736009/diff/96001/97006#newcode128 net/http/http_auth_gssapi_posix.h:128: Nit: unnecessary empty line. http://codereview.chromium.org/1736009/diff/96001/97006#newcode170 net/http/http_auth_gssapi_posix.h:170: Nit: unnecessary extra space. http://codereview.chromium.org/1736009/diff/105001/106005 File net/http/http_auth_gssapi_posix.cc (right): http://codereview.chromium.org/1736009/diff/105001/106005#newcode99 net/http/http_auth_gssapi_posix.cc:99: bool GSSAPISharedLibrary::HasSharedLibrary() { I really don't like the separation of GetLibraryName from HasSharedLibrary/LoadSharedObject. There's potential for unterminated loops, bounds checking needed inside of GetLibraryName, etc. etc. First, I'd prefer to just get rid of the unit-test specific HasSharedLibrary, and just have the unit test try to construct a GSSAPISharedLibrary object and Init it. Second, LoadSharedObject (and HasSharedLibrary if necessary) could just iterate directly over kLibraryNames. http://codereview.chromium.org/1736009/diff/105001/106005#newcode183 net/http/http_auth_gssapi_posix.cc:183: if (!import_name_) Don't add error checking code for NULL import_name_ -> cached_initialize_value_ will be set to false if any of the functions can't be bound. Just directly call the functions, and then you don't have to come up with reasonable error codes. http://codereview.chromium.org/1736009/diff/105001/106005#newcode193 net/http/http_auth_gssapi_posix.cc:193: if (!release_name_) Same issue about error checking here (and all of the below methods).
http://codereview.chromium.org/1736009/diff/66001/67003 File net/http/http_auth_gssapi_posix.cc (right): http://codereview.chromium.org/1736009/diff/66001/67003#newcode41 net/http/http_auth_gssapi_posix.cc:41: static const char* kLibraryNames[]; On 2010/05/28 20:41:14, wtc wrote: > On 2010/05/28 16:14:04, ahendrickson wrote: > > > > Moved to an internal method, as another function (for unit testing) uses it > too. > > If it's used by multiple functions, it can be a static > variable in the file scope. Would that simplify your > code? Your new GetLibraryName function seems complicated, > with the index range checking and internal state (the > function-static 'index' and 'current_index' variables). Done. http://codereview.chromium.org/1736009/diff/96001/97001 File base/native_library.h (right): http://codereview.chromium.org/1736009/diff/96001/97001#newcode69 base/native_library.h:69: string16 GetLibraryErrorMessage(); On 2010/05/28 20:41:14, wtc wrote: > This function's name should contain "Loader", such as > GetLibraryLoaderErrorMessage or GetNativeLibraryLoaderErrorMessage, > to stress the fact that the error message is associated with > the loader as opposed to some (unspecified) library. > > Please document this function. > > I'm not convinced that this function is useful, but I > see no other way to get this info on Unix because of the > <dlfcn.h> API. Done. http://codereview.chromium.org/1736009/diff/96001/97003 File base/native_library_mac.mm (right): http://codereview.chromium.org/1736009/diff/96001/97003#newcode77 base/native_library_mac.mm:77: const char* last_err_message = dlerror(); On 2010/05/28 20:41:14, wtc wrote: > The Mac implementation is a little risky because > dlerror() can only be called when the last failed function > used the <dlfcn.h> interface rather than the CFBundle interface. But it's > impossible for the caller to tell. I could set a flag to indicate what type the last operation was. http://codereview.chromium.org/1736009/diff/96001/97005 File net/http/http_auth_gssapi_posix.cc (right): http://codereview.chromium.org/1736009/diff/96001/97005#newcode21 net/http/http_auth_gssapi_posix.cc:21: gss_OID_desc GSS_C_NT_USER_NAME_VAL = { On 2010/06/01 17:35:43, cbentzel wrote: > Are any of these used at all? Do you need this due to gssapi.h? Yes to both questions. Only one is currently used, but they are declared in gssapi.h so we need to define them so they can be used elsewhere. http://codereview.chromium.org/1736009/diff/96001/97005#newcode90 net/http/http_auth_gssapi_posix.cc:90: DCHECK(!initialized_); On 2010/06/01 17:35:43, cbentzel wrote: > Should this immediately set initialized_ to true at the top instead of assuming > the caller will? Done. http://codereview.chromium.org/1736009/diff/96001/97006 File net/http/http_auth_gssapi_posix.h (right): http://codereview.chromium.org/1736009/diff/96001/97006#newcode32 net/http/http_auth_gssapi_posix.h:32: virtual gssapi::OM_uint32 import_name( On 2010/06/01 17:35:43, cbentzel wrote: > You should separate out these methods and add a simple comment mention that > these match the same named functions in the gssapi library. Done. http://codereview.chromium.org/1736009/diff/96001/97006#newcode77 net/http/http_auth_gssapi_posix.h:77: class GSSAPISharedLibrary : public GSSAPILibrary { On 2010/06/01 17:35:43, cbentzel wrote: > I think you could get away with putting this in the .cc file only. I need LoadSharedObject() in the unit test. http://codereview.chromium.org/1736009/diff/96001/97006#newcode128 net/http/http_auth_gssapi_posix.h:128: On 2010/06/01 17:35:43, cbentzel wrote: > Nit: unnecessary empty line. Done. http://codereview.chromium.org/1736009/diff/96001/97006#newcode170 net/http/http_auth_gssapi_posix.h:170: On 2010/06/01 17:35:43, cbentzel wrote: > Nit: unnecessary extra space. Done. http://codereview.chromium.org/1736009/diff/105001/106005 File net/http/http_auth_gssapi_posix.cc (right): http://codereview.chromium.org/1736009/diff/105001/106005#newcode99 net/http/http_auth_gssapi_posix.cc:99: bool GSSAPISharedLibrary::HasSharedLibrary() { On 2010/06/01 17:35:43, cbentzel wrote: > I really don't like the separation of GetLibraryName from > HasSharedLibrary/LoadSharedObject. There's potential for unterminated loops, > bounds checking needed inside of GetLibraryName, etc. etc. > > First, I'd prefer to just get rid of the unit-test specific HasSharedLibrary, > and just have the unit test try to construct a GSSAPISharedLibrary object and > Init it. > > Second, LoadSharedObject (and HasSharedLibrary if necessary) could just iterate > directly over kLibraryNames. Done. http://codereview.chromium.org/1736009/diff/105001/106005#newcode183 net/http/http_auth_gssapi_posix.cc:183: if (!import_name_) On 2010/06/01 17:35:43, cbentzel wrote: > Don't add error checking code for NULL import_name_ -> cached_initialize_value_ > will be set to false if any of the functions can't be bound. Just directly call > the functions, and then you don't have to come up with reasonable error codes. Done. http://codereview.chromium.org/1736009/diff/105001/106005#newcode193 net/http/http_auth_gssapi_posix.cc:193: if (!release_name_) On 2010/06/01 17:35:43, cbentzel wrote: > Same issue about error checking here (and all of the below methods). Done.
LGTM, with some comments. Please provide more description in the CL text (use "Edit Issue" to do this). Ideally the nativelibrary code will be added in a separate CL. http://codereview.chromium.org/1736009/diff/111002/117002 File base/native_library.h (right): http://codereview.chromium.org/1736009/diff/111002/117002#newcode69 base/native_library.h:69: string16 GetNativeLibraryLoaderErrorMessage(); Please document this (even if fairly basic). http://codereview.chromium.org/1736009/diff/111002/117003 File base/native_library_linux.cc (right): http://codereview.chromium.org/1736009/diff/111002/117003#newcode1 base/native_library_linux.cc:1: // Copyright (c) 2009 The Chromium Authors. All rights reserved. Nit: 2010 copyright (here and other files). http://codereview.chromium.org/1736009/diff/111002/117003#newcode49 base/native_library_linux.cc:49: void* lib = dlsym(library, name); Nit: just return dlsym(library, name); is needed here. http://codereview.chromium.org/1736009/diff/111002/117003#newcode62 base/native_library_linux.cc:62: if (last_err_message) { Nit: unnecessary braces (and empty lines before/after the if). http://codereview.chromium.org/1736009/diff/111002/117004 File base/native_library_mac.mm (right): http://codereview.chromium.org/1736009/diff/111002/117004#newcode80 base/native_library_mac.mm:80: if (last_err_message) { Nit: similar style issues to the linux implementation. http://codereview.chromium.org/1736009/diff/111002/117005 File base/native_library_win.cc (right): http://codereview.chromium.org/1736009/diff/111002/117005#newcode53 base/native_library_win.cc:53: DWORD last_err = GetLastError(); You could just return win_util::FormatLastWin32Error() here. http://codereview.chromium.org/1736009/diff/111002/117006 File net/http/http_auth_gssapi_posix.cc (right): http://codereview.chromium.org/1736009/diff/111002/117006#newcode54 net/http/http_auth_gssapi_posix.cc:54: gss_OID GSS_C_NT_HOSTBASED_SERVICE = &GSS_C_NT_HOSTBASED_SERVICE_VAL; The only one which needs to be defined is GSS_C_NT_HOSTBASED_SERVICE_VAL. Could you remove the others? http://codereview.chromium.org/1736009/diff/111002/117006#newcode106 net/http/http_auth_gssapi_posix.cc:106: for (unsigned int i = 0; i < num_lib_names; ++i) { Should i be a size_t to match num_lib_names? http://codereview.chromium.org/1736009/diff/111002/117006#newcode107 net/http/http_auth_gssapi_posix.cc:107: const char* library_name = kLibraryNames[i Nit: extra newline. http://codereview.chromium.org/1736009/diff/111002/117006#newcode116 net/http/http_auth_gssapi_posix.cc:116: return NULL; Nit: Not a huge deal in this case, but for Windows NativeLibrary's type is an HMODULE. It would be nice to have a platform-specific constant for InvalidNativeLibrary or something similar. http://codereview.chromium.org/1736009/diff/111002/117006#newcode181 net/http/http_auth_gssapi_posix.cc:181: status_string->length = 0; I think it's a mistake to do anything other than pass the values through to the underlying implementation. http://codereview.chromium.org/1736009/diff/111002/117006#newcode225 net/http/http_auth_gssapi_posix.cc:225: if (!wrap_size_limit_) Remove this wrap_size_limit check. http://codereview.chromium.org/1736009/diff/111002/117008 File net/http/http_auth_gssapi_unittest_posix.cc (right): http://codereview.chromium.org/1736009/diff/111002/117008#newcode12 net/http/http_auth_gssapi_unittest_posix.cc:12: TEST(HttpAuthGssapiPosixTest, GssapiStartup) { I would simply change this to call GSSAPILIbrary::CreateDefault and Init on the default version.
Chris suggested breaking out the native library code into a separate CL, so I plan to do that. http://codereview.chromium.org/1736009/diff/111002/117002 File base/native_library.h (right): http://codereview.chromium.org/1736009/diff/111002/117002#newcode69 base/native_library.h:69: string16 GetNativeLibraryLoaderErrorMessage(); On 2010/06/01 20:29:58, cbentzel wrote: > Please document this (even if fairly basic). Done. http://codereview.chromium.org/1736009/diff/111002/117003 File base/native_library_linux.cc (right): http://codereview.chromium.org/1736009/diff/111002/117003#newcode1 base/native_library_linux.cc:1: // Copyright (c) 2009 The Chromium Authors. All rights reserved. On 2010/06/01 20:29:58, cbentzel wrote: > Nit: 2010 copyright (here and other files). Done. http://codereview.chromium.org/1736009/diff/111002/117003#newcode49 base/native_library_linux.cc:49: void* lib = dlsym(library, name); On 2010/06/01 20:29:58, cbentzel wrote: > Nit: just return dlsym(library, name); is needed here. Done. http://codereview.chromium.org/1736009/diff/111002/117003#newcode62 base/native_library_linux.cc:62: if (last_err_message) { On 2010/06/01 20:29:58, cbentzel wrote: > Nit: unnecessary braces (and empty lines before/after the if). Done. http://codereview.chromium.org/1736009/diff/111002/117004 File base/native_library_mac.mm (right): http://codereview.chromium.org/1736009/diff/111002/117004#newcode80 base/native_library_mac.mm:80: if (last_err_message) { On 2010/06/01 20:29:58, cbentzel wrote: > Nit: similar style issues to the linux implementation. Done. http://codereview.chromium.org/1736009/diff/111002/117005 File base/native_library_win.cc (right): http://codereview.chromium.org/1736009/diff/111002/117005#newcode53 base/native_library_win.cc:53: DWORD last_err = GetLastError(); On 2010/06/01 20:29:58, cbentzel wrote: > You could just return win_util::FormatLastWin32Error() here. Done. http://codereview.chromium.org/1736009/diff/111002/117006 File net/http/http_auth_gssapi_posix.cc (right): http://codereview.chromium.org/1736009/diff/111002/117006#newcode54 net/http/http_auth_gssapi_posix.cc:54: gss_OID GSS_C_NT_HOSTBASED_SERVICE = &GSS_C_NT_HOSTBASED_SERVICE_VAL; On 2010/06/01 20:29:58, cbentzel wrote: > The only one which needs to be defined is GSS_C_NT_HOSTBASED_SERVICE_VAL. Could > you remove the others? Done. http://codereview.chromium.org/1736009/diff/111002/117006#newcode106 net/http/http_auth_gssapi_posix.cc:106: for (unsigned int i = 0; i < num_lib_names; ++i) { On 2010/06/01 20:29:58, cbentzel wrote: > Should i be a size_t to match num_lib_names? Done. http://codereview.chromium.org/1736009/diff/111002/117006#newcode107 net/http/http_auth_gssapi_posix.cc:107: const char* library_name = kLibraryNames[i On 2010/06/01 20:29:58, cbentzel wrote: > Nit: extra newline. Done. http://codereview.chromium.org/1736009/diff/111002/117006#newcode116 net/http/http_auth_gssapi_posix.cc:116: return NULL; On 2010/06/01 20:29:58, cbentzel wrote: > Nit: Not a huge deal in this case, but for Windows NativeLibrary's type is an > HMODULE. It would be nice to have a platform-specific constant for > InvalidNativeLibrary or something similar. Hope this makes it clearer. Done. http://codereview.chromium.org/1736009/diff/111002/117006#newcode181 net/http/http_auth_gssapi_posix.cc:181: status_string->length = 0; On 2010/06/01 20:29:58, cbentzel wrote: > I think it's a mistake to do anything other than pass the values through to the > underlying implementation. Done. http://codereview.chromium.org/1736009/diff/111002/117006#newcode225 net/http/http_auth_gssapi_posix.cc:225: if (!wrap_size_limit_) On 2010/06/01 20:29:58, cbentzel wrote: > Remove this wrap_size_limit check. Done. Sorry, I missed it last pass. http://codereview.chromium.org/1736009/diff/111002/117008 File net/http/http_auth_gssapi_unittest_posix.cc (right): http://codereview.chromium.org/1736009/diff/111002/117008#newcode12 net/http/http_auth_gssapi_unittest_posix.cc:12: TEST(HttpAuthGssapiPosixTest, GssapiStartup) { On 2010/06/01 20:29:58, cbentzel wrote: > I would simply change this to call GSSAPILIbrary::CreateDefault and Init on the > default version. Done.
http://codereview.chromium.org/1736009/diff/129001/130005 File net/http/http_auth_gssapi_posix.cc (right): http://codereview.chromium.org/1736009/diff/129001/130005#newcode7 net/http/http_auth_gssapi_posix.cc:7: #include <string> Nit: http_auth_gssapi_posix.h already includes <string> "base/native_library.h" "net/http/http_auth.h" Remove these. http://codereview.chromium.org/1736009/diff/129001/130005#newcode26 net/http/http_auth_gssapi_posix.cc:26: gss_OID GSS_C_NT_HOSTBASED_SERVICE = &GSS_C_NT_HOSTBASED_SERVICE_VAL; You should look up the data symbol GSS_C_NT_HOSTBASED_SERVICE from the GSSAPI native library. If you think it's less work to just define this constant ourselves, please use a different name than GSS_C_NT_HOSTBASED_SERVICE for our constant. Our constant should be put inside an unnamed namespace. http://codereview.chromium.org/1736009/diff/129001/130005#newcode69 net/http/http_auth_gssapi_posix.cc:69: static const char* kLibraryNames[] = { Would be nice to document these library names, such as whether they are MIT or Heimdal and which Linux distros use them. http://codereview.chromium.org/1736009/diff/129001/130005#newcode78 net/http/http_auth_gssapi_posix.cc:78: if (!library_name) Remove lines 78-79. The current code doesn't need them. http://codereview.chromium.org/1736009/diff/129001/130005#newcode85 net/http/http_auth_gssapi_posix.cc:85: return (base::NativeLibrary) NULL; Just return NULL. The base::NativeLibrary cast should not be necessary. http://codereview.chromium.org/1736009/diff/129001/130005#newcode89 net/http/http_auth_gssapi_posix.cc:89: bool FindAndBind(base::NativeLibrary library, const char* name, T* t) { Does the use of template result in the compiler instantiating six copies of this code? That would be bad. http://codereview.chromium.org/1736009/diff/129001/130005#newcode179 net/http/http_auth_gssapi_posix.cc:179: reinterpret_cast<gssapi::gss_buffer_t>(output_token), Why do we need this case? Does this mean the prototype of GSSAPISharedLibrary::init_sec_context is wrong? http://codereview.chromium.org/1736009/diff/129001/130005#newcode213 net/http/http_auth_gssapi_posix.cc:213: std::string DisplayCode(GSSAPILibrary* gssapi_lib, Perhaps DisplayCode and DisplayExtendedStatus should be methods of GSSAPILibrary? http://codereview.chromium.org/1736009/diff/129001/130006 File net/http/http_auth_gssapi_posix.h (right): http://codereview.chromium.org/1736009/diff/129001/130006#newcode11 net/http/http_auth_gssapi_posix.h:11: #include "base/gtest_prod_util.h" Nit: list the headers in alphabetical order. http://codereview.chromium.org/1736009/diff/129001/130006#newcode32 net/http/http_auth_gssapi_posix.h:32: virtual bool Init() = 0; Please document the Init() method. http://codereview.chromium.org/1736009/diff/129001/130006#newcode35 net/http/http_auth_gssapi_posix.h:35: virtual gssapi::OM_uint32 import_name( I still think the gssapi namespace is redundant. gssapi.h, being a C file, already emulates namespaces by using the OM_ and gss_ prefixes on its identifiers. http://codereview.chromium.org/1736009/diff/129001/130006#newcode127 net/http/http_auth_gssapi_posix.h:127: // Used by unit tests to determine whether or not the GSSAPI library exists. This comment should be immediately above the FRIEND_TEST_ALL_PREFIXES macro. http://codereview.chromium.org/1736009/diff/129001/130006#newcode135 net/http/http_auth_gssapi_posix.h:135: bool initialized_; Please document these data members. Nit: it may be better to rename cached_initialize_value_ to cached_initialize_status_ or cached_initialize_return_value_ as it caches the return value/status of the InitImpl() method. http://codereview.chromium.org/1736009/diff/129001/130007 File net/http/http_auth_gssapi_unittest_posix.cc (right): http://codereview.chromium.org/1736009/diff/129001/130007#newcode1 net/http/http_auth_gssapi_unittest_posix.cc:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. This file should be renamed http_auth_gssapi_posix_unittest.cc. Use "!sources" in net.gyp to exclude this file for Windows. http://codereview.chromium.org/1736009/diff/129001/130007#newcode12 net/http/http_auth_gssapi_unittest_posix.cc:12: TEST(HttpAuthGssapiPosixTest, GssapiStartup) { Nit: Spell "GSSAPI" and "POSIX" in all caps. http://codereview.chromium.org/1736009/diff/129001/130007#newcode13 net/http/http_auth_gssapi_unittest_posix.cc:13: GSSAPILibrary* gssapi = GSSAPILibrary::GetDefault(); If it's not crucial to call GSSAPILibrary::GetDefault() first, it'd be better to move this line to right before line 22. http://codereview.chromium.org/1736009/diff/129001/130008 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/1736009/diff/129001/130008#newcode7 net/http/http_network_transaction.cc:7: #include <vector> If this change is necessary, please move this change to a separate CL. Otherwise please remove this change from this CL. http://codereview.chromium.org/1736009/diff/129001/130009 File net/net.gyp (right): http://codereview.chromium.org/1736009/diff/129001/130009#newcode494 net/net.gyp:494: 'third_party/gssapi/gssapi.h', If you add third_party/gssapi/gssapi.h to net.gyp, please svn remove third_party/gssapi/gssapi.gyp.
Sorry, some more comments. http://codereview.chromium.org/1736009/diff/129001/130005 File net/http/http_auth_gssapi_posix.cc (right): http://codereview.chromium.org/1736009/diff/129001/130005#newcode45 net/http/http_auth_gssapi_posix.cc:45: if (gssapi_library_) { I just looked and there is a base/scoped_native_library.h which would do this for you. http://codereview.chromium.org/1736009/diff/129001/130005#newcode70 net/http/http_auth_gssapi_posix.cc:70: "libgssapi_krb5.so.2", You are also going to need to add libgssapi_krb5.dylib for OSX. http://codereview.chromium.org/1736009/diff/129001/130005#newcode89 net/http/http_auth_gssapi_posix.cc:89: bool FindAndBind(base::NativeLibrary library, const char* name, T* t) { On 2010/06/02 01:38:33, wtc wrote: > Does the use of template result in the compiler instantiating > six copies of this code? That would be bad. It probably will (the T is different in each case). Another option is to just make this a void* FindAndBind(base::NativeLibrary library, const char* name) which returns NULL if not found, and requires the caller to do the reinterpret_cast to the correct type. http://codereview.chromium.org/1736009/diff/129001/130005#newcode248 net/http/http_auth_gssapi_posix.cc:248: Nit: extra newline. http://codereview.chromium.org/1736009/diff/129001/130006 File net/http/http_auth_gssapi_posix.h (right): http://codereview.chromium.org/1736009/diff/129001/130006#newcode35 net/http/http_auth_gssapi_posix.h:35: virtual gssapi::OM_uint32 import_name( On 2010/06/02 01:38:33, wtc wrote: > I still think the gssapi namespace is redundant. gssapi.h, > being a C file, already emulates namespaces by using the > OM_ and gss_ prefixes on its identifiers. I agree with you now (and I was the original person who proposed using a gssapi namespace). Andy, feel free to remove the gssapi namespace in a separate CL - it's a simple refactor which shouldn't be part of this one.
Andy, Please make this CL independent of the NativeLibrary change completely, so that you can check this CL in first. You can add the native library loader error message call later after the NativeLibrary CL is checked in. http://codereview.chromium.org/1736009/diff/129001/130006 File net/http/http_auth_gssapi_posix.h (right): http://codereview.chromium.org/1736009/diff/129001/130006#newcode135 net/http/http_auth_gssapi_posix.h:135: bool initialized_; Another idea is to remove cached_initialize_value_, and set initialized_ to true only if InitImpl() succeeds. As a result, we will be calling InitImpl() redundantly if a prior InitImpl() failed, but we don't need to optimize such a corner case.
http://codereview.chromium.org/1736009/diff/129001/130005 File net/http/http_auth_gssapi_posix.cc (right): http://codereview.chromium.org/1736009/diff/129001/130005#newcode7 net/http/http_auth_gssapi_posix.cc:7: #include <string> On 2010/06/02 01:38:33, wtc wrote: > Nit: http_auth_gssapi_posix.h already includes > <string> > "base/native_library.h" > "net/http/http_auth.h" > Remove these. Done. http://codereview.chromium.org/1736009/diff/129001/130005#newcode26 net/http/http_auth_gssapi_posix.cc:26: gss_OID GSS_C_NT_HOSTBASED_SERVICE = &GSS_C_NT_HOSTBASED_SERVICE_VAL; On 2010/06/02 01:38:33, wtc wrote: > You should look up the data symbol GSS_C_NT_HOSTBASED_SERVICE > from the GSSAPI native library. > > If you think it's less work to just define this constant > ourselves, please use a different name than GSS_C_NT_HOSTBASED_SERVICE > for our constant. > > Our constant should be put inside an unnamed namespace. Done. http://codereview.chromium.org/1736009/diff/129001/130005#newcode45 net/http/http_auth_gssapi_posix.cc:45: if (gssapi_library_) { On 2010/06/02 12:53:11, cbentzel wrote: > I just looked and there is a base/scoped_native_library.h which would do this > for you. I would have to modify ScopedNativeLibrary in order to use it for this, as it doesn't take a NativeLibrary as a parameter to it's constructor, and you can't set it otherwise or query if it succeeded. http://codereview.chromium.org/1736009/diff/129001/130005#newcode69 net/http/http_auth_gssapi_posix.cc:69: static const char* kLibraryNames[] = { On 2010/06/02 01:38:33, wtc wrote: > Would be nice to document these library names, such as > whether they are MIT or Heimdal and which Linux distros > use them. Documented which are MIT and Heimdal. http://codereview.chromium.org/1736009/diff/129001/130005#newcode70 net/http/http_auth_gssapi_posix.cc:70: "libgssapi_krb5.so.2", On 2010/06/02 12:53:11, cbentzel wrote: > You are also going to need to add libgssapi_krb5.dylib for OSX. Done. http://codereview.chromium.org/1736009/diff/129001/130005#newcode78 net/http/http_auth_gssapi_posix.cc:78: if (!library_name) On 2010/06/02 01:38:33, wtc wrote: > Remove lines 78-79. The current code doesn't need them. Done. http://codereview.chromium.org/1736009/diff/129001/130005#newcode85 net/http/http_auth_gssapi_posix.cc:85: return (base::NativeLibrary) NULL; On 2010/06/02 01:38:33, wtc wrote: > Just return NULL. The base::NativeLibrary cast should > not be necessary. Done. http://codereview.chromium.org/1736009/diff/129001/130005#newcode179 net/http/http_auth_gssapi_posix.cc:179: reinterpret_cast<gssapi::gss_buffer_t>(output_token), On 2010/06/02 01:38:33, wtc wrote: > Why do we need this case? Does this mean the prototype > of GSSAPISharedLibrary::init_sec_context is wrong? Fixed. http://codereview.chromium.org/1736009/diff/129001/130005#newcode213 net/http/http_auth_gssapi_posix.cc:213: std::string DisplayCode(GSSAPILibrary* gssapi_lib, On 2010/06/02 01:38:33, wtc wrote: > Perhaps DisplayCode and DisplayExtendedStatus should be > methods of GSSAPILibrary? If we do this, I'd like to postpone it to another CL. http://codereview.chromium.org/1736009/diff/129001/130005#newcode248 net/http/http_auth_gssapi_posix.cc:248: On 2010/06/02 12:53:11, cbentzel wrote: > Nit: extra newline. Done. http://codereview.chromium.org/1736009/diff/129001/130006 File net/http/http_auth_gssapi_posix.h (right): http://codereview.chromium.org/1736009/diff/129001/130006#newcode11 net/http/http_auth_gssapi_posix.h:11: #include "base/gtest_prod_util.h" On 2010/06/02 01:38:33, wtc wrote: > Nit: list the headers in alphabetical order. Done. http://codereview.chromium.org/1736009/diff/129001/130006#newcode32 net/http/http_auth_gssapi_posix.h:32: virtual bool Init() = 0; On 2010/06/02 01:38:33, wtc wrote: > Please document the Init() method. Done. http://codereview.chromium.org/1736009/diff/129001/130006#newcode35 net/http/http_auth_gssapi_posix.h:35: virtual gssapi::OM_uint32 import_name( On 2010/06/02 12:53:11, cbentzel wrote: > On 2010/06/02 01:38:33, wtc wrote: > > I still think the gssapi namespace is redundant. gssapi.h, > > being a C file, already emulates namespaces by using the > > OM_ and gss_ prefixes on its identifiers. > > I agree with you now (and I was the original person who proposed using a gssapi > namespace). > > Andy, feel free to remove the gssapi namespace in a separate CL - it's a simple > refactor which shouldn't be part of this one. > Will do. http://codereview.chromium.org/1736009/diff/129001/130006#newcode127 net/http/http_auth_gssapi_posix.h:127: // Used by unit tests to determine whether or not the GSSAPI library exists. On 2010/06/02 01:38:33, wtc wrote: > This comment should be immediately above the FRIEND_TEST_ALL_PREFIXES macro. Actually, this comment is left over from a previous change. Removed it. http://codereview.chromium.org/1736009/diff/129001/130006#newcode135 net/http/http_auth_gssapi_posix.h:135: bool initialized_; On 2010/06/02 19:37:58, wtc wrote: > Another idea is to remove cached_initialize_value_, and > set initialized_ to true only if InitImpl() succeeds. > > As a result, we will be calling InitImpl() redundantly > if a prior InitImpl() failed, but we don't need to optimize > such a corner case. Done. http://codereview.chromium.org/1736009/diff/129001/130006#newcode135 net/http/http_auth_gssapi_posix.h:135: bool initialized_; On 2010/06/02 01:38:33, wtc wrote: > Please document these data members. > > Nit: it may be better to rename cached_initialize_value_ > to cached_initialize_status_ or cached_initialize_return_value_ > as it caches the return value/status of the InitImpl() method. Now that I've removed cached_initialize_value_, initialized_ seems pretty self-descriptive. http://codereview.chromium.org/1736009/diff/129001/130007 File net/http/http_auth_gssapi_unittest_posix.cc (right): http://codereview.chromium.org/1736009/diff/129001/130007#newcode1 net/http/http_auth_gssapi_unittest_posix.cc:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. On 2010/06/02 01:38:33, wtc wrote: > This file should be renamed http_auth_gssapi_posix_unittest.cc. > Use "!sources" in net.gyp to exclude this file for Windows. Done. http://codereview.chromium.org/1736009/diff/129001/130007#newcode12 net/http/http_auth_gssapi_unittest_posix.cc:12: TEST(HttpAuthGssapiPosixTest, GssapiStartup) { On 2010/06/02 01:38:33, wtc wrote: > Nit: Spell "GSSAPI" and "POSIX" in all caps. Done. http://codereview.chromium.org/1736009/diff/129001/130007#newcode13 net/http/http_auth_gssapi_unittest_posix.cc:13: GSSAPILibrary* gssapi = GSSAPILibrary::GetDefault(); On 2010/06/02 01:38:33, wtc wrote: > If it's not crucial to call GSSAPILibrary::GetDefault() > first, it'd be better to move this line to right before > line 22. Done. http://codereview.chromium.org/1736009/diff/129001/130008 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/1736009/diff/129001/130008#newcode7 net/http/http_network_transaction.cc:7: #include <vector> On 2010/06/02 01:38:33, wtc wrote: > If this change is necessary, please move this change to > a separate CL. Otherwise please remove this change from > this CL. Removed. http://codereview.chromium.org/1736009/diff/129001/130009 File net/net.gyp (right): http://codereview.chromium.org/1736009/diff/129001/130009#newcode494 net/net.gyp:494: 'third_party/gssapi/gssapi.h', On 2010/06/02 01:38:33, wtc wrote: > If you add third_party/gssapi/gssapi.h to net.gyp, > please svn remove third_party/gssapi/gssapi.gyp. Removed from net.gyp.
LGTM! http://codereview.chromium.org/1736009/diff/142001/143001 File base/native_library.h (right): http://codereview.chromium.org/1736009/diff/142001/143001#newcode1 base/native_library.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. Please remove the native_library files from this CL. http://codereview.chromium.org/1736009/diff/142001/143008 File net/net.gyp (right): http://codereview.chromium.org/1736009/diff/142001/143008#newcode770 net/net.gyp:770: 'http/http_auth_gssapi_posix_unittest.cc', I'm assuming this was necessary - the win_unittest.cc work that I did was able to pass without a specific include.
http://codereview.chromium.org/1736009/diff/142001/143001 File base/native_library.h (right): http://codereview.chromium.org/1736009/diff/142001/143001#newcode1 base/native_library.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. On 2010/06/03 19:47:16, cbentzel wrote: > Please remove the native_library files from this CL. Done. http://codereview.chromium.org/1736009/diff/142001/143008 File net/net.gyp (right): http://codereview.chromium.org/1736009/diff/142001/143008#newcode770 net/net.gyp:770: 'http/http_auth_gssapi_posix_unittest.cc', On 2010/06/03 19:47:16, cbentzel wrote: > I'm assuming this was necessary - the win_unittest.cc work that I did was able > to pass without a specific include. Yes.
I'll review the latest Patch Set. Andy, feel free to commit it first. http://codereview.chromium.org/1736009/diff/142001/143008 File net/net.gyp (right): http://codereview.chromium.org/1736009/diff/142001/143008#newcode770 net/net.gyp:770: 'http/http_auth_gssapi_posix_unittest.cc', On 2010/06/03 19:47:16, cbentzel wrote: > I'm assuming this was necessary - the win_unittest.cc work that I did was able > to pass without a specific include. Andy's answer puzzeled me. I found the answer in src/build/common.gypi. It has ['OS!="win"', { 'sources/': [ ['exclude', '_win(_unittest)?\\.cc$'], ['exclude', '/win/'], ['exclude', '/win_[^/]*\\.cc$'] ], }], which is why _win_unittest.cc is excluded. But it also has ['OS=="win"', { 'sources/': [ ['exclude', '_posix\\.cc$'] ], # turn on warnings for signed/unsigned mismatch on chromium code. 'msvs_settings': { 'VCCLCompilerTool': { 'AdditionalOptions': ['/we4389'], }, }, }], So we can also enhance src/build/common.gypi to exclude posix_unittest.cc.
LGTM. Some of my suggested changes below can be deferred to a future CL. Please address the one marked with "BUG" and the simple nits. I didn't have time to review HttpAuthGSSAPI::GetNextSecurityToken today, but I reviewed everything else carefully. http://codereview.chromium.org/1736009/diff/146001/147001 File net/http/http_auth_gssapi_posix.cc (right): http://codereview.chromium.org/1736009/diff/146001/147001#newcode70 net/http/http_auth_gssapi_posix.cc:70: "libgssapi.so.4", // Heimdal Could you explain why we need .4 and .1 for libgssapi.so? http://codereview.chromium.org/1736009/diff/146001/147001#newcode91 net/http/http_auth_gssapi_posix.cc:91: // Waiting for CL that adds error messages to Native Library code. Next time, please remove such code from the CL. Some people don't like #if 0 code. http://codereview.chromium.org/1736009/diff/146001/147001#newcode211 net/http/http_auth_gssapi_posix.cc:211: return StringPrintf("%x %x", major_status, minor_status); Should we use 0x%08X instead of %x in the format string? http://codereview.chromium.org/1736009/diff/146001/147001#newcode320 net/http/http_auth_gssapi_posix.cc:320: HttpAuthGSSAPI::~HttpAuthGSSAPI() { Do we not need to destroy anything in the HttpAuthGSSAPI destructor? Not even sec_context_? http://codereview.chromium.org/1736009/diff/146001/147001#newcode363 net/http/http_auth_gssapi_posix.cc:363: library_->Init(); It seems that the Init() method should be invoked by the GSSAPILibrary::GetDefault method, so that the caller of GetDefault gets an initialized library. http://codereview.chromium.org/1736009/diff/146001/147001#newcode382 net/http/http_auth_gssapi_posix.cc:382: std::string encode_output; POTENTIAL BUG: do we need to free output_token? http://codereview.chromium.org/1736009/diff/146001/147001#newcode385 net/http/http_auth_gssapi_posix.cc:385: return rv; BUG: return a net error code such as ERR_UNEXPECTED. rv is OK at this point. You may also want to log an error message as HttpAuthSSPI::GenerateAuthToken does. http://codereview.chromium.org/1736009/diff/146001/147001#newcode394 net/http/http_auth_gssapi_posix.cc:394: username_ = L""; Nit: it may be better to do username_.clear(); password_.clear(); instead of these assignments. ISSUE: But I found that username_ and password_ are not being used. So I think this method just need to check that the username and password arguments are both NULL and return OK, and we can remove the username_ and password_ members. http://codereview.chromium.org/1736009/diff/146001/147002 File net/http/http_auth_gssapi_posix.h (right): http://codereview.chromium.org/1736009/diff/146001/147002#newcode81 net/http/http_auth_gssapi_posix.h:81: class GSSAPISharedLibrary : public GSSAPILibrary { You may want to document that GSSAPISharedLibrary is defined here so that unit tests can access it. http://codereview.chromium.org/1736009/diff/146001/147002#newcode152 net/http/http_auth_gssapi_posix.h:152: HttpAuthGSSAPI(const std::string& scheme, Nit: make "GSSAPILibrary* library" the first argument of the constructor to match the HttpAuthSSPI constructor. http://codereview.chromium.org/1736009/diff/146001/147002#newcode162 net/http/http_auth_gssapi_posix.h:162: int GenerateAuthToken(const std::wstring* username, You may want to copy the comment for this method from http_auth_sspi_win.h. The out_credentials argument should be renamed auth_token to match HttpAuthSSPI. The origin argument of this function and the GetNextSecurityToken function below (line 172) probably should be renamed spn.
http://codereview.chromium.org/1736009/diff/146001/147001 File net/http/http_auth_gssapi_posix.cc (right): http://codereview.chromium.org/1736009/diff/146001/147001#newcode91 net/http/http_auth_gssapi_posix.cc:91: // Waiting for CL that adds error messages to Native Library code. On 2010/06/03 23:24:24, wtc wrote: > Next time, please remove such code from the CL. > Some people don't like #if 0 code. Done. http://codereview.chromium.org/1736009/diff/146001/147001#newcode211 net/http/http_auth_gssapi_posix.cc:211: return StringPrintf("%x %x", major_status, minor_status); On 2010/06/03 23:24:24, wtc wrote: > Should we use 0x%08X instead of %x in the format string? Done. http://codereview.chromium.org/1736009/diff/146001/147001#newcode363 net/http/http_auth_gssapi_posix.cc:363: library_->Init(); On 2010/06/03 23:24:24, wtc wrote: > It seems that the Init() method should be invoked by the > GSSAPILibrary::GetDefault method, so that the caller of > GetDefault gets an initialized library. GetDefault() gets called when the factory is created, at the start of the program. Doing it here lets us do lazy initialization, avoiding startup costs. http://codereview.chromium.org/1736009/diff/146001/147001#newcode382 net/http/http_auth_gssapi_posix.cc:382: std::string encode_output; On 2010/06/03 23:24:24, wtc wrote: > POTENTIAL BUG: do we need to free output_token? Yes. Done. http://codereview.chromium.org/1736009/diff/146001/147001#newcode385 net/http/http_auth_gssapi_posix.cc:385: return rv; On 2010/06/03 23:24:24, wtc wrote: > BUG: return a net error code such as ERR_UNEXPECTED. > rv is OK at this point. You may also want to log an error > message as HttpAuthSSPI::GenerateAuthToken does. Done. http://codereview.chromium.org/1736009/diff/146001/147001#newcode394 net/http/http_auth_gssapi_posix.cc:394: username_ = L""; On 2010/06/03 23:24:24, wtc wrote: > Nit: it may be better to do > username_.clear(); > password_.clear(); > instead of these assignments. > > ISSUE: But I found that username_ and password_ are not > being used. So I think this method just need to check > that the username and password arguments are both NULL > and return OK, and we can remove the username_ and > password_ members. I'm planning on using them in a later CL. http://codereview.chromium.org/1736009/diff/146001/147002 File net/http/http_auth_gssapi_posix.h (right): http://codereview.chromium.org/1736009/diff/146001/147002#newcode81 net/http/http_auth_gssapi_posix.h:81: class GSSAPISharedLibrary : public GSSAPILibrary { On 2010/06/03 23:24:24, wtc wrote: > You may want to document that GSSAPISharedLibrary is > defined here so that unit tests can access it. Done. http://codereview.chromium.org/1736009/diff/146001/147002#newcode152 net/http/http_auth_gssapi_posix.h:152: HttpAuthGSSAPI(const std::string& scheme, On 2010/06/03 23:24:24, wtc wrote: > Nit: make "GSSAPILibrary* library" the first argument of > the constructor to match the HttpAuthSSPI constructor. Done. http://codereview.chromium.org/1736009/diff/146001/147002#newcode162 net/http/http_auth_gssapi_posix.h:162: int GenerateAuthToken(const std::wstring* username, On 2010/06/03 23:24:24, wtc wrote: > You may want to copy the comment for this method from > http_auth_sspi_win.h. > > The out_credentials argument should be renamed auth_token > to match HttpAuthSSPI. > > The origin argument of this function and the > GetNextSecurityToken function below (line 172) > probably should be renamed spn. Done.
Andy, ks! This completes my review of this CL. Please address these issues in a follow-up CL. Thanks! http://codereview.chromium.org/1736009/diff/153001/154001 File net/http/http_auth_gssapi_posix.cc (right): http://codereview.chromium.org/1736009/diff/153001/154001#newcode204 net/http/http_auth_gssapi_posix.cc:204: return StringPrintf("0x%08x 0x%08x", major_status, minor_status); Nit: be consistent with using either %x or %X in the format string for major_status/minor_status. You use %x here but %X at line 213 below. http://codereview.chromium.org/1736009/diff/153001/154001#newcode273 net/http/http_auth_gssapi_posix.cc:273: class ScopedBuffer { ScopedBuffer is not being used. Are you planning to use it in the future? http://codereview.chromium.org/1736009/diff/153001/154001#newcode365 net/http/http_auth_gssapi_posix.cc:365: input_token.length = decoded_server_auth_token_.length(); We may need to pass GSS_NO_BUFFER instead of &input_token to the first library_->init_sec_context call. But it seems that setting input_token.length to 0 is equivalent. http://codereview.chromium.org/1736009/diff/153001/154001#newcode407 net/http/http_auth_gssapi_posix.cc:407: spn_buffer.value = const_cast<char *>(spn_principal.data()); Use c_str() instead of data(), since spn_buffer.length below has a + 1 for the terminating null byte. http://codereview.chromium.org/1736009/diff/153001/154001#newcode417 net/http/http_auth_gssapi_posix.cc:417: LOG(WARNING) << "Problem importing name. " This log message should be at the ERROR level if we return ERR_UNEXPECTED, which means a programming error (a bug in our code). Same with the LOG(WARNING) message at line 443 below. http://codereview.chromium.org/1736009/diff/153001/154001#newcode425 net/http/http_auth_gssapi_posix.cc:425: // Create a security context. This comment may not be accurate... I think it's only accurate for the first call to library_->init_sec_context. In the second call, we just use the sec_context_ created by the first call. http://codereview.chromium.org/1736009/diff/153001/154001#newcode430 net/http/http_auth_gssapi_posix.cc:430: &sec_context_, The returned sec_context_ value should be deleted with a gss_delete_sec_context call in the destructor. http://codereview.chromium.org/1736009/diff/153001/154001#newcode450 net/http/http_auth_gssapi_posix.cc:450: return (major_status != GSS_S_COMPLETE) ? ERR_IO_PENDING : OK; Are you sure it's right to return ERR_IO_PENDING if major_status is GSS_S_CONTINUE_NEEDED? This function doesn't operate in async mode (for example, it doesn't have a "completion callback" argument that we can use to notify completion asynchronously). In HttpAuthSSPI::GetNextSecurityToken, we simply return OK at this point. http://codereview.chromium.org/1736009/diff/153001/154002 File net/http/http_auth_gssapi_posix.h (right): http://codereview.chromium.org/1736009/diff/153001/154002#newcode176 net/http/http_auth_gssapi_posix.h:176: std::string* out_credentials); Please rename out_credentials to auth_token to match the comment at line 165 above (or you can change the comment to match the code). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
