Linux: control usage of Kerberos via use_kerberos gyp flag

Linux: control usage of Kerberos via use_kerberos gyp flag
(on by default)

This allows Linux distro packagers to control usage of Kerberos
explicitly instead of relying on auto-detection.

BUG=92689
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96790

[cbentzel, 2011-08-12 00:24:03]

This should perhaps be called USE_SPNEGO rather than USE_KERBEROS, as SPNEGO
doesn't explicitly require Kerberos.

Why isn't the existing mechanism sufficient, though? Is it a code size concern?
This should bypass SPNEGO authentication for a lower priority authentication if
there isn't a GSSAPI library present, or it doesn't export enough symbols.

[Paweł Hajdan Jr., 2011-08-12 16:50:57]

On 2011/08/12 00:24:03, cbentzel wrote:
> Why isn't the existing mechanism sufficient, though? Is it a code size
concern?
> This should bypass SPNEGO authentication for a lower priority authentication
if
> there isn't a GSSAPI library present, or it doesn't export enough symbols.

No, it's just getting rid of automagic detection of Kerberos. Distros which
control the entire stack want to disable such autodetection logic and instead
link directly with the libraries (instead of using dlopen). In fact, this is
going to be my next patch.

This just reflects the way we handle Gnome Keyring dependency.

[wtc, 2011-08-12 17:21:51]

cbentzel should be the primary reviewer of this CL.

http://codereview.chromium.org/7633006/diff/1/net/http/http_auth_handler_fact...
File net/http/http_auth_handler_factory_unittest.cc (right):

http://codereview.chromium.org/7633006/diff/1/net/http/http_auth_handler_fact...
net/http/http_auth_handler_factory_unittest.cc:183: }
Add a #else block here to perform a negative test.

http_auth_handler_factory->CreateAuthHandlerFromString(
    "Negotiate",

should fail with an expected error (I forgot what that is)
if the code is not compiled with -DUSE_KERBEROS.

http://codereview.chromium.org/7633006/diff/1/net/http/http_auth_unittest.cc
File net/http/http_auth_unittest.cc (right):

http://codereview.chromium.org/7633006/diff/1/net/http/http_auth_unittest.cc#...
net/http/http_auth_unittest.cc:116: }
This probably can also have a #else block.

http://codereview.chromium.org/7633006/diff/1/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/7633006/diff/1/net/net.gyp#newcode9
net/net.gyp:9: 'use_kerberos%': 1,
Should we remove the blank line?

What does the '%' character mean?

http://codereview.chromium.org/7633006/diff/1/net/net.gyp#newcode713
net/net.gyp:713: },
It seems that a direct dependent should not need to know
whether the net module is compiled with -DUSE_KERBEROS or
not.

[cbentzel, 2011-08-12 17:59:06]

Please add a bug and reference it here.

I also agree with wtc's comments about the need for #else triggered tests.

Thank you for chatting with me about this change so I could understand it's
justification.

http://codereview.chromium.org/7633006/diff/1/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/7633006/diff/1/net/net.gyp#newcode713
net/net.gyp:713: },
On 2011/08/12 17:21:51, wtc wrote:
> It seems that a direct dependent should not need to know
> whether the net module is compiled with -DUSE_KERBEROS or
> not.

Is this for net_unittests?

http://codereview.chromium.org/7633006/diff/1/net/net.gyp#newcode1096
net/net.gyp:1096: 'http/mock_gssapi_library_posix.h',
There are a number of Negotiate-oriented tests in
http/http_auth_sspi_win_unittest.cc

If there is a way to restrict use_kerberos==0 to only be valid for Linux via
GYP, that seems completely fine.

[Paweł Hajdan Jr., 2011-08-12 18:14:42]

http://codereview.chromium.org/7633006/diff/1/net/http/http_auth_handler_fact...
File net/http/http_auth_handler_factory_unittest.cc (right):

http://codereview.chromium.org/7633006/diff/1/net/http/http_auth_handler_fact...
net/http/http_auth_handler_factory_unittest.cc:183: }
On 2011/08/12 17:21:51, wtc wrote:
> Add a #else block here to perform a negative test.
> 
> http_auth_handler_factory->CreateAuthHandlerFromString(
>     "Negotiate",
> 
> should fail with an expected error (I forgot what that is)
> if the code is not compiled with -DUSE_KERBEROS.

Done.

http://codereview.chromium.org/7633006/diff/1/net/http/http_auth_unittest.cc
File net/http/http_auth_unittest.cc (right):

http://codereview.chromium.org/7633006/diff/1/net/http/http_auth_unittest.cc#...
net/http/http_auth_unittest.cc:116: }
On 2011/08/12 17:21:51, wtc wrote:
> This probably can also have a #else block.

Done.

http://codereview.chromium.org/7633006/diff/1/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/7633006/diff/1/net/net.gyp#newcode9
net/net.gyp:9: 'use_kerberos%': 1,
On 2011/08/12 17:21:51, wtc wrote:
> Should we remove the blank line?

Not sure, use_kerberos is quite different from chromium_code, so I wanted the
layout to indicate another "group". If you prefer, I can remove that empty line
of course.

> What does the '%' character mean?

% means default value. use_kerberos is meant to be overridden from command line,
chromium_code isn't.

http://codereview.chromium.org/7633006/diff/1/net/net.gyp#newcode713
net/net.gyp:713: },
On 2011/08/12 17:59:06, cbentzel wrote:
> On 2011/08/12 17:21:51, wtc wrote:
> > It seems that a direct dependent should not need to know
> > whether the net module is compiled with -DUSE_KERBEROS or
> > not.
> 
> Is this for net_unittests? 

Yes, it's for net_unittests. Of course I could duplicate the define[] logic in
net_unittests target, but I'm not sure if that's the best idea.

Actually it makes sens for clients of net/ to know whether it supports Kerberos.

[cbentzel, 2011-08-12 18:20:13]

http://codereview.chromium.org/7633006/diff/1/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/7633006/diff/1/net/net.gyp#newcode713
net/net.gyp:713: },
On 2011/08/12 18:14:42, Paweł Hajdan Jr. wrote:
> On 2011/08/12 17:59:06, cbentzel wrote:
> > On 2011/08/12 17:21:51, wtc wrote:
> > > It seems that a direct dependent should not need to know
> > > whether the net module is compiled with -DUSE_KERBEROS or
> > > not.
> > 
> > Is this for net_unittests? 
> 
> Yes, it's for net_unittests. Of course I could duplicate the define[] logic in
> net_unittests target, but I'm not sure if that's the best idea.
> 
> Actually it makes sens for clients of net/ to know whether it supports
Kerberos.

Beyond net_unittests I do not agree. I suppose some could try to create an
HttpAuthHandlerNegotiate, but I believe that the external API is
HttpAuthHandlerFactory::CreateDefault which is most used.

[Paweł Hajdan Jr., 2011-08-12 18:24:15]

http://codereview.chromium.org/7633006/diff/1/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/7633006/diff/1/net/net.gyp#newcode713
net/net.gyp:713: },
On 2011/08/12 18:20:13, cbentzel wrote:
> Beyond net_unittests I do not agree. I suppose some could try to create an
> HttpAuthHandlerNegotiate, but I believe that the external API is
> HttpAuthHandlerFactory::CreateDefault which is most used.

Do you want me to duplicate define: ['USE_KERBEROS'] for net_unittests? I can do
that, just note that _every_ other case I know about uses
direct_dependent_settings.

[cbentzel, 2011-08-14 13:04:57]

http://codereview.chromium.org/7633006/diff/1/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/7633006/diff/1/net/net.gyp#newcode713
net/net.gyp:713: },
On 2011/08/12 18:24:15, Paweł Hajdan Jr. wrote:
> On 2011/08/12 18:20:13, cbentzel wrote:
> > Beyond net_unittests I do not agree. I suppose some could try to create an
> > HttpAuthHandlerNegotiate, but I believe that the external API is
> > HttpAuthHandlerFactory::CreateDefault which is most used.
> 
> Do you want me to duplicate define: ['USE_KERBEROS'] for net_unittests? I can
do
> that, just note that _every_ other case I know about uses
> direct_dependent_settings.

I'd prefer to duplicate it in net_unittests. If other dependents end up needing
it, then keep it as direct_dependent_settings.

[Paweł Hajdan Jr., 2011-08-15 17:07:53]

PTAL

http://codereview.chromium.org/7633006/diff/1/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/7633006/diff/1/net/net.gyp#newcode713
net/net.gyp:713: },
On 2011/08/14 13:04:57, cbentzel wrote:
> On 2011/08/12 18:24:15, Paweł Hajdan Jr. wrote:
> > On 2011/08/12 18:20:13, cbentzel wrote:
> > > Beyond net_unittests I do not agree. I suppose some could try to create an
> > > HttpAuthHandlerNegotiate, but I believe that the external API is
> > > HttpAuthHandlerFactory::CreateDefault which is most used.
> > 
> > Do you want me to duplicate define: ['USE_KERBEROS'] for net_unittests? I
can
> do
> > that, just note that _every_ other case I know about uses
> > direct_dependent_settings.
> 
> I'd prefer to duplicate it in net_unittests. If other dependents end up
needing
> it, then keep it as direct_dependent_settings.

Done.

[cbentzel, 2011-08-15 17:14:22]

LGTM

[wtc, 2011-08-15 22:43:06]

LGTM.

http://codereview.chromium.org/7633006/diff/10001/net/http/http_auth_handler_...
File net/http/http_auth_handler_factory.cc (right):

http://codereview.chromium.org/7633006/diff/10001/net/http/http_auth_handler_...
net/http/http_auth_handler_factory.cc:15: #endif
The Chromium Coding Style page recommends that we list the
headers inside #ifdef's last:

http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code

[Paweł Hajdan Jr., 2011-08-18 17:04:36]

http://codereview.chromium.org/7633006/diff/10001/net/http/http_auth_handler_...
File net/http/http_auth_handler_factory.cc (right):

http://codereview.chromium.org/7633006/diff/10001/net/http/http_auth_handler_...
net/http/http_auth_handler_factory.cc:15: #endif
On 2011/08/15 22:43:06, wtc wrote:
> The Chromium Coding Style page recommends that we list the
> headers inside #ifdef's last:
> 
> http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code

Done here: http://codereview.chromium.org/7655046/