Fixes for S3 DCE/RPC GSSAPI with Heimdal

[simo]

On Mon, 2011-04-18 at 12:07 +1000, Andrew Bartlett wrote:
> 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.  

Yes, I guess we can.

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

I've read the source code, at some point there are comments in that
direction as Luke confirmed.

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

See Luke's answer.

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

Indeed, but Luke explained the issue better.

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

The patch about getting the pac looks much better to me now.
The gss_release_oid() removal is still controversial, and for the last
one I guess we need to discuss it a bit with Luke after what he wrote.

Simo.

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