Fixes for S3 DCE/RPC GSSAPI with Heimdal

[Andrew Bartlett]

On Tue, 2011-04-26 at 08:11 -0400, simo wrote:
> On Tue, 2011-04-26 at 15:21 +1000, Andrew Bartlett wrote:
> > On Tue, 2011-04-26 at 00:29 -0400, simo wrote:
> > > On Tue, 2011-04-26 at 09:24 +1000, Andrew Bartlett wrote:
> > > > On Mon, 2011-04-25 at 07:48 -0400, simo wrote:
> > 
> > > > > I explicilty avoided to make a mess by combining all the old manual
> > > > > gssapi stuff and kerberos wrapper, so that we can make head and tails of
> > > > > the new stuff. The idea was to then slowly start replacing also the
> > > > > manual gssapi stuff with gse_* functions my moving the gse stuff in
> > > > > block into a common dir if necessary. But still keeping it separate from
> > > > > the old cruft.
> > > > 
> > > > I can put it in libcli/auth/gssapi_pac.c if you prefer.  I want to have
> > > > it in the top level because a later patch in the series uses it for
> > > > Samba4's PAC needs as well.  (As I said at the outset, I want to do this
> > > > right, once for all of Samba). 
> > > > 
> > > > I'm sorry that we never really spoke about your aims and objectives for
> > > > the GSE code, so it seems I've taken a different direction to what you
> > > > were aiming for.  I wasn't aware you wanted to make the GSE layer the
> > > > common GSSAPI abstraction across all of Samba.  
> > > 
> > > I certainly do not want to have dependencies all over the code again, so
> > > we definitely need to discuss how merging is done.
> > 
> > Simo,
> > 
> > I'm sorry, I'm having difficulty pinning down your concerns.  Can you
> > point out exactly what dependencies you are concerned about, so I can
> > try and address this?
> > 
> > Is is the creation of gssapi_error_string with the other krb5
> > compatibility wrapper functions?
> 
> Yes, I'd like to avoid that for now.
> 
> > Is it moving the PAC blob fetching function in common, where I can use
> > it with Samba4 as well?
> 
> The PAC is a bit of a special case, I can see the value in having it in
> common.
> 
> > > > We could certainly do that, and perhaps we can work on that at SambaXP? 
> > > 
> > > Yup, although I won't be there for long, so grab me as soon as you can
> > > or it might be too late.
> > > 
> > > > My short-term aim was just to pull the PAC parsing and verification as
> > > > low in the stack as possible, to remove the double-verification, and put
> > > > as much as possible of it in common.  
> > > 
> > > I think we've lived with duplication long enough that we can avoid
> > > pulling at all costs. I know it is tempting, but there many other things
> > > that needs to be done too, this is not that urgent, except for the part
> > > where we make things work with both Kerberos implementations.
> > 
> > I'm sorry, I'm not sure I understand.  Your preference is that having
> > done this work, that we keep it in duplicate?  
> > 
> > With the exception that we have these basic functions in common (which
> > allows much better testing, because Samba4 does much more kerberos
> > testing) the patches I have proposed are the minimum to have Samba3 use
> > a secure, authenticated PAC, and to get Samba3 tested in combination
> > with Samba4.
> > 
> > I hope this updated branch addresses your concerns:
> > http://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/krb5-fix
> 
> Moving the pac extraction in another file is ok, although "libcli" as
> the place where to put it sounds wrong as the PAC is always verified on
> servers not clients.

It is also the location of the NTLMSSP server side code, and the common
kerberos code (including other code that has most use on the server). 

However, given your concerns I'll move it and the kerberos_pac.c to
auth/kerberos.

> You removed gse_get_authz_data() although I asked you to leave it there
> (not the use of it, the function itself).

I removed it because after the long series of discussions last week it
became clear that it would only cause a future implementer to take the
wrong path.  Any future authz data would need to be extracted via a very
similar mechanism as the PAC to be secure - directly reading the first
AD-IF-RELEVENT authz data will always do the wrong (insecure) thing.

I won't remove it however, and you can remove it yourself if you agree. 

> You still unconditionally remove gss_release_oid() when I asked you to
> ifdef it out for heimdal given it has problems, but MIT technically
> requires it.

Does anyone on the list know a sane way to detect this behaviour, or at
the very least if we have compiled against heimdal?  I can't key off
SAMBA4_INTERNAL_HEIMDAL because the same should, if I understand
correctly, happen against a system heimdal that Samba3 happens to be
compiled against. 

I do find this situation (and the lack of any clear documentation
describing the correct course of action) very frustrating.  Sadly my
frustrating isn't enough to cause this situation not to exist.  

> Please do not move gse_errstr() I prefer it to be duplicate but have the
> code confined within the gse framework, that function is super simple
> and we don't gain much by making it common, except more confusion when
> you read the gse code.

Sure.  I was trying to avoid having 3 copies of it (including the copy
in source4/auth/gensec_gssapi.c), as I need it in the common PAC code.  

I'll prepare another patch series today. 

Andrew Bartlett

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