Linux: allow linking directly with Kerberos instead of using dlopen.

Linux: allow linking directly with Kerberos instead of using dlopen.

dlopen is still the default for Google Chrome. This option
is intended for Linux distro packagers.

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

[cbentzel, 2011-08-18 21:21:17]

http://codereview.chromium.org/7655046/diff/1/net/http/http_auth_gssapi_posix.h
File net/http/http_auth_gssapi_posix.h (right):

http://codereview.chromium.org/7655046/diff/1/net/http/http_auth_gssapi_posix...
net/http/http_auth_gssapi_posix.h:9: #include <gssapi.h>
Is the third_party one included in the search path? Or is this using the system
one always, and we assume that if you build without USE_KERBEROS that gssapi.h
is installed?

http://codereview.chromium.org/7655046/diff/1/net/http/mock_gssapi_library_po...
File net/http/mock_gssapi_library_posix.cc (right):

http://codereview.chromium.org/7655046/diff/1/net/http/mock_gssapi_library_po...
net/http/mock_gssapi_library_posix.cc:281: *output_name =
reinterpret_cast<gss_name_t>(output);
Not a bad thing - but was this needed? 

The gssapi.h header in net/third_party has it defined as a void*, and casts from
void* to other types are allowed.

Also, this may need to be a static_cast rather than a reinterpret_cast

http://stackoverflow.com/questions/310451/should-i-use-static-cast-or-reinter...

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

http://codereview.chromium.org/7655046/diff/1/net/net.gyp#newcode10
net/net.gyp:10: 'linux_link_kerberos%': 0,
Should linux be removed from the name?

[Paweł Hajdan Jr., 2011-08-18 23:30:32]

http://codereview.chromium.org/7655046/diff/1/net/http/http_auth_gssapi_posix.h
File net/http/http_auth_gssapi_posix.h (right):

http://codereview.chromium.org/7655046/diff/1/net/http/http_auth_gssapi_posix...
net/http/http_auth_gssapi_posix.h:9: #include <gssapi.h>
On 2011/08/18 21:21:17, cbentzel wrote:
> Is the third_party one included in the search path? Or is this using the
system
> one always, and we assume that if you build without USE_KERBEROS that gssapi.h
> is installed?

This is using the system one. When not using Kerberos we're not using this file.

http://codereview.chromium.org/7655046/diff/1/net/http/mock_gssapi_library_po...
File net/http/mock_gssapi_library_posix.cc (right):

http://codereview.chromium.org/7655046/diff/1/net/http/mock_gssapi_library_po...
net/http/mock_gssapi_library_posix.cc:281: *output_name =
reinterpret_cast<gss_name_t>(output);
On 2011/08/18 21:21:17, cbentzel wrote:
> Not a bad thing - but was this needed? 
> 
> The gssapi.h header in net/third_party has it defined as a void*, and casts
from
> void* to other types are allowed.
> 
> Also, this may need to be a static_cast rather than a reinterpret_cast
> 
>
http://stackoverflow.com/questions/310451/should-i-use-static-cast-or-reinter...
> 

when using the system header static_cast doesn't work:

net/http/mock_gssapi_library_posix.cc:281: error: invalid static_cast from type
‘net::test::GssNameMockImpl*’ to type ‘gss_name_struct*’

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

http://codereview.chromium.org/7655046/diff/1/net/net.gyp#newcode10
net/net.gyp:10: 'linux_link_kerberos%': 0,
On 2011/08/18 21:21:17, cbentzel wrote:
> Should linux be removed from the name? 

This follows the convention used for gnome-keyring. I can update the name if you
want.

[wtc, 2011-08-18 23:48:05]

Can we just always link with krb5-config --libs gssapi
directly?  That'll simplify the code.

[cbentzel, 2011-08-19 12:22:35]

Evan - I'm not familiar enough with how we package/ship our Linux builds.

Currently for Kerberos authentication we dlopen/dlsym one of a few gssapi
library names when we see a Negotiate header.

Pawel's changing this to be linked directly, specifically to help gentoo.

Should we just take this approach for all Linux builds? My two concerns are that
the gssapi libraries may not be installed on a system which wants to run Chrome,
and will require another package dependency. Another is that it may have a small
startup time cost for the majority of Linux users who never see a Negotiate
authentication challenge.

http://codereview.chromium.org/7655046/diff/1/net/http/http_auth_gssapi_posix.h
File net/http/http_auth_gssapi_posix.h (right):

http://codereview.chromium.org/7655046/diff/1/net/http/http_auth_gssapi_posix...
net/http/http_auth_gssapi_posix.h:9: #include <gssapi.h>
On 2011/08/18 23:30:32, Paweł Hajdan Jr. wrote:
> On 2011/08/18 21:21:17, cbentzel wrote:
> > Is the third_party one included in the search path? Or is this using the
> system
> > one always, and we assume that if you build without USE_KERBEROS that
gssapi.h
> > is installed?
> 
> This is using the system one. When not using Kerberos we're not using this
file.

What I'm asking is whether all developer machines have a system level gssapi.h
installed and on the include path, as well as all build bots. You may need to
modify install-build-deps.sh. You should also remove
net/third_party/gssapi/gssapi.h in this CL.

http://codereview.chromium.org/7655046/diff/1/net/http/mock_gssapi_library_po...
File net/http/mock_gssapi_library_posix.cc (right):

http://codereview.chromium.org/7655046/diff/1/net/http/mock_gssapi_library_po...
net/http/mock_gssapi_library_posix.cc:281: *output_name =
reinterpret_cast<gss_name_t>(output);
On 2011/08/18 23:30:32, Paweł Hajdan Jr. wrote:
> On 2011/08/18 21:21:17, cbentzel wrote:
> > Not a bad thing - but was this needed? 
> > 
> > The gssapi.h header in net/third_party has it defined as a void*, and casts
> from
> > void* to other types are allowed.
> > 
> > Also, this may need to be a static_cast rather than a reinterpret_cast
> > 
> >
>
http://stackoverflow.com/questions/310451/should-i-use-static-cast-or-reinter...
> > 
> 
> when using the system header static_cast doesn't work:
> 
> net/http/mock_gssapi_library_posix.cc:281: error: invalid static_cast from
type
> ‘net::test::GssNameMockImpl*’ to type ‘gss_name_struct*’

Ah, so gss_name_t is no longer a void* in the system gssapi.h you are building
on.

This is probably OK. I believe reinterpret_cast is still OK here.

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

http://codereview.chromium.org/7655046/diff/1/net/net.gyp#newcode10
net/net.gyp:10: 'linux_link_kerberos%': 0,
On 2011/08/18 23:30:32, Paweł Hajdan Jr. wrote:
> On 2011/08/18 21:21:17, cbentzel wrote:
> > Should linux be removed from the name? 
> 
> This follows the convention used for gnome-keyring. I can update the name if
you
> want.

This is fine for consistency.

[Evan Martin, 2011-08-19 17:19:08]

On 2011/08/18 23:48:05, wtc wrote:
> Can we just always link with krb5-config --libs gssapi
> directly?  That'll simplify the code.

$ apt-cache rdepends libkrb5-3
libkrb5-3
Reverse Depends:
  libcamel1.2-14
  winbind
  gnome-settings-daemon
  evolution-data-server
  [...many more...]


It looks like this will be available on any Ubuntu system as well.  Who wrote
the initial patch?

[Evan Martin, 2011-08-19 17:19:49]

On 2011/08/19 17:19:08, Evan Martin wrote:
> Who wrote
> the initial patch?

I ask because they may know why we went the more complicated dlopen route.  I'm
fine with depending on this.

[cbentzel, 2011-08-19 17:21:59]

I wrote the initial patch, essentially mirroring firefox's approach.

I'm fine with this route.

On Fri, Aug 19, 2011 at 1:19 PM,  <evan@chromium.org> wrote:
> On 2011/08/19 17:19:08, Evan Martin wrote:
>>
>> Who wrote
>> the initial patch?
>
> I ask because they may know why we went the more complicated dlopen route.
>  I'm
> fine with depending on this.
>
> http://codereview.chromium.org/7655046/
>

[Evan Martin, 2011-08-19 17:23:54]

Lei, do you know how we can tell whether we expect krb5 to be available on all
our users' systems?  I guess for RPM systems we need to check LSB?

[Lei Zhang, 2011-08-19 18:26:21]

On 2011/08/19 17:23:54, Evan Martin wrote:
> Lei, do you know how we can tell whether we expect krb5 to be available on all
> our users' systems?  I guess for RPM systems we need to check LSB?

lsb doesn't have a direct dependency on krb5.

[Paweł Hajdan Jr., 2011-08-22 18:39:52]

I'm not sure if it's possible to make Google Chrome link to Kerberos directly.
Note that this scenario would require binary compatibility, and we don't control
the entire stack.

1. dlopen code tries different library names. I believe this would break when
linking directly, because just one name would get hardcoded.

2. Output of krb5-config --libs gssapi differs between systems and Kerberos
implementations.

Note that the problems above are only important for binary compatibility. For
distros that control the entire stack this is not an issue.

[Evan Martin, 2011-08-22 23:45:17]

I agree with cbentzel's comments (you should remove the local gssapi.h in this
change, need to check that all the bots are fixed and update install-build-deps
/ update the wiki).

Typically whenever we make this sort of change someone loses 20 minutes trying
to track down why their system broke, it'd be nice to try to prevent that.

http://codereview.chromium.org/7655046/diff/1/net/http/http_auth_handler_nego...
File net/http/http_auth_handler_negotiate_unittest.cc (right):

http://codereview.chromium.org/7655046/diff/1/net/http/http_auth_handler_nego...
net/http/http_auth_handler_negotiate_unittest.cc:346: #if
defined(DLOPEN_KERBEROS)
Maybe add a comment here explaining that this test doesn't make sense with
non-dlopen kerberos.

[wtc, 2011-08-23 00:56:25]

The reason I suggested linking with gssapi directly is to
simplify the code.

I am fine with choosing MIT Kerberos in the official Chrome
builds.  We can simply link with -lgssapi_krb5 because it is
hardcoded on line 433 of net/http/http_auth_gssapi_posix.cc.

http://codereview.chromium.org/7655046/diff/1/net/http/http_auth_gssapi_posix.cc
File net/http/http_auth_gssapi_posix.cc (right):

http://codereview.chromium.org/7655046/diff/1/net/http/http_auth_gssapi_posix...
net/http/http_auth_gssapi_posix.cc:437: "libgssapi_krb5.so.2",  // MIT Kerberos
- FC, Suse10, Debian
Note that there is only one SONAME for MIT Kerberos in this
table.

This implies that if we standardize on MIT Kerberos, then we
can simply link with -lgssapi_krb5 and the binary will work
on FC, Suse10, Debian.

[cbentzel, 2011-08-23 01:08:56]

On Mon, Aug 22, 2011 at 8:56 PM,  <wtc@chromium.org> wrote:
> The reason I suggested linking with gssapi directly is to
> simplify the code.
>
> I am fine with choosing MIT Kerberos in the official Chrome
> builds.  We can simply link with -lgssapi_krb5 because it is
> hardcoded on line 433 of net/http/http_auth_gssapi_posix.cc.

OSX Lion has moved to Heimdal, albeit with an MIT Kerberos shim.

We also have a GSSAPILibraryName entry in group policy. I'm not sure
how many people are using it, but removing the dlopen portion of the
code will prevent that from working.

>
>
>
http://codereview.chromium.org/7655046/diff/1/net/http/http_auth_gssapi_posix.cc
> File net/http/http_auth_gssapi_posix.cc (right):
>
>
http://codereview.chromium.org/7655046/diff/1/net/http/http_auth_gssapi_posix...
> net/http/http_auth_gssapi_posix.cc:437: "libgssapi_krb5.so.2",  // MIT
> Kerberos - FC, Suse10, Debian
> Note that there is only one SONAME for MIT Kerberos in this
> table.
>
> This implies that if we standardize on MIT Kerberos, then we
> can simply link with -lgssapi_krb5 and the binary will work
> on FC, Suse10, Debian.
>
> http://codereview.chromium.org/7655046/
>

[Paweł Hajdan Jr., 2011-08-23 19:17:13]

PTAL

I think the safest plan would be to commit this for now (it doesn't change
anything for Google Chrome) and let distros do some experiments (for example
switching between MIT Kerberos and Heimdal and checking if the binary
compatibility is preserved - probably not).

Again, please note this just mimics the infrastructure for gnome-keyring, which
to the best of my knowledge has negligible maintenance cost.

http://codereview.chromium.org/7655046/diff/1/net/http/http_auth_gssapi_posix.cc
File net/http/http_auth_gssapi_posix.cc (right):

http://codereview.chromium.org/7655046/diff/1/net/http/http_auth_gssapi_posix...
net/http/http_auth_gssapi_posix.cc:437: "libgssapi_krb5.so.2",  // MIT Kerberos
- FC, Suse10, Debian
On 2011/08/23 00:56:25, wtc wrote:
> Note that there is only one SONAME for MIT Kerberos in this
> table.
> 
> This implies that if we standardize on MIT Kerberos, then we
> can simply link with -lgssapi_krb5 and the binary will work
> on FC, Suse10, Debian.

This also implies that we'd be broken with Heimdal.

[cbentzel, 2011-08-23 20:04:22]

LGTM

This doesn't have the change to build/install-build-deps.sh
Was this because debian distros which are supported should already have it?
Evan's rdepends command seems to indicate that.

[Evan Martin, 2011-08-23 20:20:30]

I think we might need to install the -dev package.  Please don't land this
unless you're sure the bots won't break.

[Dmitry Polukhin, 2011-08-24 07:45:30]

http://codereview.chromium.org/7655046/diff/15001/net/http/http_auth_gssapi_p...
File net/http/http_auth_gssapi_posix.h (right):

http://codereview.chromium.org/7655046/diff/15001/net/http/http_auth_gssapi_p...
net/http/http_auth_gssapi_posix.h:9: #include <gssapi.h>
It looks like this CL broke Chrome OS build:

chromeos-chrome-15.0.859.0_rc-r1: In file included from
net/http/http_auth_gssapi_posix.cc:5:
chromeos-chrome-15.0.859.0_rc-r1: ./net/http/http_auth_gssapi_posix.h:9:20:
error: gssapi.h: No such file or directory
chromeos-chrome-15.0.859.0_rc-r1: In file included from
./net/http/http_auth_handler_negotiate.h:21,
chromeos-chrome-15.0.859.0_rc-r1:                  from
net/http/http_auth_handler_factory.cc:16:
chromeos-chrome-15.0.859.0_rc-r1: ./net/http/http_auth_gssapi_posix.h:9:20:
error: gssapi.h: No such file or directory
chromeos-chrome-15.0.859.0_rc-r1: In file included from
./net/http/http_auth_handler_negotiate.h:21,
chromeos-chrome-15.0.859.0_rc-r1:                  from
net/http/http_auth.cc:14:
chromeos-chrome-15.0.859.0_rc-r1: ./net/http/http_auth_gssapi_posix.h:9:20:
error: gssapi.h: No such file or directory