Fixes for S3 DCE/RPC GSSAPI with Heimdal

[Andrew Bartlett]

On Sun, 2011-04-17 at 19:08 -0400, simo wrote:
> On Mon, 2011-04-18 at 06:59 +1000, Andrew Bartlett wrote:
> > 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?
> 
> Technically yes, but I haven't needed it so far. I just don't want to
> break Heimdal later if it turns out I need it and revert this change.

OK, so where do we go from here?  Can we add code to open the keytab and
keep hold of it etc when we find an actual need?  The approach of using
an open keytab avoids a use of a global variable that you had a FIXME
for.  

> > > > 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?
> 
> I know MIT often returns constants, but it clearly states it may not in
> some cases. If you are sure we never hit a case where a non-constant is
> returned I guess we can remove this call. I just don't want to have to
> chase mem leaks a few years down the road.

I've been unable to find any documentation from MIT for this function in
krb5-devel on my Fedora 14 system.  Where is that detail hidden?  

Reading the krb5 sources in your git repo, all the codepaths I could
find indicated that this returns a constant. 

My gut feeling is that MIT Kerberos returning a non-constant here would
introduce memory leaks into a lot of code. 

> > 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?
> 
> If we are sure this is always done for us, I am ok removing it.

Thanks.  Hopefully Luke knows the history and can provide some
reassurance.  Ever since Heimdal started to do this for us, I've thought
'this will be a real pain to prove in a configure test when the time
comes'...

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

I've fixed things up again, and found out how to use the generic
gss_inquire_sec_context_by_oid() to get the PAC from Heimdal.  So, there
are now less #ifdefs in the code.

http://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/krb5-fix

Please let me know what you think.

Andrew Bartlett

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