credentials_krb5: use gss_acquire_cred for client-side GSSAPI use case

[Simo]

On Mon, 2017-03-06 at 07:23 +0100, Stefan Metzmacher wrote:
> Am 05.03.2017 um 20:01 schrieb Simo:
> > On Sat, 2017-03-04 at 11:19 -0500, Simo wrote:
> > > On Fri, 2017-03-03 at 13:24 +0100, Stefan Metzmacher wrote:
> > > > Am 03.03.2017 um 13:07 schrieb Alexander Bokovoy:
> > > > > On pe, 03 maalis 2017, Stefan Metzmacher wrote:
> > > > > > Hi Alexander,
> > > > > > 
> > > > > > > > > > Attached patch is needed for upcoming FreeIPA 4.5
> > > > > > > > > > release
> > > > > > > > > > to allow use
> > > > > > > > > > of Samba Python bindings in a privile separation
> > > > > > > > > > mode
> > > > > > > > > > provided by
> > > > > > > > > > GSS-proxy (https://pagure.io/gssproxy). FreeIPA bug
> > > > > > > > > > is
> > > > > > > > > > here:
> > > > > > > > > > https://pagure.io/freeipa/issue/6671, Samba bug is
> > > > > > > > > > https://bugzilla.samba.org/show_bug.cgi?id=12611
> > > > > > > > > > 
> > > > > > > > > > Please see more details in the commit message.
> > > > > > > > > 
> > > > > > > > > Please have a look at
> > > > > > > > > https://bugzilla.samba.org/show_bug.cgi?id=12480
> > > > > > > > > for the reasons why we can't use gss_acquire_cred().
> > > > > > > > > 
> > > > > > > > > There needs to be another solution, sorry.
> > > > > > > > 
> > > > > > > > As gss_acquire_cred_from() seems to be handled by
> > > > > > > > gssproxy,
> > > > > > > > I guess we need a wrapper in
> > > > > > > > lib/krb5_wrap/gss_samba.[ch]
> > > > > > > > that uses gss_acquire_cred_from() if available and
> > > > > > > > gss_krb5_import_cred() otherwise.
> > > > > > > > 
> > > > > > > > And that wrapper needs to be used everywhere we
> > > > > > > > currently
> > > > > > > > use gss_krb5_import_cred(). It should also hide the
> > > > > > > > mess
> > > > > > > > we currently use in gse_init_server() to work arround
> > > > > > > > the broken gss_krb5_import_cred() server side.
> > > > > > > 
> > > > > > > That was my initial idea but the problem here is that we
> > > > > > > deal
> > > > > > > with it 
> > > > > > > differently in different contexts. In client context we
> > > > > > > use
> > > > > > > GSSAPI creds
> > > > > > > while in other places we use KRB5 ccache handle.
> > > > > > > 
> > > > > > > This particular code path is isolated from other use of
> > > > > > > gss_krb5_import_cred(). And it does not need to specify
> > > > > > > explicit
> > > > > > > credential cache because it is running in a client
> > > > > > > context
> > > > > > > and
> > > > > > > expects
> > > > > > > to use the default ccache.
> > > > > > 
> > > > > > That's wrong this code path operates on multiple MEMORY
> > > > > > caches
> > > > > > in smbtorture.
> > > > > > 
> > > > > > gss_acquire_cred() never used again for initiator creds
> > > > > > and only for acceptor creds as a fallback for a broken
> > > > > > gss_krb5_import_cred().
> > > > > > 
> > > > > > So all places should use the wrapper that uses
> > > > > > gss_acquire_cred_from() if available. It should
> > > > > > otherwise use gss_krb5_import_cred() with
> > > > > > a fallback only for the keytab only case
> > > > > > to gsskrb5_register_acceptor_identity()/gss_acquire_cred().
> > > > > 
> > > > > So, what about Andreas' code from
> > > > > https://git.samba.org/?p=asn/samba.git;a=shortlog;h=refs/head
> > > > > s/ma
> > > > > st
> > > > > er-cli_creds
> > > > > ?
> > > > 
> > > > I know I also had some of this in my master3-libsmb-tmp,
> > > > but skipped it as it was not required yet.
> > > > 
> > > > > The only issue I see there is that when gensec_gssapi calls
> > > > > into
> > > > > cli_credentials_get_client_gss_creds() with NULL ccache, it
> > > > > wants
> > > > > to get
> > > > > the creds from the default ccache but Andreas' code will try
> > > > > to
> > > > > create a
> > > > > new MEMORY: ccache. This is wrong for the use case we have
> > > > > with
> > > > > cli_credentials_get_client_gss_creds() because this ccache is
> > > > > empty
> > > > > and
> > > > > default one contains actual credentials.
> > > > 
> > > > I think I'd preferr to have a wrapper that behaves like
> > > > gss_krb5_import_cred() on heimdal, just
> > > > 
> > > > uint32_t smb_gss_krb5_import_cred(OM_uint32 *minor_status,
> > > >                      krb5_ccache id,
> > > >                      krb5_principal keytab_principal,
> > > >                      krb5_keytab keytab,
> > > >                      gss_cred_id_t *cred);
> > > > 
> > > > And change the callers just to use it without any additional
> > > > logic change.
> > > > 
> > > > That would be much easier to understand.
> > > 
> > > Doesn't this mean the code would always try to krb5_cc_resolve()
> > > the
> > > ccache ?
> 
> Yes, and that's exactly what I want. We should do such things
> explicit
> in order to avoid unexpected magic depending on the used kerberos
> library.

Can you explain what magic you refer to ?
What's your goal here ? Keep in mind that in MIT kerberos
gss_krb5_import_cred() is implemented by calling the acquire_cred paths
internally anyway, so the solution for any "magic" is likely the same
in either case.

> > > I would rather have instead an interface that looks like
> > > gss_acquire_cred_from(), and then simulate it if it is not
> > > available.
> > > 
> > > That would be a much cleaner gssapi-like interface, and will help
> > > starting to untangle the mess we have with code always mixing
> > > lower
> > > level krb5 calls, which we should avoid where possible and higher
> > > level
> > > gssapi calls which we should prefer in most cases, except when
> > > the
> > > implementatrion has limitation we cannot deal with regular gssapi
> > > calls
> > > (and when we do we should fix it in gssapi instead of carrying
> > > workarounds forever).
> > 
> > In this patch[1] I added a patch on top of yours to detect
> > gss_acquire_cred_from() that implements smb_gss_acquire_cred_from()
> > using gss_krb5_import_cred() to emulate it if not available.
> > 
> > I have not tested this or changed the rest of the code from
> > gss_krb5_import_cred() to smb_gss_acquire_cred_from(), but would do
> > it
> > if you think this route is ok.
> 
> Is there a reason why you used the prototype of gss_add_cred_from()
> instead of gss_acquire_cred_from() in order to implement
> smb_gss_add_cred_from()?

No, I just used the wrong name as I had it in my hands, I can change
the name to smb_gss_add_cred_from(), or change the prototype to be the
same of gss_acquired_cred_from(), probably the latter is actually
better now that you point it out, it will remove some playing around
with input/output parms on the fallback to regular gss_acquire_cred().

Going to push a new change there to do that.

> I actually think the simpler gss_krb5_import_cred() interface
> keeps the logic in our callers simpler.

But it keeps our interfaces tied to resolve ccache manually, that
cannot be done with interposer mechanisms, they will just fail.
The whole point of these patches is to allow interposition, so we need
to move toward removing direct krb5 calls as much as possible.

> We can revisit this, one we would start to use gssapi for anything
> other than kerberos.

We should do that too.