Fixes for S3 DCE/RPC GSSAPI with Heimdal

[simo]

On Sat, 2011-04-16 at 19:58 +1000, Andrew Bartlett wrote:
> Simo,
> 
> I've been working to test the Samba3 binaries produced by the top level
> build, and this builds against Samba4's Heimdal at this time.
> 
> When you proposed your DCE/RPC GSSAPI patches, you asked that I check
> them against Heimdal, and sadly I only got as far are compiling them,
> not running them.
> 
> These patches makes the DCE/RPC GSSAPI server work with the newly added
> ktest tests in Samba3's make test, when run from the top level build. 
> 
> Can you let me know if these changes are OK, or if you want some further
> explaination?
> 
> http://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/krb5-fix
> 
> In particular I'm referring to:
> 
> s3-gse: Allow the GSSAPI wrapper to load a keytab using
> gss_krb5_import_cred():
> http://git.samba.org/?p=abartlet/samba.git/.git;a=commitdiff;h=abfe0bb3a73a3d00d0e75ae2405bf064f6abbf89

Why did you remove the ability to pass in an arbitrary keytab ?

On the style:
Why that code block in the middle where you moved variables previously
on top ?
It makes the patch more complex to read for no discernible reason (is it
just to avoid warnings about unused vars by chance ?), took me twice the
time to read it, to find out most of it was basically the same code as
before.

> s3-gse: Don't release the mech OID from gss_accept_security_context
> http://git.samba.org/?p=abartlet/samba.git/.git;a=commitdiff;h=e5eadad3bce2b1f57ffb01aa65b6880fd5fe20c4

The MIT API seem to require you to free it.
MIT's gss_release_oid() internally recognizes if the OID is one of its
constants and in that case it just returns without freeing.

If Heimdal doesn't do that you'll need an ifdef, or an auxiliary
function named gse_release_oid() with the ifdef in it (my preferred way
to deal with ifdefs is to bury them into their own wrapper function).

> s3-gse Use Heimdal gsskrb5_extract_authz_data_from_sec_context when
> available:
> http://git.samba.org/?p=abartlet/samba.git/.git;a=commitdiff;h=8b6c1c2b51566f74518bfa7ab4829c011b49cbad

I see the comment on this one, but it looks very strange it doesn't work
in Heimdal, this is normal GSSAPI stuff.

I see that the function you use is also available in the MIT code, so
perhaps leave gse_get_authz_data() alone and instead create a
gse_get_pac_data() that calls gse_get_authz_data() + unwrap_pac() as a
fallback.

I want to keep this authz function more neutral as I want later to be
able to plug in easily the ability to fetch different authz data in
future.

> s3-gse Don't get the auth time when validating the PAC:
> http://git.samba.org/?p=abartlet/samba.git/.git;a=commitdiff;h=85350063468dca54aefc4cc905d13d4aaa81ddd0

I don't like this one.
If it breaks against Heimdal can you simply ifdef out it's usage only
when Heimdal is used, adding a comment that it doesn't work with
Heimdal ? (Why wouldn't it ?? If it already does the check, shouldn't
this one just work as well ?)
I had no problems with this testing with MIT, and I am not completely
sure MIT does the checks itself (haven't checked).

> I don't yet have the autoconf/waf tests for the new macros (allowing a
> build against a system Heimdal), but I'll add those soon.  What I'm
> after at the moment is your comment on the meat of these patches.   
> 
> I can also address the unused variables (in each arm of the #if/#else),
> but didn't want the patches to be full of just noise at this stage of
> review). 

I am ok leaving unused variables in the code without ifdefs, I prefer a
few warnings then ugly code blocks in the middle of a function just to
add a couple of variables. But this is just style preference, if people
feel strongly about unused vars warnings, we can find a way to deal with
properly ifdeffing also unused vars.

> The good news is that with these patches, we can successfully test
> Samba3 from the top level build, and means we are but a short way away
> from testing Samba3 in combination with Samba4 in the combined build. 

That's nice,
thanks.

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>