[RFC] fix bug 12007

[Simo]

On Mon, 2016-07-04 at 21:44 +0300, Uri Simchoni wrote:
> On 07/04/2016 06:50 PM, Simo wrote:
> > 
> > Sorry have been thinking more.
> > 
> <snip>
> > 
> > > 
> > > > 
> > > > > 
> > > > > of Kerberos. The attached fix replaces gss_acquire_cred by
> > > > > gss_krb5_import_cred().
> > This sounds wrong, and we should fix Heimdal if it is trying to re-
> > acquire the same ticket, not try to work around it.
> > 
> > Are you saying the current ccache has a valid TGT but it ignores it
> > and
> > tries to acquire a new one ?
> > 
> > If there are no credentials it is perfectly normal to make an AS
> > request, I do not understand what the problem is here. Do we, by
> > chance
> > copy in some ccache just the LDAP ticket and not the TGT ?
> > 
> The problem I am trying to fix is that winbindd is emitting AS
> requests
> for root at realm, and this happens from within gss_acquire_cred().
> Heimdal
> is trying to acquire the wrong TGT, if no TGT exist in the ccache,
> and
> in a way it has no chance of succeeding whatsoever (it doesn't have
> the
> password of root so what good would a TGT of root do, even if we got
> one?).
> 
> Thinking about it some more, the issue is more fundamental:
> 1. libads expects that the default ccache either has only a TGT of
> the
> "right" user (the one the application has specified for libads to use
> -
> machine account or otherwise), or that its attempt to bind would
> fail.
> Upon failure it would do a kinit and retry.
> 2. Heimdal's gss_acquire_cred() OTOH, when encountering an empty
> ccache,
> tries to get credentials from the default keytab. It is in this code
> path that the AS request for root is made.
> 3. Usually this extra attempt with the keytab would fail, creating
> the
> nuisance with the AS request - maybe we can fix that with Heimdal for
> the case where no keytab exists.
> 4. But what if a keytab does exist, and contains valid password for
> the
> current user? In that case, gss_acquire_cred() might obtain the
> credentials of the current user, whereas libads expected to be of its
> user - could be a username specified via the -U parameter, or the
> machine account.

It seem like the whole setup is incorrect if these things happen

1. libads as any other program using GSSAPI rightfully expect to have
the proper crdentials, so the application or the user calling it needs
to provide appropriate credentials or fail.

2/3. If Heimdal tries to perform an AS request when it doesn't have a
keytab then I think Heimdal needs to be fixed.

4. In MIT a keytab is used to acquire credentials for Initiation only
if the CLIENT_KRB5_KTNAME variable is set, so again it seem that
Heimdal is not behaving properly here, and causing unnecessary churn.

If the application is properly set up (access to keytab provided if,
and only if needed, otherwise proper credentials provided) then a
gss_acquire_cred() or a gss_init_sec_context() call [in MIT acquiring
creds can be delayed to the context establishment call in some cases]
should cause an AS request only when intended (no valid TGT in the
ccache and a keytab has been provided).

> Up until the April security release, gensec_gse was used only for SMB
> connections, and there we don't seem to reuse a TGT between
> connections

This looks like a bug.

> - we always do a kinit first (in cli_session_setup_spnego_send()).

This is inefficient, is there a good reason why ?

> Libads does something new in that respect.

We should probably fix libads, in general we've shrouded this whole
process with way too much magic and made this area of the code obscure
and hard to deal with in the process unfortunately. We should unwind it
if enough energy to do that can be found, it helps nobody when it is
hard to understand what credentials are used and how, and there lie
potential bugs.

> The fix tries to probe the ccache more gently than
> gss_acquire_cred().

The "fix" is incorrect I am afraid.

>  I don't know why Heimdal is also trying the keytab, but they would
> probably insist on doing it. Assuming that's true, the fix is to
> always do a kinit before the bind attempt (or to improve libads's
> integration with gensec and cli_credentials, because those track the
> content of the ccache without a need to probe it).

A kinit requires access to credentials, which is not granted when you
are a physical user that has a perfectly valid TGT in the ccache.

I understand this is a performance issue mostly, because if you do not
have valid credentials the end state is just that you get a failure.
however I am confused by the fact you are trying to import an existing
ccache. Does it mean you have a valid ccache but that this ccache is
not the *default* ccache for the process and is not being found ?

For MIT you can use the gss_acquire_cred_from() interface in that case
(you pass a "ccache: name" pair in). I do not think Heimdal implements
it yet unfortunately, in that case you can try to have a fallback
function to call gss_krb5_ccache_name() which sets the default ccache
name before the regular gss_acquire_cred()

Be careful with this one as it applies to *any* further gssapi calls
within the process, so it could lead to undesired results if multiple
gss_acquire_cred(0 calls are performed. Unsetting the default ccache
(and resetting it to the actual default ccache) can be done by passing
NULL as name, so this may be made safe by using a tight loop and
perhaps some locking (if threads are being used).

Simo.

> > 
> > > 
> > > > 
> > > > > 
> > > > > I'd like some feedback from those familiar with this code -
> > > > > 1. It could be that the right fix is in Heimdal
> > Definitely if it really is a bug in finding a valid credential in
> > the
> > cache.
> > 
> > > 
> > > > 
> > > > > 
> > > > > 2. The reason for acquiring the credentials (in client
> > > > > context!)
> > > > > seems
> > > > > to be to be able to set GSS_KRB5_CRED_NO_CI_FLAGS_X option on
> > > > > the
> > > > > credentials - not sure what scenario this fixes and how to
> > > > > test
> > > > > there's
> > > > > no degradation there.
> > This is fundamental, otherwise LDAP binds will fail because AD uses
> > the
> > CI flags instead of the SASL negotiation, so we need to be able to
> > control exactly what flags we send (normally both Heimdal and MIT
> > Kerberos send both Confidentiality and Integrity flags
> > unconditionally,
> > because that is mandated by the Spec, but Microsoft interpreted it
> > differently in some protocols).
> > 
> I can test with Windows then. Only tested with Samba AD so far.
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > 3. Perhaps someone can easily determine the MIT behavior - if
> > > > > MIT
> > > > > is not
> > > > > sending this request then maybe the patch should be #ifdef'd
> > > > > on
> > > > > Kerberos
> > > > > type - use the more portable gss_acquire_cred() with MIT
> > > > > Kerberos.
> > We must use gss_acquire_cred() sorry.
> > 
> MIT seems to have a gss_krb5_import_cred() - can't we use that and
> fall
> back to gss_acquire_cred() only for older versions?
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > Thanks,
> > > > > Uri.
> > > > > 
> > > > One more things - the gensec_gse code also makes an extra TGS
> > > > handshake,
> > > > requesting a TGT, because it requires delegation for the
> > > > security
> > > > context. Do we need delegation for LDAP sasl binding/wrapping?
> > > 
> > > Generally no, we don't, it should be somehow selectable, base on
> > > the
> > > target machine's "allowed to delegate to" (IIRC) flag.
> > Brain fart here. We *definitely* almost never want to delegate a
> > TGT,
> > but we definitely want to request a forwardable ticket, so that the
> > server can perform constrained delegation if it is authorized and
> > so
> > configured.
> > 
> > What flags are passed in ? Can you point me at the code path that
> > generates this ?
> > 
> It's in the default gse context flags.
> In  gse_context_init(), we have:
> 
>         gse_ctx->gss_want_flags = GSS_C_MUTUAL_FLAG |
>                                 *GSS_C_DELEG_FLAG* |
>                                 GSS_C_DELEG_POLICY_FLAG |
>                                 GSS_C_REPLAY_FLAG |
>                                 GSS_C_SEQUENCE_FLAG;
> 
> 
> > 
> > Simo.
> >