Fixes for S3 DCE/RPC GSSAPI with Heimdal

[Andrew Bartlett]

Luke,

I've asked you below about this history of PAC functions in MIT.  Did
MIT kerberos ever allow PAC access without doing the crypto, authtime
and principal checking?

On Sat, 2011-04-16 at 12:16 -0400, simo wrote:
> 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 ?

Because it was unused, and the Heimdal function involved takes an open
keytab, not a name.  I just didn't want to open and figure out how to
reference an additional keytab if no caller used that facility.  Do you
have plans for this function that involve a keytab that Samba doesn't
manage?

> 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.

Yes, it was just about the unused vars.  I stopped doing that in the
later patches for exactly that reason.  Sorry, 

> > 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.

Wow!

> 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).

I'll look into this again, I know the two have API differences, but this
one takes the cake for subtle breakages!

If I can show that the MIT API also returns a constant but allows
(rather than requires) gss_release_oid() to avoid a segfault, would you
be willing to avoid this call?

> > 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.

Sure, I can do that. 

> > 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 think the issue is that in both cases that particular OID isn't
actually connected.  So the API call (essentially an ioctl) exists, but
if there isn't an implementation for that particular OID, then we error.

> I had no problems with this testing with MIT, and I am not completely
> sure MIT does the checks itself (haven't checked).

MIT does check, I found this out when chasing other bugs a while ago,
because they found a DES-related bug a while ago in that code, which
inspired my fix :-).  

Luke:  Do you know if there are version of MIT Kerberos with PAC access
functions that don't do the PAC checks?

You can't do the check properly anyway (because you don't have the key,
and the principal name is the part you really need to check to make this
secure), so why not leave this to the library to do?

> > 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.

We could finally decide to use C99 and allow inline variables.  We use
so much of C99 anyway :-)

> > 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.

I'll fix this up and send it back to you. 

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org